-
Notifications
You must be signed in to change notification settings - Fork 616
cleanup: deprecate Type field in BackendSpec #13071
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.
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
base: main
Are you sure you want to change the base?
Conversation
The Type field in BackendSpec is now optional and deprecated. The backend type is inferred from the configuration fields. - Mark BackendSpec.Type as optional with deprecation comment - Update CEL validation rules to support missing type field - Add getBackendType() helper to infer type from config - Add E2E test case for backend without explicit type Signed-off-by: devesh <[email protected]>
timflannagan
left a comment
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.
Thanks for the contribution @devc007. A couple quick comments.
| // +kubebuilder:validation:XValidation:message="aws backend must be specified when type is 'AWS'",rule="self.type == 'AWS' ? has(self.aws) : true" | ||
| // +kubebuilder:validation:XValidation:message="static backend must be specified when type is 'Static'",rule="self.type == 'Static' ? has(self.static) : true" | ||
| // +kubebuilder:validation:XValidation:message="dynamicForwardProxy backend must be specified when type is 'DynamicForwardProxy'",rule="self.type == 'DynamicForwardProxy' ? has(self.dynamicForwardProxy) : true" |
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.
I think we still want these rules in place even if the Type field becomes optional.
| // +kubebuilder:validation:XValidation:message="static backend must be specified when type is 'Static'",rule="self.type == 'Static' ? has(self.static) : true" | ||
| // +kubebuilder:validation:XValidation:message="dynamicForwardProxy backend must be specified when type is 'DynamicForwardProxy'",rule="self.type == 'DynamicForwardProxy' ? has(self.dynamicForwardProxy) : true" | ||
| // +kubebuilder:validation:ExactlyOneOf=aws;static;dynamicForwardProxy | ||
| // +kubebuilder:validation:XValidation:message="exactly one of aws, static, or dynamicForwardProxy must be specified",rule="[has(self.aws), has(self.static), has(self.dynamicForwardProxy)].filter(v, v).size() == 1" |
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.
Let's revert back to // +kubebuilder:validation:ExactlyOneOf=aws;static;dynamicForwardProxy. That ExactlyOneOf marker is the preferred pattern for these one-of based APIs.
| wantErrors: []string{"exactly one of the fields in [aws static dynamicForwardProxy] must be set"}, | ||
| }, | ||
| { | ||
| name: "Backend: inferred type from configuration", |
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.
I don't think we need to test this, but if we do, let's migrate it over to the https://github.com/kgateway-dev/kgateway/tree/af11f9bab3cfae7d2d2d9f61df70fc97325c7348/api/tests suite.
| // +required | ||
| Type BackendType `json:"type"` | ||
| // +optional | ||
| Type BackendType `json:"type,omitempty"` |
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.
Surprised the linter didn't catch this, but this should be a pointer type now. See https://github.com/kgateway-dev/kgateway/pull/12923/files for some inspiration.
Description
The Type field in BackendSpec is now optional and deprecated. The backend type is inferred from the configuration fields.
close #13029
Change Type
Changelog
Additional Notes