OCPBUGS-66211: fix(vendor): bump openshift/api dependency to current master - #7345
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds Makefile cleanup steps for SelfManagedHA CRD YAMLs, updates Go module dependencies, renames a feature gate, tightens OIDC/issuer validations and required fields, and extends many HostedCluster/HostedControlPlane CRD manifests with AWS networkLoadBalancer.subnets and eipAllocations plus cross-field and scope validations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus during review:
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
openshift-ci-robot
added
jira/valid-reference
|
@csrwng: This pull request references Jira Issue OCPBUGS-66211, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (yli2@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng 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
approved
|
/test verify |
|
/assign @jparrill |
|
Unit and Verify are failing, once solved just let me know for tagging @csrwng :) |
fe95ca5 to
08ab4ed
Compare
|
/test verify |
openshift-ci
Bot
added
area/platform/aws
|
@csrwng: 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. |
|
/lgtm |
|
/jira refresh |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-66211, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/label priority/ci-critical |
|
@csrwng: The label(s) 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. |
|
/lgtm |
|
/priority ci-critical |
|
@muraee: The label(s) 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
6471-6477: Typo inacceptedRisksdescription (“menition”)Minor doc issue: “menition” should be “mention”.
- acceptedRisks records risks which were accepted to initiate the update. - For example, it may menition an Upgradeable=False or missing signature + acceptedRisks records risks which were accepted to initiate the update. + For example, it may mention an Upgradeable=False or missing signatureapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml (1)
6548-6554: Minor typo inacceptedRisksdescription“menition” on Line 6552 should be “mention” in user‑facing status text:
- For example, it may menition an Upgradeable=False or missing signature + For example, it may mention an Upgradeable=False or missing signature
♻️ Duplicate comments (29)
api/hypershift/v1beta1/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml (1)
26-26: Feature gate rename looks correct.The rename from
DynamicResourceAllocationtoHyperShiftOnlyDynamicResourceAllocationfollows the HyperShift-specific naming convention. This change must be accompanied by updates to the Go code and testdata files as flagged in the previous review comment.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yaml (1)
3095-3230: HardeneipAllocationsvalidation and require explicit subnets when EIPs are setTwo issues around the new AWS NLB
eipAllocationswiring:
The item-level CEL validation risks runtime errors:
self.split("-", 2)[1]will fail if the value lacks a-(still possible given only length constraints).- This can surface as a CEL evaluation error instead of a clean validation failure.
eipAllocationscan be set whilesubnetsis omitted (auto-discovery), making the “must match number of subnets used” requirement unenforceable and likely unsupported.Recommend:
- Replace the two
startsWith/splitrules with a single anchored regex that fully describes the EIP allocation ID, avoiding indexing:- x-kubernetes-validations: - - message: eipAllocations should start - with 'eipalloc-' - rule: self.startsWith('eipalloc-') - - message: eipAllocations must be 'eipalloc-' - followed by exactly 17 hexadecimal - characters (0-9, a-f, A-F) - rule: self.split("-", 2)[1].matches('[0-9a-fA-F]{17}$') + x-kubernetes-validations: + - message: eipAllocations must be of the form 'eipalloc-' followed by exactly 17 hexadecimal characters (0-9, a-f, A-F) + rule: self.matches('^eipalloc-[0-9a-fA-F]{17}$')
- Add a guard at the
networkLoadBalancerlevel so EIPs cannot be configured without explicit subnets (rely on the existing per-subnet “at least one id or name” rule to enforce non-emptiness):type: object - x-kubernetes-validations: - - message: number of subnets must be equal + x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || has(self.subnets)' + - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true'The existing
scope-based prohibition foreipAllocationswhenscopeisInternallooks correct and can remain as-is.Also applies to: 3378-3383
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml (1)
3094-3233: Harden eipAllocations CEL validation to avoid split() index errors.The item-level rule:
- message: eipAllocations must be 'eipalloc-' followed by exactly 17 hexadecimal characters (0-9, a-f, A-F) rule: self.split("-", 2)[1].matches('[0-9a-fA-F]{17}$')can throw a CEL index‑out‑of‑bounds error if the value is 26 chars long but contains no
-(the priorstartsWith('eipalloc-')rule will fail, but this rule is still evaluated). That yields a generic admission error instead of the intended validation message.You can express the same constraint more safely with a single anchored regex over the whole string and drop the
split():x-kubernetes-validations: - - message: eipAllocations should start - with 'eipalloc-' - rule: self.startsWith('eipalloc-') - - message: eipAllocations must be 'eipalloc-' - followed by exactly 17 hexadecimal - characters (0-9, a-f, A-F) - rule: self.split("-", 2)[1].matches('[0-9a-fA-F]{17}$') + - message: eipAllocations must match ^eipalloc-[0-9a-fA-F]{17}$ + rule: self.matches('^eipalloc-[0-9a-fA-F]{17}$')The surrounding cross-field checks (subnets vs eipAllocations counts) and the
scope != Internalconstraint for AWS NLBs look correct.Also applies to: 3378-3383
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
3247-3381: Tighten NLBeipAllocations/subnetscontract and simplify EIP validationTwo points here:
eipAllocationsallowed withoutsubnets(ambiguous config)
The docs say the number of EIPs must match the number of subnets, but nothing currently prevents specifyingeipAllocationswith nosubnetsat all. That leaves behavior ambiguous (index-based mapping to auto-discovered subnets) and diverges from the documented contract.Consider adding a presence constraint at the
networkLoadBalancerlevel so that non-emptyeipAllocationsrequiresubnets:networkLoadBalancer: ... - type: object
x-kubernetes-validations:
type: objectx-kubernetes-validations:- message: subnets must be specified when eipAllocations are setrule: '!has(self.eipAllocations) || self.eipAllocations.size() == 0 || has(self.subnets)' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids)(Adjust exact insertion point as needed to keep this alongside the existing parity rules.)
EIP item CEL rule can be made safer and clearer
The second validation on individualeipAllocationsusesself.split("-", 2)[1]which will error for malformed values without'-', yielding a lower-signal CEL error. You can express the full constraint with a single regex instead:items: description: |- EIPAllocation is an ID for an Elastic IP (EIP) address ...
maxLength: 26minLength: 26type: stringx-kubernetes-validations:- message: eipAllocations should startwith 'eipalloc-'rule: self.startsWith('eipalloc-')- message: eipAllocations must be 'eipalloc-'followed by exactly 17 hexadecimalcharacters (0-9, a-f, A-F)rule: self.split("-", 2)[1].matches('[0-9a-fA-F]{17}$')
type: stringx-kubernetes-validations:- message: eipAllocations must be 'eipalloc-' followed by exactly 17 hexadecimal characters (0-9, a-f, A-F)rule: self.matches('^eipalloc-[0-9a-fA-F]{17}$')(You can keep or drop the explicit `minLength`/`maxLength` as preferred; the pattern already enforces length.)These changes align the schema more closely with the documented semantics and avoid brittle CEL expressions.
Also applies to: 3530-3535
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml (1)
3195-3330: Consider requiring explicit subnets whenevereipAllocationsis setRight now
eipAllocationscan be non-empty whilesubnetsis omitted (auto-discovery). In that case, the “EIP count must match subnet count” guarantee can’t be enforced at admission and misconfigurations will only surface at reconcile/provisioning time.If the intent is to only support EIPs when the user pins subnets explicitly, consider adding an extra validation at the
networkLoadBalancerobject level such as:networkLoadBalancer: type: object - x-kubernetes-validations: + x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || has(self.subnets)' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true' ...If auto-discovered subnets + EIPs is a supported configuration and the controller validates parity itself, this can be left as-is.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml (2)
3103-3238: CEL validation bug:split("-", 2)oneipAllocationswill break CRD validation.The item-level rule:
rule: self.split("-", 2)[1].matches('[0-9a-fA-F]{17}$')uses a 2-argument
splitin CEL, which is not supported in Kubernetes CRD validation and will cause the CRD’s CEL expressions to fail to compile. This can prevent applying or updating the CRD.You already have length and prefix constraints; replace the suffix check with
substringto avoidsplitentirely:- - message: eipAllocations must be 'eipalloc-' - followed by exactly 17 hexadecimal - characters (0-9, a-f, A-F) - rule: self.split("-", 2)[1].matches('[0-9a-fA-F]{17}$') + - message: eipAllocations must be 'eipalloc-' + followed by exactly 17 hexadecimal + characters (0-9, a-f, A-F) + rule: self.substring(9).matches('^[0-9a-fA-F]{17}$')This keeps the invariant “
eipalloc-+ 17 hex chars” while using CEL operations that are known to work in CRD validations.
6414-6419: User-facing typo inacceptedRisksdescription (“menition” → “mention”).The description still contains
menitioninstead ofmention; this shows up in user documentation.- acceptedRisks records risks which were accepted to initiate the update. - For example, it may menition an Upgradeable=False or missing signature + acceptedRisks records risks which were accepted to initiate the update. + For example, it may mention an Upgradeable=False or missing signatureapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
1111-1133: Useurl(self).getQuery().size() == 0instead of== {}for CEL map emptiness.
url(self).getQuery()returns a map; comparing directly to{}is less robust than checking.size() == 0and can behave inconsistently across CEL/Kubernetes versions. Using.size() == 0is the safer and more idiomatic way to assert “no query parameters”.Prefer updating the generator/template that emits these validations so all similar URL rules use
.getQuery().size() == 0, then regenerate this file.- - message: must not have a query - rule: isURL(self) && url(self).getQuery() == {} + - message: must not have a query + rule: isURL(self) && url(self).getQuery().size() == 0#!/bin/bash set -euo pipefail # Find remaining CEL validations that still compare url(self).getQuery() directly to {} rg -n "url\(self\)\.getQuery\(\) == {}" --type yamlapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml (2)
3142-3275: Tighten NLB EIP validations: require subnets when usingeipAllocationsand avoid fragilesplit()usageTwo points here:
eipAllocationsallowed without anysubnets
All three “counts must be equal” rules short-circuit totruewhenself.subnetsis absent, so a user can seteipAllocationsalone and still pass validation. If the ingress/operator logic assumes explicit subnets whenever EIPs are specified (common for AWS NLB SubnetMapping with EIPs), this becomes a runtime failure instead of a CRD validation failure. Consider adding an explicit guard that any presence ofeipAllocationsrequires at least one subnet ID or name.Fragile CEL expression using
split("-", 2)[1]
The per-item ruleself.split("-", 2)[1].matches('[0-9a-fA-F]{17}$')can raise CEL evaluation errors for malformed values that don’t contain-, leading to less clear admission errors. A single regex over the full string is safer and clearer.Suggested change for the item validation:
- x-kubernetes-validations: - - message: eipAllocations should start - with 'eipalloc-' - rule: self.startsWith('eipalloc-') - - message: eipAllocations must be 'eipalloc-' - followed by exactly 17 hexadecimal - characters (0-9, a-f, A-F) - rule: self.split("-", 2)[1].matches('[0-9a-fA-F]{17}$') + x-kubernetes-validations: + - message: eipAllocations must match ^eipalloc-[0-9a-fA-F]{17}$ + rule: self.matches('^eipalloc-[0-9a-fA-F]{17}$')And an additional NLB-level validation to force subnets when EIPs are set:
- x-kubernetes-validations: + x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || (has(self.subnets) && ((has(self.subnets.ids) && self.subnets.ids.size() > 0) || (has(self.subnets.names) && self.subnets.names.size() > 0)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids)The new load-balancer–scope-level guard forbidding
eipAllocationswhenscopeisInternalis correct and matches the documented restriction.Also applies to: 3425-3430
6444-6447: Typo inacceptedRisksdescription (“menition” → “mention”)The user-facing text still says “it may menition an Upgradeable=False…”. Please correct to “may mention” in this description block.
- For example, it may menition an Upgradeable=False or missing signature + For example, it may mention an Upgradeable=False or missing signatureapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
3177-3265: Require explicitsubnetswheneipAllocationsis set for AWS NLBsRight now
networkLoadBalancer.eipAllocationscan be provided whilenetworkLoadBalancer.subnetsis omitted. All three parity validations short‑circuit whenhas(self.subnets)is false, so such configurations pass schema validation even though the description states that the number of EIPs must match the subnets used.This can lead to misconfigured NLBs that are accepted by the API but cannot be reconciled as intended. Add a CEL guard at the
networkLoadBalancerlevel to require at least one subnet ID or name whenevereipAllocationsis set.Suggested patch (schema context shortened for clarity):
networkLoadBalancer: description: |- networkLoadBalancerParameters holds configuration parameters for an AWS network load balancer. Present only if type is NLB. properties: eipAllocations: ... subnets: ... type: object - x-kubernetes-validations: + x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || (has(self.subnets) && ((has(self.subnets.ids) && self.subnets.ids.size() > 0) || (has(self.subnets.names) && self.subnets.names.size() > 0)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && !has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids) == size(self.eipAllocations) : true' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.names) && !has(self.subnets.ids) && has(self.eipAllocations) ? size(self.subnets.names) == size(self.eipAllocations) : true'This keeps auto‑discovery behavior intact for the common case (no EIPs, no explicit subnets), while enforcing the stronger invariant only when EIPs are requested.
Also applies to: 3267-3405, 3554-3559
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
3170-3305: Potential validation gap:eipAllocationsallowed without explicitsubnets
networkLoadBalancer.eipAllocationscan be set whilenetworkLoadBalancer.subnetsis omitted (auto-discovery). All three count‑equality rules are guarded withhas(self.subnets), so they don’t fire in that case. This contradicts the description (“number of EIP allocations must match the number of subnets that are used for the load balancer”) and is likely to fail later at reconciliation/provisioning time if the discovered subnet count doesn’t match the EIP list.Consider adding a top‑level validation on
networkLoadBalancerthat requires explicit subnets (with at least one id or name) whenevereipAllocationsis provided, e.g.:networkLoadBalancer: properties: eipAllocations: ... subnets: ... type: object x-kubernetes-validations: + - message: subnets must be specified when eipAllocations are specified + rule: '!has(self.eipAllocations) || (has(self.subnets) && ((has(self.subnets.ids) && self.subnets.ids.size() > 0) || (has(self.subnets.names) && self.subnets.names.size() > 0)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true' ...This keeps auto‑discovery behavior for the common case (no EIPs), but rejects misconfigured “EIPs without explicit subnets” combinations up front. If the underlying Ingress operator does intentionally support EIPs with subnet auto‑discovery, you can ignore this suggestion.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (2)
3241-3376: Consider requiring explicitsubnetswheneipAllocationsis setRight now
eipAllocationscan be set withoutsubnets, so all the “subnets == eipAllocations” validations are skipped (thehas(self.subnets)guards short‑circuit totrue). That means a user can configure EIPs while relying on auto-discovered subnets, and the schema cannot enforce the documented constraint “The number of EIP allocations must match the number of subnets that are used for the load balancer.”If the intent is to make that constraint meaningful at admission time (and avoid hard‑to‑diagnose runtime failures), consider adding a CEL guard on the same
networkLoadBalancerobject to force explicit subnets whenevereipAllocationsis present, e.g.:x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || (has(self.subnets) && ((has(self.subnets.ids) && self.subnets.ids.size() > 0) || (has(self.subnets.names) && self.subnets.names.size() > 0)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true'(Same idea applies to the other two parity rules; they’re fine once
subnetsis guaranteed present and nonempty.)
6580-6584: Minor docstring typo and clarity inacceptedRisksThere’s still a small typo and slightly ambiguous phrasing in the
acceptedRisksdescription:- acceptedRisks records risks which were accepted to initiate the update. - For example, it may menition an Upgradeable=False or missing signature - that was overridden via desiredUpdate.force, or an update that was + acceptedRisks records risks which were accepted to initiate the update. + For example, it may mention an Upgradeable=False condition or a missing + signature that was overridden via desiredUpdate.force, or an update that wasOptional, but this would read more cleanly in docs.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (2)
3214-3350: AWS NLBeipAllocations/subnets/scope validations are mostly solid; consider tightening EIP–subnet coupling
- Per‑item validation on
eipAllocationsis now safe (prefix check + fixed length makesplit("-", 2)[1]non‑erroneous) and enforces the intendedeipalloc-<17 hex>format.- Cross‑field rules correctly ensure the number of explicit subnets (ids+names) equals the number of
eipAllocationswhen both are present, and the new load balancer–scope rule cleanly forbidseipAllocationsforscope: Internal.One behavioral gap remains: it’s still possible to set
eipAllocationswithout explicitly specifyingsubnets, despite the description requiring the counts to match. That leaves mismatches to be caught only at reconciliation time.If you want this invariant enforced at the CRD layer, you can add an extra validation on the NLB block so that EIPs are only allowed when subnets (ids or names) are specified:
- type: object - x-kubernetes-validations: + type: object + x-kubernetes-validations: + - message: eipAllocations requires explicitly specified subnets + rule: '!has(self.eipAllocations) || (has(self.subnets) && (has(self.subnets.ids) || has(self.subnets.names)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true'
6553-6557: Fix typo in user‑facingacceptedRisksdescriptionLine 6555 has a small typo:
"may menition"→"may mention". Suggest:- For example, it may menition an Upgradeable=False or missing signature + For example, it may mention an Upgradeable=False or missing signatureapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml (2)
3213-3347: NLB EIP/subnet constraints look solid; consider also requiring subnets when EIPs are setThe new schema does a good job of:
- Validating EIP IDs (
eipalloc-+ 17 hex chars, max 10, unique).- Validating subnet IDs/names and enforcing uniqueness and a combined limit of 10.
- Enforcing 1:1 parity between
eipAllocationsand subnets for all combinations of ids/names.- Forbidding
eipAllocationswhenscopeisInternal.One remaining gap is that the CRD still allows
eipAllocationsto be set whilesubnetsis completely omitted (auto-discovery case). If the controller truly supports “EIPs + auto-discovered subnets” and validates parity at runtime, this is fine. If instead you expect users to always provide an explicit subnet list when pinning EIPs, you could tighten the schema with an additional validation:networkLoadBalancer: type: object - x-kubernetes-validations: + x-kubernetes-validations: + - message: subnets must be specified when eipAllocations are set + rule: '!has(self.eipAllocations) || has(self.subnets)' - message: number of subnets must be equal to number of eipAllocationsTreating this as optional, depending on the intended UX for EIP + auto-discovery.
Also applies to: 3496-3501
6583-6588: Fix lingering typo inacceptedRisksdescription (“menition” → “mention”)User-facing description still says “menition”; suggest correcting to “mention”:
- For example, it may menition an Upgradeable=False or missing signature + For example, it may mention an Upgradeable=False or missing signatureapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml (2)
3368-3741: AWS NLBeipAllocationsstill allowed withoutsubnets—add validation to require subnets when EIPs are setEven after adding count‑equality checks, a user can still specify
providerParameters.aws.networkLoadBalancer.eipAllocationswith nonetworkLoadBalancer.subnets. That contradicts the doc (“number of EIP allocations must match the number of subnets that are used for the load balancer”) and is unlikely to be supported by the NLB implementation, since each EIP must map to a specific subnet.Recommend adding a
x-kubernetes-validationsrule undernetworkLoadBalancerto require at least one subnet ID or name whenevereipAllocationsis present, e.g.:x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || (has(self.subnets) && ((has(self.subnets.ids) && self.subnets.ids.size() > 0) || (has(self.subnets.names) && self.subnets.names.size() > 0)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids)This keeps the existing count‑equality checks but prevents unusable configurations with EIPs and no subnets.
6923-6929: Fix minor typo inacceptedRisksdescription (“menition” → “mention”)User‑facing text under
status.versionStatus.history[*].acceptedRisksstill says “menition”; suggest correcting spelling:- For example, it may menition an Upgradeable=False or missing signature + For example, it may mention an Upgradeable=False or missing signatureapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
3362-3375: EIP allocation validation: avoidsplit(...)[1]index; use a single regex on the full stringUsing
self.split("-", 2)[1]risks CEL index-out-of-range evaluation errors whenever a value is missing a-(or otherwise malformed). That turns bad input into an evaluation error instead of a cleanfalse, and is more brittle than necessary. A single regex against the whole value is simpler and safer, while still enforcing the exact same constraint.Consider tightening the rule like this:
- items: - description: |- - EIPAllocation is an ID for an Elastic IP (EIP) address that can be allocated to an ELB in the AWS environment. - Values must begin with `eipalloc-` followed by exactly 17 hexadecimal (`[0-9a-fA-F]`) characters. - maxLength: 26 - minLength: 26 - type: string - x-kubernetes-validations: - - message: eipAllocations should start - with 'eipalloc-' - rule: self.startsWith('eipalloc-') - - message: eipAllocations must be 'eipalloc-' - followed by exactly 17 hexadecimal - characters (0-9, a-f, A-F) - rule: self.split("-", 2)[1].matches('[0-9a-fA-F]{17}$') + items: + description: |- + EIPAllocation is an ID for an Elastic IP (EIP) address that can be allocated to an ELB in the AWS environment. + Values must match ^eipalloc-[0-9a-fA-F]{17}$. + maxLength: 26 + minLength: 26 + type: string + x-kubernetes-validations: + - message: eipAllocations must match ^eipalloc-[0-9a-fA-F]{17}$ + rule: self.matches('^eipalloc-[0-9a-fA-F]{17}$')This keeps the length invariant but expresses prefix and hex-suffix in one place without any indexing.
To check for similar patterns elsewhere in the repo:
#!/bin/bash # Find other EIP-related CEL rules using split() that may need the same fix rg -n "eipalloc-|EIPAllocation" -C3api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (2)
1158-1174: Fix issuerURL CEL: replacefind()withmatches()/contains()The
issuerURLvalidations still useself.find('#(.+)$')andself.find('@'), which are not part of the Kubernetes CEL environment and will cause the CRD to be rejected when applied. Use supported string predicates instead (and a safer query check):issuerURL: @@ - - message: must not have a query - rule: isURL(self) && url(self).getQuery() == {} - - message: must not have a fragment - rule: self.find('#(.+)$') == '' - - message: must not have user info - rule: self.find('@') == '' + - message: must not have a query + rule: isURL(self) && url(self).getQuery().size() == 0 + - message: must not have a fragment + rule: !self.matches('.*#.+$') + - message: must not have user info + rule: !self.contains('@')
3702-3840: Require explicit subnets wheneipAllocationsis set on AWS NLBYou added good parity checks between
subnetsandeipAllocations, but they only run whensubnetsis explicitly specified. A user can still seteipAllocationswith nosubnets, which passes CRD validation even though the description says “The number of EIP allocations must match the number of subnets that are used for the load balancer.”Add a guard so that whenever
eipAllocationsis set,subnets(ids or names) must also be explicitly non-empty:networkLoadBalancer: @@ - type: object - x-kubernetes-validations: + type: object + x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || (has(self.subnets) && ((has(self.subnets.ids) && self.subnets.ids.size() > 0) || (has(self.subnets.names) && self.subnets.names.size() > 0)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true' @@ - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.names) && !has(self.subnets.ids) && has(self.eipAllocations) ? size(self.subnets.names) == size(self.eipAllocations) : true'Consider mirroring this rule in any other HostedCluster CRD variants that expose the same
networkLoadBalancerschema.Also applies to: 3989-3994
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml (1)
3200-3338: Still a validation gap:eipAllocationsallowed without explicitsubnetsAs written,
x-kubernetes-validationsonly enforce count equality whenself.subnetsis present. If a user setseipAllocationsbut omitssubnets(auto-discovery), all three rules short‑circuit totrue, so the CRD admits configurations where the statement “the number of EIP allocations must match the number of subnets that are used for the load balancer” cannot be enforced at admission time. That likely pushes failures to reconciliation/provisioning.Consider adding a guard that forces explicit subnets (with at least one id or name) whenever
eipAllocationsis set, for example:- x-kubernetes-validations: + x-kubernetes-validations: + - message: eipAllocations requires subnets to be set with ids or names + rule: '!has(self.eipAllocations) || (has(self.subnets) && ((has(self.subnets.ids) && self.subnets.ids.size() > 0) || (has(self.subnets.names) && self.subnets.names.size() > 0)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && !has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids) == size(self.eipAllocations) : true' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.names) && !has(self.subnets.ids) && has(self.eipAllocations) ? size(self.subnets.names) == size(self.eipAllocations) : true'This keeps the existing equality checks but prevents
eipAllocationswith auto‑discovered subnets, which cannot be validated here.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml (1)
3468-3545: CEL: list concatenation viaself.ids + self.names/self.subnets.ids + self.subnets.namesis invalid and will break CRD validationKubernetes CEL does not support
+on lists, so rules likesize(self.ids + self.names)andsize(self.subnets.ids + self.subnets.names)will fail to type‑check. That makes the CRD (and any update touching these validations) invalid and effectively blocks admission. This was already flagged in a previous review and is still present here in:
aws.classicLoadBalancer.subnets.x-kubernetes-validations[0].ruleaws.networkLoadBalancer.subnets.x-kubernetes-validations[0].ruleaws.networkLoadBalancer.x-kubernetes-validations[0].rule(EIP parity with subnets)Use the sum of list sizes instead of list concatenation.
Suggested patch:
@@ - x-kubernetes-validations: - - message: the total number of subnets - cannot exceed 10 - rule: 'has(self.ids) && has(self.names) - ? size(self.ids + self.names) <= 10 - : true' + x-kubernetes-validations: + - message: the total number of subnets + cannot exceed 10 + rule: 'has(self.ids) && has(self.names) + ? (size(self.ids) + size(self.names)) + <= 10 : true' @@ - x-kubernetes-validations: - - message: the total number of subnets - cannot exceed 10 - rule: 'has(self.ids) && has(self.names) - ? size(self.ids + self.names) <= 10 - : true' + x-kubernetes-validations: + - message: the total number of subnets + cannot exceed 10 + rule: 'has(self.ids) && has(self.names) + ? (size(self.ids) + size(self.names)) + <= 10 : true' @@ - - message: number of subnets must be equal - to number of eipAllocations - rule: 'has(self.subnets) && has(self.subnets.ids) - && has(self.subnets.names) && has(self.eipAllocations) - ? size(self.subnets.ids + self.subnets.names) - == size(self.eipAllocations) : true' + - message: number of subnets must be equal + to number of eipAllocations + rule: 'has(self.subnets) && has(self.subnets.ids) + && has(self.subnets.names) && has(self.eipAllocations) + ? (size(self.subnets.ids) + size(self.subnets.names)) + == size(self.eipAllocations) : true'To double‑check after applying this, you can run
kubectl apply -f <crd.yaml>(or the equivalent in CI) against a recent Kubernetes/OpenShift API server and confirm the CRD is accepted with no CEL compile errors.Also applies to: 3590-3686
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml (1)
3112-3247: Consider requiring explicit subnets wheneipAllocationsis setThe parity checks between
subnetsandeipAllocationswork whensubnetsis present, but the CRD still allowseipAllocationsto be set with no explicitsubnets, while the description says “number of EIP allocations must match the number of subnets used”. With auto-discovered subnets, that constraint can only be enforced at runtime and may lead to confusing failures.If the intent is to only support EIPs when subnets are explicitly configured, a small CEL guard at
networkLoadBalancerlevel would tighten the spec:networkLoadBalancer: type: object x-kubernetes-validations: + - message: eipAllocations requires explicit subnets configuration + rule: '!has(self.eipAllocations) || has(self.subnets)' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true'If auto-discovery + EIPs is intentionally supported and validated elsewhere, then the current schema is acceptable.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (2)
6415-6421: Fix typo inacceptedRisksdescription (“menition” → “mention”).User‑facing text still contains the typo previously flagged; suggest correcting it:
- For example, it may menition an Upgradeable=False or missing signature + For example, it may mention an Upgradeable=False or missing signature
3031-3250: Still missing: require explicit subnets when configuring NLBeipAllocations.The new schema and count-parity validations for Classic/NLB subnets and
eipAllocationslook good, buteipAllocationscan still be set without anysubnetsblock at all. In that case, all three parity rules short‑circuit totrue, so invalid configs like “EIPs set, relying on auto‑discovered subnets” are admitted, which can later fail during reconciliation/provisioning. This is the same gap called out in the earlier review on this file.Add a
networkLoadBalancer‑level x‑validation to require at least one subnet id or name whenevereipAllocationsis present, e.g.:type: object - x-kubernetes-validations: + x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || (has(self.subnets) && ((has(self.subnets.ids) && self.subnets.ids.size() > 0) || (has(self.subnets.names) && self.subnets.names.size() > 0)))' - message: number of subnets must be equal to number of eipAllocations rule: 'has(self.subnets) && has(self.subnets.ids) && has(self.subnets.names) && has(self.eipAllocations) ? size(self.subnets.ids + self.subnets.names) == size(self.eipAllocations) : true'api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml (1)
3191-3330: NLBeipAllocations/subnetsparity validations look good; consider also requiringsubnetswheneipAllocationsis setThe new schema and CEL validations for AWS NLB:
- Enforce EIP ID shape and length.
- Enforce uniqueness in
eipAllocations,ids, andnames.- Ensure total
ids + names≤ 10 and at least one subnet whensubnetsis present.- Enforce 1:1 parity between subnets and
eipAllocationsfor the three cases (ids+names, ids only, names only).One behavioral detail: configs with
eipAllocationsbut no explicitsubnets(i.e., relying on auto-discovered subnets) still pass, because all parity rules guard onhas(self.subnets). If the intent is that EIP mapping is only supported when subnets are explicitly specified (and not with auto-discovery), you may want an additional guard at thenetworkLoadBalancerlevel:- x-kubernetes-validations: + x-kubernetes-validations: + - message: subnets must be specified when eipAllocations is set + rule: '!has(self.eipAllocations) || has(self.subnets)' - message: number of subnets must be equal to number of eipAllocationsIf auto-discovery +
eipAllocationsis an intentional and supported combination, then the current shape is fine; in that case, no change is required, but it might be worth double-checking controller behavior against this schema.
🧹 Nitpick comments (1)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
3385-3481: NLBsubnets/eipAllocationsparity and scope checks mostly align with the docs, but allow EIPs without explicit subnetsThe new rules correctly enforce:
- duplicate-free ids/names with combined
<= 10subnets,len(subnets) == len(eipAllocations)in all combinations of ids/names when both are present, and- forbidding
eipAllocationswhenscope == Internal.However, the schema currently permits
eipAllocationsto be set while omittingsubnetsentirely; that relies on controller-side logic to reconcile EIP count with auto-discovered subnets, even though the description states “The number of EIP allocations must match the number of subnets that are used for the load balancer.”If you intend to require explicit subnets whenever
eipAllocationsis set, consider an additional guard such as:x-kubernetes-validations: - message: eipAllocations require subnets to be explicitly specified rule: '!has(self.eipAllocations) || has(self.subnets)'Also applies to: 3630-3635
| description: AWSSubnetName is a | ||
| reference to an AWS subnet name. | ||
| maxLength: 256 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: subnet name cannot contain | ||
| a comma | ||
| rule: '!self.contains('','')' | ||
| - message: subnet name cannot start | ||
| with 'subnet-' | ||
| rule: '!self.startsWith(''subnet-'')' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the relevant sections
if [ -f "api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml" ]; then
echo "=== Lines 3157-3168 ==="
sed -n '3157,3168p' "api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml" | cat -n
echo ""
echo "=== Lines 3278-3289 ==="
sed -n '3278,3289p' "api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml" | cat -n
else
echo "File not found - searching for similar generated files..."
find . -name "*OpenStack.yaml" -type f 2>/dev/null | head -5
fiRepository: openshift/hypershift
Length of output: 287
🏁 Script executed:
# Search for the specific CEL rule pattern to find all occurrences
echo "=== Searching for contains('','') pattern ==="
rg "contains\('',''\)" --type yaml -A 1 -B 1Repository: openshift/hypershift
Length of output: 287
🌐 Web query:
CEL (Common Expression Language) string contains method syntax validation
💡 Result:
Short answer: CEL uses a string instance method syntax x.contains(y) (and also x.startsWith(y), x.endsWith(y)); you can also use the infix operator "in" for membership tests on lists. The contains form expects x and y to be strings (returns a boolean) and matches when y is a substring of x. Example: "hello world".contains("world") == true. [1][2][3]
Notes for validation:
- Use the instance-method form: .contains(). [2][3]
- Both operands must be string-typed (or cast to string) for substring checks. [2][6]
- For list membership use: item in list. [1][3]
Sources:
[1] Kubernetes CEL overview and examples (string/CIDR libraries).
[2] Google Cloud CEL matcher language reference (x.contains, startsWith, endsWith).
[3] CEL examples showing "one-more-group".contains("more").
🌐 Web query:
CEL Kubernetes validation rules string.contains comma example
💡 Result:
Example: use the string contains method in a CEL validation expression to check for a comma.
ValidatingAdmissionPolicy / CRD validation rule example (check that spec.tags contains a comma):
-
Rule (CEL): self.spec.tags.contains(",")
-
As a CRD x-kubernetes-validations example:
rule: |
self.spec.tags.contains(",")
message: 'spec.tags must include a comma-separated list.'
Or as a ValidatingAdmissionPolicy validation expression:
spec:
validations:
- expression: 'object.spec.tags.contains(",")'
messageExpression: '"spec.tags must contain a comma."'
Notes:
- Kubernetes CEL supports string methods such as contains; CEL regex helpers (find/findAll/matches) are also available if you need more complex checks. (kubernetes.io)
If you want, I can show a full CRD snippet or a regex-based alternative (e.g., to require at least two items).
Citations:
Fix CEL rule for "subnet name cannot contain a comma" in both classic/NLB schemas
The current rule !self.contains('','') is invalid CEL syntax: the contains() method expects a single string argument, not two. This will cause validation to fail or be skipped.
Change both occurrences (classic and network NLB AWSSubnetName validators) to:
- - message: subnet name cannot contain
- a comma
- rule: '!self.contains('','')'
+ - message: subnet name cannot contain
+ a comma
+ rule: "!self.contains(',')"And similarly for the names validator under networkLoadBalancer.subnets.
Also applies to: 3278-3289
🤖 Prompt for AI Agents
In
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml
around lines 3157-3168 (and similarly the block at 3278-3289), the CEL rule for
"subnet name cannot contain a comma" is invalid because it uses contains with
two arguments; replace the invalid rule with a single-argument contains check
(negated) so it becomes a valid CEL expression that tests for a comma (i.e., use
!self.contains(',') in both the AWSSubnetName validators and the names validator
under networkLoadBalancer.subnets), preserving surrounding YAML quoting/escaping
as needed.
|
/override ci/prow/okd-scos-images |
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/okd-scos-images 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. |
|
The okd-scos-images failure does not look like it's related to this bump |
|
/override ci/prow/okd-scos-images |
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/okd-scos-images 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. |
|
/hold Revision a854d0b was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/hold cancel The e2e-aws-4-21 job passed earlier today for this PR. However, recent runs have either failed to build a proper release or failed in a network policy check. |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@csrwng: Overrode contexts on behalf of csrwng: ci/prow/e2e-aws-4-21 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. |
|
@csrwng: Jira Issue OCPBUGS-66211: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-66211 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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. |
|
@csrwng: The following tests 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. |
5 participants
What this PR does / why we need it:
This updates the OpenShift API dependencies to the current master branch versions as of 2025-12-04.
Key dependency changes:
Code changes made to accommodate API updates:
🤖 Generated with Claude Code