fix: Allow Policy to attach to multiple http listeners #2967
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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, 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, 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, attachedToGateway bool) error { | ||||||
| // Find Listener IR | ||||||
| // TODO: Support TLSRoute and TCPRoute once | ||||||
| // https://github.com/envoyproxy/gateway/issues/1635 is completed | ||||||
|
|
@@ -328,8 +328,29 @@ 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 { | ||||||
|
There was a problem hiding this comment. this is now O(n^3)
this takes an approach of first policy wins, which imo is better than no policy winning There was a problem hiding this comment.
"First policy wins" doesn't make sense. It's not a conflict between two policies where one can win and the other can lose, it's that translating a single policy (no other policies existing in the system) would result in it affecting multiple listeners including listeners that should not be affected by the policy. There was a problem hiding this comment.
Looking closely at the func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds, attachedToGateway bool) error {
irListenerName := irHTTPListenerName(l)
var httpIR *ir.HTTPListener
// O(n) loop
for _, http := range xds.HTTP {
if http.Name == irListenerName {
httpIR = http
break
}
}
// IR must exist since we're past validation
if httpIR != nil {
// listenersWithSameHTTPPort is O(n) and it is called once.
// This is an 'if' statment, not a 'for' statement.
if sameListeners := listenersWithSameHTTPPort(xds, httpIR); len(sameListeners) != 0 {
if attachedToGateway {
gatewayName := irListenerName[0:strings.LastIndex(irListenerName, "/")]
conflictingListeners := []string{}
// sameListeners is an array with an upper bound of O(n)
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
}There was a problem hiding this comment. @liorokman adding some reasons why we should consider first policy wins
There was a problem hiding this comment. @arkodg The bug occurs when there is exactly one policy. The scenario where there are two policies is out of scope for this issue. There's no way to implement "first policy wins", because "first policy wins" means "reject the policy provided". Here's a recreation of the bug that this PR is trying to solve.: apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
metadata:
name: gateway
spec:
gatewayClassName: envoy-gateway-class
listeners:
- name: http
port: 80
protocol: HTTP
hostname: foo.example.com
allowedRoutes:
namespaces:
from: Same
- name: http2
port: 80
protocol: HTTP
hostname: bar.example.com
allowedRoutes:
namespaces:
from: Same
---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: ClientTrafficPolicy
metadata:
name: target-gateway
spec:
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway
namespace: default
sectionName: http
path:
escapedSlashesAction: RejectRequest
timeout:
http:
requestReceivedTimeout: "5s"
The above single Without this PR the translated IR will be correct - the various settings from the |
||||||
| // 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 { | ||||||
| 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 { | ||||||
| // 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, ", ")) | ||||||
|
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. The suggested error message is a bit misleading, since the policy document as applied by the user is explicitly not attached to any other listener. How about There was a problem hiding this comment. ah instead of
|
||||||
| } | ||||||
| } | ||||||
| } | ||||||
| return nil | ||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,225 @@ | ||
| 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 | ||
| - 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 | ||
| 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: HTTPRoute | ||
| name: httproute-3 | ||
| namespace: default | ||
| 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 |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding comments for the new logic would really help