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 42.89% 42.89%
=======================================
Files 766 766
Lines 94684 94684
=======================================
Hits 40617 40617
Misses 51259 51259
Partials 2808 2808
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
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-2782
branch
from
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
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-2782
branch
from
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. |
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-2782
branch
from
7a5fd4f to
9f2c384
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
PR title and description need to be updated |
|
/address-review-comments |
|
🤖 Addressing review comments: workflow run |
github-actions
Bot
changed the title
|
Done. Updated the PR title and description to reflect the full scope of changes, including the incorporation of #8223 (CNTRLPLANE-2781). The description now covers all four fields (imageID, publisher, offer, sku), the validation changes, and the envtest additions. AI-assisted response via Claude Code |
|
verify is failing |
|
/address-review-comments |
|
🤖 Addressing review comments: workflow run |
github-actions
Bot
changed the title
|
@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. 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. |
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-2782
branch
from
9f2c384 to
89b5227
Compare
|
/restructure-commits |
|
🤖 Restructuring commits: 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)
hypershift-jira-solve-ci
Bot
force-pushed
the
fix-CNTRLPLANE-2782
branch
from
89b5227 to
90cb7db
Compare
|
@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. |
|
Only one error — the Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe CI verify job runs Root CauseThe PR modifies The PR includes updated
The CI pipeline sequence is:
Since both Recommendations
Evidence
|
|
Verify is failing and needs to be fixed |
|
/address-review-comments |
|
🤖 Addressing review comments: workflow run |
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: