fix: skip the ReasonTargetNotFound for all policies (envoyproxy#2802)

* stop populating ReasonTargetNotFound for all the policies Signed-off-by: shawnh2 <shawnhxh@outlook.com> * add test to ensure the status is expected Signed-off-by: shawnh2 <shawnhxh@outlook.com> * fix gen-check and lint Signed-off-by: shawnh2 <shawnhxh@outlook.com> --------- Signed-off-by: shawnh2 <shawnhxh@outlook.com>
tmsnan · Mar 8, 2024 · 07d3ec9 · 07d3ec9
1 parent ff108c3
commit 07d3ec9
Show file tree

Hide file tree

Showing 13 changed files with 159 additions and 70 deletions.
diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go
@@ -77,7 +77,7 @@ func (t *Translator) ProcessBackendTrafficPolicies(backendTrafficPolicies []*egv
 
 	// Translate
 	// 1. First translate Policies targeting xRoutes
-	// 2.. Finally, the policies targeting Gateways
+	// 2. Finally, the policies targeting Gateways
 
 	// Process the policies targeting xRoutes
 	for _, policy := range backendTrafficPolicies {
@@ -167,9 +167,9 @@ func resolveBTPolicyGatewayTargetRef(policy *egv1a1.BackendTrafficPolicy, gatewa
 
 	// Ensure Policy and target are in the same namespace
 	if policy.Namespace != string(*targetNs) {
-
 		message := fmt.Sprintf("Namespace:%s TargetRef.Namespace:%s, BackendTrafficPolicy can only target a resource in the same namespace.",
 			policy.Namespace, *targetNs)
+
 		status.SetBackendTrafficPolicyCondition(policy,
 			gwv1a2.PolicyConditionAccepted,
 			metav1.ConditionFalse,
@@ -188,14 +188,6 @@ func resolveBTPolicyGatewayTargetRef(policy *egv1a1.BackendTrafficPolicy, gatewa
 
 	// Gateway not found
 	if !ok {
-		message := fmt.Sprintf("Gateway:%s not found.", policy.Spec.TargetRef.Name)
-
-		status.SetBackendTrafficPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonTargetNotFound,
-			message,
-		)
 		return nil
 	}
 
@@ -228,9 +220,9 @@ func resolveBTPolicyRouteTargetRef(policy *egv1a1.BackendTrafficPolicy, routes m
 
 	// Ensure Policy and target are in the same namespace
 	if policy.Namespace != string(*targetNs) {
-
 		message := fmt.Sprintf("Namespace:%s TargetRef.Namespace:%s, BackendTrafficPolicy can only target a resource in the same namespace.",
 			policy.Namespace, *targetNs)
+
 		status.SetBackendTrafficPolicyCondition(policy,
 			gwv1a2.PolicyConditionAccepted,
 			metav1.ConditionFalse,
@@ -250,14 +242,6 @@ func resolveBTPolicyRouteTargetRef(policy *egv1a1.BackendTrafficPolicy, routes m
 
 	// Route not found
 	if !ok {
-		message := fmt.Sprintf("%s/%s/%s not found.", policy.Spec.TargetRef.Kind, string(*targetNs), policy.Spec.TargetRef.Name)
-
-		status.SetBackendTrafficPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonTargetNotFound,
-			message,
-		)
 		return nil
 	}
 

diff --git a/internal/gatewayapi/clienttrafficpolicy.go b/internal/gatewayapi/clienttrafficpolicy.go
@@ -147,7 +147,6 @@ func (t *Translator) ProcessClientTrafficPolicies(resources *Resources,
 				)
 
 				continue
-
 			}
 
 			// Check if another policy targeting the same Gateway exists
@@ -227,9 +226,9 @@ func resolveCTPolicyTargetRef(policy *egv1a1.ClientTrafficPolicy, gateways []*Ga
 
 	// Ensure Policy and target Gateway are in the same namespace
 	if policy.Namespace != string(*targetNs) {
-
 		message := fmt.Sprintf("Namespace:%s TargetRef.Namespace:%s, ClientTrafficPolicy can only target a Gateway in the same namespace.",
 			policy.Namespace, *targetNs)
+
 		status.SetClientTrafficPolicyCondition(policy,
 			gwv1a2.PolicyConditionAccepted,
 			metav1.ConditionFalse,
@@ -250,14 +249,6 @@ func resolveCTPolicyTargetRef(policy *egv1a1.ClientTrafficPolicy, gateways []*Ga
 
 	// Gateway not found
 	if gateway == nil {
-		message := fmt.Sprintf("Gateway:%s not found.", policy.Spec.TargetRef.Name)
-
-		status.SetClientTrafficPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonTargetNotFound,
-			message,
-		)
 		return nil
 	}
 
@@ -271,13 +262,6 @@ func resolveCTPolicyTargetRef(policy *egv1a1.ClientTrafficPolicy, gateways []*Ga
 			}
 		}
 		if !found {
-			message := fmt.Sprintf("SectionName(Listener):%s not found.", *(policy.Spec.TargetRef.SectionName))
-			status.SetClientTrafficPolicyCondition(policy,
-				gwv1a2.PolicyConditionAccepted,
-				metav1.ConditionFalse,
-				gwv1a2.PolicyReasonTargetNotFound,
-				message,
-			)
 			return nil
 		}
 	}

diff --git a/internal/gatewayapi/envoypatchpolicy.go b/internal/gatewayapi/envoypatchpolicy.go
@@ -45,17 +45,6 @@ func (t *Translator) ProcessEnvoyPatchPolicies(envoyPatchPolicies []*egv1a1.Envo
 
 		gwXdsIR, ok := xdsIR[irKey]
 		if !ok {
-			// This status condition will not get updated in the resource because
-			// the IR is missing, but it has been kept here in case we publish
-			// the status from this layer instead of the xds layer.
-			message := fmt.Sprintf("%s:%s not found.", targetKind, policy.Spec.TargetRef.Name)
-
-			status.SetEnvoyPatchPolicyCondition(policy,
-				gwv1a2.PolicyConditionAccepted,
-				metav1.ConditionFalse,
-				gwv1a2.PolicyReasonTargetNotFound,
-				message,
-			)
 			continue
 		}
 

diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go
@@ -182,10 +182,10 @@ func resolveSecurityPolicyGatewayTargetRef(
 
 	// Ensure Policy and target are in the same namespace
 	if policy.Namespace != string(*targetNs) {
-
 		message := fmt.Sprintf(
 			"Namespace:%s TargetRef.Namespace:%s, SecurityPolicy can only target a resource in the same namespace.",
 			policy.Namespace, *targetNs)
+
 		status.SetSecurityPolicyCondition(policy,
 			gwv1a2.PolicyConditionAccepted,
 			metav1.ConditionFalse,
@@ -204,14 +204,6 @@ func resolveSecurityPolicyGatewayTargetRef(
 
 	// Gateway not found
 	if !ok {
-		message := fmt.Sprintf("Gateway:%s not found.", policy.Spec.TargetRef.Name)
-
-		status.SetSecurityPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonTargetNotFound,
-			message,
-		)
 		return nil
 	}
 
@@ -246,10 +238,10 @@ func resolveSecurityPolicyRouteTargetRef(
 
 	// Ensure Policy and target are in the same namespace
 	if policy.Namespace != string(*targetNs) {
-
 		message := fmt.Sprintf(
 			"Namespace:%s TargetRef.Namespace:%s, SecurityPolicy can only target a resource in the same namespace.",
 			policy.Namespace, *targetNs)
+
 		status.SetSecurityPolicyCondition(policy,
 			gwv1a2.PolicyConditionAccepted,
 			metav1.ConditionFalse,
@@ -269,17 +261,6 @@ func resolveSecurityPolicyRouteTargetRef(
 
 	// Route not found
 	if !ok {
-		message := fmt.Sprintf(
-			"%s/%s/%s not found.",
-			policy.Spec.TargetRef.Kind,
-			string(*targetNs), policy.Spec.TargetRef.Name)
-
-		status.SetSecurityPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonTargetNotFound,
-			message,
-		)
 		return nil
 	}
 

diff --git a/...l/gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.in.yaml b/...l/gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.in.yaml
@@ -0,0 +1,23 @@
+backendTrafficPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: BackendTrafficPolicy
+  metadata:
+    namespace: envoy-gateway
+    name: target-unknown-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: unknown-gateway
+      namespace: envoy-gateway
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: BackendTrafficPolicy
+  metadata:
+    namespace: envoy-gateway
+    name: target-unknown-httproute
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: HTTPRoute
+      name: unknown-httproute
+      namespace: envoy-gateway
diff --git a/.../gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.out.yaml b/.../gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.out.yaml
@@ -0,0 +1,29 @@
+backendTrafficPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: BackendTrafficPolicy
+  metadata:
+    creationTimestamp: null
+    name: target-unknown-httproute
+    namespace: envoy-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: HTTPRoute
+      name: unknown-httproute
+      namespace: envoy-gateway
+  status: {}
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: BackendTrafficPolicy
+  metadata:
+    creationTimestamp: null
+    name: target-unknown-gateway
+    namespace: envoy-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: unknown-gateway
+      namespace: envoy-gateway
+  status: {}
+infraIR: {}
+xdsIR: {}
diff --git a/internal/gatewayapi/testdata/clienttrafficpolicy-no-status-for-unknown-gateway.in.yaml b/internal/gatewayapi/testdata/clienttrafficpolicy-no-status-for-unknown-gateway.in.yaml
@@ -0,0 +1,12 @@
+clientTrafficPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: ClientTrafficPolicy
+  metadata:
+    namespace: envoy-gateway
+    name: target-unknown-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: unknown-gateway
+      namespace: envoy-gateway
diff --git a/internal/gatewayapi/testdata/clienttrafficpolicy-no-status-for-unknown-gateway.out.yaml b/internal/gatewayapi/testdata/clienttrafficpolicy-no-status-for-unknown-gateway.out.yaml
@@ -0,0 +1,16 @@
+clientTrafficPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: ClientTrafficPolicy
+  metadata:
+    creationTimestamp: null
+    name: target-unknown-gateway
+    namespace: envoy-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: unknown-gateway
+      namespace: envoy-gateway
+  status: {}
+infraIR: {}
+xdsIR: {}
diff --git a/internal/gatewayapi/testdata/envoypatchpolicy-no-status-for-unknown-gateway.in.yaml b/internal/gatewayapi/testdata/envoypatchpolicy-no-status-for-unknown-gateway.in.yaml
@@ -0,0 +1,13 @@
+envoyPatchPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: EnvoyPatchPolicy
+  metadata:
+    namespace: envoy-gateway
+    name: target-unknown-gateway
+  spec:
+    type: "JSONPatch"
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: unknown-gateway
+      namespace: envoy-gateway
diff --git a/internal/gatewayapi/testdata/envoypatchpolicy-no-status-for-unknown-gateway.out.yaml b/internal/gatewayapi/testdata/envoypatchpolicy-no-status-for-unknown-gateway.out.yaml
@@ -0,0 +1,2 @@
+infraIR: {}
+xdsIR: {}
diff --git a/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.in.yaml b/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.in.yaml
@@ -0,0 +1,23 @@
+securityPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: SecurityPolicy
+  metadata:
+    namespace: envoy-gateway
+    name: target-unknown-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: unknown-gateway
+      namespace: envoy-gateway
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: SecurityPolicy
+  metadata:
+    namespace: envoy-gateway
+    name: target-unknown-httproute
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: HTTPRoute
+      name: unknown-httproute
+      namespace: envoy-gateway
diff --git a/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.out.yaml b/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.out.yaml
@@ -0,0 +1,29 @@
+infraIR: {}
+securityPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: SecurityPolicy
+  metadata:
+    creationTimestamp: null
+    name: target-unknown-httproute
+    namespace: envoy-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: HTTPRoute
+      name: unknown-httproute
+      namespace: envoy-gateway
+  status: {}
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: SecurityPolicy
+  metadata:
+    creationTimestamp: null
+    name: target-unknown-gateway
+    namespace: envoy-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: unknown-gateway
+      namespace: envoy-gateway
+  status: {}
+xdsIR: {}
diff --git a/internal/gatewayapi/translator_test.go b/internal/gatewayapi/translator_test.go
@@ -242,8 +242,12 @@ func TestTranslate(t *testing.T) {
 			want := &TranslateResult{}
 			mustUnmarshal(t, output, want)
 
-			opts := cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime")
-			require.Empty(t, cmp.Diff(want, got, opts))
+			opts := []cmp.Option{
+				cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"),
+				cmpopts.EquateEmpty(),
+			}
+
+			require.Empty(t, cmp.Diff(want, got, opts...))
 		})
 	}
 }