Support importing certificates into ACM #40

cPu1 · 2024-07-17T14:46:27Z

Description of changes:

Adds support for importing certificates into ACM. When Spec.Certificate is set, the controller attempts to import a certificate into ACM, otherwise it requests a certificate. It is an error to set fields used to request a certificate when importing a certificate and vice versa. The two fields required for importing a certificate (Certificate and PrivateKey) are both secret references. While the certificate portion may not be private information, it’s common to store TLS certificates as secrets in Kubernetes. The ACK runtime currently only supports resolving Opaque secrets and not TLS certificates, so users wanting to import existing TLS secrets into ACM will have to migrate them first.

The late_initialize_post_read_one hook is used to allow late initialization of optional fields, otherwise the controller will keep retrying DescribeCertificate for setting fields that will never be returned from the API.

GoCodeSetSDKForStruct in code-generator does not support resolving secret references for custom []byte fields, so a wrapper type importCertificateInput has been introduced as a workaround.

Issue #, if available: aws-controllers-k8s/community#2043

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow · 2024-07-17T14:46:37Z

Hi @cPu1. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

a-hilaly · 2024-07-17T15:43:12Z

/ok-to-test

Description of changes: As part of aws-controllers-k8s/acm-controller#40, an integration test was added that has a dependency on the `cryptography` module. In order for this to work, `libffi` needs to be installed in Alpine-based images. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

cPu1 · 2024-07-17T19:19:12Z

/retest

mahadh02 · 2024-07-22T17:20:01Z

this matches our requirement for importing certi into ACM

apis/v1alpha1/generator.yaml

anbaig · 2024-07-29T19:13:20Z

apis/v1alpha1/certificate.go

@@ -32,6 +33,7 @@ type CertificateSpec struct {
 	//
 	// arn:aws:acm-pca:region:account:certificate-authority/12345678-1234-1234-1234-123456789012
 	CertificateAuthorityARN *string `json:"certificateAuthorityARN,omitempty"`


Similar to: https://github.com/aws-controllers-k8s/acmpca-controller/blob/main/apis/v1alpha1/certificate.go#L42

Can we allow the ref of a ACK PCA Certificate authority to be passed in?

Makes sense. I'll look into it.

I have made this change but I'll note that this field was not introduced in this PR and is not related to importing certificates either.

a-hilaly · 2024-08-07T06:26:35Z

@cPu1 can you please rebase and address @anbaig comments

cPu1 · 2024-08-07T14:14:52Z

@cPu1 can you please rebase and address @anbaig comments

I will have to get back to this after my current priorities.

cPu1 · 2024-08-19T13:15:27Z

I have rebased and pushed a change to use a secret for CertificateChain and a ref for CertificateAuthorityARN.

a-hilaly

Thanks @cPu1 ! i left a few comments below

a-hilaly · 2024-09-05T02:01:55Z

apis/v1alpha1/certificate.go

+	Certificate    *ackv1alpha1.SecretKeyReference `json:"certificate,omitempty"`
+	CertificateARN *string                         `json:"certificateARN,omitempty"`


We might want to add some custom documentation for these fields. I'm getting a bit confused with all the Certificate* fields 😄

Good point. I'll add it.

a-hilaly · 2024-09-05T02:04:35Z

pkg/resource/certificate/hooks.go

+func (c *importCertificateInput) SetPrivateKey(_ *ackv1alpha1.SecretKeyReference) {}
+
+func (c *importCertificateInput) SetCertificate(_ *ackv1alpha1.SecretKeyReference) {}
+
+func (c *importCertificateInput) SetCertificateChain(_ *ackv1alpha1.SecretKeyReference) {}


Do we need these to be implemented

We do not. There's a comment on the importCertificateInput struct explaining why this is needed.

a-hilaly · 2024-09-05T02:06:54Z

test/e2e/tests/test_certificate.py

+            'spec': {
+                'options': {
+                    'certificateTransparencyLoggingPreference': 'ENABLED'
+                }
+            },
+        }
+        k8s.patch_custom_resource(ref, updates)
+        time.sleep(10)
+        assert k8s.wait_on_condition(
+            ref,
+            condition.CONDITION_TYPE_TERMINAL,
+            'True',
+            wait_periods=MAX_WAIT_FOR_SYNCED_MINUTES,
+        )


Why does the resource go in terminal state when certificateTransparencyLoggingPreference is enabled?

It's because certificateTransparencyLoggingPreference is an immutable field for imported certificates.

a-hilaly · 2024-09-05T02:08:40Z

pkg/resource/certificate/hooks.go

+		created, err := rm.importCertificate(ctx, r, input)
+		if err != nil {
+			return nil, false, err
+		}
+		return created, true, nil


Do we need the boolean in the outputs? looks like we return true only when created is returned.

While the caller can also use nil for checking if this is for importing an existing certificate, a separate boolean return value makes the intent clearer and the calling code more readable.

apis/v1alpha1/ack-generate-metadata.yaml

Adds support for [importing certificates](https://docs.aws.amazon.com/acm/latest/APIReference/API_ImportCertificate.html) into ACM. When `Spec.Certificate` is set, the controller attempts to import a certificate into ACM, otherwise it requests a certificate. It is an error to set fields used to request a certificate when importing a certificate and vice versa. The two fields required for importing a certificate (`Certificate` and `PrivateKey`) are both secret references. While the certificate portion may not be private information, it’s common to store TLS certificates as secrets in Kubernetes. The ACK runtime currently only supports resolving `Opaque` secrets and not TLS certificates, so users wanting to import existing TLS secrets into ACM will have to migrate them first. The `late_initialize_post_read_one` hook is used to allow late initialization of optional fields, otherwise the controller will keep retrying `DescribeCertificate` for setting fields that will never be returned from the API. `GoCodeSetSDKForStruct` in code-generator does not support resolving secret references for custom `[]byte` fields, so a wrapper type `importCertificateInput` has been introduced as a workaround. Signed-off-by: cpu1 <[email protected]>

Signed-off-by: cpu1 <[email protected]>

ack-prow · 2024-11-06T19:26:38Z

@cPu1: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
acm-verify-attribution	`45a1cdb`	link	false	`/test acm-verify-attribution`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

michaelhtm

/lgtm

ack-prow · 2024-11-06T19:59:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, cPu1, michaelhtm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [a-hilaly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Issue #, if available: Description of changes: Update recommended inline policy to support importing certificates which was added as part of #40 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow bot requested a review from a-hilaly July 17, 2024 14:46

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 17, 2024

cPu1 force-pushed the feat/import-certificate branch 2 times, most recently from 3168efa to a386d8f Compare July 17, 2024 14:58

ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 17, 2024

cPu1 mentioned this pull request Jul 17, 2024

Install libffi for ACM in pytest image aws-controllers-k8s/test-infra#459

Merged

a-hilaly mentioned this pull request Jul 22, 2024

ACM service controller aws-controllers-k8s/community#482

Closed

cPu1 force-pushed the feat/import-certificate branch from a386d8f to 5dfea68 Compare July 29, 2024 16:09

anbaig reviewed Jul 29, 2024

View reviewed changes

apis/v1alpha1/generator.yaml Show resolved Hide resolved

anbaig reviewed Jul 29, 2024

View reviewed changes

cPu1 force-pushed the feat/import-certificate branch from 5dfea68 to 90f4aab Compare August 19, 2024 13:08

a-hilaly reviewed Sep 5, 2024

View reviewed changes

cPu1 force-pushed the feat/import-certificate branch 2 times, most recently from 9a25cb4 to da90cd6 Compare September 16, 2024 10:58

cPu1 and others added 4 commits November 6, 2024 11:19

Use a secret for CertificateChain and a ref for CertificateAuthorityARN

40aa90d

Signed-off-by: cpu1 <[email protected]>

Add documentation

bff57bc

Signed-off-by: cpu1 <[email protected]>

Regenerate with latest code-generator

45a1cdb

a-hilaly force-pushed the feat/import-certificate branch from 2d07780 to 45a1cdb Compare November 6, 2024 19:26

a-hilaly approved these changes Nov 6, 2024

View reviewed changes

ack-prow bot added the approved label Nov 6, 2024

michaelhtm approved these changes Nov 6, 2024

View reviewed changes

ack-prow bot assigned michaelhtm Nov 6, 2024

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2024

ack-prow bot merged commit 4004d1f into aws-controllers-k8s:main Nov 6, 2024
5 of 7 checks passed

smcavallo mentioned this pull request Nov 8, 2024

Update recommended-inline-policy #49

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support importing certificates into ACM #40

Support importing certificates into ACM #40

cPu1 commented Jul 17, 2024

ack-prow bot commented Jul 17, 2024

a-hilaly commented Jul 17, 2024

cPu1 commented Jul 17, 2024

mahadh02 commented Jul 22, 2024

anbaig Jul 29, 2024

cPu1 Aug 7, 2024

cPu1 Aug 19, 2024 •

edited

Loading

a-hilaly commented Aug 7, 2024

cPu1 commented Aug 7, 2024

cPu1 commented Aug 19, 2024

a-hilaly left a comment

a-hilaly Sep 5, 2024

cPu1 Sep 6, 2024

cPu1 Sep 16, 2024

a-hilaly Sep 5, 2024

cPu1 Sep 6, 2024

a-hilaly Sep 5, 2024

cPu1 Sep 6, 2024

a-hilaly Sep 5, 2024

cPu1 Sep 6, 2024

ack-prow bot commented Nov 6, 2024 •

edited

Loading

michaelhtm left a comment

ack-prow bot commented Nov 6, 2024

		Certificate *ackv1alpha1.SecretKeyReference `json:"certificate,omitempty"`
		CertificateARN *string `json:"certificateARN,omitempty"`

Support importing certificates into ACM #40

Support importing certificates into ACM #40

Conversation

cPu1 commented Jul 17, 2024

ack-prow bot commented Jul 17, 2024

a-hilaly commented Jul 17, 2024

cPu1 commented Jul 17, 2024

mahadh02 commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cPu1 Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

a-hilaly commented Aug 7, 2024

cPu1 commented Aug 7, 2024

cPu1 commented Aug 19, 2024

a-hilaly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ack-prow bot commented Nov 6, 2024 • edited Loading

michaelhtm left a comment

Choose a reason for hiding this comment

ack-prow bot commented Nov 6, 2024

cPu1 Aug 19, 2024 •

edited

Loading

ack-prow bot commented Nov 6, 2024 •

edited

Loading