CNTRLPLANE-2783: Document imageID field format and add CEL validation for Azure VM images - #8106
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
/auto-cc |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2783 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2783 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/api
openshift-ci
Bot
added
area/documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated kubebuilder validation annotations and docs for Azure-related API fields in 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2783 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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. |
| // +kubebuilder:validation:XValidation:rule=`oldSelf.hasValue() && oldSelf.value() == self || size(self.split('/')) == 9 && self.split('/')[8].matches('^[a-zA-Z0-9-_\\.]{1,80}$')`,message="the imageName should be between 1 and 80 characters, consisting only of alphanumeric characters, hyphens, underscores and periods",optionalOldSelf=true | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() && oldSelf.value() == self || size(self.split('/')) == 9 && !self.split('/')[8].endsWith('.')",message="the imageName in the imageID must not end with a period (.) character",optionalOldSelf=true | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=285 |
There was a problem hiding this comment.
Is ImageID actually used in the codebase? I thought we removed the functionality behind this API field.
There was a problem hiding this comment.
Yes, ImageID is still actively used in the codebase. In hypershift-operator/controllers/nodepool/azure.go:
- Line 41:
defaultAzureNodePoolImagechecksImageID != nilto skip marketplace defaulting when a user has explicitly set a VHD image. - Lines 176-179:
azureMachineTemplateSpecvalidates that eitherImageIDorAzureMarketplaceis set. - Lines 198-201: The
switchonImage.Typehandleshyperv1.ImageIDby passing the value to the CAPIAzureMachineTemplateSpec.Image.ID.
It's also used in cmd/cluster/azure/create.go (lines 391-395) when creating clusters with a boot image ID. So it remains a functional code path — it's the alternative to Azure Marketplace images for specifying the VM boot image.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Is it actually used in the hypershift operator or control plane operator code? Basically is it used elsewhere other than the CLI and API?
There was a problem hiding this comment.
Yes, ImageID is actively used in the hypershift-operator nodepool controller (hypershift-operator/controllers/nodepool/azure.go):
defaultAzureNodePoolImage(line 41): ChecksImageID != nilto skip marketplace image defaulting — if the user explicitly set a VHD image, don't override it.azureMachineTemplateSpec(lines 176–179): Validates that eitherImageIDorAzureMarketplaceis set after defaulting; returns an error if neither is configured.azureMachineTemplateSpec(lines 198–201): WhenType == ImageID, it maps the value directly into the CAPIAzureMachineTemplateSpec.Image.IDfield, which is what CAPZ uses to provision the VM.
So it flows from the API through the hypershift-operator's nodepool reconciler into the CAPI AzureMachineTemplate — it's not limited to CLI and API definitions.
AI-assisted response via Claude Code
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/hold prefer to remove this if it isn't being used. Are these production paths? #8106 (comment) |
openshift-ci
Bot
added
the
do-not-merge/hold
|
prefer to remove this if it isn't being used. Are these production paths? #8106 (comment) |
|
@sjenning Acknowledged the hold. To clarify what the code shows: The However, I cannot determine from the code alone whether this path is exercised in production (e.g., whether any ARO HCP or self-managed Azure deployment actually uses If the consensus is that
Please advise on how you'd like to proceed. AI-assisted response via Claude Code |
|
/uncc |
openshift-ci
Bot
added
the
needs-rebase
aaac951 to
412f1eb
Compare
|
@coderabbitai Re: MinLength=1 suggestion Good analysis. You're correct that In practice, since Note: Kubernetes 1.31+ also has CRD validation ratcheting (KEP-4008) which would skip schema validations for unchanged field values, providing another safety net. But removing the redundant This PR is currently on hold pending maintainer discussion on whether the AI-assisted response via Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8106 +/- ##
=======================================
Coverage 36.66% 36.66%
=======================================
Files 765 765
Lines 93300 93300
=======================================
Hits 34209 34209
Misses 56410 56410
Partials 2681 2681
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Generally looks fine from the API team's perspective
| // +kubebuilder:validation:XValidation:rule=`oldSelf.hasValue() && oldSelf.value() == self || size(self.split('/')) == 9 && self.split('/')[8].matches('^[a-zA-Z0-9-_\\.]{1,80}$')`,message="the imageName should be between 1 and 80 characters, consisting only of alphanumeric characters, hyphens, underscores and periods",optionalOldSelf=true | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() && oldSelf.value() == self || size(self.split('/')) == 9 && !self.split('/')[8].endsWith('.')",message="the imageName in the imageID must not end with a period (.) character",optionalOldSelf=true | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=285 |
There was a problem hiding this comment.
(minor API comment): we generally try to have it explicit in the godoc as well for the overall length (e.g. // imageID must be between 1 and 285 characters in length.) but might be ok as is since each sub-segment is validated.
There was a problem hiding this comment.
+1. Please include the overall length constraints explicitly in the GoDoc.
There was a problem hiding this comment.
Done. Added // The imageID must be between 1 and 285 characters in length. to the GoDoc, consistent with the convention shown on other fields like subnetID.
AI-assisted response via Claude Code
| // +unionMember | ||
| // +kubebuilder:validation:MaxLength=255 | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() && oldSelf.value() == self || size(self.split('/')) == 9 && self.matches('^/subscriptions/.*/resourceGroups/.*/providers/Microsoft.Compute/images/.*$')",message="imageID must be in the format `/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Compute/images/{imageName}`",optionalOldSelf=true | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() && oldSelf.value() == self || size(self.split('/')) == 9 && self.split('/')[2].matches('^[{]?[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}[}]?$')",message="the subscriptionId in the imageID must be a valid UUID. It should be 5 groups of hyphen separated hexadecimal characters in the form 8-4-4-4-12",optionalOldSelf=true |
There was a problem hiding this comment.
It looks like the regular expression here allows the subscription ID to contain { and } at the beginning and end.
Is this actually valid? I would assume this would need to be a proper UUID string which wouldn't contain the { and } characters.
There was a problem hiding this comment.
Good catch. The [{]? and [}]? were allowing brace-wrapped GUIDs (a Windows GUID convention), but Azure subscription IDs in resource paths are standard UUIDs without braces. Removed the optional brace characters from the regex — it now strictly validates the UUID format: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$.
This fix was applied to all 4 occurrences: imageID, both subnetID fields, and encryptionSetID.
AI-assisted response via Claude Code
| // +kubebuilder:validation:XValidation:rule="size(self.split('/')) == 11 && self.matches('^/subscriptions/.*/resourceGroups/.*/providers/Microsoft.Network/virtualNetworks/.*/subnets/.*$')",message="encryptionSetID must be in the format `/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName}/subnets/{subnetName}`" | ||
| // +kubeubilder:validation:XValidation:rule="self.split('/')[2].matches('^[{]?[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}[}]?$')",message="the subscriptionId in the encryptionSetID must be a valid UUID. It should be 5 groups of hyphen separated hexadecimal characters in the form 8-4-4-4-12" | ||
| // +kubebuilder:validation:XValidation:rule="size(self.split('/')) == 11 && self.matches('^/subscriptions/.*/resourceGroups/.*/providers/Microsoft.Network/virtualNetworks/.*/subnets/.*$')",message="subnetID must be in the format `/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Network/virtualNetworks/{vnetName}/subnets/{subnetName}`" | ||
| // +kubebuilder:validation:XValidation:rule="self.split('/')[2].matches('^[{]?[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}[}]?$')",message="the subscriptionId in the subnetID must be a valid UUID. It should be 5 groups of hyphen separated hexadecimal characters in the form 8-4-4-4-12" |
There was a problem hiding this comment.
Same regex comment re: { and } characters
There was a problem hiding this comment.
Done. Removed the {/} characters from the UUID regex here as well. (Also addresses feedback from @everettraven on the imageID field.)
AI-assisted response via Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/hypershift/v1beta1/azure.go`:
- Around line 144-146: The CEL XValidation character classes in the kubebuilder
annotations for resourceGroupName and imageName (the rules that use
self.split('/')[4].matches('^[a-zA-Z0-9-_\\(\\)\\.]{1,90}$') and
self.split('/')[8].matches('^[a-zA-Z0-9-_\\.]{1,80}$')) create an unintended
range because the hyphen is between characters; update those regexes to either
escape the hyphen (\\-) or place the hyphen at the start or end of the class
(e.g., [-a-zA-Z0-9_\\.]) so the hyphen is treated literally and the validation
uses RE2-compatible character classes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 90f12730-b691-4884-9a2d-01ecc7d0a71f
⛔ Files ignored due to path filters (40)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/nodepools.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/nodepools-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
api/hypershift/v1beta1/azure.go
04378cf to
2d83608
Compare
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Looks fine from an API perspective.
/approve
openshift-ci
Bot
added
the
approved
|
Can you use the restructure commits skill in this repo to redo the commits please? |
|
Re: #8106 (comment) Done. Restructured the 7 commits into 3 clean component-based commits:
Content is identical to the previous branch tip (verified with AI-assisted response via Claude Code |
|
Re: #8106 (comment) Done. Restructured the 7 commits into 3 clean component-based commits:
All gitlint checks pass and the tree is identical to the previous HEAD (no content lost). AI-assisted response via Claude Code |
Replace the TODO comment on the imageID field in AzureVMImage with
proper documentation of the expected Azure resource ID format and
add XValidation rules consistent with other Azure resource ID fields
(subnetID, encryptionSetID).
The imageID must be a full Azure managed image resource ID in the format:
/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Compute/images/{imageName}
Validation rules enforce:
- Correct path structure (9 segments)
- Valid UUID for subscriptionId
- Valid resourceGroupName (1-90 chars, alphanumeric/hyphens/underscores/periods/parenthesis)
- Valid imageName (1-80 chars, alphanumeric/hyphens/underscores/periods)
- MaxLength updated from 255 to 285 for consistency with encryptionSetID
Ref: CNTRLPLANE-2783
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Regenerate CRD manifests and vendor copy to include the new imageID validation rules added to the AzureVMImage type. Ref: CNTRLPLANE-2783 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Regenerate API documentation to reflect the updated imageID field description, removing the TODO and replacing it with the proper Azure resource ID format documentation. Ref: CNTRLPLANE-2783 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rrors
Address review findings for imageID XValidation rules:
- Add optionalOldSelf=true to grandfather existing non-conforming values
- Add segment count guard to each rule to prevent CEL index panics
- Add endsWith('.') check for imageName (missing vs resourceGroupName)
- Anchor regex patterns with ^...$ for unambiguous matching
- Standardize error message casing to lowercase-start
- Fix pre-existing SubnetID copy-paste errors referencing "encryptionSetID"
- Fix kubeubilder typo in AzurePlatformSpec.SubnetID marker
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… backward compat The schema-level minLength validation is enforced independently of CEL rules, so it would block updates to legacy empty values even though the optionalOldSelf=true CEL rules are designed to allow unchanged values through. The CEL format rules already implicitly enforce non-emptiness for new values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the optional `{`/`}` characters from the UUID validation regex
in subscriptionId CEL rules. Azure subscription IDs are standard UUIDs
and should not accept brace-wrapped formats. Also adds overall length
constraint (1-285 chars) to the imageID GoDoc as requested by reviewers.
Changes applied to all 4 UUID regex locations: imageID (line 142),
subnetID (lines 95, 438), and encryptionSetID (line 346).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Regenerate the TLSAdherence feature-gated CRD manifests and aggregated docs to include the SubnetID validation fixes and imageID length documentation from the rebased branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2d83608 to
ebfb10e
Compare
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, everettraven, hypershift-jira-solve-ci[bot] 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 |
|
/close I missed the hold message. We should remove the imageID field |
|
@bryan-cox: Closed this PR. 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. |
|
Now I have the complete picture. The PR was closed (not merged) at Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll 7 CI jobs were aborted by the Prow trigger plugin — none of them are real test or build failures. The PR was closed (without merging) at Root CausePR #8106 was closed (not merged) 43 seconds after CI jobs were triggered, causing Prow to abort all 7 jobs. The timeline is:
The JUnit XML artifacts (where present) show only context-cancellation and timeout errors (e.g., This is standard Prow behavior: when a PR is closed, the trigger plugin cancels all pending and running jobs associated with that PR since their results are no longer relevant. Recommendations
Evidence
|
7 participants
What this PR does / why we need it:
Documents the valid format and constraints for the Azure
imageIDfield inAzureVMImageand adds CEL validation rules, addressing the TODO that was left in the API definition.Changes:
imageIDwith proper documentation of the expected Azure managed image resource ID format:/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Compute/images/{imageName}XValidationCEL rules consistent with existing Azure resource ID fields (subnetID,encryptionSetID), enforcing correct segment count, valid UUID subscription ID, valid resource group name, and valid image nameoptionalOldSelf=trueto grandfather existing non-conforming valuesSubnetIDcopy-paste errors that referenced "encryptionSetID" in validation messageskubeubildertypo inAzurePlatformSpec.SubnetIDmarkermaxLengthfrom 255 to 285 for consistency withencryptionSetIDWhich issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-2783
Special notes for your reviewer:
The CEL validation rules follow the same patterns already established for
subnetIDandencryptionSetIDin the same file. TheoptionalOldSelf=trueannotation ensures existing clusters with non-conforming imageID values are not broken by the new validation on update.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-2783](https://redhat.atlassian.net/browse/CNTRLPLANE-2783)Summary by CodeRabbit
Bug Fixes
Documentation