From 07d3ec95b4e12e8dad54af46299f0408e341d976 Mon Sep 17 00:00:00 2001 From: sh2 Date: Fri, 8 Mar 2024 10:21:46 +0800 Subject: [PATCH] fix: skip the ReasonTargetNotFound for all policies (#2802) * stop populating ReasonTargetNotFound for all the policies Signed-off-by: shawnh2 * add test to ensure the status is expected Signed-off-by: shawnh2 * fix gen-check and lint Signed-off-by: shawnh2 --------- Signed-off-by: shawnh2 --- internal/gatewayapi/backendtrafficpolicy.go | 22 ++------------ internal/gatewayapi/clienttrafficpolicy.go | 18 +----------- internal/gatewayapi/envoypatchpolicy.go | 11 ------- internal/gatewayapi/securitypolicy.go | 23 ++------------- ...tatus-for-unknown-gateway-or-route.in.yaml | 23 +++++++++++++++ ...atus-for-unknown-gateway-or-route.out.yaml | 29 +++++++++++++++++++ ...licy-no-status-for-unknown-gateway.in.yaml | 12 ++++++++ ...icy-no-status-for-unknown-gateway.out.yaml | 16 ++++++++++ ...licy-no-status-for-unknown-gateway.in.yaml | 13 +++++++++ ...icy-no-status-for-unknown-gateway.out.yaml | 2 ++ ...tatus-for-unknown-gateway-or-route.in.yaml | 23 +++++++++++++++ ...atus-for-unknown-gateway-or-route.out.yaml | 29 +++++++++++++++++++ internal/gatewayapi/translator_test.go | 8 +++-- 13 files changed, 159 insertions(+), 70 deletions(-) create mode 100644 internal/gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.in.yaml create mode 100644 internal/gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.out.yaml create mode 100644 internal/gatewayapi/testdata/clienttrafficpolicy-no-status-for-unknown-gateway.in.yaml create mode 100644 internal/gatewayapi/testdata/clienttrafficpolicy-no-status-for-unknown-gateway.out.yaml create mode 100644 internal/gatewayapi/testdata/envoypatchpolicy-no-status-for-unknown-gateway.in.yaml create mode 100644 internal/gatewayapi/testdata/envoypatchpolicy-no-status-for-unknown-gateway.out.yaml create mode 100644 internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.in.yaml create mode 100644 internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.out.yaml diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go index f57f17e5726..1987ebb6e88 100644 --- 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 index ff10d791080..5960758718c 100644 --- 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 index 2b3025d809a..1ec835e51a4 100644 --- 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 index 79176930dd7..8e453d3e8c3 100644 --- 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/internal/gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.in.yaml new file mode 100644 index 00000000000..aed29a51471 --- /dev/null +++ b/internal/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/internal/gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-no-status-for-unknown-gateway-or-route.out.yaml new file mode 100644 index 00000000000..eb00e89c260 --- /dev/null +++ b/internal/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 new file mode 100644 index 00000000000..85d8004b375 --- /dev/null +++ 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 new file mode 100644 index 00000000000..bb1b94a2748 --- /dev/null +++ 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 new file mode 100644 index 00000000000..f23931ad622 --- /dev/null +++ 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 new file mode 100644 index 00000000000..fca40f1fe38 --- /dev/null +++ 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 new file mode 100644 index 00000000000..91d4841271b --- /dev/null +++ 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 new file mode 100644 index 00000000000..5ce1f5dc799 --- /dev/null +++ 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 index 86d76ebe293..ffadaf8bcbf 100644 --- 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...)) }) } }