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 <i@am.so-aweso.me>

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",

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 {

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")
+}

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 (

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()

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
 	}

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",