BUGFIX: CertificateRequest short names must be unique.

[inteon]

Fixes #6342

The new mechanism is explained here:

cert-manager/pkg/controller/certificates/requestmanager/requestmanager_controller.go

Lines 401 to 411 in 860df22

	// The CertificateRequest name is limited to 253 characters, assuming the nextRevision and hyphen
	// can be represented using 20 characters, we can directly accept certificate names up to 233
	// characters. Certificate names that are longer than this will be hashed to a shorter name. We want
	// to make crafting two Certificates with the same truncated name as difficult as possible, so we
	// use a cryptographic hash function to hash the full certificate name to 64 characters.
	// Finally, for Certificates with a name longer than 233 characters, we build the CertificateRequest
	// name as follows: <first-168-chars-of-certificate-name>-<64-char-hash>-<19-char-nextRevision>
	crName, err := apiutil.ComputeSecureUniqueDeterministicNameFromData(crt.Name, 233)
	if err != nil {
	return err
	}

Kind

/kind bug

Release Note

BUGFIX: fix CertificateRequest name collision bug in StableCertificateRequestName feature.

[SgtCoDFish]

I've left a couple of comments; what do you think?

[inteon]

/cherrypick release-1.13

[jetstack-bot]

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.13 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.13
/cherrypick release-1.12

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.

[inteon]

/cherrypick release-1.12

[jetstack-bot]

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.12 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.12

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.

[SgtCoDFish]

Couple more comments!

[SgtCoDFish]

Love this comment!

[SgtCoDFish]

/lgtm
/approve
/hold

This is an improvement and it fixes the issue as far as I can tell! I think it'd be helpful for another maintainer to take a look at this since it's essentially setting the format for all CertificateRequests going forwards. Maybe someone else will have thoughts on it. But I'd be happy to merge I think!

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

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 [SgtCoDFish]

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

[maelvls]

I am happy with the new format.

Maybe bumping the name limit from a DNS label (63 chars) to a DNS subdomain (253 chars) would have solved 99% of the problems, including the original author of the issue. It seems like the hashing feature addresses a small corner case: folks with very very very large certificate names (longer than 232 chars) would still get the issue reported by the original author. But it's good that we solved this corner case anyways.

Thanks for working on this!

I'll hold it if you need to fix some of the non-blocking suggestions.

/lgtm
/hold

[inteon]

/unhold

[SgtCoDFish]

/lgtm

(quick lgtm as requested given this has been reviewed)

[jetstack-bot]

@inteon: new pull request created: #6358

In response to this:

/cherrypick release-1.13

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.

[jetstack-bot]

@inteon: new pull request created: #6359

In response to this:

/cherrypick release-1.12

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.