From 69f36123cef90676dc4bfa065d1cf72cf98f409a Mon Sep 17 00:00:00 2001 From: Jake Sanders Date: Tue, 21 Sep 2021 09:20:34 +0100 Subject: [PATCH] Use IngressClassName field for v1 Ingresses As of Kubernetes 1.18, the deprecated annotation should not be used. This coincides with the support for networking v1 Ingresses in the Kubernetes API. This commit adds logic to the internal conversion code to add the annotation back on v1beta1 Ingresses, used on Kubernetes 1.16 and 1.17 Signed-off-by: Jake Sanders --- pkg/internal/ingress/BUILD.bazel | 6 +- pkg/internal/ingress/convert.go | 53 ++++++++- pkg/internal/ingress/convert_test.go | 157 +++++++++++++++++++++++++++ pkg/internal/ingress/ingress.go | 8 +- pkg/internal/ingress/ingress_test.go | 2 + pkg/issuer/acme/http/ingress.go | 18 ++- pkg/issuer/acme/http/ingress_test.go | 1 - 7 files changed, 229 insertions(+), 16 deletions(-) create mode 100644 pkg/internal/ingress/convert_test.go diff --git a/pkg/internal/ingress/BUILD.bazel b/pkg/internal/ingress/BUILD.bazel index 3df865a22..cc61fb136 100644 --- a/pkg/internal/ingress/BUILD.bazel +++ b/pkg/internal/ingress/BUILD.bazel @@ -29,7 +29,10 @@ go_library( go_test( name = "go_default_test", - srcs = ["ingress_test.go"], + srcs = [ + "convert_test.go", + "ingress_test.go", + ], embed = [":go_default_library"], deps = [ "//pkg/controller:go_default_library", @@ -40,6 +43,7 @@ go_test( "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_apimachinery//pkg/labels:go_default_library", "@io_k8s_apimachinery//pkg/runtime/schema:go_default_library", + "@io_k8s_apimachinery//pkg/util/intstr:go_default_library", "@io_k8s_client_go//informers:go_default_library", "@io_k8s_client_go//kubernetes/fake:go_default_library", "@io_k8s_utils//pointer:go_default_library", diff --git a/pkg/internal/ingress/convert.go b/pkg/internal/ingress/convert.go index 0f9751ea5..aef164390 100644 --- a/pkg/internal/ingress/convert.go +++ b/pkg/internal/ingress/convert.go @@ -45,8 +45,30 @@ See the License for the specific language governing permissions and limitations under the License. */ +// Convert_networking_Ingress_To_v1beta1_Ingress uses unsafe pointer manipulation to manipulate a +// *networkingv1beta1.Ingress into pointing at the same underlying data as the input *networkingv1.Ingress. +// Both the `in` and `out` Object's data will be manipulated by this function. +// +// Recommended usage: +// // as in and out will point to the same data, make sure any manipulation doesn't affect the original Ingress +// in := myIngress.DeepCopy() +// out := new(networkingv1beta1.Ingress) +// err := Convert_networking_Ingress_To_v1beta1_Ingress(in, out, nil) func Convert_networking_Ingress_To_v1beta1_Ingress(in *networkingv1.Ingress, out *networkingv1beta1.Ingress, s conversion.Scope) error { - return autoConvert_networking_Ingress_To_v1beta1_Ingress(in, out, s) + err := autoConvert_networking_Ingress_To_v1beta1_Ingress(in, out, s) + if err != nil { + return err + } + // v1beta1 Ingresses should not have IngressClassName set but instead use the deprecated annotation. + // Move the ingress class to the annotations and then zero the IngressClassName field + if out.Spec.IngressClassName != nil { + if out.Annotations == nil { + out.Annotations = make(map[string]string) + } + out.Annotations["kubernetes.io/ingress.class"] = *out.Spec.IngressClassName + out.Spec.IngressClassName = nil + } + return nil } func autoConvert_networking_Ingress_To_v1beta1_Ingress(in *networkingv1.Ingress, out *networkingv1beta1.Ingress, s conversion.Scope) error { @@ -193,8 +215,35 @@ func autoConvert_networking_IngressBackend_To_v1beta1_IngressBackend(in *network return nil } +// Convert_v1beta1_Ingress_To_networking_Ingress uses unsafe pointer manipulation to manipulate a +// *networkingv1.Ingress into pointing at the same underlying data as the input *networkingv1beta1.Ingress. +// Both the `in` and `out` Object's data will be manipulated by this function. +// +// Recommended usage: +// // as in and out will point to the same data, make sure any manipulation doesn't affect the original Ingress +// in := myIngress.DeepCopy() +// out := new(networkingv1.Ingress) +// err := Convert_v1beta1_Ingress_To_networking_Ingress(in, out, nil) func Convert_v1beta1_Ingress_To_networking_Ingress(in *networkingv1beta1.Ingress, out *networkingv1.Ingress, s conversion.Scope) error { - return autoConvert_v1beta1_Ingress_To_networking_Ingress(in, out, s) + err := autoConvert_v1beta1_Ingress_To_networking_Ingress(in, out, s) + if err != nil { + return err + } + // v1beta1 Ingresses should not have IngressClassName set but instead use the deprecated annotation. + // Move the ingress class from the annotations to the Spec + if in.Annotations == nil { + return nil + } + if ingressClass, found := in.Annotations["kubernetes.io/ingress.class"]; found { + out.Spec.IngressClassName = &ingressClass + // HERE BE DRAGONS: + // in.Annotations and out.Annotations point to the same map. + // This mutates in as well as out, so make sure in is not an object in + // client-go's cache, for example by only passing DeepCopy()d objects + // to Convert_v1beta1_Ingress_To_networking_Ingress + delete(out.Annotations, "kubernetes.io/ingress.class") + } + return nil } func autoConvert_v1beta1_Ingress_To_networking_Ingress(in *networkingv1beta1.Ingress, out *networkingv1.Ingress, s conversion.Scope) error { diff --git a/pkg/internal/ingress/convert_test.go b/pkg/internal/ingress/convert_test.go new file mode 100644 index 000000000..fb09d391e --- /dev/null +++ b/pkg/internal/ingress/convert_test.go @@ -0,0 +1,157 @@ +/* +Copyright 2021 The cert-manager Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package ingress + +import ( + "testing" + + "github.com/stretchr/testify/assert" + networkingv1 "k8s.io/api/networking/v1" + networkingv1beta1 "k8s.io/api/networking/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/pointer" +) + +var v1TestIngress = &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-networkingv1-ingress", + Namespace: "test-networkingv1-namespace", + Annotations: map[string]string{ + "test.key": "test.value", + }, + Labels: map[string]string{ + "labelkey": "labelvalue", + }, + }, + Spec: networkingv1.IngressSpec{ + IngressClassName: pointer.String("bogus-ingress-class"), + DefaultBackend: &networkingv1.IngressBackend{ + Service: &networkingv1.IngressServiceBackend{ + Name: "default-backend-svc", + Port: networkingv1.ServiceBackendPort{ + Number: 1234, + }, + }, + }, + TLS: []networkingv1.IngressTLS{ + { + Hosts: []string{"aaa.", "bbb.", "ccc.ddd"}, + SecretName: "test-secret-1", + }, + { + Hosts: []string{"eee"}, + SecretName: "test-secret-2", + }, + }, + Rules: []networkingv1.IngressRule{ + { + Host: "aaa", + IngressRuleValue: networkingv1.IngressRuleValue{ + HTTP: &networkingv1.HTTPIngressRuleValue{ + Paths: []networkingv1.HTTPIngressPath{ + { + Path: "/.well-known/acme-challenge", + PathType: func() *networkingv1.PathType { p := networkingv1.PathTypeImplementationSpecific; return &p }(), + Backend: networkingv1.IngressBackend{ + Service: &networkingv1.IngressServiceBackend{ + Name: "test-solver-backend", + Port: networkingv1.ServiceBackendPort{Number: 80}, + }, + }, + }, + }, + }, + }, + }, + }, + }, +} + +var v1beta1TestIngress = &networkingv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-networkingv1-ingress", + Namespace: "test-networkingv1-namespace", + Annotations: map[string]string{ + "test.key": "test.value", + "kubernetes.io/ingress.class": "bogus-ingress-class", + }, + Labels: map[string]string{ + "labelkey": "labelvalue", + }, + }, + Spec: networkingv1beta1.IngressSpec{ + Backend: &networkingv1beta1.IngressBackend{ + ServiceName: "default-backend-svc", + ServicePort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 1234, + }, + }, + Rules: []networkingv1beta1.IngressRule{ + { + Host: "aaa", + IngressRuleValue: networkingv1beta1.IngressRuleValue{ + HTTP: &networkingv1beta1.HTTPIngressRuleValue{ + Paths: []networkingv1beta1.HTTPIngressPath{ + { + Path: "/.well-known/acme-challenge", + PathType: func() *networkingv1beta1.PathType { p := networkingv1beta1.PathTypeImplementationSpecific; return &p }(), + Backend: networkingv1beta1.IngressBackend{ + ServiceName: "test-solver-backend", + ServicePort: intstr.IntOrString{ + Type: intstr.Int, + IntVal: 80, + }, + }, + }, + }, + }, + }, + }, + }, + TLS: []networkingv1beta1.IngressTLS{ + { + Hosts: []string{"aaa.", "bbb.", "ccc.ddd"}, + SecretName: "test-secret-1", + }, + { + Hosts: []string{"eee"}, + SecretName: "test-secret-2", + }, + }, + }, + Status: networkingv1beta1.IngressStatus{}, +} + +func TestConvert_networking_Ingress_To_v1beta1_Ingress(t *testing.T) { + in := v1TestIngress.DeepCopy() + out := &networkingv1beta1.Ingress{} + err := Convert_networking_Ingress_To_v1beta1_Ingress(in, out, nil) + assert.NoError(t, err, "converting networking v1 to networking v1beta1 Ingress should not fail") + expected := v1beta1TestIngress.DeepCopy() + assert.Equal(t, expected, out, "Conversion from networking v1 to networking v1beta1 Ingress was not as expected") +} + +func TestConvert_v1beta1_Ingress_To_networking_Ingress(t *testing.T) { + in := v1beta1TestIngress.DeepCopy() + out := &networkingv1.Ingress{} + err := Convert_v1beta1_Ingress_To_networking_Ingress(in, out, nil) + assert.NoError(t, err, "converting networking v1beta1 to networking v1 Ingress should not fail") + expected := v1TestIngress.DeepCopy() + assert.Equal(t, expected, out, "Conversion from networking v1beta1 to networking v1 Ingress was not as expected") +} diff --git a/pkg/internal/ingress/ingress.go b/pkg/internal/ingress/ingress.go index 65977815e..6b8d01542 100644 --- a/pkg/internal/ingress/ingress.go +++ b/pkg/internal/ingress/ingress.go @@ -14,7 +14,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package ingress lets us use an internal type for supporting multiple kinds of ingresses +// Package ingress lets us use an internal type for supporting multiple kinds of ingresses. +// +// This package's sole reason for existence is for compatibility with Kubernetes API servers +// below v1.18. However, our client-go library is already beyond the supported version skew +// (https://kubernetes.io/releases/version-skew-policy/) so it is not guaranteed to continue +// working in the future, and will definitely be removed once cert-manager no longer supports +// Kubernetes 1.17. package ingress import ( diff --git a/pkg/internal/ingress/ingress_test.go b/pkg/internal/ingress/ingress_test.go index ec931aa32..8fb1fc2d0 100644 --- a/pkg/internal/ingress/ingress_test.go +++ b/pkg/internal/ingress/ingress_test.go @@ -36,6 +36,8 @@ import ( discoveryfake "github.com/jetstack/cert-manager/test/unit/discovery" ) +// Important: these tests cannot run in parallel as the cache holds internal state at the package level. + func TestFunctionalityAgainstV1(t *testing.T) { // wipe known versions cache cacheLock.Lock() diff --git a/pkg/issuer/acme/http/ingress.go b/pkg/issuer/acme/http/ingress.go index 803c90d87..c94ddc384 100644 --- a/pkg/issuer/acme/http/ingress.go +++ b/pkg/issuer/acme/http/ingress.go @@ -29,7 +29,6 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1" - cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" "github.com/jetstack/cert-manager/pkg/issuer/acme/http/solver" logf "github.com/jetstack/cert-manager/pkg/logs" ) @@ -73,7 +72,7 @@ func (s *Solver) getIngressesForChallenge(ctx context.Context, ch *cmacme.Challe // that the ingress has an appropriate challenge path configured func (s *Solver) ensureIngress(ctx context.Context, ch *cmacme.Challenge, svcName string) (ing *networkingv1.Ingress, err error) { log := logf.FromContext(ctx).WithName("ensureIngress") - httpDomainCfg, err := httpDomainCfgForChallenge(ch) + httpDomainCfg, err := http01IngressCfgForChallenge(ch) if err != nil { return nil, err } @@ -134,13 +133,13 @@ func (s *Solver) createIngress(ctx context.Context, ch *cmacme.Challenge, svcNam } func buildIngressResource(ch *cmacme.Challenge, svcName string) (*networkingv1.Ingress, error) { - httpDomainCfg, err := httpDomainCfgForChallenge(ch) + http01IngressCfg, err := http01IngressCfgForChallenge(ch) if err != nil { return nil, err } var ingClass *string - if httpDomainCfg.Class != nil { - ingClass = httpDomainCfg.Class + if http01IngressCfg.Class != nil { + ingClass = http01IngressCfg.Class } podLabels := podLabels(ch) @@ -150,10 +149,6 @@ func buildIngressResource(ch *cmacme.Challenge, svcName string) (*networkingv1.I // TODO: Figure out how to remove this without breaking users who depend on it. ingAnnotations["nginx.ingress.kubernetes.io/whitelist-source-range"] = "0.0.0.0/0,::/0" - if ingClass != nil { - ingAnnotations[cmapi.IngressClassAnnotationKey] = *ingClass - } - ingPathToAdd := ingressPath(ch.Spec.Token, svcName) httpHost := ch.Spec.DNSName @@ -170,6 +165,7 @@ func buildIngressResource(ch *cmacme.Challenge, svcName string) (*networkingv1.I OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(ch, challengeGvk)}, }, Spec: networkingv1.IngressSpec{ + IngressClassName: ingClass, Rules: []networkingv1.IngressRule{ { Host: httpHost, @@ -212,7 +208,7 @@ func (s *Solver) mergeIngressObjectMetaWithIngressResourceTemplate(ingress *netw } func (s *Solver) addChallengePathToIngress(ctx context.Context, ch *cmacme.Challenge, svcName string) (*networkingv1.Ingress, error) { - httpDomainCfg, err := httpDomainCfgForChallenge(ch) + httpDomainCfg, err := http01IngressCfgForChallenge(ch) if err != nil { return nil, err } @@ -270,7 +266,7 @@ func (s *Solver) cleanupIngresses(ctx context.Context, ch *cmacme.Challenge) err return nil } - httpDomainCfg, err := httpDomainCfgForChallenge(ch) + httpDomainCfg, err := http01IngressCfgForChallenge(ch) if err != nil { return err } diff --git a/pkg/issuer/acme/http/ingress_test.go b/pkg/issuer/acme/http/ingress_test.go index 7453ccb05..e601fee5b 100644 --- a/pkg/issuer/acme/http/ingress_test.go +++ b/pkg/issuer/acme/http/ingress_test.go @@ -530,7 +530,6 @@ func TestMergeIngressObjectMetaWithIngressResourceTemplate(t *testing.T) { cmacme.SolverIdentificationLabelKey: "true", } expectedIngress.Annotations = map[string]string{ - "kubernetes.io/ingress.class": "nginx", "nginx.ingress.kubernetes.io/whitelist-source-range": "0.0.0.0/0,::/0", "nginx.org/mergeable-ingress-type": "minion", "traefik.ingress.kubernetes.io/frontend-entry-points": "http",