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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
|
Code Review by Qodo
1. Omitted behavior undocumented lists
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
/jira refresh |
|
@joshbranham: This pull request references Jira Issue OCPBUGS-43353, which is valid. 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. |
| // Entries must not include tags (e.g., ":latest") or digests (e.g., "@sha256:..."). | ||
| // +optional | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:items:Pattern=`^\*(?:\.(?:[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]+)+)?)+)?$` |
There was a problem hiding this comment.
We generally discourage the usage of this marker because it spits the entire regular expression string out to the end user as opposed to a helpful error message.
Instead, we prefer the use of a the +kubebuilder:validation:items:XValidation marker where the rule parameter enforces the regular expression and the message parameter returns a user-friendly error message that outlines the constraints - much like the changes you've made to the field description do.
There was a problem hiding this comment.
This generally also means needing to enforce a maximum length on the input to prevent excessive cost estimations for evaluating the CEL expressions
|
@everettraven Updated to use Telemetry validation for limitsQueried CCX/Insights archive to confirm the limits won't break existing clusters: Max individual entry length (245 chars): SELECT
cluster_id,
entry,
length(entry) as entry_length
FROM ccx_sensitive.raw_io_archives
CROSS JOIN UNNEST(
concat(
COALESCE(cast(json_extract(content, '$.spec.registrySources.blockedRegistries') as array(varchar)), ARRAY[]),
COALESCE(cast(json_extract(content, '$.spec.registrySources.allowedRegistries') as array(varchar)), ARRAY[]),
COALESCE(cast(json_extract(content, '$.spec.registrySources.insecureRegistries') as array(varchar)), ARRAY[])
)
) AS t(entry)
WHERE file_path = 'config/image.json'
AND content LIKE '%registrySources%'
ORDER BY entry_length DESC
LIMIT 20;Max item count per list (32 entries): SELECT
cluster_id,
json_array_length(json_extract(content, '$.spec.registrySources.blockedRegistries')) as blocked_count,
json_array_length(json_extract(content, '$.spec.registrySources.allowedRegistries')) as allowed_count,
json_array_length(json_extract(content, '$.spec.registrySources.insecureRegistries')) as insecure_count
FROM ccx_sensitive.raw_io_archives
WHERE file_path = 'config/image.json'
AND content LIKE '%registrySources%'
ORDER BY blocked_count DESC, allowed_count DESC, insecure_count DESC
LIMIT 20;
|
|
@reedcort: This pull request references Jira Issue OCPBUGS-43353, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (xiuwang@redhat.com), skipping review request. 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. |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from a small comment and a question, this LGTM overall.
Thanks for doing an analysis with telemetry. A 16x overhead on the max items seems a bit excessive, I wonder if using something like 128 would be enough (4x increase)?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1/tests/images.config.openshift.io/AAA_ungated.yaml (1)
39-62: Use canonical digest strings in negative tests to harden regression detectionThe
@sha256:abc123samples are not valid digest syntax, so failures could be attributed to malformed digest format rather than the intended “no digests allowed” rule. Use a canonical 64-hex digest to make this assertion more precise.Proposed test input refinement
- blockedRegistries: ["registry.io/myrepo@sha256:abc123"] + blockedRegistries: ["registry.io/myrepo@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"] ... - allowedRegistries: ["registry.io/myrepo@sha256:abc123"] + allowedRegistries: ["registry.io/myrepo@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"] ... - insecureRegistries: ["registry.io/myrepo@sha256:abc123"] + insecureRegistries: ["registry.io/myrepo@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"]🤖 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 39 - 62, The negative tests use an invalid digest fragment ("@sha256:abc123"); update the three test entries named "Should not allow creating an Image..." to use a canonical valid digest string instead of "sha256:abc123" so the failure is about digests being disallowed and not malformed syntax; specifically replace the registry entries in spec.registrySources.blockedRegistries[0], spec.registrySources.allowedRegistries[0], and spec.registrySources.insecureRegistries[0] with a proper sha256 digest consisting of "sha256:" followed by 64 lowercase hex characters.
🤖 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 39-62: The negative tests use an invalid digest fragment
("@sha256:abc123"); update the three test entries named "Should not allow
creating an Image..." to use a canonical valid digest string instead of
"sha256:abc123" so the failure is about digests being disallowed and not
malformed syntax; specifically replace the registry entries in
spec.registrySources.blockedRegistries[0],
spec.registrySources.allowedRegistries[0], and
spec.registrySources.insecureRegistries[0] with a proper sha256 digest
consisting of "sha256:" followed by 64 lowercase hex characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9ca7589a-0200-49f5-9de7-08b52814f737
⛔ 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 (4)
config/v1/tests/images.config.openshift.io/AAA_ungated.yamlconfig/v1/types_image.goconfig/v1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_10_config-operator_01_images.crd.yaml
✅ Files skipped from review due to trivial changes (1)
- config/v1/zz_generated.swagger_doc_generated.go
🚧 Files skipped from review as they are similar to previous changes (2)
- config/v1/types_image.go
- payload-manifests/crds/0000_10_config-operator_01_images.crd.yaml
128 works for me. If no one disagrees I can update MaxItems to 128. |
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>
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
| // 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 128 entries. | ||
| // +optional | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=128 | ||
| // +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.
Wildcard entries currently reject documented :port and /path scopes.
The regex splits wildcard hosts into a separate ^\\*...$ branch, so *.example.com/ns and *.example.com:5000/ns fail validation even though the updated godoc says the format is hostname[:port][/path] with an optional *. prefix. Please fold the wildcard prefix into the main hostname alternative, or narrow the docs/message if wildcarded repo scopes are intentionally unsupported.
Also applies to: 182-195, 198-211
There was a problem hiding this comment.
This seems like a good find, although I will note that I haven't personally verified the regex myself.
@reedcort do we intend for something like the examples above to be valid wildcards or only something like *.example.com?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@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. |
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
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=128andMaxLength=512per item for CEL cost estimation (validated against fleet telemetry: max entry length 245, max item count 32)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