Repository: kubernetes/ingress-nginx
Pull request #6445 (2020-11-09 03:46:58)

fix: empty IngressClassName, Error handling
<!--- Provide a general summary of your changes in the Title above --->
<!--- Please don't @-mention people in PR or commit messages (do so in an additional comment). --->

## What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Which issue/s this PR fixes
<!--
(optional, in `fixes #<issue number>` format, will close that issue when PR gets merged):

fixes #
-->

```yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
  name: wisecloud-test
  namespace: wisecloud-controller
spec:
  ingressClassName: nginx1
  rules:
  - host: "10.0.0.2.nip.io"
    http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: cluster-proxy
            port:
              number: 8080
```

```yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
  name: wisecloud-test
  namespace: wisecloud-controller
spec:
  ingressClassName: nginx
  rules:
  - host: "10.0.0.2.nip.io"
    http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: cluster-proxy
            port:
              number: 8080
```

Before fixing, the following two rules will conflict

```yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: "nginx1"
  name: wisecloud-test
  namespace: wisecloud-controller
spec:
  rules:
  - host: "10.0.0.2.nip.io"
    http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: cluster-proxy
            port:
              number: 8080
```

```yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: "nginx"
  name: wisecloud-test
  namespace: wisecloud-controller
spec:
  rules:
  - host: "10.0.0.2.nip.io"
    http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: cluster-proxy
            port:
              number: 8080
```

They belong to different ʻIngressClass`, using annotation, the behavior obtained is expected. However, passing ingressClassName will affect the normal operation of multiple Ingress

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

I think the following tests are needed:

 - Deploy the above yaml
 - Set the default IngressClass, Deploy the above yaml
 - Set IngressClass of Ingress, Deploy the above yaml
 - Set ingressClassName and kubernetes.io/ingress.class at the same time. According to the design, the priority of kubernetes.io/ingress.class is greater than ingressClassName

If missing related tests, hope to add

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/master/CONTRIBUTING.md) guide
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.



Patch:
diff --git a/internal/ingress/annotations/class/main.go b/internal/ingress/annotations/class/main.go
index 0f5284f43..b2fe32d63 100644
--- a/internal/ingress/annotations/class/main.go
+++ b/internal/ingress/annotations/class/main.go
@@ -41,30 +41,24 @@ var (
 // IsValid returns true if the given Ingress specify the ingress.class
 // annotation or IngressClassName resource for Kubernetes >= v1.18
 func IsValid(ing *networking.Ingress) bool {
-	// 1. with annotation
+	// 1. with annotation or IngressClass
 	ingress, ok := ing.GetAnnotations()[IngressKey]
-	if ok {
-		// empty annotation and same annotation on ingress
-		if ingress == "" && IngressClass == DefaultClass {
-			return true
-		}
-
-		return ingress == IngressClass
+	if !ok && ing.Spec.IngressClassName != nil {
+		ingress = *ing.Spec.IngressClassName
 	}
 
-	// 2. k8s < v1.18. Check default annotation
-	if !k8s.IsIngressV1Beta1Ready {
-		return IngressClass == DefaultClass
+	// empty ingress and IngressClass equal default
+	if len(ingress) == 0 && IngressClass == DefaultClass {
+		return true
 	}
 
-	// 3. without annotation and IngressClass. Check default annotation
-	if k8s.IngressClass == nil {
-		return IngressClass == DefaultClass
+	// k8s > v1.18.
+	// Processing may be redundant because k8s.IngressClass is obtained by IngressClass
+	// 3. without annotation and IngressClass. Check IngressClass
+	if k8s.IngressClass != nil {
+		return ingress == k8s.IngressClass.Name
 	}
 
 	// 4. with IngressClass
-	if ing.Spec.IngressClassName != nil {
-		return k8s.IngressClass.Name == *ing.Spec.IngressClassName
-	}
-	return false
+	return ingress == IngressClass
 }


Test patch:
diff --git a/internal/ingress/annotations/class/main_test.go b/internal/ingress/annotations/class/main_test.go
index 9aa891822..2d863f072 100644
--- a/internal/ingress/annotations/class/main_test.go
+++ b/internal/ingress/annotations/class/main_test.go
@@ -39,28 +39,32 @@ func TestIsValidClass(t *testing.T) {
 	}()
 
 	tests := []struct {
-		ingress    string
-		controller string
-		defClass   string
-		annotation bool
-		k8sClass   *networking.IngressClass
-		v1Ready    bool
-		isValid    bool
+		ingress          string
+		controller       string
+		defClass         string
+		annotation       bool
+		ingressClassName bool
+		k8sClass         *networking.IngressClass
+		v1Ready          bool
+		isValid          bool
 	}{
-		{"", "", "nginx", true, nil, false, true},
-		{"", "nginx", "nginx", true, nil, false, true},
-		{"nginx", "nginx", "nginx", true, nil, false, true},
-		{"custom", "custom", "nginx", true, nil, false, true},
-		{"", "killer", "nginx", true, nil, false, false},
-		{"custom", "nginx", "nginx", true, nil, false, false},
-		{"", "custom", "nginx", false,
+		{"", "", "nginx", true, false, nil, false, true},
+		{"", "nginx", "nginx", true, false, nil, false, true},
+		{"nginx", "nginx", "nginx", true, false, nil, false, true},
+		{"custom", "custom", "nginx", true, false, nil, false, true},
+		{"", "killer", "nginx", true, false, nil, false, false},
+		{"custom", "nginx", "nginx", true, false, nil, false, false},
+		{"nginx", "nginx", "nginx", false, true, nil, false, true},
+		{"custom", "nginx", "nginx", false, true, nil, true, false},
+		{"nginx", "nginx", "nginx", false, true, nil, true, true},
+		{"", "custom", "nginx", false, false,
 			&networking.IngressClass{
 				ObjectMeta: meta_v1.ObjectMeta{
 					Name: "custom",
 				},
 			},
 			false, false},
-		{"", "custom", "nginx", false,
+		{"", "custom", "nginx", false, false,
 			&networking.IngressClass{
 				ObjectMeta: meta_v1.ObjectMeta{
 					Name: "custom",
@@ -82,6 +86,9 @@ func TestIsValidClass(t *testing.T) {
 		if test.annotation {
 			ing.Annotations[IngressKey] = test.ingress
 		}
+		if test.ingressClassName {
+			ing.Spec.IngressClassName = &[]string{test.ingress}[0]
+		}
 
 		IngressClass = test.controller
 		DefaultClass = test.defClass