fix: Allow Policy to attach to multiple http listeners

[liorokman]

Which issue(s) this PR fixes:
Fixes #2963

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.55%. Comparing base (a270dd8) to head (4736eb4).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2967      +/-   ##
==========================================
+ Coverage   64.49%   64.55%   +0.05%     
==========================================
  Files         121      121              
  Lines       21388    21397       +9     
==========================================
+ Hits        13794    13812      +18     
+ Misses       6723     6715       -8     
+ Partials      871      870       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arkodg]

adding comments for the new logic would really help

[arkodg]

this is now O(n^3)
can we instead hold a map containing listener name as the key and policy as the value ?

• if key is missing, add key, and return
• if key is present, policy is same, return
• if key is present, and a different policy, reject

this takes an approach of first policy wins, which imo is better than no policy winning

[liorokman]

this takes an approach of first policy wins, which imo is better than no policy winning

"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.

[liorokman]

this is now O(n^3)

Looking closely at the validatePortOverlapForClientTrafficPolicy method, it seems to me that it's O(n) where n is the number of HTTP listeners attached to a Gateway XDS representation.

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
}

[arkodg]

@liorokman adding some reasons why we should consider first policy wins

• we already oldest config wins in most places

  gateway/internal/gatewayapi/clienttrafficpolicy.go

  Line 43 in 6fa99be

  // Sort based on timestamp

• this allows existing traffic to not get interrupted/dropped, ensures that is policy1 targets listener1 at t1 and policy2 targets listener2 at t2, listener1 is not affected

[liorokman]

@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 ClientTrafficPolicy tries to target only one listener - the one called http - and because both http and http2 are non-TLS and on the same port, the requestReceivedTimeout configured for http2 will also be 5s. Even worse, while http should have the RejectRequest action for escaped slashes, http2 should use the default value (UnescapeAndForward) but will instead be configured to RejectRequest.

Without this PR the translated IR will be correct - the various settings from the ClientTrafficPolicy will be set in the correct location - only on http and not on http2. The bug occurs because the correct IR is not translatable to valid XDS.

[zhaohuabing]

We probably won't need this #2970

[arkodg]

@zhaohuabing we still need it for CTP which adds fields like MergeSlashes on the hcm directly, so we have a conflict for http (non https/tls) case

[zhaohuabing]

We don't need this for SecurityPolicy anymore. It's handled in #2970

[liorokman]

I removed the code that validates SecurityPolicy for this issue.

[arkodg]

Suggested change

	return fmt.Errorf("affects additional listeners: %s", strings.Join(sameListeners, ", "))
	return fmt.Errorf("policy attached to multiple http (non https) listeners %s on the same port is not allowed", strings.Join(sameListeners, ", "))

[liorokman]

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 policy would affect additional http (non https) listeners to which it was not attached: %s ?

[arkodg]

ah instead of attached, we could use applied :)

ClientTrafficPolicy is being applied to multiple http (non https) listeners %s on the same port, which is not allowed

[arkodg]

same as below

[arkodg]

thanks for adding comments @liorokman, added a minor comment around the error string to make it clearer why the policy was rejected, else LGTM !

[arkodg]

LGTM thanks !

[zirain]

/retest

[arkodg]

all e2e tests consistently seem to be consistently failing @liorokman

[liorokman]

/retest

[liorokman]

all e2e tests consistently seem to be consistently failing @liorokman

@arkodg all e2e tests have passed.