Skip to content

Commit

Permalink
fix: Allow Policy to attach to multiple http listeners (envoyproxy#2967)
Browse files Browse the repository at this point in the history 
* Fixing the clienttrafficpolicy validation.

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Make SecurityPolicy validate correctly.

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Reverted the SecurityPolicy validation - handled differently via
another feature.

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Updated the tests to reflect that this validation isn't required for SecurityPolicy

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Added some comments to explain the validation being performed.

Signed-off-by: Lior Okman <lior.okman@sap.com>

* Updated the error message as requested in the review.

Signed-off-by: Lior Okman <lior.okman@sap.com>

---------

Signed-off-by: Lior Okman <lior.okman@sap.com>
(cherry picked from commit f9409e4)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
  • Loading branch information
@liorokman @arkodg
liorokman authored and arkodg committed Apr 8, 2024
1 parent c8d4de4 commit 460faaf
Show file tree
Hide file tree
Showing 5 changed files with 954 additions and 34 deletions.
29 changes: 25 additions & 4 deletions internal/gatewayapi/clienttrafficpolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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 {
return fmt.Errorf("affects additional listeners: %s", strings.Join(sameListeners, ", "))
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 {
if strings.Index(currName, gatewayName) != 0 {
conflictingListeners = append(conflictingListeners, currName)
}
}
if len(conflictingListeners) != 0 {
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("ClientTrafficPolicy is being applied to multiple http (non https) listeners (%s) on the same port, which is not allowed", strings.Join(sameListeners, ", "))
}
}
}
return nil
Expand Down
28 changes: 2 additions & 26 deletions internal/gatewayapi/securitypolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
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,
Expand Down Expand Up @@ -496,20 +486,6 @@ func (t *Translator) translateSecurityPolicyForGateway(
return errs
}

func validatePortOverlapForSecurityPolicyGateway(xds *ir.Xds) error {
affectedListeners := []string{}
for _, http := range xds.HTTP {
if sameListeners := listenersWithSameHTTPPort(xds, http); len(sameListeners) != 0 {
affectedListeners = append(affectedListeners, sameListeners...)
}
}

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

Expand Down
23 changes: 19 additions & 4 deletions internal/gatewayapi/testdata/conflicting-policies.out.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -261,9 +262,9 @@ securityPolicies:
namespace: default
conditions:
- lastTransitionTime: null
message: 'Affects multiple listeners: default/mfqjpuycbgjrtdww/http, 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:
Expand Down Expand Up @@ -314,6 +315,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:
Expand Down
225 changes: 225 additions & 0 deletions internal/gatewayapi/testdata/merge-with-isolated-policies-2.in.yaml
View file
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
Loading

0 comments on commit 460faaf

Please sign in to comment.