OperatorConfig CEL Validation#1957
Conversation
bernot-dev
force-pushed
the
operatorconfig-cel-validation
branch
from
95ec286 to
86a9805
Compare
![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 various OpenAPI and CEL validation constraints to the OperatorConfig CRD definitions and adds corresponding end-to-end validation tests. The reviewer identified several critical issues: the use of the non-existent CEL function isURL which will cause CRD application failures, and a MinLength constraint on queryProjectID that incorrectly prevents it from being left blank. Additionally, the reviewer noted a duplicate import of k8s.io/api/core/v1 under different aliases in the test file, recommending the consistent use of the existing v1 alias.
bernot-dev
force-pushed
the
operatorconfig-cel-validation
branch
2 times, most recently
from
50f73ff to
314c2bd
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces declarative CEL validations and kubebuilder constraints to OperatorConfig CRD fields, ensuring stricter validation for URLs, GCP project IDs, external labels, and secret names directly at the API level. Corresponding end-to-end tests were updated to clean up test resources and verify these new validation rules. The review feedback suggests further strengthening the validations by enforcing mutual exclusivity between the secret and configMap fields in SecretOrConfigMap, as well as adding DNS subdomain validation for the configMap name.
bernot-dev
force-pushed
the
operatorconfig-cel-validation
branch
2 times, most recently
from
9c4ddb7 to
12de88d
Compare
bernot-dev
force-pushed
the
operatorconfig-cel-validation
branch
2 times, most recently
from
32ed855 to
e5d8bdc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds robust OpenAPI v3 and CEL validation rules to the OperatorConfig CRD and its Go definitions, along with a new differential fuzz test to align CRD-level validations with the Go webhook. While these changes significantly improve validation coverage, the reviewer identified several missing constraints that should be added for completeness. Specifically, CEL validation rules should be implemented for queryProjectID format constraints, AlertmanagerEndpoints fields, and externalLabels keys to ensure valid Prometheus label names. Additionally, the reviewer advised restoring the enum validation for CompressionType (including an empty string option) to prevent invalid inputs, and adding name validation for ConfigMap selectors for consistency.
Adds kubebuilder CEL validations to `OperatorConfig` fields based on the migration away from webhook validation. - 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 Signed-off-by: Adam Bernot <bernot@google.com>
bernot-dev
force-pushed
the
operatorconfig-cel-validation
branch
from
cfc7993 to
af9d3ac
Compare
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Thanks, it looks clean. I wonder how maintainable this CEL vs webhook fuzzing is overtime but I assume it's tmp
2 participants
As part of the migration away from webhook validation toward declarative Kubernetes CRD validations, this PR introduces declarative OpenAPIv3 format markers and CEL (XValidation) rules to the OperatorConfig Custom Resource Definition (monitoring.googleapis.com/v1).
To ensure parity and prevent regression or behavioral drift during the transition from programmatic webhook validation to declarative schema/CEL validation, this PR also introduces Go native differential fuzz testing between the OpenAPI/CEL validators and the programmatic webhook validator (oc.Validate()).
This change slightly relaxes duration parsing, due to a bug in kube-openapi. Hopefully that will be fixed on the Kubernetes side in the future.