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:
I also verified that setting 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.
|
|
@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