feat: add support to secretTemplates

[jonathansp]

Signed-off-by: jonathansp jonathansimonprates@gmail.com

What this PR does / why we need it:

This change introduces the concept of SecretTemplate for Certificates. When a certificate is issued, a new secret is created to hold the certificate data. This secret is created by cert-manager. In order to use solutions like kubed to copy this secret to multiple namespaces, this created secret must be annotated.

SecretTemplate is a property of CertificateSpec. Labels and Annotations defined there will be copied to the Secret when required.

Which issue this PR fixes:

fixes #2576

Special notes for your reviewer:

This PR is not 100% finished yet. After reviewing I will add the documentation of the new feature as well as the release note.

Release note:

Add support for adding custom annotations and labels to the Secret containing the TLS key pair.

[jetstack-bot]

Hi @jonathansp. Thanks for your PR.

I'm waiting for a jetstack or cert-manager 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.

Details

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.

[jonathansp]

/assign @JoshVanL

[lzecca78]

so, when this PR will be merged?

[jonathansp]

I will be happy to change or improve any implementation or design you guys are not happy with. Let me know.

[ubergesundheit]

How would one use this through the an Ingress?

[wallrj]

/milestone Next

I'm adding this to the next milestone during our 2021-05-27 triage session, because it seems like it is close to being mergable and because it solves a much reported issue.

It also ties in quite closely with #3537 which makes cert-manager copy the Certificate labels to the Secret.

[deveshk0]

When can we expect this to get merged?

[wallrj]

Thanks @jonathansp

This is a much requested feature and the code and tests all make sense.

I asked myself if this feature needs an E2E test, but I think these additional SecretManager integration tests are sufficient.

I think you'll see quite a few bazel build and verification checks fail until you make various changes to Bazel.build files, regenerate the CRD files, and add the new fields to all APIs.

/ok-to-test

[wallrj]

Thanks for all your work on this @jonathansp

I tried it locally and it worked well. Thanks for creating the followup GitHub issue.
We can add the label and annotation cleanup feature in a future PR.

/lgtm
/approve

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonathansp, wallrj

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [wallrj]

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

[wallrj]

Added a release note.

[wallrj]

/kind feature

[jonathansp]

Thanks for all your work on this @jonathansp

I tried it locally and it worked well. Thanks for creating the followup GitHub issue.
We can add the label and annotation cleanup feature in a future PR.

/lgtm
/approve

Thanks @wallrj. I'm glad to contribute. Anything I can help with, just let me know.

[reda-sj]

Awesome work @jonathansp 👏

[ubergesundheit]

Hey sorry to chime in again,

just wanting to confirm this does not make it possible to add the secretTemplates to certificate Secrets automatically generated for Ingresses, right?

Thanks!

[wallrj]

just wanting to confirm this does not make it possible to add the secretTemplates to certificate Secrets automatically generated for Ingresses, right?

@ubergesundheit No, I'm afraid not. The ingress-shim creates a Certificate based on annotations on your Ingress. But there are only a few Certificate fields that can be controlled by Ingress annotations. See https://cert-manager.io/docs/usage/ingress/#supported-annotations

There is an idea for Certificate presets, which would allow you to add defaults for every field in the Certificates created by ingress-shim. See #2239

[maelvls]

I just noticed that we are iterating on a map here. Since the iterating order is random on Go maps, this means we end up with flakiness, e.g. in https://prow.build-infra.jetstack.net/view/gs/jetstack-logs/logs/ci-cert-manager-previous-previous-experimental/1439782520404054016 😅

To reproduce:

$ bazel run //pkg/internal/apis/certmanager/validation:go_default_test
$ ./bazel-bin/pkg/internal/apis/certmanager/validation/go_default_test_/go_default_test -test.run "TestValidateCertificate/invalid_with_disallowed_'CertificateSecretTemplate'_annotations" -test.count=10
--- FAIL: TestValidateCertificate (0.00s)
    --- FAIL: TestValidateCertificate/invalid_with_disallowed_'CertificateSecretTemplate'_annotations (0.00s)
        certificate_test.go:747: Expected error spec.secretTemplate.annotations: Invalid value: "cert-manager.io/alt-names": cert-manager.io/* annotations are not allowed but got spec.secretTemplate.annotations: Invalid value: "cert-manager.io/certificate-name": cert-manager.io/* annotations are not allowed
        certificate_test.go:747: Expected error spec.secretTemplate.annotations: Invalid value: "cert-manager.io/certificate-name": cert-manager.io/* annotations are not allowed but got spec.secretTemplate.annotations: Invalid value: "cert-manager.io/alt-names": cert-manager.io/* annotations are not allowed
FAIL

We can fix the flakiness by expecting the errors in the unit test to appear in any order.

Instead of

for i, e := range errs {
	expectedErr := s.errs[i]
	if !reflect.DeepEqual(e, expectedErr) {
		t.Errorf("Expected error %v but got %v", expectedErr, e)
	}
}

we could use ElementsMatch (from the testify module):

assert.ElementsMatch(t, s.errs, errs)

[maelvls]

Oops, I just noticed that #4365 fixes the flakiness issue!

The reason we have this flakiness is that we forgot to backport this fix to the branch release-1.5.