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

Jump to bottom

BackendTrafficPolicy openapi schemas do not validate correctly #4746

Open
nicks opened this issue Nov 20, 2024 · 4 comments · May be fixed by #4931
Open

BackendTrafficPolicy openapi schemas do not validate correctly #4746

nicks opened this issue Nov 20, 2024 · 4 comments · May be fixed by #4931
Assignees
@arkodg
Labels
kind/bug Something isn't working release-note Indicates a required release note
Milestone
v1.3.0-rc.1

Comments

@nicks
Copy link

nicks commented Nov 20, 2024 •
edited
Loading

Description:
I'm trying to use kubeconform to validate BackendTrafficPolicy

I get this error:

stdin - BackendTrafficPolicy my-policy is invalid: problem validating schema. Check JSON formatting: jsonschema: '/spec/healthCheck/active/interval' does not validate with https://raw.githubusercontent.com/datreeio/CRDs-catalog/main/gateway.envoyproxy.io/backendtrafficpolicy_v1alpha1.json#/properties/spec/properties/healthCheck/properties/active/properties/interval/format: '3s' is not valid 'duration'
stdin - BackendTrafficPolicy my-policy is invalid: problem validating schema. Check JSON formatting: jsonschema: '/spec/healthCheck/active/timeout' does not validate with https://raw.githubusercontent.com/datreeio/CRDs-catalog/main/gateway.envoyproxy.io/backendtrafficpolicy_v1alpha1.json#/properties/spec/properties/healthCheck/properties/active/properties/timeout/format: '20ms' is not valid 'duration'

Additional info:
I think the openapi schemas that we're generating are wrong. Particularly this line:

gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml

Line 314 in 6c6633c

format: duration

They say that timeout and interval are openapi durations. But they're not, they're a Kubernetes-specific duration format.
@nicks nicks added the triage label Nov 20, 2024
@arkodg
Copy link
Contributor

arkodg commented Nov 20, 2024

thanks for raising this, do we need to edit these to string instead ?
The CEL validation on the metav1.Duration should still kick in and allow acceptable values

gateway/api/v1alpha1/healthcheck_types.go

Line 81 in 6c6633c

// +kubebuilder:validation:Format=duration

@nicks
Copy link
Author

nicks commented Nov 20, 2024

ya, i poked around at how Cluster API is doing it, and i think they just leave the kubebuilder validation off entirely?
https://github.com/kubernetes-sigs/cluster-api/blob/781d1e4c39286d3d1e93c71557b5d02c2e78961b/api/v1beta1/machinehealthcheck_types.go#L101

and it will do the right thing with the existing metav1.Duration type?

@arkodg
Copy link
Contributor

arkodg commented Nov 20, 2024

ok I see an issue

we ended up using metav1.Duration in this API but we really should have used https://github.com/kubernetes-sigs/gateway-api/blob/962b35ea3c71ae604e0db696780c76a86b58ee17/apis/v1/shared_types.go#L765
which is used everywhere else in the project

gateway/api/v1alpha1/keepalive_types.go

Line 26 in 6c6633c

IdleTime *gwapiv1.Duration `json:"idleTime,omitempty"`

this has a CEL validation e.g.

gateway/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml

Line 1305 in 6c6633c

pattern: ^([0-9]{1,5}(h|m|s|ms)){1,4}$

, and doesnt require a kubebuilder validation

this is a breaking change, because we are tightening the validation, not loosening it

https://github.com/kubernetes/apimachinery/blob/96b97de8d6ba49bc192968551f2120ef3881f42d/pkg/apis/meta/v1/duration.go#L27

cc @envoyproxy/gateway-maintainers

@arkodg arkodg added kind/bug Something isn't working release-note Indicates a required release note and removed triage labels Nov 20, 2024
@arkodg
Copy link
Contributor

arkodg commented Nov 20, 2024

thanks for helping us find this @nicks

we'll also need to add an entry in

gateway/tools/linter/golangci-lint/.golangci.yml

Line 34 in 6c6633c

deny:
to make sure this doesnt happen again

@arkodg arkodg added the help wanted Extra attention is needed label Nov 20, 2024
@arkodg arkodg added this to the v1.3.0-rc.1 milestone Nov 21, 2024
arkodg added a commit to arkodg/gateway that referenced this issue Dec 14, 2024
@arkodg
Use gwapiv1.Duration instead of metav1.Duration in the API
58c92d7 
* Also rm `+kubebuilder:validation:Format=duration` and instead
rely on the CEL validation of gwapiv1.Duration

Fixes: envoyproxy#4746

Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg linked a pull request Dec 14, 2024 that will close this issue
Draft
@arkodg arkodg removed the help wanted Extra attention is needed label Dec 14, 2024
@arkodg arkodg self-assigned this Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arkodg arkodg

Labels
kind/bug Something isn't working release-note Indicates a required release note
Projects
None yet
Milestone
v1.3.0-rc.1
Development

Successfully merging a pull request may close this issue.

Use gwapiv1.Duration instead of metav1.Duration in the API arkodg/gateway
2 participants
@nicks @arkodg