From b1a59164e00727830c46192a99e56f50c400905e Mon Sep 17 00:00:00 2001 From: irbekrm Date: Tue, 23 May 2023 12:01:30 +0100 Subject: [PATCH 1/4] Don't import controller's feature gate setup into a shared library To prevent controller's feature gates from overwriting other component's feature gates Signed-off-by: irbekrm --- .../requestmanager_controller.go | 3 +- pkg/util/pki/csr.go | 59 +++++++++++-------- pkg/util/pki/csr_test.go | 5 +- 3 files changed, 37 insertions(+), 30 deletions(-) diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index 381d3ce0748..2a6cc5c9019 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -349,7 +349,8 @@ func (c *controller) deleteRequestsNotMatchingSpec(ctx context.Context, crt *cma func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi.Certificate, pk crypto.Signer, nextRevision int, nextPrivateKeySecretName string) error { log := logf.FromContext(ctx) - x509CSR, err := pki.GenerateCSR(crt) + + x509CSR, err := pki.GenerateCSR(crt, pki.WithUseLiteralSubject(utilfeature.DefaultMutableFeatureGate.Enabled(feature.LiteralCertificateSubject))) if err != nil { log.Error(err, "Failed to generate CSR - will not retry") return nil diff --git a/pkg/util/pki/csr.go b/pkg/util/pki/csr.go index 5e1087ab42c..a4a64a6e329 100644 --- a/pkg/util/pki/csr.go +++ b/pkg/util/pki/csr.go @@ -30,10 +30,8 @@ import ( "net/url" "strings" - "github.com/cert-manager/cert-manager/internal/controller/feature" apiutil "github.com/cert-manager/cert-manager/pkg/api/util" v1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" - utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" ) func IPAddressesForCertificate(crt *v1.Certificate) []net.IP { @@ -165,6 +163,7 @@ func BuildCertManagerKeyUsages(ku x509.KeyUsage, eku []x509.ExtKeyUsage) []v1.Ke type generateCSROptions struct { EncodeBasicConstraintsInRequest bool + UseLiteralSubject bool } type GenerateCSROption func(*generateCSROptions) @@ -178,6 +177,12 @@ func WithEncodeBasicConstraintsInRequest(encode bool) GenerateCSROption { } } +func WithUseLiteralSubject(useLiteralSubject bool) GenerateCSROption { + return func(o *generateCSROptions) { + o.UseLiteralSubject = useLiteralSubject + } +} + // GenerateCSR will generate a new *x509.CertificateRequest template to be used // by issuers that utilise CSRs to obtain Certificates. // The CSR will not be signed, and should be passed to either EncodeCSR or @@ -185,14 +190,23 @@ func WithEncodeBasicConstraintsInRequest(encode bool) GenerateCSROption { func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.CertificateRequest, error) { opts := &generateCSROptions{ EncodeBasicConstraintsInRequest: false, + UseLiteralSubject: false, } for _, opt := range optFuncs { opt(opts) } - commonName, err := extractCommonName(crt.Spec) - if err != nil { - return nil, err + var ( + commonName = crt.Spec.CommonName + err error + ) + + if opts.UseLiteralSubject { + commonName, err = extractCommonNameFromLiteralSubject(crt.Spec) + if err != nil { + return nil, err + } + } iPAddresses := IPAddressesForCertificate(crt) @@ -250,7 +264,7 @@ func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.Cert ExtraExtensions: extraExtensions, } - if isLiteralCertificateSubjectEnabled() && len(crt.Spec.LiteralSubject) > 0 { + if opts.UseLiteralSubject && len(crt.Spec.LiteralSubject) > 0 { rawSubject, err := ParseSubjectStringToRawDERBytes(crt.Spec.LiteralSubject) if err != nil { return nil, err @@ -449,30 +463,25 @@ func SignatureAlgorithm(crt *v1.Certificate) (x509.PublicKeyAlgorithm, x509.Sign return pubKeyAlgo, sigAlgo, nil } -func extractCommonName(spec v1.CertificateSpec) (string, error) { - var commonName = spec.CommonName - if isLiteralCertificateSubjectEnabled() && len(spec.LiteralSubject) > 0 { - commonName = "" - sequence, err := UnmarshalSubjectStringToRDNSequence(spec.LiteralSubject) - if err != nil { - return "", err - } +func extractCommonNameFromLiteralSubject(spec v1.CertificateSpec) (string, error) { + if spec.LiteralSubject == "" { + return spec.CommonName, nil + } + commonName := "" + sequence, err := UnmarshalSubjectStringToRDNSequence(spec.LiteralSubject) + if err != nil { + return "", err + } - for _, rdns := range sequence { - for _, atv := range rdns { - if atv.Type.Equal(OIDConstants.CommonName) { - if str, ok := atv.Value.(string); ok { - commonName = str - } + for _, rdns := range sequence { + for _, atv := range rdns { + if atv.Type.Equal(OIDConstants.CommonName) { + if str, ok := atv.Value.(string); ok { + commonName = str } } } } return commonName, nil - -} - -func isLiteralCertificateSubjectEnabled() bool { - return utilfeature.DefaultFeatureGate.Enabled(feature.LiteralCertificateSubject) } diff --git a/pkg/util/pki/csr_test.go b/pkg/util/pki/csr_test.go index 1b0e5fd2413..7159ce6a164 100644 --- a/pkg/util/pki/csr_test.go +++ b/pkg/util/pki/csr_test.go @@ -29,12 +29,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - featuregatetesting "k8s.io/component-base/featuregate/testing" - "github.com/cert-manager/cert-manager/internal/controller/feature" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" "github.com/cert-manager/cert-manager/pkg/util" - utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" ) func buildCertificate(cn string, dnsNames ...string) *cmapi.Certificate { @@ -614,10 +611,10 @@ func TestGenerateCSR(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, feature.LiteralCertificateSubject, tt.literalCertificateSubjectFeatureEnabled)() got, err := GenerateCSR( tt.crt, WithEncodeBasicConstraintsInRequest(tt.basicConstraintsFeatureEnabled), + WithUseLiteralSubject(tt.literalCertificateSubjectFeatureEnabled), ) if (err != nil) != tt.wantErr { t.Errorf("GenerateCSR() error = %v, wantErr %v", err, tt.wantErr) From 8a34cbc0a0b23968f5e8f90343a40ccb1af12ff5 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Tue, 23 May 2023 12:02:55 +0100 Subject: [PATCH 2/4] Adds some warnings for folks to not import feature gates into shared code Really we should restructure this to remove the possibility of accidentally overwriting other component's feature gates Signed-off-by: irbekrm --- internal/cainjector/feature/features.go | 4 ++++ internal/controller/feature/features.go | 4 ++++ internal/webhook/feature/features.go | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/internal/cainjector/feature/features.go b/internal/cainjector/feature/features.go index 196d3ea6382..951ee7f25e3 100644 --- a/internal/cainjector/feature/features.go +++ b/internal/cainjector/feature/features.go @@ -14,6 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ +// feature contains cainjector feature gate setup code. Do not import this +// package into any code that's shared with other components to prevent +// overwriting other component's featue gates, see i.e +// https://github.com/cert-manager/cert-manager/issues/6011 package feature import ( diff --git a/internal/controller/feature/features.go b/internal/controller/feature/features.go index 10ce0b40c6d..87d4c9ed5a7 100644 --- a/internal/controller/feature/features.go +++ b/internal/controller/feature/features.go @@ -14,6 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ +// feature contains controller's feature gate setup functionality. Do not import +// this package into any code that's shared with other components to prevent +// overwriting other component's featue gates, see i.e +// https://github.com/cert-manager/cert-manager/issues/6011 package feature import ( diff --git a/internal/webhook/feature/features.go b/internal/webhook/feature/features.go index a096cb2744f..d9a44dcffd3 100644 --- a/internal/webhook/feature/features.go +++ b/internal/webhook/feature/features.go @@ -14,6 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ +// feature contains webhook's feature gate setup functionality. Do not import +// this package into any code that's shared with other components to prevent +// overwriting other component's featue gates, see i.e +// https://github.com/cert-manager/cert-manager/issues/6011 package feature import ( From acf07419f599fb0f6f31b46b3c14062921fd9888 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Tue, 23 May 2023 12:44:31 +0100 Subject: [PATCH 3/4] Fix a bug in helm chart where webhook had controller feature gates passed This will break anyone who relied on featureGates field to pass feature gates to webhook- they will need to use the new webhook.featureGates field Signed-off-by: irbekrm --- .../charts/cert-manager/templates/webhook-deployment.yaml | 2 +- deploy/charts/cert-manager/values.yaml | 6 +++++- make/e2e-setup.mk | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/deploy/charts/cert-manager/templates/webhook-deployment.yaml b/deploy/charts/cert-manager/templates/webhook-deployment.yaml index 043c4b15071..db85b947a58 100644 --- a/deploy/charts/cert-manager/templates/webhook-deployment.yaml +++ b/deploy/charts/cert-manager/templates/webhook-deployment.yaml @@ -72,7 +72,7 @@ spec: - --secure-port={{ .Values.webhook.securePort }} {{- end }} {{- if .Values.featureGates }} - - --feature-gates={{ .Values.featureGates }} + - --feature-gates={{ .Values.webhook.featureGates }} {{- end }} {{- $tlsConfig := default $config.tlsConfig "" }} {{ if or (not $config.tlsConfig) (and (not $tlsConfig.dynamic) (not $tlsConfig.filesystem) ) -}} diff --git a/deploy/charts/cert-manager/values.yaml b/deploy/charts/cert-manager/values.yaml index def8de1b900..109c0ebbe86 100644 --- a/deploy/charts/cert-manager/values.yaml +++ b/deploy/charts/cert-manager/values.yaml @@ -70,7 +70,7 @@ podDisruptionBudget: # or a percentage value (e.g. 25%) # Comma separated list of feature gates that should be enabled on the -# controller pod & webhook pod. +# controller pod. featureGates: "" # The maximum number of challenges that can be scheduled as 'processing' at once @@ -341,6 +341,10 @@ webhook: # Path to a file containing a WebhookConfiguration object used to configure the webhook # - --config= + # Comma separated list of feature gates that should be enabled on the + # webhok pod. + featureGates: "" + resources: {} # requests: # cpu: 10m diff --git a/make/e2e-setup.mk b/make/e2e-setup.mk index 2c77dc181af..09b3126aadf 100644 --- a/make/e2e-setup.mk +++ b/make/e2e-setup.mk @@ -270,7 +270,7 @@ e2e-setup-certmanager: $(BINDIR)/cert-manager.tgz $(foreach binaryname,controlle --set installCRDs=true \ --set featureGates="$(feature_gates_controller)" \ --set "extraArgs={--kube-api-qps=9000,--kube-api-burst=9000,--concurrent-workers=200}" \ - --set "webhook.extraArgs={--feature-gates=$(feature_gates_webhook)}" \ + --set webhook.featureGates="$(feature_gates_webhook)" \ --set "cainjector.extraArgs={--feature-gates=$(feature_gates_cainjector)}" \ --set "dns01RecursiveNameservers=$(SERVICE_IP_PREFIX).16:53" \ --set "dns01RecursiveNameserversOnly=true" \ From 55ebaa31b58fdf92667455beb049819ce8b36376 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Wed, 24 May 2023 12:19:22 +0200 Subject: [PATCH 4/4] fix typo Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- deploy/charts/cert-manager/values.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deploy/charts/cert-manager/values.yaml b/deploy/charts/cert-manager/values.yaml index 109c0ebbe86..46513b65a06 100644 --- a/deploy/charts/cert-manager/values.yaml +++ b/deploy/charts/cert-manager/values.yaml @@ -342,7 +342,7 @@ webhook: # - --config= # Comma separated list of feature gates that should be enabled on the - # webhok pod. + # webhook pod. featureGates: "" resources: {}