Merge pull request #4482 from wallrj/4380-stop-serving-deprecated-apis

Stop serving deprecated APIs
2021-09-29 15:22:05 +01:00 · 2021-09-29 15:22:05 +01:00 · d24702fb5f
commit d24702fb5f
parent 898dc0e254 481fc5e43a
10 changed files with 52 additions and 32 deletions
--- 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:
--- 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:
--- 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: {}
--- 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:
--- 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:
--- 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:
--- 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",
--- 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()

--- 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 {
@ -67,6 +67,14 @@ 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.
+	// 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`
--- 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
+		}
+	}
+}