CNTRLPLANE-2049: fix(api): audit and improve GCP API validation rules - #8003
Conversation
- Replace Pattern markers with XValidation (CEL) for better error messages - Add omitempty/omitzero to required fields per KAL conventions - Create GCPServiceAccountEmail type alias for reusable validation - Fix GCPResourceLabel.Value validation to match GCP documentation - Drop unnecessary pointers where MinLength prevents zero values - Add listType markers for Server-Side Apply compatibility - Add MinLength, MinItems validation markers where applicable - Remove 21 stale linter exclusions, add exclusions for intentional pointer usage - Use +default instead of +kubebuilder:default - Remove patchStrategy/patchMergeKey from CRD Conditions - Replace complex IPv4/IPv6 regex with self.isIP() CEL function Co-authored-by: Nirshal <37444255+Nirshal@users.noreply.github.com> Assisted-by: Claude:claude-opus-4-6
- Add MinItems=1 to optional list fields (ResourceLabels, NetworkTags, Scopes) per JoelSpeed review: if list is optional with omitempty, MinItems=1 ensures empty arrays are not persisted - Set GCPServiceAccountEmail MaxLength=85 (theoretical max: 30-char name + @ + 30-char project + .iam.gserviceaccount.com) per JoelSpeed recommendation - Fix GCPResourceLabel.Value regex to require non-empty values start with a lowercase letter per GCP Compute Engine documentation Assisted-by: Claude:claude-opus-4-6
Introduce GCPResourceName type alias for RFC 1035 resource names to DRY
out repeated validation markers across 5 fields. Fix several validation
inaccuracies found by cross-referencing GCP documentation:
- Fix region error message to use concrete examples instead of confusing
template notation
- Fix serviceAttachmentURI region regex to be consistent with standalone
region validation
- Remove unnecessary capturing groups from project/pool/provider regexes
- Widen consumerAcceptList project number range from {6,12} to {1,30}
All changes are backward compatible: GCPResourceName serializes
identically to string in JSON, regex changes are cosmetic or strictly
more permissive, and the type alias follows the existing
GCPServiceAccountEmail pattern.
Assisted-by: Claude:claude-opus-4-6
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis pull request replaces several generic GCP-related string fields with specialized types ( ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
area/api
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
cblecker
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@cblecker: This pull request references CNTRLPLANE-2049 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target either version "4.22." or "openshift-4.22.", but it targets "openshift-4.21" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @Nirshal @JoelSpeed |
|
@cblecker: This pull request references CNTRLPLANE-2049 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target either version "4.22." or "openshift-4.22.", but it targets "openshift-4.21" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/hypershift/v1beta1/gcpprivateserviceconnect_types.go`:
- Around line 122-125: The EndpointIP field's XValidation allows an explicit
empty string but the struct tag includes +kubebuilder:validation:MinLength=3
which will reject "" before XValidation runs; to fix, decide the intended
behavior and apply one of two changes: if explicit empty string should be
permitted, remove the +kubebuilder:validation:MinLength=3 tag (keep EndpointIP
string `json:"endpointIP,omitempty"` and the XValidation/isIP check), or if
empty string should be forbidden, change the XValidation rule to remove `self ==
''` so MinLength=3 enforces non-empty; update the EndpointIP field accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 42bb2059-f8b1-42fc-bc3a-c970e1375cb7
⛔ Files ignored due to path filters (19)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/gcpprivateserviceconnects.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/gcpnodepoolplatform.gois excluded by!client/**cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/gcpprivateserviceconnects-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/gcpprivateserviceconnects-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcp.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go
📒 Files selected for processing (5)
api/.golangci.ymlapi/hypershift/v1beta1/gcp.goapi/hypershift/v1beta1/gcpprivateserviceconnect_types.gohypershift-operator/controllers/nodepool/gcp.gohypershift-operator/controllers/nodepool/gcp_test.go
💤 Files with no reviewable changes (1)
- api/.golangci.yml
✅ Files skipped from review due to trivial changes (1)
- hypershift-operator/controllers/nodepool/gcp_test.go
Remove `self == '' ||` from endpointIP XValidation and restore MinLength=3. The field has omitempty so absent values skip validation entirely — no need to allow explicit empty string in the CEL rule. Assisted-by: Claude:claude-opus-4-6
|
@cblecker: This pull request references CNTRLPLANE-2049 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target either version "4.22." or "openshift-4.22.", but it targets "openshift-4.21" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
api/hypershift/v1beta1/gcpprivateserviceconnect_types.go (4)
40-54: Documentation marker inconsistency.The
Namefield has+required(line 42) but the JSON tag usesomitempty. This creates ambiguity: the documentation indicates the field is required, but serialization allows omission. Consider aligning these by either removingomitemptyor changing to+optionalwith validation constraints.The
Recordsfield withMinItems=1and+optionalis correctly designed—ensures non-empty when provided.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/gcpprivateserviceconnect_types.go` around lines 40 - 54, The DNSZoneStatus struct has a mismatch: the Name field is marked +required but the JSON tag uses omitempty, so update DNSZoneStatus by making the Name field required in serialization (remove omitempty from the json tag for Name) so it reads json:"name" while keeping the kubebuilder validation markers on Name; leave the Records field and its +optional/MinItems markers unchanged.
72-83: Same stale+requiredmarker on ConsumerAcceptList; validation regex looks correct.Line 76 still has
+requiredbut the JSON tag was changed toomitempty. The regex correctly validates both project IDs (6-30 char format) and project numbers (1-30 digits, widened as intended).📝 Suggested fix
// See https://cloud.google.com/resource-manager/docs/creating-managing-projects for project ID and number formats. - // +required + // +optional // +listType=set🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/gcpprivateserviceconnect_types.go` around lines 72 - 83, The struct field ConsumerAcceptList has a stale +required marker but its JSON tag is `omitempty`; remove the `+required` kubebuilder marker (or alternatively remove `omitempty` if you intend the field to be required) so the annotations and tag are consistent; update the annotations next to ConsumerAcceptList to either delete `+required` or make the json tag non-omitempty and ensure MinItems/MaxItems remain correct.
61-65: Stale+requiredmarker on LoadBalancerIP.The AI summary indicates this field was intentionally changed from required to optional, but the
+requiredmarker on line 61 remains. The comment on lines 58-60 also states it's "Populated by the observer," which suggests it shouldn't be required at creation time.📝 Suggested fix
// loadBalancerIP is the IP address of the Internal Load Balancer // Populated by the observer from service status // This value must be a valid IPv4 or IPv6 address. - // +required + // +optional // +kubebuilder:validation:XValidation:rule="isIP(self)",message="loadBalancerIP must be a valid IPv4 or IPv6 address"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/gcpprivateserviceconnect_types.go` around lines 61 - 65, The field annotation for LoadBalancerIP is stale: remove the "+required" kubebuilder marker (or replace it with "+optional") so the LoadBalancerIP string field is truly optional as intended; update the comment/markers around the LoadBalancerIP declaration in the LoadBalancerIP struct field (the LoadBalancerIP string `json:"loadBalancerIP,omitempty"`) to reflect it is populated by the observer and not required at creation time.
98-98: MinItems=1 on optional status collections.
Conditions(line 98) andDNSZones(line 129) are+optionalbut haveMinItems=1. This means:
- Omitting the field: ✓ valid
- Providing
[]: ✗ fails MinItems- Providing non-empty list: ✓ valid
For status fields that are populated asynchronously, ensure controllers don't set empty arrays. This is likely fine since status validation is typically lenient, but worth noting.
Also applies to: 129-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/hypershift/v1beta1/gcpprivateserviceconnect_types.go` at line 98, The status slice fields Conditions and DNSZones currently have +kubebuilder:validation:MinItems=1 while also marked +optional, which rejects empty arrays; either remove the MinItems=1 constraint from those fields (Conditions and DNSZones in gcpprivateserviceconnect_types.go) so empty slices are allowed, or make the fields required and document that controllers must never write an empty slice—pick the former for typical async status fields and update the kubebuilder markers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/hypershift/v1beta1/gcpprivateserviceconnect_types.go`:
- Around line 40-54: The DNSZoneStatus struct has a mismatch: the Name field is
marked +required but the JSON tag uses omitempty, so update DNSZoneStatus by
making the Name field required in serialization (remove omitempty from the json
tag for Name) so it reads json:"name" while keeping the kubebuilder validation
markers on Name; leave the Records field and its +optional/MinItems markers
unchanged.
- Around line 72-83: The struct field ConsumerAcceptList has a stale +required
marker but its JSON tag is `omitempty`; remove the `+required` kubebuilder
marker (or alternatively remove `omitempty` if you intend the field to be
required) so the annotations and tag are consistent; update the annotations next
to ConsumerAcceptList to either delete `+required` or make the json tag
non-omitempty and ensure MinItems/MaxItems remain correct.
- Around line 61-65: The field annotation for LoadBalancerIP is stale: remove
the "+required" kubebuilder marker (or replace it with "+optional") so the
LoadBalancerIP string field is truly optional as intended; update the
comment/markers around the LoadBalancerIP declaration in the LoadBalancerIP
struct field (the LoadBalancerIP string `json:"loadBalancerIP,omitempty"`) to
reflect it is populated by the observer and not required at creation time.
- Line 98: The status slice fields Conditions and DNSZones currently have
+kubebuilder:validation:MinItems=1 while also marked +optional, which rejects
empty arrays; either remove the MinItems=1 constraint from those fields
(Conditions and DNSZones in gcpprivateserviceconnect_types.go) so empty slices
are allowed, or make the fields required and document that controllers must
never write an empty slice—pick the former for typical async status fields and
update the kubebuilder markers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2035c117-b5a1-4b81-965a-f63a002d0b30
⛔ Files ignored due to path filters (4)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/gcpprivateserviceconnects.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/gcpprivateserviceconnects-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/gcpprivateserviceconnects-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/gcpprivateserviceconnect_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
api/hypershift/v1beta1/gcpprivateserviceconnect_types.go
|
GKE test failing until #7965 merges |
| // +optional | ||
| // +kubebuilder:default="pd-balanced" | ||
| // +default="pd-balanced" | ||
| // +kubebuilder:validation:Enum=pd-standard;pd-ssd;pd-balanced |
There was a problem hiding this comment.
Before we ship, would be good to make sure Enum values are consistently PascalCase to align with K8s API conventions
There was a problem hiding this comment.
We'll want this API to pass the linter clean before promoting it.
|
Scheduling tests matching the |
| x-kubernetes-validations: | ||
| - message: loadBalancerIP must be a valid IPv4 or IPv6 address | ||
| rule: self.matches('^((\\d{1,3}\\.){3}\\d{1,3})$') || self.matches('^(([0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:))$') | ||
| rule: isIP(self) |
There was a problem hiding this comment.
we'll want to address #8016 before removing the gate for this API
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker, enxebre, JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Test Resultse2e-aws
e2e-aks
|
|
/retest |
1 similar comment
|
/retest |
|
/skip |
|
/verified by e2e @cblecker |
openshift-ci-robot
added
the
verified
|
@cblecker: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@cblecker: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Update expected error substrings in TestOnCreateAPIUX to match the new XValidation rule messages introduced in PR openshift#8003. The old expectations used Pattern-style messages ("in body should match") which were replaced by custom XValidation messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update expected error substrings in TestOnCreateAPIUX to match the new XValidation rule messages introduced in PR openshift#8003. The old expectations used Pattern-style messages ("in body should match") which were replaced by custom XValidation messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update expected error substrings in the v2 e2e test suite (api_ux_validation_test.go) to match the new XValidation rule messages introduced in PR openshift#8003, same as the v1 e2e test fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update expected error substrings in TestOnCreateAPIUX to match the new XValidation rule messages introduced in PR openshift#8003. The old expectations used Pattern-style messages ("in body should match") which were replaced by custom XValidation messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update expected error substrings in the v2 e2e test suite (api_ux_validation_test.go) to match the new XValidation rule messages introduced in PR openshift#8003, same as the v1 e2e test fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
5 participants
Summary
GCPResourceNametype alias for RFC 1035 resource names, consolidating repeated MinLength/MaxLength/XValidation markers across 5 fields (GCPResourceReference.Name,GCPNodePoolPlatform.Subnet,GCPNodePoolPlatform.NetworkTags,GCPPrivateServiceConnectSpec.ForwardingRuleName,GCPPrivateServiceConnectSpec.NATSubnet)geographic-area-location-numbertemplateserviceAttachmentURIregion regex segment to be consistent with standalone region validation ([a-z]+-[a-z]+[0-9]+)consumerAcceptListproject number range from{6,12}to{1,30}Compatibility
All changes are backward compatible:
GCPResourceNameserializes identically tostringin JSON (N-1/N+1 safe)GCPServiceAccountEmailpatternTest plan
make updatesucceeds (CRDs, deepcopy, clients, apply configs regenerated)make verifypasses (staticcheck, go fmt, go vet all clean)make testpasses (all unit tests with race detection)Summary by CodeRabbit
Refactor
Bug Fixes
Tests