Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(aws/acm): Add caching agent for AWS Certificate Manager #5553

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaydee864
Copy link

Spinnaker Managed Delivery for EC2 depends on Clouddriver to list the available SSL certificates in each AWS account. If a certificate is not in the Clouddriver cache, then Managed Delivery cannot use that certificate.

Currently, Clouddriver only caches IAM certificates, using AmazonCerificateCachingAgent. This pull request seeks to enable Clouddriver to cache certificates stored in AWS Certificate Manager as well.

I have tested this change on our development Spinnaker environment, and after deploying I have verified that Clouddriver's /certificates/aws API returned both ACM and IAM certificates. I was also able to use an ACM certificate in my managed delivery configuration; I set the certificate: value under the listener config to the domain name of the certificate I wanted to use.

Spinnaker's contributing guidelines recommend avoiding creating new classes in Groovy, but in this case I don't think it's really feasible to do so. The AmazonCertificate class in the Groovy code uses the Canonical annotation to generate its constructors, and there is an issue at compile-time when referencing such a constructor from Java code in the same module. There is a proposed fix for a Maven project in the linked StackOverflow question, but I don't know if that's applicable to Gradle and I'm also not confident in making that change to Clouddriver's configuration without breaking other things.

@spinnakerbot
Copy link
Contributor

We prefer that non-test backend code be written in Java or Kotlin, rather than Groovy. The following files have been added and written in Groovy:

  • clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/provider/agent/AwsCertificateManagerCachingAgent.groovy

See our server-side commit conventions here.

@JonGilmore
Copy link

@luispollo wondering if you might be able to get some eyes on this PR? We're facing the same issue in our environment as well.

@luispollo
Copy link
Contributor

I'll have to defer to @robfletcher and/or @emjburns on this one. My knowledge of clouddriver is pretty limited.

@dmart
Copy link
Contributor

dmart commented Feb 11, 2022

Any chance we can get some eyes on this?

protected Instant lastFailure

static final Set<AgentDataType> types = Collections.unmodifiableSet([
AUTHORITATIVE.forType(CERTIFICATES.ns)
Copy link
Contributor

@deverton deverton Feb 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So looking at

* <p>If an agent is an Authoritative source of data, then it's resulting data set will be
it seems like if this is listing itself as AUTHORITATIVE then it's going to fight with https://github.com/spinnaker/clouddriver/blob/master/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/provider/agent/AmazonCertificateCachingAgent.groovy over which agent has the complete list.

I think it might be better to add the reading from Certificate Manager to the AmazonCertificateCachingAgent so that there's only one agent.

You should be able to test if this is an issue by deleting a certificate from one of IAM or CertificateManager and seeing if the cache is updated correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants