feat: Add CEL validation to OperatorConfig CRD#1911
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces CEL-based validations and kubebuilder markers to the OperatorConfig CRD and its corresponding Go types to enforce stricter validation for labels, URLs, GCP project IDs, and Kubernetes resource names. The feedback identifies that the validation for Secret names is overly restrictive, as it currently limits names to 63 characters and excludes dots, which violates the DNS subdomain standard (RFC 1123) used by Kubernetes. It is recommended to update these regex patterns and size constraints to allow up to 253 characters and include dots.
| // Struct containing the client cert file for the targets. | ||
| Cert *SecretOrConfigMap `json:"cert,omitempty"` | ||
| // Secret containing the client key file for the targets. | ||
| // +kubebuilder:validation:XValidation:rule="has(self.name) && self.name.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?$') && size(self.name) <= 63" |
There was a problem hiding this comment.
The CEL validation for the Secret name is too restrictive. Kubernetes Secret names follow the DNS subdomain standard (RFC 1123), which allows dots (.) and can be up to 253 characters long. The current regex and size constraint only allow a single DNS label of up to 63 characters, which will reject valid secret names like my-secret.v1.
Additionally, to fully move validation logic out of the webhook as intended, this validation should be applied consistently to all other SecretKeySelector fields in the API (e.g., CollectionSpec.Credentials, RuleEvaluatorSpec.Credentials, and ManagedAlertmanagerSpec.ConfigSecret).
| // +kubebuilder:validation:XValidation:rule="has(self.name) && self.name.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?$') && size(self.name) <= 63" | |
| // +kubebuilder:validation:XValidation:rule="has(self.name) && self.name.matches('^[a-z0-9]([-a-z0-9.]*[a-z0-9])?$') && size(self.name) <= 253" |
There was a problem hiding this comment.
@jules make this change, but escape the . properly with a backslash so it doesn't match "any character."
There was a problem hiding this comment.
I've escaped the . correctly using double backslashes in the Go string, and updated the pattern to properly match an RFC 1123 sub-domain format.
| - rule: has(self.name) && self.name.matches('^[a-z0-9]([-a-z0-9]*[a-z0-9])?$') | ||
| && size(self.name) <= 63 |
There was a problem hiding this comment.
The regex and size limit for the Secret name are too restrictive. Kubernetes Secrets use DNS subdomain names, which allow dots and up to 253 characters. The current rule restricts names to a single DNS label of 63 characters.
- rule: has(self.name) && self.name.matches('^[a-z0-9]([-a-z0-9.]*[a-z0-9])?$')
&& size(self.name) <= 253
Adds kubebuilder CEL validations to `OperatorConfig` fields based on the migration away from webhook validation, per the provided design doc requirements. - Validates prometheus label keys in `externalLabels` - Validates `queryProjectID` constraints (length and regex pattern) - Adds `isURL` checks for `generatorUrl`, `exports.url`, `externalURL` - Implements RFC 1123 label constraints for AlertmanagerEndpoints namespace and name - Validates TLSConfig KeySecret name - Enforces HTTP/HTTPS scheme for AlertmanagerEndpoints Co-authored-by: bernot-dev <34751694+bernot-dev@users.noreply.github.com>
Addresses code review feedback to properly escape the `.` character in the CEL regex validation for the KeySecret name, so it correctly matches literal dots instead of "any character". Also correctly implemented the full RFC1123 subdomain rules `^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` Co-authored-by: bernot-dev <34751694+bernot-dev@users.noreply.github.com>
Addresses code review feedback to properly escape the `.` character in the CEL regex validation for the KeySecret name, so it correctly matches literal dots instead of "any character". Also correctly implemented the full RFC1123 subdomain rules `^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` Co-authored-by: bernot-dev <34751694+bernot-dev@users.noreply.github.com>
Updates commit messages to conform to conventional commits standards. Co-authored-by: bernot-dev <34751694+bernot-dev@users.noreply.github.com>
bernot-dev
force-pushed
the
operatorconfig-cel-validation-10161733801110130447
branch
from
e3df1a4 to
64f16ce
Compare
1 participant
Implement CEL-based validation and OpenAPIv3 defaulting for OperatorConfig CRD. This moves validation logic out of the webhook server and embeds it directly into the Custom Resource Definitions, improving reliability by avoiding startup races and zero-node issues.
PR created automatically by Jules for task 10161733801110130447 started by @bernot-dev