OCPBUGS-43353: Add pattern validation for registry entries in image config#2787
OCPBUGS-43353: Add pattern validation for registry entries in image config#2787reedcort wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @reedcort! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/L
|
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:
📝 WalkthroughWalkthroughAdded per-item and per-list validation to RegistrySources fields 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 `@config/v1/types_image.go`:
- Line 176: The XValidation:rule regex used in the kubebuilder tag for registry
hostnames in types_image.go is too permissive and must be replaced with a
stricter pattern that: disallows empty labels and consecutive dots, disallows
labels that start or end with a hyphen, allows an optional leading wildcard of
the form "*.example.com", allows an optional ":port" immediately after the
hostname (port = one or more digits), and permits optional path segments
("/repo" parts) composed of lowercase letters, digits, dot, underscore or
hyphen; update the XValidation:rule value (the kubebuilder tag string containing
the regex) to enforce these constraints (i.e., ensure each label matches
[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])? and labels are separated by single
dots, optional leading "*." allowed, optional :[0-9]+ after the hostname, then
optional (/[a-z0-9._-]+)* path segments) so valid hosts like "good.example.com",
"quay.io/ns/repo", "*.example.com", and "a.b" are accepted while invalid ones
like "example..com" or "a.-b.com" are rejected.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3199948e-925c-4a7a-8be3-c0988a0268e3
⛔ Files ignored due to path filters (4)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/images.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/images.config.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (3)
config/v1/types_image.goconfig/v1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_images.crd.yaml
|
@reedcort These validations are restricting existing GA APIs. How can you be confident in the maximum limits you have added, and that they won't break existing users/use cases? Do you have any telemetry data that would demonstrate the existing usage of these fields? |
|
@JoelSpeed I noticed that as well. I'm updating the validation to use Pattern instead, which removes the MaxItems and MaxLength constraints entirely to avoid impacting current users. |
Any change you make here is going to potentially be breaking. There are existing users of this API. You need to understand/explain how this will impact existing users before we can accept any change that makes the API more restrictive. We actually do prefer CEL to pattern btw, but we still need to answer my previous questions. What do you know about existing usage of these API fields? |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Thanks, makes sense. I'll work on gathering telemetry data on the existing usage of the fields across the fleet. |
reedcort
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@reedcort: This pull request references Jira Issue OCPBUGS-43353, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@JoelSpeed I gathered telemetry data to answer your questions. TelemetryQueried CCX/Insights archive across the fleet:
ApproachI plan to update the PR to use the same regex pattern already used by Kubernetes CRD validation ratcheting automatically handles existing invalid entries — on update, the API server suppresses validation errors for unchanged values. The ~108 clusters with existing tagged entries would be unaffected; only new invalid entries would be rejected. One nuance worth noting: during testing I found that Do you think this validation belongs here in the API, or would it be better placed in the MCO / HyperShift operator instead? |
|
Thanks for doing the research. As long as we can add a test case that demonstrates the ratcheting behaviour of currently invalid values (see our tests folder readme and look for ratcheting to see examples), then I think we can proceed with adding the validations here |
reedcort
changed the title
|
/test lint |
|
PR-Agent: could not fine a component named |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Review Summary by QodoAdd pattern validation for registry entries in image config
WalkthroughsDescription• Add per-item Pattern validation to registry entry fields in RegistrySources - Validates insecureRegistries, blockedRegistries, allowedRegistries - Rejects entries with tags (:latest) or digests (@sha256:...) • Enforce valid registry scope format: hostname[:port][/path] with optional wildcard prefix • CRD validation ratcheting preserves pre-existing invalid entries on update • Comprehensive test coverage for invalid/valid entries and ratcheting behavior Diagramflowchart LR
A["RegistrySources Fields"] -->|Pattern Validation| B["insecureRegistries"]
A -->|Pattern Validation| C["blockedRegistries"]
A -->|Pattern Validation| D["allowedRegistries"]
B -->|Reject| E["Invalid: tags/digests"]
C -->|Reject| E
D -->|Reject| E
B -->|Accept| F["Valid: hostname[:port][/path]"]
C -->|Accept| F
D -->|Accept| F
F -->|Wildcard Support| G["*.example.com"]
File Changes1. config/v1/types_image.go
|
128 works for me. If no one disagrees I can update MaxItems to 128. |
openshift-ci
Bot
added
size/XL
openshift-ci-robot
added
jira/invalid-bug
|
@reedcort: This pull request references Jira Issue OCPBUGS-43353, which is invalid:
Comment 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/v1/tests/images.config.openshift.io/AAA_ungated.yaml (1)
15-86: Please assert the friendly CEL message in at least one negative case.These cases only check the field path, so they prove rejection but not the main UX requirement behind switching to
items:XValidation: hiding the raw regex. Pin one representative failure to the custom message substring so that regression is covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/images.config.openshift.io/AAA_ungated.yaml` around lines 15 - 86, Update one representative negative test to also assert the user-friendly CEL message substring (not just the field path). For example, in the test named "Should not allow creating an Image with a tagged registry entry in blockedRegistries" (the case that sets spec.registrySources.blockedRegistries: ["registry.io/myrepo:latest"]), add an assertion field such as expectedMessageContains: "must not contain a tag or digest" (or the actual CEL-friendly substring emitted by the XValidation), so the test checks both the error field path ("spec.registrySources.blockedRegistries[0]") and that the human-facing message hides the raw regex; keep the rest of the cases unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/v1/tests/images.config.openshift.io/AAA_ungated.yaml`:
- Around line 15-86: Update one representative negative test to also assert the
user-friendly CEL message substring (not just the field path). For example, in
the test named "Should not allow creating an Image with a tagged registry entry
in blockedRegistries" (the case that sets
spec.registrySources.blockedRegistries: ["registry.io/myrepo:latest"]), add an
assertion field such as expectedMessageContains: "must not contain a tag or
digest" (or the actual CEL-friendly substring emitted by the XValidation), so
the test checks both the error field path
("spec.registrySources.blockedRegistries[0]") and that the human-facing message
hides the raw regex; keep the rest of the cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f887ede-9d04-401f-a6f3-2a2bd4694a6b
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/images.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/images.config.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (3)
config/v1/tests/images.config.openshift.io/AAA_ungated.yamlconfig/v1/types_image.gopayload-manifests/crds/0000_10_config-operator_01_images.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- payload-manifests/crds/0000_10_config-operator_01_images.crd.yaml
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from the comment from coderabbit related to the wildcard regular expression seeming to be a reasonable thing to look into, this LGTM
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven 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
|
Intentionally ratcheting validations and have testing and data backing that this is OK to do. /override ci/prow/verify-crdify |
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crdify 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. |
|
/test e2e-aws-ovn-hypershift |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
New changes are detected. LGTM label has been removed. |
|
Updated limits based on fleet telemetry data from Snowflake (Starburst has been archived):
|
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
| // +kubebuilder:validation:MaxItems=780 | ||
| // +kubebuilder:validation:items:MinLength=1 | ||
| // +kubebuilder:validation:items:MaxLength=512 | ||
| // +kubebuilder:validation:items:XValidation:rule="self.matches('^\\\\*(?:\\\\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+$|^((?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\\\\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(?::[0-9]+)?)(?:(?:/[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?)+)?$')",message="each registry must be a valid hostname[:port][/path] or wildcard *.hostname format without tags or digests" | ||
| BlockedRegistries []string `json:"blockedRegistries,omitempty"` | ||
| // allowedRegistries are the only registries permitted for image pull and push actions. All other registries are denied. | ||
| // Each entry must be a valid registry scope in the format hostname[:port][/path], | ||
| // optionally prefixed with "*." for wildcard subdomains (e.g., "*.example.com"). | ||
| // The hostname must consist of valid DNS labels separated by dots, where each label | ||
| // contains only alphanumeric characters and hyphens and does not start or end with a hyphen. | ||
| // Entries must not be empty, must not include tags (e.g., ":latest") or digests (e.g., "@sha256:..."), | ||
| // and must be at most 512 characters in length. The list may contain at most 780 entries. | ||
| // | ||
| // Only one of BlockedRegistries or AllowedRegistries may be set. | ||
| // +optional | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=780 | ||
| // +kubebuilder:validation:items:MinLength=1 | ||
| // +kubebuilder:validation:items:MaxLength=512 | ||
| // +kubebuilder:validation:items:XValidation:rule="self.matches('^\\\\*(?:\\\\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+$|^((?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\\\\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(?::[0-9]+)?)(?:(?:/[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?)+)?$')",message="each registry must be a valid hostname[:port][/path] or wildcard *.hostname format without tags or digests" |
There was a problem hiding this comment.
Do we want these to match the insecureRegistries validations?
There was a problem hiding this comment.
yeah, all three should match, I made the update.
Adds CEL XValidation, MinLength, MaxLength, and MaxItems constraints to insecureRegistries, blockedRegistries, and allowedRegistries fields in RegistrySources to reject invalid entries (e.g., tagged or digest references) that can cause container runtime failures. Includes ratcheting tests to ensure clusters with pre-existing invalid entries can still update other fields without being blocked. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@reedcort: The following test 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
Summary
insecureRegistries,blockedRegistries, andallowedRegistriesfields in theRegistrySourcesstruct (config/v1/types_image.go)ImageDigestMirrors.Sourcefor consistency:latest) or digests (e.g.,@sha256:...) that would generate invalid/etc/containers/policy.jsonand can cause container runtime failuresMinLength=1to reject empty string entries (which break node bootstrap on HCP)MaxItems=1024andMaxLength=256per item for CEL cost estimation (validated against fleet telemetry: max entry length 120, max item count 669)Background
Invalid registry entries like
registry.io/myrepo:latestinspec.registrySources.blockedRegistriespass through without validation and generate an invalid/etc/containers/policy.json, which can cause container runtime failures.Validation was initially proposed in hypershift#8070 at the Go level (webhook, controller, nodepool config). That PR was closed in favor of adding the canonical validation here at the API level, as requested by the maintainer.
Test plan
make update— all CRD manifests regeneratedmake -C config/v1 test— all integration tests passmake verify— passes🤖 Generated with Claude Code