NO-ISSUE: resolve symlinked graph root in containers-storage policy key - #6389
NO-ISSUE: resolve symlinked graph root in containers-storage policy key#6389qxtaiba wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThe daemon resolves symlinked container storage graph roots before generating ChangesContainer storage policy handling
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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: qxtaiba 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @qxtaiba. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
hey @sergiordlr / @dkhater-redhat – let me know what i can do to get this filed properly. not sure how/where to file an OCP issue for this, but put up a fix if you want to review/merge and/or reimplement. i've added context in the linked issue and pr description, along with some tests to reproduce. |
c8447ec to
c9a7661
Compare
qxtaiba
changed the title
qxtaiba
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@qxtaiba: This pull request explicitly references no jira issue. 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. |
c9a7661 to
64f291a
Compare
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.github.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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/daemon/rpm-ostree.go`:
- Line 334: Update the fallback warning in the graph-root resolution flow to
avoid logging raw graphRoot and err values, which may expose host-specific data.
Keep the diagnostic message generic or pass values through the project’s
approved redaction helper before calling klog.V(2).Infof.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 50c6e656-c213-4a45-9b49-8a933d14c491
📒 Files selected for processing (3)
pkg/daemon/rpm-ostree.gopkg/daemon/rpm-ostree_test.gopkg/daemon/update_test.go
| func canonicalizeGraphRoot(graphRoot string) string { | ||
| resolved, err := filepath.EvalSymlinks(graphRoot) | ||
| if err != nil { | ||
| klog.V(2).Infof("Could not resolve container storage graph root %q, falling back to a cleaned path: %v", graphRoot, err) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not log the raw graph root or filesystem error.
graphRoot and err can contain user-specific storage paths, usernames, or other host data. Keep this diagnostic message generic or pass values through an approved redaction helper.
Proposed fix
- klog.V(2).Infof("Could not resolve container storage graph root %q, falling back to a cleaned path: %v", graphRoot, err)
+ klog.V(2).Infof("Could not resolve container storage graph root; falling back to a cleaned path")As per coding guidelines: **/*.{js,ts,go,java,py,rb,php,cs,cpp,c,sh}: Flag logging that may expose passwords, tokens, API keys, PII (email, SSN, credit card), session IDs, internal hostnames, or customer data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| klog.V(2).Infof("Could not resolve container storage graph root %q, falling back to a cleaned path: %v", graphRoot, err) | |
| klog.V(2).Infof("Could not resolve container storage graph root; falling back to a cleaned path") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/daemon/rpm-ostree.go` at line 334, Update the fallback warning in the
graph-root resolution flow to avoid logging raw graphRoot and err values, which
may expose host-specific data. Keep the diagnostic message generic or pass
values through the project’s approved redaction helper before calling
klog.V(2).Infof.
Source: Coding guidelines
2 participants
- What I did
I'm running into a bug where the
rpm-ostreerebase from local container storage fails withis rejected by policywhen the container store's graph root is a symlink. I found that the MCD writes a temporary allow rule sorpm-ostreecan rebase from local container storage under a restrictive policy. The rule is keyed on the container storage graph root, but the writer and the evaluator disagree about what that is.generateTransportPolicyKeyForReferenceinpkg/daemon/rpm-ostree.gotakes the graph root as the store reports it, unresolved:The symlink is deliberate here. The container store is relocated onto a separate disk and relocating the store is a supported configuration, so the daemon should not assume the default path.
The containers/storage path resolves the graph root with
EvalSymlinkswhen it opens the store, so the key we write and the keyPolicyConfigurationIdentity()returns differ, such that we write:Then what gets looked up:
The
PolicyConfigurationNamespaces()fn derives its fallback scopes from the same resolved root, so nothing matches and evaluation reaches the policy default. The rebase fails withis rejected by policy.This fix resolves the graph root before building the key, using the same
EvalSymlinks-with-Cleanfallback that containers/storage uses inexpandEnvPath, so the written key matches what the evaluator looks up.This PR fixes #6388
- How to verify it
- Description for the changelog
This PR resolves the container storage graph root symlink before building the transport policy key, fixing rpm-ostree rebase rejections from local storage.
Summary by CodeRabbit