ACM-27290: Explicitly set docker transport in policy.json #8493
Conversation
WalkthroughModifies GenerateInsecurePolicyJSON to add docker transport entries with insecureAcceptAnything policy for the default docker entry and two Red Hat registries (registry.redhat.io and registry.access.redhat.com). Corresponding test assertions expanded to verify these new policy entries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
openshift-ci
bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omer-vishlitzky The full list of commands accepted by this bot can be found here.
Needs 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: 0
🧹 Nitpick comments (1)
internal/controller/controllers/agentserviceconfig_controller.go (1)
2316-2413: Upgrade script behavior is robust but auto-wipe on repeated failures is riskyThe
postgresUpgradeScriptimplements a solid flow (attempt tracking, fresh-install short‑circuit, version checks, stalepostmaster.pidcleanup, upgrade viarun-postgresql, and readiness polling). Two aspects are worth double‑checking:
Data wipe after MAX_ATTEMPTS
After more thanMAX_ATTEMPTSthe script doesrm -rf /var/lib/pgsql/data/*and exits 0, assuming all data is safely reconstructible from Kubernetes CRs. If any user or operator data lives only in Postgres, or if repeated failures are caused by configuration issues (e.g., wrong MIN/TARGET or too‑short TIMEOUT for a legitimately large DB), this becomes destructive. Consider:
- Restricting auto‑wipe to clearly transient failure modes, or
- Gating it behind an explicit opt‑in flag/annotation, or
- Failing hard (non‑zero exit) once attempts are exhausted, leaving recovery to an operator.
Tight coupling between image and hardcoded MIN/TARGET versions
The script relies onPG_VERSIONmatchingPostgresMinVersion/PostgresTargetVersionconstants. If an operator overridesDATABASE_IMAGEto a different major without updating these constants in code, you can get:
- Unsupported version failures (
CURRENT_VERSION!=MIN_VERSION) that will still count towardsMAX_ATTEMPTSand eventually trigger the wipe.- Or a mismatch where the image’s actual major doesn’t align with the
TARGET_VERSIONencoded into the script.If custom
DATABASE_IMAGEoverrides are expected, consider tying the script’s version expectations to configuration (e.g., env‑based values or the new helpers inimages.go) and/or clearly documenting that the DB image and these constants must be kept in lockstep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
internal/controller/controllers/agentserviceconfig_controller.go(3 hunks)internal/controller/controllers/images.go(2 hunks)pkg/mirrorregistries/generator.go(1 hunks)pkg/mirrorregistries/mirror_registry_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/mirrorregistries/generator.gopkg/mirrorregistries/mirror_registry_test.gointernal/controller/controllers/images.gointernal/controller/controllers/agentserviceconfig_controller.go
🧬 Code graph analysis (1)
internal/controller/controllers/agentserviceconfig_controller.go (1)
internal/controller/controllers/images.go (3)
DatabaseImage(50-52)PostgresTargetVersion(33-33)PostgresMinVersion(34-34)
🔇 Additional comments (4)
pkg/mirrorregistries/generator.go (1)
122-132: Docker transport entries correctly extend insecure policy behaviorAdding the
dockertransport (default,registry.redhat.io, andregistry.access.redhat.com) withinsecureAcceptAnythingcleanly aligns policy.json with howpodman pullresolves transports and overrides per‑registry policies whenForceInsecurePolicyis enabled. The structure matches the existingdocker-daemonmap and looks sound.pkg/mirrorregistries/mirror_registry_test.go (1)
351-359: Test coverage accurately reflects new docker transport policyThe additional assertions for the
dockertransport (default,registry.redhat.io,registry.access.redhat.com) correctly mirror the generator behavior and will catch regressions in the policy layout.internal/controller/controllers/images.go (1)
30-36: Postgres defaults and accessors are consistent and maintainableCentralizing the PostgreSQL target/minimum versions and default image, and wiring
DatabaseImage()plus the new helper accessors throughgetEnvVar, keeps configuration consistent with the rest of the image helpers while still allowing overrides via env vars. No issues spotted.Also applies to: 51-60
internal/controller/controllers/agentserviceconfig_controller.go (1)
2067-2124: Postgres readinessProbe and upgrade initContainer wiring look correctThe postgres container’s
pg_isreadyExec readinessProbe and thepostgres-upgradeinitContainer both use the sameDatabaseImage()and PVC (postgresdb), and the initContainer is correctly wired viaInitContainers = []corev1.Container{postgresUpgradeInitContainer}. This should ensure upgrades run before the main DB comes up and that readiness reflects actual DB availability. No structural issues spotted here.Also applies to: 2253-2255
omer-vishlitzky
force-pushed
the
fix/ACM-21309-policy-json
branch
from
8af184a to
5d84da5
Compare
openshift-ci
bot
added
size/S
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8493 +/- ##
=======================================
Coverage 43.49% 43.50%
=======================================
Files 411 411
Lines 71076 71087 +11
=======================================
+ Hits 30916 30924 +8
- Misses 37406 37408 +2
- Partials 2754 2755 +1
🚀 New features to boost your workflow:
|
The previous fix (PR openshift#7807) was insufficient as it only configured the 'docker-daemon' transport. However, 'podman pull' uses the 'docker' transport. Additionally, RHCOS 4.19 ships with a restrictive /etc/containers/policy.json (see OCPBUGS-55106) that explicitly enforces signature verification for 'registry.redhat.io' and 'registry.access.redhat.com'. These specific scopes take precedence over the global 'default' insecure setting. This commit updates GenerateInsecurePolicyJSON to explicitly set 'insecureAcceptAnything' for the 'docker' transport, specifically overriding the entries for Red Hat registries to shadow system defaults.
omer-vishlitzky
force-pushed
the
fix/ACM-21309-policy-json
branch
from
5d84da5 to
efcced9
Compare
omer-vishlitzky
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@omer-vishlitzky: This pull request references ACM-27290 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 bug to target the "4.21.0" version, but no target version was set. In 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. |
|
@omer-vishlitzky: This pull request references ACM-27290 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 bug to target the "4.21.0" version, but no target version was set. In 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. |
|
@omer-vishlitzky: This pull request references ACM-27290 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 bug to target the "4.21.0" version, but no target version was set. In 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. |
| "docker": map[string]interface{}{ | ||
| "": []map[string]string{ | ||
| {"type": "insecureAcceptAnything"}, | ||
| }, | ||
| "registry.redhat.io": []map[string]string{ | ||
| {"type": "insecureAcceptAnything"}, | ||
| }, | ||
| "registry.access.redhat.com": []map[string]string{ | ||
| {"type": "insecureAcceptAnything"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I think it would be helpful to add some documentation here (or in the GenerateInsecurePolicyJSON docstring) to clarify the reasoning behind this behavior.
3 participants
https://issues.redhat.com/browse/ACM-27290
Summary
GenerateInsecurePolicyJSONto include thedockertransport.insecureAcceptAnythingforregistry.redhat.ioandregistry.access.redhat.com.Reason
The previous fix (#7807) was insufficient because:
docker-daemon(local) instead ofdocker(remote) transport used bypodman pull.defaultsetting. Explicitly defining the registry scopes is required to bypass these restrictions, as verified by the customer.