OCPBUGS-77827 fix(api): add missing has() check for servingCerts in validation#7901
OCPBUGS-77827 fix(api): add missing has() check for servingCerts in validation#7901eponafyrefly wants to merge 1 commit intoopenshift:mainfrom
Conversation
PR openshift#6091 introduced a CEL validation rule that checks if the APIServer loadBalancer hostname conflicts with namedCertificates. However, the validation directly accesses configuration.apiServer.servingCerts.namedCertificates without checking if the optional servingCerts field exists. This causes validation failures when patching HostedCluster resources that don't have the servingCerts field populated. Add has(self.configuration.apiServer.servingCerts) check before accessing namedCertificates to fix the regression.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: eponafyrefly The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
area/api
WalkthroughThe pull request adds defensive null-check guards to the APIServer loadBalancer hostname validation rule across CRD manifests. The change ensures Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @eponafyrefly. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
/test all |
|
@eponafyrefly: Cannot trigger testing until a trusted user reviews the PR and leaves an 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 kubernetes-sigs/prow 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: 7
🤖 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/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml`:
- Around line 6004-6006: The CEL expression dereferences optional fields without
guards—specifically self.configuration.apiServer.servingCerts.namedCertificates
and cert.names—so update the predicate to wrap those accesses with has() checks
(e.g., has(self.configuration.apiServer.servingCerts.namedCertificates) and
has(cert.names)) before calling .exists; adjust the inner exists() lambdas (the
one iterating namedCertificates and the cert.names.exists(...) call) to only run
when those has() checks pass so you don't access missing fields in the
namedCertificates and cert.names references.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml`:
- Around line 6355-6356: The CEL expression calls .exists() on
self.configuration.apiServer.servingCerts.namedCertificates without first
checking the field exists; update the boolean guard by adding
has(self.configuration.apiServer.servingCerts.namedCertificates) before calling
.exists() so the expression becomes ... && has(self.configuration.apiServer) &&
has(self.configuration.apiServer.servingCerts) &&
has(self.configuration.apiServer.servingCerts.namedCertificates) &&
self.configuration.apiServer.servingCerts.namedCertificates.exists(...),
ensuring namedCertificates is present before dereferencing in the validation
rule.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml`:
- Around line 6365-6369: The CEL validation rule currently dereferences optional
fields without guarding them; update the rule string so each optional access is
protected: add a has(s.servicePublishingStrategy.loadBalancer.hostname) (or
check hostname != "" only after confirming has(loadBalancer) and has(hostname)),
add has(self.configuration.apiServer.servingCerts.namedCertificates) before
calling namedCertificates.exists(...), and guard cert.names with has(cert.names)
(or ensure cert.names exists/non-empty) before calling cert.names.exists; keep
the existing checks (e.g., has(self.configuration) and
has(self.configuration.apiServer) and
has(self.configuration.apiServer.servingCerts)) and apply these additional
has(...) checks around s.servicePublishingStrategy.loadBalancer.hostname,
self.configuration.apiServer.servingCerts.namedCertificates, and cert.names in
the CEL rule.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml`:
- Around line 6009-6010: The CEL expression dereferences an optional field:
before calling
self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, ...)
you must guard namedCertificates with
has(self.configuration.apiServer.servingCerts.namedCertificates); update the
condition that currently checks has(self.configuration.apiServer) &&
has(self.configuration.apiServer.servingCerts) to also include
has(self.configuration.apiServer.servingCerts.namedCertificates) so the call to
.exists() on namedCertificates is only evaluated when that field is present.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml`:
- Around line 6538-6540: The CEL rule dereferences two optional fields without
presence checks: self.configuration.apiServer.servingCerts.namedCertificates and
cert.names; update the expression used in the rule to guard those accesses with
has() checks (e.g., add
has(self.configuration.apiServer.servingCerts.namedCertificates) before
iterating namedCertificates and add has(cert.names) before calling
cert.names.exists) so the overall condition only evaluates those sub-expressions
when those optional fields are present.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml`:
- Around line 6698-6700: The CEL expression dereferences an optional field:
guard self.configuration.apiServer.servingCerts.namedCertificates before calling
.exists(); update the predicate that currently uses
self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, ...) to
first check has(self.configuration.apiServer.servingCerts.namedCertificates) &&
then call .exists(...). Apply the same change to the identical expressions in
hostedclusters-Hypershift-Default.crd.yaml,
hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml, and
hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml so all occurrences check
namedCertificates presence before .exists().
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`:
- Around line 7675-7677: The CEL validation currently checks
self.configuration.apiServer.servingCerts but calls .exists() on two optional
lists without guarding them: servingCerts.namedCertificates.exists(...) and
cert.names.exists(...); update the rule to first check
has(self.configuration.apiServer.servingCerts.namedCertificates) before calling
namedCertificates.exists(...) and has(cert.names) before calling
cert.names.exists(...) so both optional lists are guarded and CEL evaluation
won't fail when those lists are absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b02f72d4-f9bd-4b1c-8424-0a6f937d158f
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
api/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | ||
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | ||
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
head -c 500000 api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml | tail -c 50000 | tail -n 100Repository: openshift/hypershift
Length of output: 5603
🏁 Script executed:
# Get the exact lines mentioned in the review
sed -n '5990,6020p' api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlRepository: openshift/hypershift
Length of output: 2471
🏁 Script executed:
# Search for the schema definition of servingCerts to see if namedCertificates is required
sed -n '1,6000p' api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml | grep -A 50 "servingCerts" | head -100Repository: openshift/hypershift
Length of output: 3530
🌐 Web query:
Kubernetes CEL optional fields has() guard namedCertificates exists() semantics
💡 Result:
has() (presence guard) in Kubernetes CEL
has(e.f)is a CEL macro used to test presence / availability of a field before you rely on it. Kubernetes explicitly calls outhas()as the way to check whether a field of a dynamically typed value is accessible (to avoid errors when the field might not exist in the schema for the object you’re evaluating). (kubernetes.io)- Precise CEL-spec semantics depend on the underlying type of
e(github.com):- Map:
has(e.f)is true iff key"f"exists (even if the value isnull; null ≠ absence in CEL). (github.com) - Message / object
- If
fis repeated (list) or map field:has(e.f)is true iff it is non-empty. (github.com) - For proto3 scalar fields,
has(e.f)can effectively mean “value is not the default” (because proto3 historically can’t always distinguish “unset” from “set-to-default”). (github.com)
- If
- Map:
Implication for namedCertificates (a list field):
has(self.namedCertificates) is true only when the list is non-empty; it’s false for both “missing” and “present but empty”. (github.com)
exists() semantics (including error/short-circuit behavior)
list.exists(x, p)returns true if any element satisfies predicatep; for an empty list it returns false. (github.com)exists()“behaves like logical OR (||)” regarding short-circuiting and error absorption: if some element makes the result true, errors from other elements/predicates can be ignored (consistent with CEL’s OR behavior). (github.com)
So for namedCertificates:
self.namedCertificates.exists(c, /*match*/)is false when the list is empty (and typically also when it’s absent but treated as empty by the underlying object model). (github.com)- If you need “only enforce when configured”, a common pattern is:
!has(self.namedCertificates) || self.namedCertificates.exists(c, /*match*/)(skip rule unless non-empty), becausehas()on a list means non-empty. (github.com)
Citations:
- 1: https://kubernetes.io/docs/reference/using-api/cel
- 2: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 3: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 4: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 5: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 6: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 7: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 8: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 9: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 10: https://github.com/google/cel-spec/blob/master/doc/langdef.md
Guard namedCertificates and cert.names before dereferencing.
Lines 6005–6006 access two optional fields without has() guards: servingCerts.namedCertificates and cert.names. Both are optional in the schema—namedCertificates states "If no named certificates are provided..." and names is marked "optional list". Per Kubernetes CEL best practices, optional fields must be guarded with has() before dereferencing to prevent errors when fields are absent or empty.
Suggested fix
- && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
- && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
- cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))'
+ && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
+ && has(self.configuration.apiServer.servingCerts.namedCertificates)
+ && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
+ has(cert.names) && cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' | |
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && has(self.configuration.apiServer.servingCerts.namedCertificates) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| has(cert.names) && cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml`
around lines 6004 - 6006, The CEL expression dereferences optional fields
without guards—specifically
self.configuration.apiServer.servingCerts.namedCertificates and cert.names—so
update the predicate to wrap those accesses with has() checks (e.g.,
has(self.configuration.apiServer.servingCerts.namedCertificates) and
has(cert.names)) before calling .exists; adjust the inner exists() lambdas (the
one iterating namedCertificates and the cert.names.exists(...) call) to only run
when those has() checks pass so you don't access missing fields in the
namedCertificates and cert.names references.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | ||
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes CRD CEL validation rules: when should optional object/list fields be guarded with has(self.field), and are null or absent fields treated as absent during evaluation?
💡 Result:
When to guard optional object/list fields with has(self.field)
Guard any optional field when your rule would otherwise dereference it (access subfields) or call methods/macros on it (for lists: .size(), .all(), .exists(), indexing, etc.). The canonical pattern is to use short-circuiting:
- Optional list:
!has(self.items) || self.items.all(x, /* ... */) - Optional object:
!has(self.cfg) || self.cfg.mode in ['A','B']
Kubernetes’ own examples use this style for optional fields (including optional lists) in CRD validation/transition rules. (kubernetes.io)
Are null and “absent” treated the same?
No.
- Absent field:
has(self.field)is false (field is unset). (kubernetes.io) - Field present with value
null(only possible if the schema allowsnull): CEL sees a realnull_typevalue. (kubernetes.io) - In CEL,
nullcounts as “present” rather than “absent” (sohas(self.field)can be true even if the value isnull). (github.com)
So, if your schema permits null and you want “null behaves like unset”, explicitly guard both:
!has(self.field) || self.field == null || /* safe use of self.field */
(or equivalently: has(self.field) && self.field != null && ... when it must be provided).
Citations:
- 1: https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy
- 2: https://kubernetes.io/blog/2022/09/23/crd-validation-rules-beta/
- 3: https://kubernetes.io/docs/reference/using-api/cel
- 4: https://github.com/google/cel-spec/wiki/proposal-246
🏁 Script executed:
# First, check if the file exists and examine the context around lines 6355-6356
head -n 6360 api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml | tail -n 20Repository: openshift/hypershift
Length of output: 1690
🏁 Script executed:
# Find the schema definition for namedCertificates to confirm it's optional
rg "namedCertificates" api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml -B 5 -A 5Repository: openshift/hypershift
Length of output: 2361
🏁 Script executed:
# Check if namedCertificates is in any required array
rg "required:" api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml -A 20 | grep -i "namedCertificates\|servingCerts"Repository: openshift/hypershift
Length of output: 46
🏁 Script executed:
# Find the schema definition for servingCerts and check its required array
rg "servingCerts:" api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml -A 50 | head -n 70Repository: openshift/hypershift
Length of output: 3530
🏁 Script executed:
# Search more broadly for the required array that contains servingCerts properties
rg -A 100 "namedCertificates:" api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml | grep -A 30 "type:"Repository: openshift/hypershift
Length of output: 5122
Guard namedCertificates with has() before calling .exists().
Line 6356 calls .exists() on self.configuration.apiServer.servingCerts.namedCertificates without first checking that the field exists. namedCertificates is an optional list field (not required), so a resource with servingCerts: {} will still hit the same validation failure this patch intended to fix. Kubernetes CEL validation rules require optional fields to be guarded with has(self.field) before dereferencing or calling methods on them.
Suggested fix
&& has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
+ && has(self.configuration.apiServer.servingCerts.namedCertificates)
&& self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && has(self.configuration.apiServer.servingCerts.namedCertificates) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml`
around lines 6355 - 6356, The CEL expression calls .exists() on
self.configuration.apiServer.servingCerts.namedCertificates without first
checking the field exists; update the boolean guard by adding
has(self.configuration.apiServer.servingCerts.namedCertificates) before calling
.exists() so the expression becomes ... && has(self.configuration.apiServer) &&
has(self.configuration.apiServer.servingCerts) &&
has(self.configuration.apiServer.servingCerts.namedCertificates) &&
self.configuration.apiServer.servingCerts.namedCertificates.exists(...),
ensuring namedCertificates is present before dereferencing in the validation
rule.
| rule: '!self.services.exists(s, s.service == ''APIServer'' && has(s.servicePublishingStrategy.loadBalancer) | ||
| && s.servicePublishingStrategy.loadBalancer.hostname != "" && has(self.configuration) | ||
| && has(self.configuration.apiServer) && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | ||
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | ||
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | ||
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
There was a problem hiding this comment.
Guard the other optional fields in this CEL rule too.
This only fixes the servingCerts dereference. Line 6366 still reads the optional loadBalancer.hostname, and Lines 6368-6369 still read optional namedCertificates and cert.names. Valid specs such as servingCerts: {} or named certificates that rely on implicit names will still fail validation here instead of evaluating to false.
Suggested fix
- rule: '!self.services.exists(s, s.service == ''APIServer'' && has(s.servicePublishingStrategy.loadBalancer)
- && s.servicePublishingStrategy.loadBalancer.hostname != "" && has(self.configuration)
- && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
- && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
- cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))'
+ rule: '!self.services.exists(s, s.service == ''APIServer'' && has(s.servicePublishingStrategy.loadBalancer)
+ && has(s.servicePublishingStrategy.loadBalancer.hostname)
+ && s.servicePublishingStrategy.loadBalancer.hostname != "" && has(self.configuration)
+ && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
+ && has(self.configuration.apiServer.servingCerts.namedCertificates)
+ && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
+ has(cert.names) && cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rule: '!self.services.exists(s, s.service == ''APIServer'' && has(s.servicePublishingStrategy.loadBalancer) | |
| && s.servicePublishingStrategy.loadBalancer.hostname != "" && has(self.configuration) | |
| && has(self.configuration.apiServer) && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' | |
| rule: '!self.services.exists(s, s.service == ''APIServer'' && has(s.servicePublishingStrategy.loadBalancer) | |
| && has(s.servicePublishingStrategy.loadBalancer.hostname) | |
| && s.servicePublishingStrategy.loadBalancer.hostname != "" && has(self.configuration) | |
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && has(self.configuration.apiServer.servingCerts.namedCertificates) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| has(cert.names) && cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml`
around lines 6365 - 6369, The CEL validation rule currently dereferences
optional fields without guarding them; update the rule string so each optional
access is protected: add a
has(s.servicePublishingStrategy.loadBalancer.hostname) (or check hostname != ""
only after confirming has(loadBalancer) and has(hostname)), add
has(self.configuration.apiServer.servingCerts.namedCertificates) before calling
namedCertificates.exists(...), and guard cert.names with has(cert.names) (or
ensure cert.names exists/non-empty) before calling cert.names.exists; keep the
existing checks (e.g., has(self.configuration) and
has(self.configuration.apiServer) and
has(self.configuration.apiServer.servingCerts)) and apply these additional
has(...) checks around s.servicePublishingStrategy.loadBalancer.hostname,
self.configuration.apiServer.servingCerts.namedCertificates, and cert.names in
the CEL rule.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | ||
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and read the context around lines 6009-6010
file_path="api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml"
# Check file size first
wc -l "$file_path"Repository: openshift/hypershift
Length of output: 215
🏁 Script executed:
# Read the context around lines 6009-6010 (lines 6000-6020 for context)
file_path="api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml"
sed -n '6000,6020p' "$file_path"Repository: openshift/hypershift
Length of output: 1977
🏁 Script executed:
# Let's also look at the broader context to find this validation rule
file_path="api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml"
# Look at more context - the rule definition should be visible
sed -n '5995,6025p' "$file_path"Repository: openshift/hypershift
Length of output: 2471
🏁 Script executed:
# Search for the schema definition of namedCertificates to confirm if it's optional
# First, let's find where the CRD schema is defined
rg -A 5 -B 5 "namedCertificates" api/hypershift/v1beta1/ --type yaml | head -100Repository: openshift/hypershift
Length of output: 19415
🏁 Script executed:
# Let's also look for the servingCerts definition to understand the structure
rg -A 10 "servingCerts:" api/hypershift/v1beta1/ --type yaml | head -80Repository: openshift/hypershift
Length of output: 15269
🌐 Web query:
Kubernetes CEL optional fields has() macro .exists() method documentation
💡 Result:
-
Kubernetes CEL (how to use
has()for optional / dynamically-typed fields, and example usingexists): Kubernetes “Common Expression Language in Kubernetes” reference. (kubernetes.io)- Shows
has(self.expired)for checking presence before access, and anexistsexample likeself.widgets.exists(w, ...). (kubernetes.io)
- Shows
-
CEL language spec (what
has()andexists()actually are): CEL spec “Language Definition → Macros”. (github.com)has(e.f)is a macro for field/key presence checks (with detailed semantics for maps/proto2/proto3). (github.com)e.exists(x, p)is a comprehension macro on lists (elements) or maps (keys), returning true if any element/key satisfies the predicate. (github.com)
Citations:
- 1: https://kubernetes.io/docs/reference/using-api/cel
- 2: https://kubernetes.io/docs/reference/using-api/cel
- 3: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 4: https://github.com/google/cel-spec/blob/master/doc/langdef.md
- 5: https://github.com/google/cel-spec/blob/master/doc/langdef.md
🌐 Web query:
CEL validation rule optional field dereference Kubernetes CRD schema
💡 Result:
In Kubernetes CRD CEL rules (x-kubernetes-validations), you must guard any dereference of an optional / possibly-absent field using the has() macro (and rely on && / || short-circuiting), otherwise the expression can fail when that field is unset.
Pattern (recommended):
!has(self.spec.optionalField) || self.spec.optionalField.subField == "x"
or
has(self.spec.optionalField) && self.spec.optionalField.subField == "x"
Kubernetes’ CEL docs explicitly show this style (for example: has(self.expired) && ...) and list has as a standard macro available for CRD CEL. [1] The Kubernetes blog post on CRD immutability also demonstrates guarding optional fields with has() (for example: !has(oldSelf.value) || has(self.value)). [2]
If you’re on Kubernetes v1.29+, Kubernetes also supports CEL optional types, but has()-guarding remains the portable approach across versions. [1]
Sources
[1] Kubernetes docs: “Common Expression Language (CEL) in Kubernetes” https://kubernetes.io/docs/reference/using-api/cel/
[2] Kubernetes blog: “Enforce CRD Immutability with CEL Transition Rules” https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/
Guard namedCertificates before calling .exists().
servingCerts is now protected, but namedCertificates is optional in the schema. If servingCerts is present while namedCertificates is omitted, the rule will attempt to dereference an absent field. Kubernetes CEL requires guarding optional field access with has() before dereferencing. Add has(self.configuration.apiServer.servingCerts.namedCertificates) before calling .exists().
Suggested fix
- && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
- && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
+ && has(self.configuration.apiServer)
+ && has(self.configuration.apiServer.servingCerts)
+ && has(self.configuration.apiServer.servingCerts.namedCertificates)
+ && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| && has(self.configuration.apiServer) | |
| && has(self.configuration.apiServer.servingCerts) | |
| && has(self.configuration.apiServer.servingCerts.namedCertificates) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yaml`
around lines 6009 - 6010, The CEL expression dereferences an optional field:
before calling
self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, ...)
you must guard namedCertificates with
has(self.configuration.apiServer.servingCerts.namedCertificates); update the
condition that currently checks has(self.configuration.apiServer) &&
has(self.configuration.apiServer.servingCerts) to also include
has(self.configuration.apiServer.servingCerts.namedCertificates) so the call to
.exists() on namedCertificates is only evaluated when that field is present.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | ||
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | ||
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and get basic info
file_path="api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml"
if [ -f "$file_path" ]; then
echo "File found."
wc -l "$file_path"
else
echo "File not found. Searching for similar paths..."
find . -name "OpenStack.yaml" -type f 2>/dev/null | head -20
fiRepository: openshift/hypershift
Length of output: 197
🏁 Script executed:
# Read the specific lines mentioned in the review
file_path="api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml"
if [ -f "$file_path" ]; then
echo "=== Lines 6530-6550 context ==="
sed -n '6530,6550p' "$file_path"
fiRepository: openshift/hypershift
Length of output: 1939
🏁 Script executed:
# Search for the CEL rule pattern to understand the validation rule
rg -A 10 -B 5 "servicePublishingStrategy.loadBalancer.hostname" . --type yaml 2>/dev/null | head -50Repository: openshift/hypershift
Length of output: 10165
🏁 Script executed:
# Look for schema definitions related to servingCerts and namedCertificates
rg -A 5 "servingCerts" . --type yaml 2>/dev/null | grep -A 5 "namedCertificates" | head -40Repository: openshift/hypershift
Length of output: 6385
🏁 Script executed:
# Search for the source schema or struct definition for servingCerts
find . -name "*.go" -type f -exec grep -l "namedCertificates" {} \; 2>/dev/null | head -20Repository: openshift/hypershift
Length of output: 150
🏁 Script executed:
# Look for CRD source or schema files that define servingCerts structure
find . -name "*.go" -type f -exec grep -l "servingCerts" {} \; 2>/dev/null | head -20Repository: openshift/hypershift
Length of output: 150
🏁 Script executed:
# Search for apiserver servingcerts structure definition
rg "type.*ServingCert" . --type go 2>/dev/null | head -20Repository: openshift/hypershift
Length of output: 1660
🏁 Script executed:
# Read the APIServerServingCerts struct definition
cat -n ./api/vendor/github.com/openshift/api/config/v1/types_apiserver.go | grep -A 30 "type APIServerServingCerts struct"Repository: openshift/hypershift
Length of output: 2216
🏁 Script executed:
# Also check the APIServerNamedServingCert struct
cat -n ./api/vendor/github.com/openshift/api/config/v1/types_apiserver.go | grep -A 15 "type APIServerNamedServingCert struct"Repository: openshift/hypershift
Length of output: 1137
🌐 Web query:
Kubernetes CEL optional field access validation rules kubernetes.io
💡 Result:
To safely write Kubernetes CEL validation rules that touch optional (possibly absent) fields, use presence checks and/or CEL Optional types so the API server never evaluates a field access that isn’t valid.
1) Guard optional field access with has() + short-circuiting
Use the CEL has() macro to check presence, then rely on || / && short-circuiting so the field is only accessed when present. This pattern is shown in Kubernetes docs for validating optional parameters:
!has(params.optionalNumber) || (params.optionalNumber >= 5 && params.optionalNumber <= 10)
[1]
The Kubernetes CEL reference also calls out has() as the way to check accessibility before reading a (possibly missing) field (especially on dynamically-typed variables like object in admission policies). [2]
2) For CRD transition rules, compare presence across updates
When validating transitions with oldSelf, you can validate optional field “set/unset” transitions using:
has(self.field) == has(oldSelf.field)
(as documented in the CRD validation rules blog). [3]
3) If you need oldSelf during create / missing-old cases: optionalOldSelf
In CRD x-kubernetes-validations, setting optionalOldSelf: true makes oldSelf a CEL Optional so you can safely check and unwrap it:
oldSelf.hasValue()to test presenceoldSelf.value()to unwrap
Example from the CRD docs:
self.foo == "foo" || (oldSelf.hasValue() && oldSelf.value().foo != "foo")
[4]
4) Know the version gate for CEL Optional Values
Kubernetes documents that CEL Optional Values were introduced in Kubernetes 1.29, and only API servers 1.29+ accept new writes of expressions using those optional features. [2]
Sources
[1] Kubernetes docs – Validating Admission Policy (optional parameters + has() + short-circuit)
[2] Kubernetes docs – Common Expression Language (CEL) in Kubernetes (has() guidance; Optional Values availability)
[3] Kubernetes blog – CRD Validation Rules (transition rule example has(self.field) == has(oldSelf.field))
[4] Kubernetes docs – Extend the Kubernetes API with CRDs (optionalOldSelf, oldSelf.hasValue(), oldSelf.value() examples)
Guard the remaining optional fields in this CEL rule.
The validation still dereferences two optional fields without presence checks: servingCerts.namedCertificates and cert.names. Both are defined as +optional in the schema, so the rule can fail on valid specs like servingCerts: {} or named certificate entries without explicit names. Kubernetes CEL guidance requires guarding optional field access with has() before dereferencing. (kubernetes.io)
Suggested fix
- && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
- && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
- cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))'
+ && has(self.configuration.apiServer)
+ && has(self.configuration.apiServer.servingCerts)
+ && has(self.configuration.apiServer.servingCerts.namedCertificates)
+ && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
+ has(cert.names) && cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' | |
| && has(self.configuration.apiServer) | |
| && has(self.configuration.apiServer.servingCerts) | |
| && has(self.configuration.apiServer.servingCerts.namedCertificates) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| has(cert.names) && cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml`
around lines 6538 - 6540, The CEL rule dereferences two optional fields without
presence checks: self.configuration.apiServer.servingCerts.namedCertificates and
cert.names; update the expression used in the rule to guard those accesses with
has() checks (e.g., add
has(self.configuration.apiServer.servingCerts.namedCertificates) before
iterating namedCertificates and add has(cert.names) before calling
cert.names.exists) so the overall condition only evaluates those sub-expressions
when those optional fields are present.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | ||
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | ||
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Schema/source definitions for servingCerts and namedCertificates =="
rg -n -C3 'servingCerts:|namedCertificates:|json:".*namedCertificates,omitempty"|json:".*servingCerts,omitempty"' \
api/hypershift/v1beta1 \
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests
echo
echo "== All copies of the APIServer loadBalancer hostname validation =="
rg -n -C2 "APIServer loadBalancer hostname cannot be in ClusterConfiguration\\.apiserver\\.servingCerts\\.namedCertificates\\[\\]|namedCertificates\\.exists\\(cert" \
api \
cmd/installRepository: openshift/hypershift
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if namedCertificates is optional in the schema (check required fields) =="
# Find servingCerts definitions and check if namedCertificates is in required list
rg -A 10 "servingCerts:" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml | head -30
echo ""
echo "== Find the exact CEL validation rule for loadBalancer.hostname =="
rg -n "namedCertificates\.exists\(cert" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml
echo ""
echo "== Count total occurrences of this pattern in all generated manifests =="
rg "namedCertificates\.exists\(cert" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/ | wc -l
echo ""
echo "== List files with this pattern =="
rg -l "namedCertificates\.exists\(cert" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/Repository: openshift/hypershift
Length of output: 1572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full context around the CEL rule (lines 6695-6705) =="
sed -n '6695,6705p' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml
echo ""
echo "== Check for 'required' list in servingCerts section =="
# Look for the required fields list near servingCerts definition
rg -B 5 -A 30 "servingCerts:" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml | grep -A 20 "servingCerts:" | grep -E "(required:|namedCertificates)" | head -20Repository: openshift/hypershift
Length of output: 1424
🏁 Script executed:
#!/bin/bash
set -euo pipefannat
echo "== Check all 3 files for the same pattern == "
for file in \
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml \
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml \
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml; do
echo "=== File: $(basename $file) ==="
rg -B 3 "namedCertificates\.exists\(cert" "$file"
echo ""
done
echo "== Verify namedCertificates is NOT in required list =="
# Search the actual schema for required fields under servingCerts
rg -A 50 '"servingCerts"' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml | grep -A 40 'properties:' | head -50Repository: openshift/hypershift
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check all 3 files for the same pattern == "
for file in \
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml \
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml \
cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml; do
echo "=== File: $(basename $file) ==="
rg -B 3 "namedCertificates\.exists\(cert" "$file"
echo ""
done
echo "== Verify namedCertificates is NOT in required list =="
# Search the actual schema for required fields under servingCerts
rg -A 50 '"servingCerts"' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml | grep -A 40 'properties:' | head -50Repository: openshift/hypershift
Length of output: 1617
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the section defining servingCerts and check for required fields
echo "== Context: servingCerts schema definition with required list check =="
rg -n "servingCerts:" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml | head -1
# Get line number and read 100 lines from that point
LINE=$(rg -n "servingCerts:" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml | head -1 | cut -d: -f1)
START=$((LINE - 2))
END=$((LINE + 50))
echo "Lines $START to $END:"
sed -n "${START},${END}p" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml
echo ""
echo "== Check if namedCertificates field has minItems or is in required list =="
sed -n "${START},${END}p" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml | grep -E "required:|minItems:|namedCertificates"Repository: openshift/hypershift
Length of output: 4048
Guard namedCertificates before calling .exists().
Line 6699 still dereferences an optional field. servingCerts is now checked, but namedCertificates is also optional in the schema (not in any required list), so a spec like configuration.apiServer.servingCerts: {} remains valid and can still fail CEL evaluation.
Proposed fix
- && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
- && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
+ && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
+ && has(self.configuration.apiServer.servingCerts.namedCertificates)
+ && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,This same pattern appears in 3 manifest files and must be fixed in all of them:
hostedclusters-Hypershift-Default.crd.yamlhostedclusters-Hypershift-CustomNoUpgrade.crd.yamlhostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yaml`
around lines 6698 - 6700, The CEL expression dereferences an optional field:
guard self.configuration.apiServer.servingCerts.namedCertificates before calling
.exists(); update the predicate that currently uses
self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, ...) to
first check has(self.configuration.apiServer.servingCerts.namedCertificates) &&
then call .exists(...). Apply the same change to the identical expressions in
hostedclusters-Hypershift-Default.crd.yaml,
hostedclusters-Hypershift-CustomNoUpgrade.crd.yaml, and
hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml so all occurrences check
namedCertificates presence before .exists().
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | ||
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | ||
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get the exact content at lines 7675-7677
wc -l cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlRepository: openshift/hypershift
Length of output: 190
🏁 Script executed:
# Read the target lines with context
sed -n '7670,7680p' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlRepository: openshift/hypershift
Length of output: 1094
🏁 Script executed:
# Find the schema definitions for servingCerts, namedCertificates, and names fields
# Search for where these are defined in the CRD
rg -n "servingCerts|namedCertificates" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml | head -30Repository: openshift/hypershift
Length of output: 644
🏁 Script executed:
# Find the apiServer schema definition to check if servingCerts is optional
rg -n "apiServer:" cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml | head -20Repository: openshift/hypershift
Length of output: 130
🏁 Script executed:
# Get context around servingCerts definition to check required/optional status
sed -n '555,590p' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlRepository: openshift/hypershift
Length of output: 2574
🏁 Script executed:
# Search for namedCertificates item schema and names field definition
sed -n '560,620p' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlRepository: openshift/hypershift
Length of output: 4091
🏁 Script executed:
# Check the required fields at each level to confirm optionality
# Looking at servingCerts definition
sed -n '555,610p' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml | grep -A 5 "required:"Repository: openshift/hypershift
Length of output: 319
🏁 Script executed:
# Check if there's a required array for servingCerts or namedCertificates
sed -n '555,625p' cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml | cat -nRepository: openshift/hypershift
Length of output: 5045
🌐 Web query:
Kubernetes CRD CEL validation .exists() on optional list fields
💡 Result:
In CRD x-kubernetes-validations CEL, an optional list field may be absent, and calling list macros like .exists() on an absent field will fail evaluation. Guard the call with has() (field presence check) or short-circuit around it. (kubernetes.io)
Common patterns:
-
If the list is optional (skip validation when missing):
!has(self.spec.items) || self.spec.items.exists(x, x.name == "foo") -
If the list must be present and satisfy a condition:
has(self.spec.items) && self.spec.items.exists(x, x.name == "foo") -
If you also need “non-empty”:
has(self.spec.items) && size(self.spec.items) > 0 && self.spec.items.exists(x, x.name == "foo")
.exists() itself is the right macro for “any element matches” once you’ve ensured the list value is present. (kubernetes.io)
Citations:
- 1: https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy?utm_source=openai
- 2: https://kubernetes.io/docs/reference/using-api/cel?utm_source=openai
🌐 Web query:
Kubernetes CEL validation has() guard for optional array fields before exists()
💡 Result:
In Kubernetes CEL, guard an optional list/array field with has() and boolean short-circuiting before calling .exists(...):
!has(self.spec.myList) || self.spec.myList.exists(x, /* predicate using x */)
For nested optional parents, guard each level:
!has(self.spec) || !has(self.spec.myList) || self.spec.myList.exists(x, /* predicate */)
This works because Kubernetes CEL supports has() for field presence checks, and || short-circuits (so .exists() won’t be evaluated when the field is absent). It also supports list macros like .exists(...) on list fields. [1][2]
Sources:
[1] https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/
[2] https://kubernetes.io/docs/reference/using-api/cel/
Guard optional list fields before calling .exists() to prevent CEL validation failures.
The rule at lines 7675-7677 guards servingCerts presence but omits guards for two optional list fields: namedCertificates and names. Since both are optional per the schema, a spec with an absent namedCertificates list or a namedCertificates item with an absent names field will cause CEL evaluation to fail, rejecting otherwise valid HostedCluster specs. Add has() checks before each .exists() call on these optional lists.
Suggested patch
- && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
- && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
- cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))'
+ && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts)
+ && has(self.configuration.apiServer.servingCerts.namedCertificates)
+ && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert,
+ has(cert.names) && cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' | |
| && has(self.configuration.apiServer) && has(self.configuration.apiServer.servingCerts) | |
| && has(self.configuration.apiServer.servingCerts.namedCertificates) | |
| && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, | |
| has(cert.names) && cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yaml`
around lines 7675 - 7677, The CEL validation currently checks
self.configuration.apiServer.servingCerts but calls .exists() on two optional
lists without guarding them: servingCerts.namedCertificates.exists(...) and
cert.names.exists(...); update the rule to first check
has(self.configuration.apiServer.servingCerts.namedCertificates) before calling
namedCertificates.exists(...) and has(cert.names) before calling
cert.names.exists(...) so both optional lists are guarded and CEL evaluation
won't fail when those lists are absent.
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
lifecycle/stale
|
PR needs rebase. 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. |
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
The analysis is complete. The root cause is clear from all evidence. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryNo CI test jobs ran or failed. The sole reported failure is from tide — the Prow merge controller — which refuses to merge the PR because it has a git merge conflict with the current Root CauseThis is not a test failure — it is a merge conflict preventing tide from processing the PR. The PR modifies GitHub reports The Prow bot has automatically applied the
Recommendations
Evidence
|
3 participants
What this PR does / why we need it:
PR #6091 introduced a validation rule that checks if the APIServer
loadBalancer hostname conflicts with namedCertificates. However, the
validation directly accesses configuration.apiServer.servingCerts.namedCertificates
without checking if the optional servingCerts field exists.
This causes CEL validation failures when patching HostedCluster resources
that don't have the servingCerts field populated.
Add has(self.configuration.apiServer.servingCerts) check before
accessing namedCertificates field to prevent validation errors on resources without this optional field.
Which issue(s) this PR fixes:
OCPBUGS-77827 - Validation of hostedcluster CRD servingCerts causes a CEL rule failure when patched
Special notes for your reviewer:
Checklist:
Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.
Summary by CodeRabbit
Bug Fixes
Refactor