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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded per-item regex validation to RegistrySources' ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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 |
Invalid registry entries (e.g., with tags like ":latest" or digests like "@sha256:...") in registrySources fields generate an invalid /etc/containers/policy.json, which can cause container runtime failures. Add per-item Pattern validation to insecureRegistries, blockedRegistries, and allowedRegistries fields in the RegistrySources struct using the same regex pattern already used by ImageDigestMirrors.Source. Each entry must match a valid registry scope: hostname[:port][/path], optionally with a wildcard prefix (*.). CRD validation ratcheting ensures pre-existing invalid entries are preserved on update. Includes integration tests demonstrating: - Invalid tagged/digest entries are rejected on create - Valid entries (hostname, path, wildcard, port) are accepted - Ratcheting preserves persisted invalid entries when other fields change - Modifying an atomic list with a persisted invalid entry is rejected Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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}
| // insecureRegistries are registries which do not have a valid TLS certificates or only support HTTP connections. | ||
| // 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 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]+)+)?)+)?$` | ||
| InsecureRegistries []string `json:"insecureRegistries,omitempty"` | ||
| // blockedRegistries cannot be used for image pull and push actions. All other registries are permitted. | ||
| // 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 include tags (e.g., ":latest") or digests (e.g., "@sha256:..."). | ||
| // | ||
| // Only one of BlockedRegistries or AllowedRegistries may be set. | ||
| // +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]+)+)?)+)?$` | ||
| 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 include tags (e.g., ":latest") or digests (e.g., "@sha256:..."). | ||
| // | ||
| // Only one of BlockedRegistries or AllowedRegistries may be set. | ||
| // +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]+)+)?)+)?$` | ||
| AllowedRegistries []string `json:"allowedRegistries,omitempty"` |
There was a problem hiding this comment.
1. Omitted behavior undocumented lists 📘 Rule violation ⚙ Maintainability
InsecureRegistries, BlockedRegistries, and AllowedRegistries are marked +optional and now have item Pattern validation, but their comments do not document what happens when the fields are omitted/defaulted. This violates the requirement to document optionality and omitted/default behavior for API fields with validation markers.
Agent Prompt
## Issue description
`RegistrySources` optional list fields (`insecureRegistries`, `blockedRegistries`, `allowedRegistries`) have validation markers but their Go doc comments do not describe what happens when the field is omitted (default behavior).
## Issue Context
These fields are `+optional` and now include per-item `Pattern` validation, so the field documentation must also state omitted/default behavior in human-readable terms.
## Fix Focus Areas
- config/v1/types_image.go[167-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@reedcort: 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. |
3 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 failuresBackground
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