Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fail validation if baseInterval is 0s #5176

Merged
merged 2 commits into from
Jan 30, 2025
+280 −14
Merged

fail validation if baseInterval is 0s #5176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98f4ce7
fail validation if baseInterval is 0s
arkodg Jan 30, 2025
5d08c17
more validations
arkodg Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions internal/gatewayapi/backendtrafficpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,9 +333,12 @@
err = perr.WithMessage(err, "TCPKeepalive")
errs = errors.Join(errs, err)
}
if policy.Spec.Retry != nil {
rt = buildRetry(policy.Spec.Retry)

if rt, err = buildRetry(policy.Spec.Retry); err != nil {
err = perr.WithMessage(err, "Retry")
errs = errors.Join(errs, err)
}

if to, err = buildClusterSettingsTimeout(policy.Spec.ClusterSettings); err != nil {
err = perr.WithMessage(err, "Timeout")
errs = errors.Join(errs, err)
Expand Down Expand Up @@ -484,9 +487,12 @@
err = perr.WithMessage(err, "TCPKeepalive")
errs = errors.Join(errs, err)
}
if policy.Spec.Retry != nil {
rt = buildRetry(policy.Spec.Retry)

if rt, err = buildRetry(policy.Spec.Retry); err != nil {
err = perr.WithMessage(err, "Retry")
errs = errors.Join(errs, err)

Check warning on line 493 in internal/gatewayapi/backendtrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/backendtrafficpolicy.go#L492-L493 

Added lines #L492 - L493 were not covered by tests
}

if ct, err = buildClusterSettingsTimeout(policy.Spec.ClusterSettings); err != nil {
err = perr.WithMessage(err, "Timeout")
errs = errors.Join(errs, err)
Expand Down
26 changes: 19 additions & 7 deletions internal/gatewayapi/clustersettings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,10 @@
ret.HTTP2 = h2
}

ret.Retry = buildRetry(policy.Retry)
var err error
if ret.Retry, err = buildRetry(policy.Retry); err != nil {
return nil, err
}

Check warning on line 78 in internal/gatewayapi/clustersettings.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/clustersettings.go#L77-L78 

Added lines #L77 - L78 were not covered by tests

// If nothing was set in any of the above calls, return nil instead of an empty
// container
Expand Down Expand Up @@ -477,9 +480,9 @@
}
}

func buildRetry(r *egv1a1.Retry) *ir.Retry {
func buildRetry(r *egv1a1.Retry) (*ir.Retry, error) {
if r == nil {
return nil
return nil, nil
}

rt := &ir.Retry{}
Expand Down Expand Up @@ -518,13 +521,22 @@
if r.PerRetry.BackOff != nil {
if r.PerRetry.BackOff.MaxInterval != nil || r.PerRetry.BackOff.BaseInterval != nil {
bop := &ir.BackOffPolicy{}
if r.PerRetry.BackOff.BaseInterval != nil {
bop.BaseInterval = r.PerRetry.BackOff.BaseInterval
if bop.BaseInterval.Duration == 0 {
return nil, fmt.Errorf("baseInterval cannot be set to 0s")
}
}
if r.PerRetry.BackOff.MaxInterval != nil {
bop.MaxInterval = r.PerRetry.BackOff.MaxInterval
if bop.MaxInterval.Duration == 0 {
return nil, fmt.Errorf("maxInterval cannot be set to 0s")
}

Check warning on line 534 in internal/gatewayapi/clustersettings.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/clustersettings.go#L533-L534 

Added lines #L533 - L534 were not covered by tests
if bop.BaseInterval != nil && bop.BaseInterval.Duration > bop.MaxInterval.Duration {
return nil, fmt.Errorf("maxInterval cannot be less than baseInterval")
}
}

if r.PerRetry.BackOff.BaseInterval != nil {
bop.BaseInterval = r.PerRetry.BackOff.BaseInterval
}
pr.BackOff = bop
bpr = true
}
Expand All @@ -535,5 +547,5 @@
}
}

return rt
return rt, nil
}
69 changes: 68 additions & 1 deletion internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.in.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,44 @@ httpRoutes:
backendRefs:
- name: service-1
port: 8080
- apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
namespace: default
name: httproute-2
spec:
hostnames:
- gateway.envoyproxy.io
parentRefs:
- namespace: envoy-gateway
name: gateway-2
sectionName: http
rules:
- matches:
- path:
value: "/route2"
backendRefs:
- name: service-1
port: 8080
- apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
namespace: default
name: httproute-3
spec:
hostnames:
- gateway.envoyproxy.io
parentRefs:
- namespace: envoy-gateway
name: gateway-2
sectionName: http
rules:
- matches:
- path:
value: "/route3"
backendRefs:
- name: service-1
port: 8080
backendTrafficPolicies:
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
Expand All @@ -86,7 +124,7 @@ backendTrafficPolicies:
kind: BackendTrafficPolicy
metadata:
namespace: default
name: policy-for-route
name: policy-for-route-1
spec:
targetRef:
group: gateway.networking.k8s.io
Expand All @@ -106,3 +144,32 @@ backendTrafficPolicies:
backoff:
baseInterval: 100ms
maxInterval: 10s
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
namespace: default
name: policy-for-route-2
spec:
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: httproute-2
retry:
perRetry:
backoff:
baseInterval: 0s
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
namespace: default
name: policy-for-route-3
spec:
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: httproute-3
retry:
perRetry:
backoff:
baseInterval: 2s
maxInterval: 1s
185 changes: 183 additions & 2 deletions internal/gatewayapi/testdata/backendtrafficpolicy-with-retries.out.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ backendTrafficPolicies:
kind: BackendTrafficPolicy
metadata:
creationTimestamp: null
name: policy-for-route
name: policy-for-route-1
namespace: default
spec:
retry:
Expand Down Expand Up @@ -39,6 +39,67 @@ backendTrafficPolicies:
status: "True"
type: Accepted
controllerName: gateway.envoyproxy.io/gatewayclass-controller
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
creationTimestamp: null
name: policy-for-route-2
namespace: default
spec:
retry:
perRetry:
backOff:
baseInterval: 0s
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: httproute-2
status:
ancestors:
- ancestorRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-2
namespace: envoy-gateway
sectionName: http
conditions:
- lastTransitionTime: null
message: 'Retry: baseInterval cannot be set to 0s.'
reason: Invalid
status: "False"
type: Accepted
controllerName: gateway.envoyproxy.io/gatewayclass-controller
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
creationTimestamp: null
name: policy-for-route-3
namespace: default
spec:
retry:
perRetry:
backOff:
baseInterval: 2s
maxInterval: 1s
targetRef:
group: gateway.networking.k8s.io
kind: HTTPRoute
name: httproute-3
status:
ancestors:
- ancestorRef:
group: gateway.networking.k8s.io
kind: Gateway
name: gateway-2
namespace: envoy-gateway
sectionName: http
conditions:
- lastTransitionTime: null
message: 'Retry: maxInterval cannot be less than baseInterval.'
reason: Invalid
status: "False"
type: Accepted
controllerName: gateway.envoyproxy.io/gatewayclass-controller
- apiVersion: gateway.envoyproxy.io/v1alpha1
kind: BackendTrafficPolicy
metadata:
Expand Down Expand Up @@ -131,7 +192,7 @@ gateways:
protocol: HTTP
status:
listeners:
- attachedRoutes: 1
- attachedRoutes: 3
conditions:
- lastTransitionTime: null
message: Sending translated listener configuration to the data plane
Expand Down Expand Up @@ -227,6 +288,82 @@ httpRoutes:
name: gateway-2
namespace: envoy-gateway
sectionName: http
- apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
creationTimestamp: null
name: httproute-2
namespace: default
spec:
hostnames:
- gateway.envoyproxy.io
parentRefs:
- name: gateway-2
namespace: envoy-gateway
sectionName: http
rules:
- backendRefs:
- name: service-1
port: 8080
matches:
- path:
value: /route2
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: envoy-gateway
sectionName: http
- apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
creationTimestamp: null
name: httproute-3
namespace: default
spec:
hostnames:
- gateway.envoyproxy.io
parentRefs:
- name: gateway-2
namespace: envoy-gateway
sectionName: http
rules:
- backendRefs:
- name: service-1
port: 8080
matches:
- path:
value: /route3
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: envoy-gateway
sectionName: http
infraIR:
envoy-gateway/gateway-1:
proxy:
Expand Down Expand Up @@ -325,6 +462,50 @@ xdsIR:
mergeSlashes: true
port: 10080
routes:
- destination:
name: httproute/default/httproute-2/rule/0
settings:
- addressType: IP
endpoints:
- host: 7.7.7.7
port: 8080
protocol: HTTP
weight: 1
directResponse:
statusCode: 500
hostname: gateway.envoyproxy.io
isHTTP2: false
metadata:
kind: HTTPRoute
name: httproute-2
namespace: default
name: httproute/default/httproute-2/rule/0/match/0/gateway_envoyproxy_io
pathMatch:
distinct: false
name: ""
prefix: /route2
- destination:
name: httproute/default/httproute-3/rule/0
settings:
- addressType: IP
endpoints:
- host: 7.7.7.7
port: 8080
protocol: HTTP
weight: 1
directResponse:
statusCode: 500
hostname: gateway.envoyproxy.io
isHTTP2: false
metadata:
kind: HTTPRoute
name: httproute-3
namespace: default
name: httproute/default/httproute-3/rule/0/match/0/gateway_envoyproxy_io
pathMatch:
distinct: false
name: ""
prefix: /route3
- destination:
name: httproute/default/httproute-1/rule/0
settings:
Expand Down