From c6896b2f93e7d2f9db71dd5ed6c637acfaf31a0b Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 29 Sep 2021 12:17:32 +0100 Subject: [PATCH 1/5] Set all non-v1 CRD versions as not-served Signed-off-by: Richard Wall --- deploy/crds/crd-certificaterequests.yaml | 6 +++--- deploy/crds/crd-certificates.yaml | 6 +++--- deploy/crds/crd-challenges.yaml | 6 +++--- deploy/crds/crd-clusterissuers.yaml | 6 +++--- deploy/crds/crd-issuers.yaml | 6 +++--- deploy/crds/crd-orders.yaml | 6 +++--- 6 files changed, 18 insertions(+), 18 deletions(-) diff --git a/deploy/crds/crd-certificaterequests.yaml b/deploy/crds/crd-certificaterequests.yaml index 6e2a803dd..fb5e3b75c 100644 --- a/deploy/crds/crd-certificaterequests.yaml +++ b/deploy/crds/crd-certificaterequests.yaml @@ -212,7 +212,7 @@ spec: description: FailureTime stores the time that this CertificateRequest failed. This is used to influence garbage collection and back-off. type: string format: date-time - served: true + served: false storage: false - name: v1alpha3 subresources: @@ -381,7 +381,7 @@ spec: description: FailureTime stores the time that this CertificateRequest failed. This is used to influence garbage collection and back-off. type: string format: date-time - served: true + served: false storage: false - name: v1beta1 subresources: @@ -552,7 +552,7 @@ spec: description: FailureTime stores the time that this CertificateRequest failed. This is used to influence garbage collection and back-off. type: string format: date-time - served: true + served: false storage: false - name: v1 subresources: diff --git a/deploy/crds/crd-certificates.yaml b/deploy/crds/crd-certificates.yaml index 0d49ba5cc..24f2e5db9 100644 --- a/deploy/crds/crd-certificates.yaml +++ b/deploy/crds/crd-certificates.yaml @@ -360,7 +360,7 @@ spec: revision: description: "The current 'revision' of the certificate as issued. \n When a CertificateRequest resource is created, it will have the `cert-manager.io/certificate-revision` set to one greater than the current value of this field. \n Upon issuance, this field will be set to the value of the annotation on the CertificateRequest resource used to issue the certificate. \n Persisting the value on the CertificateRequest resource allows the certificates controller to know whether a request is part of an old issuance or if it is part of the ongoing revision's issuance by checking if the revision value in the annotation is greater than this field." type: integer - served: true + served: false storage: false - name: v1alpha3 subresources: @@ -677,7 +677,7 @@ spec: revision: description: "The current 'revision' of the certificate as issued. \n When a CertificateRequest resource is created, it will have the `cert-manager.io/certificate-revision` set to one greater than the current value of this field. \n Upon issuance, this field will be set to the value of the annotation on the CertificateRequest resource used to issue the certificate. \n Persisting the value on the CertificateRequest resource allows the certificates controller to know whether a request is part of an old issuance or if it is part of the ongoing revision's issuance by checking if the revision value in the annotation is greater than this field." type: integer - served: true + served: false storage: false - name: v1beta1 subresources: @@ -996,7 +996,7 @@ spec: revision: description: "The current 'revision' of the certificate as issued. \n When a CertificateRequest resource is created, it will have the `cert-manager.io/certificate-revision` set to one greater than the current value of this field. \n Upon issuance, this field will be set to the value of the annotation on the CertificateRequest resource used to issue the certificate. \n Persisting the value on the CertificateRequest resource allows the certificates controller to know whether a request is part of an old issuance or if it is part of the ongoing revision's issuance by checking if the revision value in the annotation is greater than this field." type: integer - served: true + served: false storage: false - name: v1 subresources: diff --git a/deploy/crds/crd-challenges.yaml b/deploy/crds/crd-challenges.yaml index 05f1c3380..2398991b7 100644 --- a/deploy/crds/crd-challenges.yaml +++ b/deploy/crds/crd-challenges.yaml @@ -1010,7 +1010,7 @@ spec: - invalid - expired - errored - served: true + served: false storage: false subresources: status: {} @@ -1981,7 +1981,7 @@ spec: - invalid - expired - errored - served: true + served: false storage: false subresources: status: {} @@ -2953,7 +2953,7 @@ spec: - invalid - expired - errored - served: true + served: false storage: false subresources: status: {} diff --git a/deploy/crds/crd-clusterissuers.yaml b/deploy/crds/crd-clusterissuers.yaml index 6cf78f2dd..04a874850 100644 --- a/deploy/crds/crd-clusterissuers.yaml +++ b/deploy/crds/crd-clusterissuers.yaml @@ -1223,7 +1223,7 @@ spec: type: description: Type of the condition, known values are (`Ready`). type: string - served: true + served: false storage: false - name: v1alpha3 subresources: @@ -2406,7 +2406,7 @@ spec: type: description: Type of the condition, known values are (`Ready`). type: string - served: true + served: false storage: false - name: v1beta1 subresources: @@ -3591,7 +3591,7 @@ spec: type: description: Type of the condition, known values are (`Ready`). type: string - served: true + served: false storage: false - name: v1 subresources: diff --git a/deploy/crds/crd-issuers.yaml b/deploy/crds/crd-issuers.yaml index 314e70ded..e603fbaf8 100644 --- a/deploy/crds/crd-issuers.yaml +++ b/deploy/crds/crd-issuers.yaml @@ -1223,7 +1223,7 @@ spec: type: description: Type of the condition, known values are (`Ready`). type: string - served: true + served: false storage: false - name: v1alpha3 subresources: @@ -2406,7 +2406,7 @@ spec: type: description: Type of the condition, known values are (`Ready`). type: string - served: true + served: false storage: false - name: v1beta1 subresources: @@ -3591,7 +3591,7 @@ spec: type: description: Type of the condition, known values are (`Ready`). type: string - served: true + served: false storage: false - name: v1 subresources: diff --git a/deploy/crds/crd-orders.yaml b/deploy/crds/crd-orders.yaml index 98238c2db..26c55ebea 100644 --- a/deploy/crds/crd-orders.yaml +++ b/deploy/crds/crd-orders.yaml @@ -198,7 +198,7 @@ spec: url: description: URL of the Order. This will initially be empty when the resource is first created. The Order controller will populate this field when the Order is first processed. This field will be immutable after it is initially set. type: string - served: true + served: false storage: false - name: v1alpha3 subresources: @@ -355,7 +355,7 @@ spec: url: description: URL of the Order. This will initially be empty when the resource is first created. The Order controller will populate this field when the Order is first processed. This field will be immutable after it is initially set. type: string - served: true + served: false storage: false - name: v1beta1 subresources: @@ -513,7 +513,7 @@ spec: url: description: URL of the Order. This will initially be empty when the resource is first created. The Order controller will populate this field when the Order is first processed. This field will be immutable after it is initially set. type: string - served: true + served: false storage: false - name: v1 subresources: From 969ca6d91a315a794ca026465049806d3bd45181 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 29 Sep 2021 12:54:42 +0100 Subject: [PATCH 2/5] Use the v1 API rather than v1alpha2 in the API checker Signed-off-by: Richard Wall --- pkg/util/cmapichecker/BUILD.bazel | 4 ++-- pkg/util/cmapichecker/cmapichecker.go | 17 ++++++----------- pkg/util/cmapichecker/cmapichecker_test.go | 2 +- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/pkg/util/cmapichecker/BUILD.bazel b/pkg/util/cmapichecker/BUILD.bazel index 0b4ce65d6..34405fa15 100644 --- a/pkg/util/cmapichecker/BUILD.bazel +++ b/pkg/util/cmapichecker/BUILD.bazel @@ -6,7 +6,7 @@ go_library( importpath = "github.com/jetstack/cert-manager/pkg/util/cmapichecker", visibility = ["//visibility:public"], deps = [ - "//pkg/apis/certmanager/v1alpha2:go_default_library", + "//pkg/apis/certmanager/v1:go_default_library", "//pkg/apis/meta/v1:go_default_library", "@com_github_pkg_errors//:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", @@ -35,7 +35,7 @@ go_test( srcs = ["cmapichecker_test.go"], embed = [":go_default_library"], deps = [ - "//pkg/apis/certmanager/v1alpha2:go_default_library", + "//pkg/apis/certmanager/v1:go_default_library", "@io_k8s_apimachinery//pkg/runtime:go_default_library", "@io_k8s_sigs_controller_runtime//pkg/client:go_default_library", "@io_k8s_sigs_controller_runtime//pkg/client/fake:go_default_library", diff --git a/pkg/util/cmapichecker/cmapichecker.go b/pkg/util/cmapichecker/cmapichecker.go index e9dbd6477..dbb396421 100644 --- a/pkg/util/cmapichecker/cmapichecker.go +++ b/pkg/util/cmapichecker/cmapichecker.go @@ -27,11 +27,7 @@ import ( "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" - // Use v1alpha2 API to ensure that the API server has also connected to the - // cert-manager conversion webhook. - // TODO(wallrj): Only change this when the old deprecated APIs are removed, - // at which point the conversion webhook may be removed anyway. - cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" ) @@ -81,11 +77,13 @@ func New(restcfg *rest.Config, scheme *runtime.Scheme, namespace string) (Interf }, nil } -// Check attempts to perform a dry-run create of a cert-manager *v1alpha2* +// Check attempts to perform a dry-run create of a cert-manager // Certificate resource in order to verify that CRDs are installed and all the // required webhooks are reachable by the K8S API server. -// We use v1alpha2 API to ensure that the API server has also connected to the -// cert-manager conversion webhook. +// Originally we used the v1alpha2 API to ensure that the API server has also +// connected to the cert-manager conversion webhook, but since cert-manager 1.6 +// we have disabled the serving of non-v1 CRD versions, so it is no longer +// possible to test the reachability of the conversion webhook. func (o *cmapiChecker) Check(ctx context.Context) error { cert := &cmapi.Certificate{ ObjectMeta: metav1.ObjectMeta{ @@ -135,13 +133,10 @@ func (e *ApiCheckError) Unwrap() error { // - error finding the scope of the object: failed to get restmapping: no matches for kind "Certificate" in group "cert-manager.io" // ErrWebhookServiceFailure: // - Internal error occurred: failed calling webhook "webhook.cert-manager.io": Post "https://cert-manager-webhook.cert-manager.svc:443/mutate?timeout=10s": service "cert-manager-webhook" not found -// - conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": service "cert-manager-webhook" not found // ErrWebhookDeploymentFailure: // - Internal error occurred: failed calling webhook "webhook.cert-manager.io": Post "https://cert-manager-webhook.cert-manager.svc:443/mutate?timeout=10s": dial tcp 10.96.38.90:443: connect: connection refused -// - conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": dial tcp 10.96.38.90:443: connect: connection refused // ErrWebhookCertificateFailure: // - Internal error occurred: failed calling webhook "webhook.cert-manager.io": Post "https://cert-manager-webhook.cert-manager.svc:443/mutate?timeout=10s": x509: certificate signed by unknown authority (possibly because of "x509: ECDSA verification failure" while trying to verify candidate authority certificate "cert-manager-webhook-ca") -// - conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": x509: certificate signed by unknown authority func translateToSimpleError(err error) error { s := err.Error() diff --git a/pkg/util/cmapichecker/cmapichecker_test.go b/pkg/util/cmapichecker/cmapichecker_test.go index cd8f5638e..0cc3c0d4b 100644 --- a/pkg/util/cmapichecker/cmapichecker_test.go +++ b/pkg/util/cmapichecker/cmapichecker_test.go @@ -26,7 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" ) type fakeErrorClient struct { From b71eb11fd1aa36e8897e8318ea6b32c78a4533fc Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 29 Sep 2021 13:02:44 +0100 Subject: [PATCH 3/5] A note about the relevance of conversion webhook unit-tests Signed-off-by: Richard Wall --- pkg/util/cmapichecker/cmapichecker_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/util/cmapichecker/cmapichecker_test.go b/pkg/util/cmapichecker/cmapichecker_test.go index 0cc3c0d4b..49b4c8ee9 100644 --- a/pkg/util/cmapichecker/cmapichecker_test.go +++ b/pkg/util/cmapichecker/cmapichecker_test.go @@ -67,6 +67,12 @@ const ( errMutatingWebhookDeploymentFailure = `Internal error occurred: failed calling webhook "webhook.cert-manager.io": Post "https://cert-manager-webhook.cert-manager.svc:443/mutate?timeout=10s": dial tcp 10.96.38.90:443: connect: connection refused` errMutatingWebhookCertificateFailure = `Internal error occurred: failed calling webhook "webhook.cert-manager.io": Post "https://cert-manager-webhook.cert-manager.svc:443/mutate?timeout=10s": x509: certificate signed by unknown authority (possibly because of "x509: ECDSA verification failure" while trying to verify candidate authority certificate "cert-manager-webhook-ca"` + // These /convert error examples test that we can correctly parse errors + // while connecting to the conversion webhook, + // but as of cert-manager 1.6 the conversion webhook will no-longer be used + // because legacy CRD versions will no longer be "served" + // and in 1.7 the conversion webhook may be removed at which point these can + // be removed too. errConversionWebhookServiceFailure = `conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": service "cert-manager-webhook" not found` errConversionWebhookDeploymentFailure = `conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": dial tcp 10.96.38.90:443: connect: connection refused` errConversionWebhookCertificateFailure = `conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": x509: certificate signed by unknown authority` From 41ef0e3f2b32ae5743ef4d5bc63093b67349a316 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 29 Sep 2021 13:05:53 +0100 Subject: [PATCH 4/5] A note about testing the handling of errors relating to the ValidatingWebhook Signed-off-by: Richard Wall --- pkg/util/cmapichecker/cmapichecker_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/util/cmapichecker/cmapichecker_test.go b/pkg/util/cmapichecker/cmapichecker_test.go index 49b4c8ee9..a86a2a2fa 100644 --- a/pkg/util/cmapichecker/cmapichecker_test.go +++ b/pkg/util/cmapichecker/cmapichecker_test.go @@ -73,6 +73,8 @@ const ( // because legacy CRD versions will no longer be "served" // and in 1.7 the conversion webhook may be removed at which point these can // be removed too. + // TODO: Add tests for errors when connecting to the /validate + // ValidatingWebhook endpoint. errConversionWebhookServiceFailure = `conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": service "cert-manager-webhook" not found` errConversionWebhookDeploymentFailure = `conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": dial tcp 10.96.38.90:443: connect: connection refused` errConversionWebhookCertificateFailure = `conversion webhook for cert-manager.io/v1alpha2, Kind=Certificate failed: Post "https://cert-manager-webhook.cert-manager.svc:443/convert?timeout=30s": x509: certificate signed by unknown authority` From 481fc5e43af69197a1128a3efacfe26e14ff3b82 Mon Sep 17 00:00:00 2001 From: Richard Wall Date: Wed, 29 Sep 2021 14:32:35 +0100 Subject: [PATCH 5/5] Force the "served: true" in the CRDs when used in integration tests These tests still need to create legacy API versions. Signed-off-by: Richard Wall --- test/integration/framework/apiserver.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/integration/framework/apiserver.go b/test/integration/framework/apiserver.go index cc05d1d80..31ff4d190 100644 --- a/test/integration/framework/apiserver.go +++ b/test/integration/framework/apiserver.go @@ -57,6 +57,7 @@ func RunControlPlane(t *testing.T, ctx context.Context) (*rest.Config, StopFunc) t.Logf("Found CRD with name %q", crd.Name) } patchCRDConversion(crds, webhookOpts.URL, webhookOpts.CAPEM) + patchCRDServed(crds) if _, err := envtest.InstallCRDs(config, envtest.CRDInstallOptions{ CRDs: crdsToRuntimeObjects(crds), @@ -98,6 +99,9 @@ func init() { func patchCRDConversion(crds []*v1.CustomResourceDefinition, url string, caPEM []byte) { for _, crd := range crds { + for i := range crd.Spec.Versions { + crd.Spec.Versions[i].Served = true + } if crd.Spec.Conversion == nil { continue } @@ -262,3 +266,16 @@ func getMutatingWebhookConfig(url string, caPEM []byte) client.Object { return &webhook } + +// patchCRDServed ensures that even the API versions which are not served are +// available in the integration tests. +// This workaround allows the conversion tests and the ctl convert tests to run. +// TODO: Remove this workaround in cert-manager 1.7 when all the legacy API +// versions will finally be removed. +func patchCRDServed(crds []*v1.CustomResourceDefinition) { + for _, crd := range crds { + for i := range crd.Spec.Versions { + crd.Spec.Versions[i].Served = true + } + } +}