CNTRLPLANE-2782, CNTRLPLANE-2781: Document and validate Azure Marketplace image fields - #8211
Conversation
|
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-2782 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
|
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:
📝 WalkthroughWalkthroughInline documentation and kubebuilder validation for AzureMarketplaceImage fields were updated. TODOs for Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant DocsBuild as Docs Build (GH Action)
participant ArtifactStore as Actions Artifact
participant DocsDeploy as Docs Deploy (GH Action)
participant Cloudflare as Cloudflare Pages
PR->>DocsBuild: Trigger on PR targeting main/release-4.22 (docs/**)
DocsBuild->>DocsBuild: Install Python, mkdocs build --strict
DocsBuild->>ArtifactStore: Upload docs/site as docs-site (include pr-number.txt)
ArtifactStore-->>DocsDeploy: Artifact available (workflow_run trigger)
DocsDeploy->>ArtifactStore: Download docs-site (by run id)
DocsDeploy->>DocsDeploy: Read pr-number from site/pr-number.txt
DocsDeploy->>Cloudflare: Deploy site to branch pr-<PR_NUMBER> using wrangler (with secrets)
🚥 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-2782 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 #8211 +/- ##
=======================================
Coverage 44.51% 44.51%
=======================================
Files 774 774
Lines 96997 96997
=======================================
Hits 43179 43179
Misses 50830 50830
Partials 2988 2988
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| // For example, 22_04-lts-gen2, 8-lvm-gen2. | ||
| // The value must consist only of lowercase letters, numbers, and hyphens (-) and underscores (_). | ||
| // TODO: What about length limits? | ||
| // The value must be between 1 and 255 characters in length. |
There was a problem hiding this comment.
Where did that value come from? Can you add the source of this validation?
There was a problem hiding this comment.
Done. Added a source reference noting these length constraints are defined by the Azure Compute REST API (Microsoft.Compute virtualMachineImages). Also added the az vm image list-skus CLI command for discoverability and updated the example to use a real ARO SKU (aro_417_rhel8_gen2).
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Added a source link to the Azure REST API documentation for the SKU length constraint: https://learn.microsoft.com/en-us/rest/api/compute/virtual-machine-images/list-skus
The 255-character limit was already present in the kubebuilder annotations (+kubebuilder:validation:MaxLength=255) since marketplace image support was first introduced in this API. This PR was just documenting it in the godoc. I've now also added an az vm image list-skus CLI reference and more ARO-specific examples (e.g., "aro_421").
AI-assisted response via Claude Code
| // It must be between 3 and 50 characters in length, and consist of only lowercase letters, numbers, and hyphens (-) and underscores (_). | ||
| // This value can be obtained using the Azure CLI: `az vm image list-publishers --location <region>`. | ||
| // For example, "redhat" for Red Hat images. | ||
| // It must be between 3 and 50 characters in length, and consist of only lowercase letters, numbers, hyphens (-), and underscores (_). |
There was a problem hiding this comment.
A real example would be helpful, maybe use quay.io/openshift-release-dev/ocp-release:4.21.10-x86_64
There was a problem hiding this comment.
Done. Updated the publisher example from "redhat" to "azureopenshift", which is the publisher used for ARO (Azure Red Hat OpenShift) images throughout the codebase (e.g., in hypershift-operator/controllers/nodepool/azure_test.go and test/e2e/v2/tests/api_ux_validation_test.go).
Note: the quay.io/openshift-release-dev/ocp-release:4.21.10-x86_64 reference is a container release image, not an Azure Marketplace publisher — so I used the real ARO marketplace publisher instead.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Updated the publisher example to "azureopenshift" for Azure Red Hat OpenShift images (keeping "redhat" as a secondary example). These are real Azure Marketplace publisher names — the quay.io/openshift-release-dev/ocp-release:4.21.10-x86_64 reference is the OCP release payload container image, which is a different concept from Azure Marketplace image URN components (publisher/offer/sku/version). Users can verify these values with az vm image list-publishers --location <region>.
AI-assisted response via Claude Code
|
|
||
| // offer specifies the name of a group of related images created by the publisher. | ||
| // TODO: What is the valid character set for this field? What about minimum and maximum lengths? | ||
| // This value can be obtained using the Azure CLI: `az vm image list-offers --location <region> --publisher <publisher>`. |
There was a problem hiding this comment.
A real example would be helpful, maybe use quay.io/openshift-release-dev/ocp-release:4.21.10-x86_64
There was a problem hiding this comment.
Done. Updated the offer example to tie it to the publisher: now reads "aro4" for Azure Red Hat OpenShift images (publisher "azureopenshift"). Also added the Azure Compute REST API source reference for the length constraints, consistent with the SKU field.
The existing "aro4" example is a real Azure Marketplace offer — used throughout our codebase for ARO image lookups.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Done. Updated the offer examples and added a source link to the Azure REST API docs. The current examples "aro4" and "rhel" are real Azure Marketplace offer names — the quay.io/openshift-release-dev/ocp-release:4.21.10-x86_64 reference is the OCP release payload container image, which is a different concept from Azure Marketplace image URN components (publisher/offer/sku/version). Users can verify offer values with az vm image list-offers --location <region> --publisher <publisher>. (Also addresses the same feedback on the publisher field.)
AI-assisted response via Claude Code
8ad8b1c to
ca093ab
Compare
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2782 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. |
|
verify is failing |
|
/test address-review-comments |
|
Re: #8211 (comment) Done. Fixed the verify failure — two issues:
The fix commit regenerates all 6 CRD YAML files, both doc files, and the vendor copy. AI-assisted response via Claude Code |
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2782 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. |
| // It must be between 3 and 50 characters in length, and consist of only lowercase letters, numbers, and hyphens (-) and underscores (_). | ||
| // This value can be obtained using the Azure CLI: `az vm image list-publishers --location <region>`. | ||
| // For example, "azureopenshift" for Azure Red Hat OpenShift images. | ||
| // It must be between 3 and 50 characters in length, and consist of only lowercase letters, numbers, hyphens (-), and underscores (_). |
There was a problem hiding this comment.
It also looks like this must start with a lowercase alphanumeric character.
It also looks like ending with a hyphen/underscore would be allowed with the currently enforced regular expression. Is that actually valid?
There was a problem hiding this comment.
Good catch on both points.
-
Starting character: Yes, the regex
^[a-z0-9][a-z0-9-_]{2,49}$enforces starting with a lowercase alphanumeric, and the godoc already reflects this: "It must start with a lowercase letter or a number." -
Ending with hyphen/underscore: You're right — the current regex allows a trailing
-or_, which is almost certainly not a valid Azure publisher name. All real publisher names in our codebase (azureopenshift,redhat) and in Azure's marketplace (canonical,microsoftwindowsserver) end with alphanumeric characters. We should tighten the regex to^[a-z0-9][a-z0-9-_]*[a-z0-9]$(withMinLength=3still enforced separately) to require both the first and last characters to be alphanumeric.
I'll make this fix — good eye.
AI-assisted response via Claude Code
| Publisher string `json:"publisher,omitempty"` | ||
|
|
||
| // offer specifies the name of a group of related images created by the publisher. | ||
| // TODO: What is the valid character set for this field? What about minimum and maximum lengths? |
There was a problem hiding this comment.
It looks like the valid character set TODO here wasn't resolved. Is there a particular pattern that should be enforced here to ensure only inputs that resemble a valid offer name are provided?
There was a problem hiding this comment.
Good point — the valid character set TODO wasn't fully resolved because Azure doesn't publish a strict regex for offer names, and the upstream CAPZ ImagePlan.Offer field also has no pattern validation (just MinLength=1).
Looking at real Azure Marketplace offer names, they can include mixed-case letters — e.g., CAPZ's own examples are UbuntuServer and WindowsServer (see vendor/sigs.k8s.io/cluster-api-provider-azure/api/v1beta1/types.go:493). Our codebase uses aro4 (lowercase), but enforcing only lowercase would break users with non-ARO marketplace images.
A reasonable approach would be to add a permissive pattern like ^[a-zA-Z0-9][a-zA-Z0-9-_.]*[a-zA-Z0-9]$ (alphanumeric start/end, allow letters, numbers, hyphens, underscores, and dots in between) — similar to what we see in real Azure offer names. Alternatively, we could follow CAPZ's lead and rely on Azure's own API validation to reject invalid names, keeping just the length constraints.
I'll add a pattern that matches the known Azure conventions. Let me know if you'd prefer a specific approach.
AI-assisted response via Claude Code
7adf3b0 to
2a0a90b
Compare
|
@hypershift-jira-solve-ci[bot]: This pull request references CNTRLPLANE-2782 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. |
openshift-ci
Bot
added
the
needs-rebase
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
|
/remove-lifecycle stale |
openshift-ci
Bot
removed
the
lifecycle/stale
90cb7db to
73a2044
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
verify is failing in the github action job |
|
/test address-review-comments |
|
Review agent triggered. View job |
openshift-ci
Bot
added
needs-rebase
|
@hypershift-jira-solve-ci[bot]: 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. |
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
- Improve publisher regex to require ending with alphanumeric character - Add CEL validation rule for offer field pattern (alphanumeric, hyphens, underscores, periods, must start with alphanumeric) - Replace TODO comments with concrete documentation, examples, and links to Azure docs for publisher, offer, sku, and imageID fields - Add sku length constraints (1-255 characters) Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Regenerate NodePool CRD manifests for Default, CustomNoUpgrade, and TechPreviewNoUpgrade profiles with updated Azure validation rules - Update vendor copy of azure.go to match API changes - Update stable NodePool Azure test suite for new validation patterns Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Update API reference and aggregated docs to reflect improved Azure Marketplace image field documentation and validation rules - Remove TODO comments and add examples, Azure doc links, and character constraints for publisher, offer, sku, and imageID fields Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Description changes are documentation-only improvements that don't affect API compatibility. Pattern changes (e.g. tightening validation regex) are deliberate API evolution decisions that should be visible for review but not block PRs. Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The api/ directory is a separate Go module with its own go.mod. Running `go test ./api/...` from the root module fails with "does not contain package." Skip api/ packages in the test-changed pre-push hook, matching the existing exclusions for vendor/ and hack/tools/. Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d08e61c to
692d8c0
Compare
openshift-ci
Bot
removed
the
needs-rebase
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
/test address-review-comments
There was a problem hiding this comment.
This PR changes CRD field descriptions (documentation improvements) and patterns (tightened validation regex). Without this config, crdify flags those as errors and blocks CI. Adding description and pattern as Warn enforcement means they're still visible in review output but don't fail the build. Description changes are documentation-only and never affect API compatibility. Pattern changes (like tightening a regex) are deliberate API evolution decisions that should be reviewed but shouldn't auto-block — this is consistent with the existing unhandledEnforcement: Warn policy already in this file.
AI-assisted response via Claude Code
There was a problem hiding this comment.
@everettraven do you agree with this change? I think someone else ran into something similar the other day on a different PR. I'm not super familiar with crdify configurations just yet.
There was a problem hiding this comment.
The api/ directory is a separate Go module with its own go.mod. The test-changed target collects changed .go files and runs go test on their packages, but go test ./api/... from the root module fails with "does not contain package" because Go can't test across module boundaries. This exclusion matches the existing pattern for vendor/ and hack/tools/ (also separate modules). The api/ tests run independently via make -C api test.
AI-assisted response via Claude Code
|
Review agent triggered. View job |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Stale PRs 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
4 participants
What this PR does / why we need it:
Resolves all TODO comments in the Azure Marketplace image API types (
AzureMarketplaceImageandAzureVMImage) by adding comprehensive documentation, concrete examples, and validation for fields that were previously undocumented. This PR also incorporates the changes from #8223.Changes:
AzureVMImage): Documented Azure resource ID format with examples for managed images and gallery image versions, linked to Microsoft Learn naming rulesAzureMarketplaceImage): Added examples ("azureopenshift", "canonical", "redhat"), linked to Microsoft Learn docs, tightened regex to also require ending with alphanumeric (^[a-z0-9][a-z0-9-_]*[a-z0-9]$)AzureMarketplaceImage): Documented valid character set (alphanumeric, hyphens, underscores, periods), added CEL validation viaXValidation, added examples ("RHEL", "WindowsServer", "0001-com-ubuntu-server-jammy"), linked to Microsoft Learn docsAzureMarketplaceImage): Documented length limits (1-255 characters), added Microsoft Learn referenceAll generated CRDs (including OSStreams feature gate variant), API reference docs, and vendor directory have been regenerated.
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-2782
Supersedes #8223 (https://redhat.atlassian.net/browse/CNTRLPLANE-2781)
Checklist: