From 8a83f8e0cd632116af0ee78187a26d74aada3415 Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Mon, 18 Mar 2024 08:21:12 +0200 Subject: [PATCH 1/6] Fixing the clienttrafficpolicy validation. Signed-off-by: Lior Okman --- internal/gatewayapi/clienttrafficpolicy.go | 21 +- .../merge-with-isolated-policies-2.in.yaml | 188 ++++++++++++++++++ 2 files changed, 205 insertions(+), 4 deletions(-) create mode 100644 internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml diff --git a/internal/gatewayapi/clienttrafficpolicy.go b/internal/gatewayapi/clienttrafficpolicy.go index 562bdcde1d3..27c47090a68 100644 --- a/internal/gatewayapi/clienttrafficpolicy.go +++ b/internal/gatewayapi/clienttrafficpolicy.go @@ -124,7 +124,7 @@ func (t *Translator) ProcessClientTrafficPolicies(resources *Resources, // It must exist since we've already finished processing the gateways gwXdsIR := xdsIR[irKey] if string(l.Name) == section { - err = validatePortOverlapForClientTrafficPolicy(l, gwXdsIR) + err = validatePortOverlapForClientTrafficPolicy(l, gwXdsIR, false) if err == nil { err = t.translateClientTrafficPolicyForListener(policy, l, xdsIR, infraIR, resources) } @@ -234,7 +234,7 @@ func (t *Translator) ProcessClientTrafficPolicies(resources *Resources, irKey := t.getIRKey(l.gateway) // It must exist since we've already finished processing the gateways gwXdsIR := xdsIR[irKey] - if err := validatePortOverlapForClientTrafficPolicy(l, gwXdsIR); err != nil { + if err := validatePortOverlapForClientTrafficPolicy(l, gwXdsIR, true); err != nil { errs = errors.Join(errs, err) } else if err := t.translateClientTrafficPolicyForListener(policy, l, xdsIR, infraIR, resources); err != nil { errs = errors.Join(errs, err) @@ -312,7 +312,7 @@ func resolveCTPolicyTargetRef(policy *egv1a1.ClientTrafficPolicy, gateways map[t return gateway.GatewayContext, nil } -func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds) error { +func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds, attachedToGateway bool) error { // Find Listener IR // TODO: Support TLSRoute and TCPRoute once // https://github.com/envoyproxy/gateway/issues/1635 is completed @@ -329,7 +329,20 @@ func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds) // IR must exist since we're past validation if httpIR != nil { if sameListeners := listenersWithSameHTTPPort(xds, httpIR); len(sameListeners) != 0 { - return fmt.Errorf("affects additional listeners: %s", strings.Join(sameListeners, ", ")) + if attachedToGateway { + gatewayName := irListenerName[0:strings.LastIndex(irListenerName, "/")] + conflictingListeners := []string{} + for _, currName := range sameListeners { + if strings.Index(currName, gatewayName) != 0 { + conflictingListeners = append(conflictingListeners, currName) + } + } + if len(conflictingListeners) != 0 { + return fmt.Errorf("affects additional listeners: %s", strings.Join(conflictingListeners, ", ")) + } + } else { + return fmt.Errorf("affects additional listeners: %s", strings.Join(sameListeners, ", ")) + } } } return nil diff --git a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml new file mode 100644 index 00000000000..83d9eed4343 --- /dev/null +++ b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml @@ -0,0 +1,188 @@ +envoyproxy: + apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: EnvoyProxy + metadata: + namespace: envoy-gateway-system + name: test + spec: + mergeGateways: true +gateways: + - apiVersion: gateway.networking.k8s.io/v1beta1 + kind: Gateway + metadata: + name: gateway-1 + namespace: default + spec: + gatewayClassName: envoy-gateway-class + listeners: + - name: http + port: 80 + protocol: HTTP + hostname: bar.example.com + allowedRoutes: + namespaces: + from: Same + - name: http-2 + port: 80 + hostname: foo.example.com + protocol: HTTP + allowedRoutes: + namespaces: + from: Same + - apiVersion: gateway.networking.k8s.io/v1beta1 + kind: Gateway + metadata: + name: gateway-2 + namespace: default + spec: + gatewayClassName: envoy-gateway-class + listeners: + - name: http + port: 81 + protocol: HTTP + hostname: bar.example.com + allowedRoutes: + namespaces: + from: Same + - name: http-2 + port: 81 + hostname: foo.example.com + protocol: HTTP + allowedRoutes: + namespaces: + from: Same +httpRoutes: + - apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-1 + spec: + hostnames: + - bar.example.com + parentRefs: + - namespace: default + name: gateway-1 + sectionName: http + rules: + - matches: + - path: + value: "/" + backendRefs: + - name: service-1 + port: 8080 + - apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-2 + spec: + hostnames: + - foo.example.com + parentRefs: + - namespace: default + name: gateway-1 + sectionName: http-2 + rules: + - matches: + - path: + value: "/" + backendRefs: + - name: service-2 + port: 8080 +securityPolicies: + - apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: SecurityPolicy + metadata: + namespace: default + name: policy-for-route-1 + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + cors: + allowOrigins: + - "*" + allowMethods: + - GET + - POST + allowHeaders: + - "x-header-5" + - "x-header-6" + exposeHeaders: + - "x-header-7" + - "x-header-8" + maxAge: 2000s + - apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: SecurityPolicy + metadata: + namespace: default + name: policy-for-route-2 + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-2 + namespace: default + sectionName: http + cors: + allowOrigins: + - "*" + allowMethods: + - GET + - POST + allowHeaders: + - "x-header-5" + - "x-header-6" + exposeHeaders: + - "x-header-7" + - "x-header-8" + maxAge: 2000s +clientTrafficPolicies: + - apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: ClientTrafficPolicy + metadata: + namespace: default + name: target-gateway-2 + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-2 + sectionName: http + namespace: default + timeout: + http: + requestReceivedTimeout: "5s" + - apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: ClientTrafficPolicy + metadata: + namespace: default + name: target-gateway + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + timeout: + http: + requestReceivedTimeout: "5s" +backendTrafficPolicies: + - apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + namespace: default + name: policy-for-gateway + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + tcpKeepalive: + probes: 3 + idleTime: 20m + interval: 60s From 3683dacb7bb31f145de1f1c051921e04d785ed9a Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Mon, 18 Mar 2024 09:17:43 +0200 Subject: [PATCH 2/6] Make SecurityPolicy validate correctly. Signed-off-by: Lior Okman --- internal/gatewayapi/securitypolicy.go | 23 +- .../testdata/conflicting-policies.out.yaml | 2 +- .../merge-with-isolated-policies-2.in.yaml | 43 +- .../merge-with-isolated-policies-2.out.yaml | 667 ++++++++++++++++++ 4 files changed, 727 insertions(+), 8 deletions(-) create mode 100755 internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go index 67bf2314bef..50b225ed88d 100644 --- a/internal/gatewayapi/securitypolicy.go +++ b/internal/gatewayapi/securitypolicy.go @@ -191,7 +191,7 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security irKey := t.getIRKey(targetedGateway.Gateway) // Should exist since we've validated this xds := xdsIR[irKey] - err := validatePortOverlapForSecurityPolicyGateway(xds) + err := validatePortOverlapForSecurityPolicyGateway(xds, targetedGateway) if err == nil { err = t.translateSecurityPolicyForGateway(policy, targetedGateway, resources, xdsIR) } @@ -508,11 +508,26 @@ func (t *Translator) translateSecurityPolicyForGateway( return errs } -func validatePortOverlapForSecurityPolicyGateway(xds *ir.Xds) error { +// should return error if the policy attaches to listeners that originate from gateways other than the one requested on the policy. +func validatePortOverlapForSecurityPolicyGateway(xds *ir.Xds, targetedGateway *GatewayContext) error { + targetedGwName := irStringKey(targetedGateway.Namespace, targetedGateway.Name) + relevantPorts := map[uint32]bool{} + for _, listener := range targetedGateway.listeners { + containerPort := servicePortToContainerPort(int32(listener.Port)) + relevantPorts[uint32(containerPort)] = true + } affectedListeners := []string{} for _, http := range xds.HTTP { - if sameListeners := listenersWithSameHTTPPort(xds, http); len(sameListeners) != 0 { - affectedListeners = append(affectedListeners, sameListeners...) + if _, found := relevantPorts[http.Port]; !found { + continue + } + // look for listeners on this XDS that aren't from the targetedGateway + for _, currListener := range listenersWithSameHTTPPort(xds, http) { + listenerName := currListener[0:strings.LastIndex(currListener, "/")] + if listenerName != targetedGwName { + affectedListeners = append(affectedListeners, currListener) + } + } } diff --git a/internal/gatewayapi/testdata/conflicting-policies.out.yaml b/internal/gatewayapi/testdata/conflicting-policies.out.yaml index 7e0d52a41d1..37b1d55f5f2 100644 --- a/internal/gatewayapi/testdata/conflicting-policies.out.yaml +++ b/internal/gatewayapi/testdata/conflicting-policies.out.yaml @@ -261,7 +261,7 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: 'Affects multiple listeners: default/mfqjpuycbgjrtdww/http, default/gateway-1/http' + message: 'Affects multiple listeners: default/gateway-1/http' reason: Invalid status: "False" type: Accepted diff --git a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml index 83d9eed4343..888c4e4b713 100644 --- a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml +++ b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml @@ -90,6 +90,44 @@ httpRoutes: backendRefs: - name: service-2 port: 8080 + - apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-3 + spec: + hostnames: + - bar.example.com + parentRefs: + - namespace: default + name: gateway-2 + sectionName: http + rules: + - matches: + - path: + value: "/" + backendRefs: + - name: service-1 + port: 8080 + - apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-4 + spec: + hostnames: + - foo.example.com + parentRefs: + - namespace: default + name: gateway-2 + sectionName: http-2 + rules: + - matches: + - path: + value: "/" + backendRefs: + - name: service-2 + port: 8080 securityPolicies: - apiVersion: gateway.envoyproxy.io/v1alpha1 kind: SecurityPolicy @@ -123,10 +161,9 @@ securityPolicies: spec: targetRef: group: gateway.networking.k8s.io - kind: Gateway - name: gateway-2 + kind: HTTPRoute + name: httproute-3 namespace: default - sectionName: http cors: allowOrigins: - "*" diff --git a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml new file mode 100755 index 00000000000..25b7c579d54 --- /dev/null +++ b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml @@ -0,0 +1,667 @@ +backendTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + creationTimestamp: null + name: policy-for-gateway + namespace: default + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + tcpKeepalive: + idleTime: 20m + interval: 60s + probes: 3 + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller +clientTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: ClientTrafficPolicy + metadata: + creationTimestamp: null + name: target-gateway-2 + namespace: default + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-2 + namespace: default + sectionName: http + timeout: + http: + requestReceivedTimeout: 5s + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-2 + namespace: default + sectionName: http + conditions: + - lastTransitionTime: null + message: 'Affects additional listeners: default/gateway-2/http-2' + reason: Invalid + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: ClientTrafficPolicy + metadata: + creationTimestamp: null + name: target-gateway + namespace: default + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + timeout: + http: + requestReceivedTimeout: 5s + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller +gateways: +- apiVersion: gateway.networking.k8s.io/v1beta1 + kind: Gateway + metadata: + creationTimestamp: null + name: gateway-1 + namespace: default + spec: + gatewayClassName: envoy-gateway-class + listeners: + - allowedRoutes: + namespaces: + from: Same + hostname: bar.example.com + name: http + port: 80 + protocol: HTTP + - allowedRoutes: + namespaces: + from: Same + hostname: foo.example.com + name: http-2 + port: 80 + protocol: HTTP + status: + listeners: + - attachedRoutes: 1 + conditions: + - lastTransitionTime: null + message: Sending translated listener configuration to the data plane + reason: Programmed + status: "True" + type: Programmed + - lastTransitionTime: null + message: Listener has been successfully translated + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Listener references have been resolved + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + name: http + supportedKinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute + - group: gateway.networking.k8s.io + kind: GRPCRoute + - attachedRoutes: 1 + conditions: + - lastTransitionTime: null + message: Sending translated listener configuration to the data plane + reason: Programmed + status: "True" + type: Programmed + - lastTransitionTime: null + message: Listener has been successfully translated + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Listener references have been resolved + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + name: http-2 + supportedKinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute + - group: gateway.networking.k8s.io + kind: GRPCRoute +- apiVersion: gateway.networking.k8s.io/v1beta1 + kind: Gateway + metadata: + creationTimestamp: null + name: gateway-2 + namespace: default + spec: + gatewayClassName: envoy-gateway-class + listeners: + - allowedRoutes: + namespaces: + from: Same + hostname: bar.example.com + name: http + port: 81 + protocol: HTTP + - allowedRoutes: + namespaces: + from: Same + hostname: foo.example.com + name: http-2 + port: 81 + protocol: HTTP + status: + listeners: + - attachedRoutes: 1 + conditions: + - lastTransitionTime: null + message: Sending translated listener configuration to the data plane + reason: Programmed + status: "True" + type: Programmed + - lastTransitionTime: null + message: Listener has been successfully translated + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Listener references have been resolved + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + name: http + supportedKinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute + - group: gateway.networking.k8s.io + kind: GRPCRoute + - attachedRoutes: 1 + conditions: + - lastTransitionTime: null + message: Sending translated listener configuration to the data plane + reason: Programmed + status: "True" + type: Programmed + - lastTransitionTime: null + message: Listener has been successfully translated + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Listener references have been resolved + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + name: http-2 + supportedKinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute + - group: gateway.networking.k8s.io + kind: GRPCRoute +httpRoutes: +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + creationTimestamp: null + name: httproute-1 + namespace: default + spec: + hostnames: + - bar.example.com + parentRefs: + - name: gateway-1 + namespace: default + sectionName: http + rules: + - backendRefs: + - name: service-1 + port: 8080 + matches: + - path: + value: / + status: + parents: + - conditions: + - lastTransitionTime: null + message: Route is accepted + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Resolved all the Object references for the Route + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + controllerName: gateway.envoyproxy.io/gatewayclass-controller + parentRef: + name: gateway-1 + namespace: default + sectionName: http +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + creationTimestamp: null + name: httproute-2 + namespace: default + spec: + hostnames: + - foo.example.com + parentRefs: + - name: gateway-1 + namespace: default + sectionName: http-2 + rules: + - backendRefs: + - name: service-2 + port: 8080 + matches: + - path: + value: / + status: + parents: + - conditions: + - lastTransitionTime: null + message: Route is accepted + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Resolved all the Object references for the Route + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + controllerName: gateway.envoyproxy.io/gatewayclass-controller + parentRef: + name: gateway-1 + namespace: default + sectionName: http-2 +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + creationTimestamp: null + name: httproute-3 + namespace: default + spec: + hostnames: + - bar.example.com + parentRefs: + - name: gateway-2 + namespace: default + sectionName: http + rules: + - backendRefs: + - name: service-1 + port: 8080 + matches: + - path: + value: / + status: + parents: + - conditions: + - lastTransitionTime: null + message: Route is accepted + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Resolved all the Object references for the Route + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + controllerName: gateway.envoyproxy.io/gatewayclass-controller + parentRef: + name: gateway-2 + namespace: default + sectionName: http +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + creationTimestamp: null + name: httproute-4 + namespace: default + spec: + hostnames: + - foo.example.com + parentRefs: + - name: gateway-2 + namespace: default + sectionName: http-2 + rules: + - backendRefs: + - name: service-2 + port: 8080 + matches: + - path: + value: / + status: + parents: + - conditions: + - lastTransitionTime: null + message: Route is accepted + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Resolved all the Object references for the Route + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + controllerName: gateway.envoyproxy.io/gatewayclass-controller + parentRef: + name: gateway-2 + namespace: default + sectionName: http-2 +infraIR: + envoy-gateway-class: + proxy: + config: + apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: EnvoyProxy + metadata: + creationTimestamp: null + name: test + namespace: envoy-gateway-system + spec: + logging: {} + mergeGateways: true + status: {} + listeners: + - address: null + name: default/gateway-1/http + ports: + - containerPort: 10080 + name: default/gateway-1/http + protocol: HTTP + servicePort: 80 + - address: null + name: default/gateway-2/http + ports: + - containerPort: 10081 + name: default/gateway-2/http + protocol: HTTP + servicePort: 81 + metadata: + labels: + gateway.envoyproxy.io/owning-gatewayclass: envoy-gateway-class + name: envoy-gateway-class +securityPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: SecurityPolicy + metadata: + creationTimestamp: null + name: policy-for-route-2 + namespace: default + spec: + cors: + allowHeaders: + - x-header-5 + - x-header-6 + allowMethods: + - GET + - POST + allowOrigins: + - '*' + exposeHeaders: + - x-header-7 + - x-header-8 + maxAge: 33m20s + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: httproute-3 + namespace: default + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-2 + namespace: default + sectionName: http + conditions: + - lastTransitionTime: null + message: 'Affects multiple listeners: default/gateway-2/http-2' + reason: Invalid + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: SecurityPolicy + metadata: + creationTimestamp: null + name: policy-for-route-1 + namespace: default + spec: + cors: + allowHeaders: + - x-header-5 + - x-header-6 + allowMethods: + - GET + - POST + allowOrigins: + - '*' + exposeHeaders: + - x-header-7 + - x-header-8 + maxAge: 33m20s + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: default + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller +xdsIR: + envoy-gateway-class: + accessLog: + text: + - path: /dev/stdout + http: + - address: 0.0.0.0 + hostnames: + - bar.example.com + isHTTP2: false + name: default/gateway-1/http + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 10080 + routes: + - backendWeights: + invalid: 0 + valid: 0 + cors: + allowHeaders: + - x-header-5 + - x-header-6 + allowMethods: + - GET + - POST + allowOrigins: + - distinct: false + name: "" + safeRegex: .* + exposeHeaders: + - x-header-7 + - x-header-8 + maxAge: 33m20s + destination: + name: httproute/default/httproute-1/rule/0 + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8080 + protocol: HTTP + weight: 1 + hostname: bar.example.com + isHTTP2: false + name: httproute/default/httproute-1/rule/0/match/0/bar_example_com + pathMatch: + distinct: false + name: "" + prefix: / + tcpKeepalive: + idleTime: 1200 + interval: 60 + probes: 3 + timeout: + http: + requestReceivedTimeout: 5s + - address: 0.0.0.0 + hostnames: + - foo.example.com + isHTTP2: false + name: default/gateway-1/http-2 + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 10080 + routes: + - backendWeights: + invalid: 0 + valid: 0 + cors: + allowHeaders: + - x-header-5 + - x-header-6 + allowMethods: + - GET + - POST + allowOrigins: + - distinct: false + name: "" + safeRegex: .* + exposeHeaders: + - x-header-7 + - x-header-8 + maxAge: 33m20s + destination: + name: httproute/default/httproute-2/rule/0 + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8080 + protocol: HTTP + weight: 1 + hostname: foo.example.com + isHTTP2: false + name: httproute/default/httproute-2/rule/0/match/0/foo_example_com + pathMatch: + distinct: false + name: "" + prefix: / + tcpKeepalive: + idleTime: 1200 + interval: 60 + probes: 3 + timeout: + http: + requestReceivedTimeout: 5s + - address: 0.0.0.0 + hostnames: + - bar.example.com + isHTTP2: false + name: default/gateway-2/http + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 10081 + routes: + - backendWeights: + invalid: 0 + valid: 0 + destination: + name: httproute/default/httproute-3/rule/0 + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8080 + protocol: HTTP + weight: 1 + hostname: bar.example.com + isHTTP2: false + name: httproute/default/httproute-3/rule/0/match/0/bar_example_com + pathMatch: + distinct: false + name: "" + prefix: / + - address: 0.0.0.0 + hostnames: + - foo.example.com + isHTTP2: false + name: default/gateway-2/http-2 + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 10081 + routes: + - backendWeights: + invalid: 0 + valid: 0 + destination: + name: httproute/default/httproute-4/rule/0 + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8080 + protocol: HTTP + weight: 1 + hostname: foo.example.com + isHTTP2: false + name: httproute/default/httproute-4/rule/0/match/0/foo_example_com + pathMatch: + distinct: false + name: "" + prefix: / From 35dcf2bd35cc067800b9ab07afade42ba5e568d5 Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Thu, 28 Mar 2024 11:32:58 +0200 Subject: [PATCH 3/6] Reverted the SecurityPolicy validation - handled differently via another feature. Signed-off-by: Lior Okman --- internal/gatewayapi/securitypolicy.go | 43 ++------------------------- 1 file changed, 2 insertions(+), 41 deletions(-) diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go index 50b225ed88d..63d13b60777 100644 --- a/internal/gatewayapi/securitypolicy.go +++ b/internal/gatewayapi/securitypolicy.go @@ -134,9 +134,7 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security continue } - err := t.translateSecurityPolicyForRoute(policy, targetedRoute, resources, xdsIR) - - if err != nil { + if err := t.translateSecurityPolicyForRoute(policy, targetedRoute, resources, xdsIR); err != nil { status.SetTranslationErrorForPolicyAncestors(&policy.Status, parentGateways, t.GatewayControllerName, @@ -188,15 +186,7 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security continue } - irKey := t.getIRKey(targetedGateway.Gateway) - // Should exist since we've validated this - xds := xdsIR[irKey] - err := validatePortOverlapForSecurityPolicyGateway(xds, targetedGateway) - if err == nil { - err = t.translateSecurityPolicyForGateway(policy, targetedGateway, resources, xdsIR) - } - - if err != nil { + if err := t.translateSecurityPolicyForGateway(policy, targetedGateway, resources, xdsIR); err != nil { status.SetTranslationErrorForPolicyAncestors(&policy.Status, parentGateways, t.GatewayControllerName, @@ -508,35 +498,6 @@ func (t *Translator) translateSecurityPolicyForGateway( return errs } -// should return error if the policy attaches to listeners that originate from gateways other than the one requested on the policy. -func validatePortOverlapForSecurityPolicyGateway(xds *ir.Xds, targetedGateway *GatewayContext) error { - targetedGwName := irStringKey(targetedGateway.Namespace, targetedGateway.Name) - relevantPorts := map[uint32]bool{} - for _, listener := range targetedGateway.listeners { - containerPort := servicePortToContainerPort(int32(listener.Port)) - relevantPorts[uint32(containerPort)] = true - } - affectedListeners := []string{} - for _, http := range xds.HTTP { - if _, found := relevantPorts[http.Port]; !found { - continue - } - // look for listeners on this XDS that aren't from the targetedGateway - for _, currListener := range listenersWithSameHTTPPort(xds, http) { - listenerName := currListener[0:strings.LastIndex(currListener, "/")] - if listenerName != targetedGwName { - affectedListeners = append(affectedListeners, currListener) - } - - } - } - - if len(affectedListeners) > 0 { - return fmt.Errorf("affects multiple listeners: %s", strings.Join(affectedListeners, ", ")) - } - return nil -} - func (t *Translator) buildCORS(cors *egv1a1.CORS) *ir.CORS { var allowOrigins []*ir.StringMatch From dc31dfe01cbdf95c98fbedce21c5932baea7b786 Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Thu, 28 Mar 2024 12:39:07 +0200 Subject: [PATCH 4/6] Updated the tests to reflect that this validation isn't required for SecurityPolicy Signed-off-by: Lior Okman --- .../testdata/conflicting-policies.out.yaml | 20 +++++++++++++++--- .../merge-with-isolated-policies-2.out.yaml | 21 ++++++++++++++++--- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/internal/gatewayapi/testdata/conflicting-policies.out.yaml b/internal/gatewayapi/testdata/conflicting-policies.out.yaml index 37b1d55f5f2..f2958d41b04 100644 --- a/internal/gatewayapi/testdata/conflicting-policies.out.yaml +++ b/internal/gatewayapi/testdata/conflicting-policies.out.yaml @@ -261,9 +261,9 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: 'Affects multiple listeners: default/gateway-1/http' - reason: Invalid - status: "False" + message: Policy has been accepted. + reason: Accepted + status: "True" type: Accepted controllerName: gateway.envoyproxy.io/gatewayclass-controller xdsIR: @@ -314,6 +314,20 @@ xdsIR: - backendWeights: invalid: 0 valid: 0 + cors: + allowCredentials: true + allowMethods: + - PUT + - GET + - POST + - DELETE + - PATCH + - OPTIONS + allowOrigins: + - distinct: false + name: "" + safeRegex: http://.*\.foo\.com + maxAge: 10m0s destination: name: httproute/default/mfqjpuycbgjrtdww/rule/0 settings: diff --git a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml index 25b7c579d54..6517a90f2b5 100755 --- a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml +++ b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml @@ -455,9 +455,9 @@ securityPolicies: sectionName: http conditions: - lastTransitionTime: null - message: 'Affects multiple listeners: default/gateway-2/http-2' - reason: Invalid - status: "False" + message: Policy has been accepted. + reason: Accepted + status: "True" type: Accepted controllerName: gateway.envoyproxy.io/gatewayclass-controller - apiVersion: gateway.envoyproxy.io/v1alpha1 @@ -620,6 +620,21 @@ xdsIR: - backendWeights: invalid: 0 valid: 0 + cors: + allowHeaders: + - x-header-5 + - x-header-6 + allowMethods: + - GET + - POST + allowOrigins: + - distinct: false + name: "" + safeRegex: .* + exposeHeaders: + - x-header-7 + - x-header-8 + maxAge: 33m20s destination: name: httproute/default/httproute-3/rule/0 settings: From bc70ce9e8d7b2c19174a604a77ce7a3de3069d8d Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Thu, 28 Mar 2024 12:45:30 +0200 Subject: [PATCH 5/6] Added some comments to explain the validation being performed. Signed-off-by: Lior Okman --- internal/gatewayapi/clienttrafficpolicy.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/gatewayapi/clienttrafficpolicy.go b/internal/gatewayapi/clienttrafficpolicy.go index 27c47090a68..a2020c5f5e6 100644 --- a/internal/gatewayapi/clienttrafficpolicy.go +++ b/internal/gatewayapi/clienttrafficpolicy.go @@ -328,8 +328,14 @@ func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds, // IR must exist since we're past validation if httpIR != nil { + // Get a list of all other non-TLS listeners on this Gateway that share a port with + // the listener in question. if sameListeners := listenersWithSameHTTPPort(xds, httpIR); len(sameListeners) != 0 { if attachedToGateway { + // If this policy is attached to an entire gateway and the mergeGateways feature + // is turned on, validate that all the listeners affected by this policy originated + // from the same Gateway resource. The name of the Gateway from which this listener + // originated is part of the listener's name by construction. gatewayName := irListenerName[0:strings.LastIndex(irListenerName, "/")] conflictingListeners := []string{} for _, currName := range sameListeners { @@ -341,6 +347,8 @@ func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds, return fmt.Errorf("affects additional listeners: %s", strings.Join(conflictingListeners, ", ")) } } else { + // If this policy is attached to a specific listener, any other listeners in the list + // would be affected by this policy but should not be, so this policy can't be accepted. return fmt.Errorf("affects additional listeners: %s", strings.Join(sameListeners, ", ")) } } From 4736eb4afd6290a4d5a7f8856b1c313311af7848 Mon Sep 17 00:00:00 2001 From: Lior Okman Date: Thu, 28 Mar 2024 14:28:10 +0200 Subject: [PATCH 6/6] Updated the error message as requested in the review. Signed-off-by: Lior Okman --- internal/gatewayapi/clienttrafficpolicy.go | 4 ++-- internal/gatewayapi/testdata/conflicting-policies.out.yaml | 3 ++- .../testdata/merge-with-isolated-policies-2.out.yaml | 3 ++- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/gatewayapi/clienttrafficpolicy.go b/internal/gatewayapi/clienttrafficpolicy.go index a2020c5f5e6..1ea04b30fc3 100644 --- a/internal/gatewayapi/clienttrafficpolicy.go +++ b/internal/gatewayapi/clienttrafficpolicy.go @@ -344,12 +344,12 @@ func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds, } } if len(conflictingListeners) != 0 { - return fmt.Errorf("affects additional listeners: %s", strings.Join(conflictingListeners, ", ")) + return fmt.Errorf("ClientTrafficPolicy is being applied to multiple http (non https) listeners (%s) on the same port, which is not allowed", strings.Join(conflictingListeners, ", ")) } } else { // If this policy is attached to a specific listener, any other listeners in the list // would be affected by this policy but should not be, so this policy can't be accepted. - return fmt.Errorf("affects additional listeners: %s", strings.Join(sameListeners, ", ")) + return fmt.Errorf("ClientTrafficPolicy is being applied to multiple http (non https) listeners (%s) on the same port, which is not allowed", strings.Join(sameListeners, ", ")) } } } diff --git a/internal/gatewayapi/testdata/conflicting-policies.out.yaml b/internal/gatewayapi/testdata/conflicting-policies.out.yaml index f2958d41b04..f8afcaee6da 100644 --- a/internal/gatewayapi/testdata/conflicting-policies.out.yaml +++ b/internal/gatewayapi/testdata/conflicting-policies.out.yaml @@ -23,7 +23,8 @@ clientTrafficPolicies: namespace: default conditions: - lastTransitionTime: null - message: 'Affects additional listeners: default/gateway-1/http' + message: ClientTrafficPolicy is being applied to multiple http (non https) + listeners (default/gateway-1/http) on the same port, which is not allowed reason: Invalid status: "False" type: Accepted diff --git a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml index 6517a90f2b5..15e35504f91 100755 --- a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml +++ b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml @@ -56,7 +56,8 @@ clientTrafficPolicies: sectionName: http conditions: - lastTransitionTime: null - message: 'Affects additional listeners: default/gateway-2/http-2' + message: ClientTrafficPolicy is being applied to multiple http (non https) + listeners (default/gateway-2/http-2) on the same port, which is not allowed reason: Invalid status: "False" type: Accepted