Merge pull request #2159 from songrx1997/check-gke-ingress

Add frontendConfig annotation check before frontendConfig checks and update tests
kubernetes · Jun 8, 2023 · a5e5f72 · a5e5f72
2 parents e83abbd + 276d6b8
commit a5e5f72
Show file tree

Hide file tree

Showing 3 changed files with 102 additions and 19 deletions.
diff --git a/cmd/check-gke-ingress/app/ingress/ingress.go b/cmd/check-gke-ingress/app/ingress/ingress.go
@@ -80,16 +80,19 @@ func CheckAllIngresses(namespace string, client kubernetes.Interface, beconfigCl
 			addCheckResult(ingressRes, checkName, msg, res)
 		}
 
-		// FrontendConfig related checks
-		feconfigChecker := &FrontendConfigChecker{
-			client:    feConfigClient,
-			namespace: ingress.Namespace,
-			name:      ingress.Name,
-		}
+		feConfigName, ok := getFrontendConfigAnnotation(&ingress)
+		if ok {
+			// FrontendConfig related checks
+			feconfigChecker := &FrontendConfigChecker{
+				client:    feConfigClient,
+				namespace: ingress.Namespace,
+				name:      feConfigName,
+			}
 
-		for _, check := range feconfigChecks {
-			checkName, res, msg := check(feconfigChecker)
-			addCheckResult(ingressRes, checkName, msg, res)
+			for _, check := range feconfigChecks {
+				checkName, res, msg := check(feconfigChecker)
+				addCheckResult(ingressRes, checkName, msg, res)
+			}
 		}
 
 		// Get the names of the services referenced by the ingress.

diff --git a/cmd/check-gke-ingress/app/ingress/rule.go b/cmd/check-gke-ingress/app/ingress/rule.go
@@ -118,7 +118,7 @@ func CheckL7ILBFrontendConfig(c *IngressChecker) (string, string, string) {
 		return L7ILBFrontendConfigCheck, report.Skipped, fmt.Sprintf("Ingress %s/%s is not for L7 internal load balancing", c.ingress.Namespace, c.ingress.Name)
 	}
 	if _, ok := getFrontendConfigAnnotation(c.ingress); ok {
-		return L7ILBFrontendConfigCheck, report.Failed, fmt.Sprintf("Ingress %s/%s for L7 internal load balancing has a frontendConfig annotation", c.ingress.Namespace, c.ingress.Name)
+		return L7ILBFrontendConfigCheck, report.Failed, fmt.Sprintf("Ingress %s/%s for L7 internal load balancing has a frontendConfig annotation, frontendConfig can only be used with external ingresses", c.ingress.Namespace, c.ingress.Name)
 	}
 	return L7ILBFrontendConfigCheck, report.Passed, fmt.Sprintf("Ingress %s/%s for L7 internal load balancing does not have a frontendConfig annotation", c.ingress.Namespace, c.ingress.Name)
 }
@@ -205,7 +205,7 @@ func CheckL7ILBNegAnnotation(c *ServiceChecker) (string, string, string) {
 	}
 	val, ok := getNegAnnotation(c.service)
 	if !ok {
-		return L7ILBNegAnnotationCheck, report.Failed, fmt.Sprintf("No Neg annotation found in service %s/%s for internal HTTP(S) load balancing", c.namespace, c.name)
+		return L7ILBNegAnnotationCheck, report.Failed, fmt.Sprintf("No Neg annotation found in service %s/%s for internal HTTP(S) load balancing, internal ingress requires Neg as backends", c.namespace, c.name)
 	}
 	var res annotations.NegAnnotation
 	if err := json.Unmarshal([]byte(val), &res); err != nil {

diff --git a/cmd/check-gke-ingress/app/ingress/rule_test.go b/cmd/check-gke-ingress/app/ingress/rule_test.go
@@ -645,6 +645,9 @@ func TestCheckAllIngresses(t *testing.T) {
 	beClient := fakebeconfig.NewSimpleClientset()
 	feClient := fakefeconfig.NewSimpleClientset()
 
+	thirtyVar := int64(30)
+	twentyVar := int64(20)
+
 	ingress1 := networkingv1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "ingress1",
@@ -667,7 +670,7 @@ func TestCheckAllIngresses(t *testing.T) {
 			Name:      "ingress2",
 			Namespace: "test",
 			Annotations: map[string]string{
-				annotations.IngressClassKey: annotations.GceL7ILBIngressClass,
+				annotations.FrontendConfigKey: "feConfig-2",
 			},
 		},
 		Spec: networkingv1.IngressSpec{
@@ -681,7 +684,7 @@ func TestCheckAllIngresses(t *testing.T) {
 									Path: "/*",
 									Backend: networkingv1.IngressBackend{
 										Service: &networkingv1.IngressServiceBackend{
-											Name: "svc-1",
+											Name: "svc-2",
 										},
 									},
 								},
@@ -698,26 +701,57 @@ func TestCheckAllIngresses(t *testing.T) {
 	client.NetworkingV1().Ingresses("test").Create(context.TODO(), &ingress1, metav1.CreateOptions{})
 	client.NetworkingV1().Ingresses("test").Create(context.TODO(), &ingress2, metav1.CreateOptions{})
 
-	serivce1 := corev1.Service{
+	client.CoreV1().Services("test").Create(context.TODO(), &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "svc-1",
 			Namespace: "test",
 			Annotations: map[string]string{
-				annotations.BackendConfigKey: `{"default": "beconfig1"}`,
+				annotations.BackendConfigKey:              `{"default": "beconfig1"}`,
+				annotations.ServiceApplicationProtocolKey: `{"8443":"HTTP3","8080":"HTTP"}`,
 			},
 		},
-	}
-	client.CoreV1().Services("test").Create(context.TODO(), &serivce1, metav1.CreateOptions{})
+	}, metav1.CreateOptions{})
+
+	client.CoreV1().Services("test").Create(context.TODO(), &corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "svc-2",
+			Namespace: "test",
+			Annotations: map[string]string{
+				annotations.BackendConfigKey: `{"default": "beconfig2"}`,
+			},
+		},
+	}, metav1.CreateOptions{})
 
 	beClient.CloudV1().BackendConfigs("test").Create(context.TODO(), &beconfigv1.BackendConfig{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "test",
 			Name:      "beconfig1",
 		},
 	}, metav1.CreateOptions{})
-	report := CheckAllIngresses("test", client, beClient, feClient)
+
+	beClient.CloudV1().BackendConfigs("test").Create(context.TODO(), &beconfigv1.BackendConfig{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "test",
+			Name:      "beconfig2",
+		},
+		Spec: beconfigv1.BackendConfigSpec{
+			HealthCheck: &beconfigv1.HealthCheckConfig{
+				CheckIntervalSec: &twentyVar,
+				TimeoutSec:       &thirtyVar,
+			},
+		},
+	}, metav1.CreateOptions{})
+
+	feClient.NetworkingV1beta1().FrontendConfigs("test").Create(context.TODO(), &feconfigv1beta1.FrontendConfig{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "test",
+			Name:      "feConfig-2",
+		},
+	}, metav1.CreateOptions{})
+
+	result := CheckAllIngresses("test", client, beClient, feClient)
 	checkSet := make(map[string]struct{})
-	for _, resource := range report.Resources {
+	for _, resource := range result.Resources {
 		for _, check := range resource.Checks {
 			checkSet[check.Name] = struct{}{}
 		}
@@ -739,4 +773,50 @@ func TestCheckAllIngresses(t *testing.T) {
 			t.Errorf("Missing check %s in CheckAllIngresses", check)
 		}
 	}
+
+	expect := report.Report{
+		Resources: []*report.Resource{
+			{
+				Kind:      "Ingress",
+				Namespace: "test",
+				Name:      "ingress1",
+				Checks: []*report.Check{
+					{Name: "IngressRuleCheck", Result: "PASSED"},
+					{Name: "L7ILBFrontendConfigCheck", Result: "FAILED"},
+					{Name: "RuleHostOverwriteCheck", Result: "PASSED"},
+					{Name: "FrontendConfigExistenceCheck", Result: "FAILED"},
+					{Name: "ServiceExistenceCheck", Result: "PASSED"},
+					{Name: "BackendConfigAnnotationCheck", Result: "PASSED"},
+					{Name: "AppProtocolAnnotationCheck", Result: "FAILED"},
+					{Name: "L7ILBNegAnnotationCheck", Result: "FAILED"},
+					{Name: "BackendConfigExistenceCheck", Result: "PASSED"},
+					{Name: "HealthCheckTimeoutCheck", Result: "SKIPPED"},
+				},
+			},
+			{
+				Kind:      "Ingress",
+				Namespace: "test",
+				Name:      "ingress2",
+				Checks: []*report.Check{
+					{Name: "IngressRuleCheck", Result: "FAILED"},
+					{Name: "L7ILBFrontendConfigCheck", Result: "SKIPPED"},
+					{Name: "RuleHostOverwriteCheck", Result: "FAILED"},
+					{Name: "FrontendConfigExistenceCheck", Result: "PASSED"},
+					{Name: "ServiceExistenceCheck", Result: "PASSED"},
+					{Name: "BackendConfigAnnotationCheck", Result: "PASSED"},
+					{Name: "AppProtocolAnnotationCheck", Result: "SKIPPED"},
+					{Name: "L7ILBNegAnnotationCheck", Result: "SKIPPED"},
+					{Name: "BackendConfigExistenceCheck", Result: "PASSED"},
+					{Name: "HealthCheckTimeoutCheck", Result: "FAILED"},
+				},
+			},
+		},
+	}
+	for i, resource := range result.Resources {
+		for j, check := range resource.Checks {
+			if diff := cmp.Diff(expect.Resources[i].Checks[j].Result, check.Result); diff != "" {
+				t.Errorf("For ingress check %s for ingress %s/%s, (-want +got):\n%s", check.Name, resource.Namespace, resource.Name, diff)
+			}
+		}
+	}
 }