CNTRLPLANE-2781: Document valid character sets and add validation for Azure Marketplace image fields#8223
Conversation
…Marketplace image fields - Replace TODO comments with proper documentation for imageID, publisher, offer, and sku fields in AzureVMImage and AzureMarketplaceImage - Add kubebuilder validation pattern for offer field enforcing valid Azure Marketplace characters (alphanumeric, hyphens, underscores, periods) - Add concrete examples and links to Microsoft Learn documentation - Document expected Azure resource ID format for imageID with examples Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…lidation - Revendor api/hypershift/v1beta1/azure.go with updated field docs - Regenerate NodePool CRD manifests (Default, CustomNoUpgrade, TechPreviewNoUpgrade) with new offer pattern and expanded descriptions Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Correct map literal alignment for armNodeLabels in testARM64Provisioning Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2781 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. |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2781 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. |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2781 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates field documentation and validation for Azure image selection in api/hypershift/v1beta1/azure.go: it clarifies AzureVMImage.ImageID as an Azure resource ID (with examples), expands AzureMarketplaceImage.Publisher docs and allowed characters, specifies format and adds a kubebuilder XValidation rule for AzureMarketplaceImage.Offer (must match ^[a-zA-Z0-9][a-zA-Z0-9._-]*$), and documents AzureMarketplaceImage.SKU length/characters. In test/e2e/karpenter_test.go, only whitespace/indentation in the armNodeLabels map was adjusted. 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2781 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. |
![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 174-176: The publisher examples in the azure.go comment contradict
the lowercase-only validation (regex ^[a-z0-9][a-z0-9-_]{2,49}$) — replace the
mixed-case examples "Canonical" and "RedHat" with lowercase-valid examples
(e.g., "canonical", "redhat") so the examples match the enforced rule; update
the comment near the publisher description/regex in azure.go accordingly.
🪄 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: Pro Plus
Run ID: ad6816d4-78e6-4a0c-a1c0-11173828cb9a
⛔ Files ignored due to path filters (9)
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/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/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (2)
api/hypershift/v1beta1/azure.gotest/e2e/karpenter_test.go
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-2781
branch
from
c1239e4 to
1276b77
Compare
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2781 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8223 +/- ##
==========================================
+ Coverage 34.63% 35.61% +0.98%
==========================================
Files 767 767
Lines 93186 93333 +147
==========================================
+ Hits 32277 33245 +968
+ Misses 58236 57399 -837
- Partials 2673 2689 +16 🚀 New features to boost your workflow:
|
|
@coderabbitai Thanks for the review. The publisher examples have already been updated to lowercase-valid values ( AI-assisted response via Claude Code |
|
Is this essentially a duplicate of #8211 ? |
|
@everettraven Yes, there is significant overlap. Both PRs address the same TODO comments on the Key differences:
These will conflict if both are merged. They should be reconciled — either close one in favor of the other, or merge the best parts of both into a single PR. AI-assisted response via Claude Code |
- Update api.md and aggregated-docs.md with improved field descriptions, examples, and documentation links for Azure Marketplace image types Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
Re: review PRR_kwDOE7ekcc70eIzd @coderabbitai Good catch on the mixed-case publisher examples. The Go source ( AI-assisted response via Claude Code |
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-2781
branch
from
1276b77 to
1fd2eaf
Compare
| // See https://learn.microsoft.com/en-us/azure/virtual-machines/linux/cli-ps-findimage for more | ||
| // information on Azure Marketplace image offers. | ||
| // | ||
| // +kubebuilder:validation:Pattern=`^[a-zA-Z0-9][a-zA-Z0-9._-]*$` |
There was a problem hiding this comment.
please use // +kubebuilder:validation:XValidation:rule="self.matches and add an envtest to validate this
There was a problem hiding this comment.
Done. Replaced +kubebuilder:validation:Pattern with +kubebuilder:validation:XValidation:rule="self.matches('^[a-zA-Z0-9][a-zA-Z0-9._-]*$')" for the offer field, consistent with the pattern used elsewhere in the codebase (e.g., GCP project IDs in gcp.go, Azure client IDs in azure.go).
Added three envtest cases to stable.nodepools.azure.testsuite.yaml:
- Rejects offer with invalid characters (spaces,
!) - Rejects offer starting with a hyphen
- Accepts offer with valid mixed characters (alphanumeric, hyphens, underscores, periods)
Regenerated CRDs and vendor.
AI-assisted response via Claude Code
|
/test address-review-comments |
Replace kubebuilder Pattern validation with XValidation CEL rule using self.matches() for the Azure Marketplace offer field, as requested in review. This approach is consistent with other field validations in the codebase (e.g., GCP project IDs, Azure client IDs) and provides better error messages via the message parameter. Also adds envtest cases to validate the offer field character constraints: rejects invalid characters and non-alphanumeric starts, accepts valid offer strings with hyphens, underscores, and periods. Regenerated CRDs and vendor to reflect the change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
area/platform/azure
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2781 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 "5.0.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. |
|
/retest |
|
Both agents converged on the same root cause. Here is the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth Enterprise Contract checks failed because the Snyk SAST scanner ( Root CauseThe Snyk CLI, running as part of the The Snyk task itself completed as a Tekton task (exit code 0) but set its Comparison with other PRs confirms this is transient:
All other build tasks (clair-scan, clamav-scan, sast-unicode-check, rpms-signature-scan, deprecated-base-image-check) passed identically across all PRs. The 38 warnings (sast-shell-check, coverity-availability-check, trusted_task.current) are informative-only and also identical across PRs. Recommendations
Evidence
|
|
/uncc |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre, 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 |
openshift-ci
Bot
added
the
approved
|
PTAL @everettraven /label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
Re: #8223 (comment) @everettraven Good question — there is significant overlap, but also some meaningful differences between the two PRs that may be worth considering: Overlap: Both PRs resolve the same TODO comments on the Key differences:
Consolidating into a single PR would make sense to avoid merge conflicts on the same lines. The decision on which approach to keep (XValidation vs Pattern for AI-assisted response via Claude Code |
|
Re: #8223 (comment) @everettraven Good question — there is definite overlap, but also some meaningful differences between the two PRs:
Both resolve the same TODO comments on Since @enxebre has already approved this PR, happy to defer to you both on whether to consolidate or proceed with this one as-is. Either approach works — if proceeding separately, whichever merges second would need a rebase to resolve the AI-assisted response via Claude Code |
| // For example, "azureopenshift", "canonical", or "redhat". | ||
| // It must be between 3 and 50 characters in length, and consist of only lowercase letters, numbers, hyphens (-), and underscores (_). | ||
| // It must start with a lowercase letter or a number. |
There was a problem hiding this comment.
Would something like publisher10------__----- actually be valid?
There was a problem hiding this comment.
Yes, publisher10------__----- would pass this regex. The validation here is intentionally scoped to character set and length — it ensures only the documented characters (lowercase alphanumeric, hyphens, underscores) are used and the value starts with a letter or digit, but it doesn't enforce restrictions on consecutive special characters or trailing patterns.
The rationale is that Azure's Compute API will reject any publisher name that doesn't match an actual registered publisher at deployment time, so the CRD validation acts as a first-pass format check rather than a semantic one. Tightening the regex (e.g., disallowing consecutive hyphens/underscores) risks rejecting valid Azure publisher names we haven't encountered, since Azure's exact naming rules aren't fully documented.
That said, if you'd prefer stricter validation here (e.g., requiring special chars to be non-consecutive or non-trailing), happy to tighten it — just want to be cautious about false negatives.
AI-assisted response via Claude Code
| // The value must consist of only alphanumeric characters (a-z, A-Z, 0-9), | ||
| // hyphens (-), underscores (_), and periods (.). | ||
| // It must start with an alphanumeric character. |
There was a problem hiding this comment.
Would something like offer-thing----___---..... actually be valid?
There was a problem hiding this comment.
Yes, offer-thing----___---..... would pass this regex. Same reasoning as above — the validation is scoped to allowed character set (alphanumeric, hyphens, underscores, periods) and requiring an alphanumeric first character, without restricting consecutive or trailing special characters.
Real-world Azure offer names vary widely in format (e.g., 0001-com-ubuntu-server-jammy, RHEL, WindowsServer), and the periods in particular appear in some offer names. Since Azure's exact naming constraints for offers aren't exhaustively documented, the regex errs on the permissive side to avoid rejecting valid values. Azure's API will return a clear error at deployment time for any non-existent offer.
If you'd like stricter rules (e.g., no consecutive special chars, must end with alphanumeric), happy to adjust — just flagging the trade-off with potential false negatives.
AI-assisted response via Claude Code
| // The value must be between 1 and 255 characters in length, and consist of only lowercase | ||
| // letters, numbers, hyphens (-), and underscores (_). |
There was a problem hiding this comment.
Would something like __---_------------sku123 actually be valid?
There was a problem hiding this comment.
Yes, __---_------------sku123 would pass this regex (^[a-z0-9-_]+$). Notably, unlike the publisher and offer fields, the SKU regex doesn't require starting with an alphanumeric character — it allows hyphens and underscores anywhere, including as the first character.
Real-world SKU values like 22_04-lts-gen2 and 8-lvm-gen2 do use hyphens and underscores freely. However, looking at this more carefully, the documentation on this PR says "The value must consist of only lowercase letters, numbers, hyphens (-), and underscores (_)" without explicitly saying it must start with alphanumeric. If you think the SKU field should also require starting with an alphanumeric character (consistent with the publisher field), the regex could be tightened to ^[a-z0-9][a-z0-9-_]*$. That would reject values starting with _ or -.
Let me know if you'd like that tightened, or if the current permissive approach is acceptable given that Azure's API provides the ultimate validation.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Yes, __---_------------sku123 would technically pass the current regex (^[a-z0-9-_]+$). Unlike the publisher and offer fields, the SKU regex doesn't require starting with an alphanumeric character — so values beginning with _ or - are accepted.
In practice, real Azure SKUs (e.g. 22_04-lts-gen2, 8-lvm-gen2, 2022-datacenter) always start with an alphanumeric character. If you'd like, we can tighten this to ^[a-z0-9][a-z0-9-_]*$ to match that pattern consistently with the publisher field. Let me know if you'd like that change.
AI-assisted response via Claude Code
|
@hypershift-jira-solve-ci[bot]: all tests passed! 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. |
|
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
the
lifecycle/stale
… image fields - Resolve imageID TODO with Azure resource ID format documentation - Replace Azure CLI references with Microsoft Learn documentation links - Switch offer field validation from kubebuilder Pattern to XValidation (CEL) for improved error messages - Add multiple examples for publisher field - Add envtest test cases for offer field character validation - Regenerate CRDs, vendor, and docs Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… image fields - Resolve imageID TODO with Azure resource ID format documentation - Replace Azure CLI references with Microsoft Learn documentation links - Switch offer field validation from kubebuilder Pattern to XValidation (CEL) for improved error messages - Add multiple examples for publisher field - Add envtest test cases for offer field character validation - Regenerate CRDs, vendor, and docs Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… image fields - Resolve imageID TODO with Azure resource ID format documentation - Replace Azure CLI references with Microsoft Learn documentation links - Switch offer field validation from kubebuilder Pattern to XValidation (CEL) for improved error messages - Add multiple examples for publisher field - Add envtest test cases for offer field character validation - Regenerate CRDs, vendor, and docs Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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
5 participants
What this PR does / why we need it:
Documents the valid character sets for Azure Marketplace image fields (
offer,publisher,sku,imageID) and adds kubebuilder validation for theofferfield.Previously, these API fields had TODO comments instead of proper documentation. This PR:
offerfield enforcing valid Azure Marketplace characters (alphanumeric, hyphens, underscores, periods)Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-2781
Special notes for your reviewer:
The validation pattern
^[a-zA-Z0-9._-]+$for theofferfield matches the character set accepted by Azure Marketplace. Other fields (publisher,sku) were documented but not given validation patterns since their constraints may vary — this can be added in a follow-up if desired.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via
/jira:solve [CNTRLPLANE-2781](https://redhat.atlassian.net/browse/CNTRLPLANE-2781)Summary by CodeRabbit