MGMT-23545: Mount pull secret token as a file#2053
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pastequo: This pull request references MGMT-23545 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 task to target the "4.22.0" version, but no target version was set. 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughSwitched pull-secret delivery from an environment variable to a file-mounted Kubernetes secret. Deployment manifest mounts the secret, the config struct tag no longer binds to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/L
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.template`:
- Around line 112-114: The secret volume used by the pod (volume with name
"pull-secret" and secretName "assisted-installer-controller-secret") must be
marked optional so parsePullSecretToken() in assisted_installer_main.go can fall
back to the PULL_SECRET_TOKEN env var; update the secret volume definition by
adding optional: true under the secret: block for the "pull-secret" volume to
allow the pod to start when the secret is absent and let parsePullSecretToken()
perform the env fallback.
In `@src/main/assisted-installer-controller/assisted_installer_main.go`:
- Around line 350-354: The parsePullSecretToken function currently returns on
any file read error which prevents falling back to the PULL_SECRET_TOKEN
environment variable; update parsePullSecretToken to only treat a non-existent
file (os.IsNotExist(err)) or empty file content as a trigger to read and return
os.Getenv("PULL_SECRET_TOKEN"), while still returning other read errors (i.e.,
permission or IO errors) immediately; also trim whitespace from the file content
before returning and apply the same fallback logic to the analogous code
handling pull secret parsing around the referenced 361-369 region so both code
paths honor the environment variable when the file is missing or empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54b47abf-28f7-4d81-86c0-7daaeb9ee9ec
📒 Files selected for processing (4)
deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.templatesrc/assisted_installer_controller/assisted_installer_controller.gosrc/main/assisted-installer-controller/assisted_installer_main.gosrc/main/assisted-installer-controller/assisted_installer_main_test.go
deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.template
Show resolved
Hide resolved
src/main/assisted-installer-controller/assisted_installer_main.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2053 +/- ##
==========================================
+ Coverage 48.79% 48.82% +0.03%
==========================================
Files 20 20
Lines 4382 4397 +15
==========================================
+ Hits 2138 2147 +9
- Misses 2020 2026 +6
Partials 224 224
🚀 New features to boost your workflow:
|
pastequo
force-pushed
the
feat/mount-secrets-as-file-not-env-vars
branch
from
f330bd2 to
17b2d3a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.template (1)
112-114:⚠️ Potential issue | 🟠 MajorMake the
pull-secretvolume optional so env fallback can actually execute.On Line 114, the secret volume is currently mandatory. If the secret is missing, Kubernetes blocks pod startup, so
parsePullSecretToken()never reaches itsPULL_SECRET_TOKENfallback path.Proposed manifest fix
- name: pull-secret secret: secretName: assisted-installer-controller-secret + optional: trueUse this to verify the mismatch (required volume vs runtime fallback):
#!/bin/bash set -euo pipefail echo "== pull-secret volume block ==" sed -n '108,118p' deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.template echo echo "== parsePullSecretToken fallback block ==" sed -n '350,377p' src/main/assisted-installer-controller/assisted_installer_main.goExpected result: volume block shows no
optional: true, while Go code shows fallback when file is missing/empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.template` around lines 112 - 114, The pull-secret secret volume is currently required which prevents the pod from starting when the secret is absent and blocks parsePullSecretToken() from using its PULL_SECRET_TOKEN fallback; update the manifest's volume entry for the "pull-secret" secretName assisted-installer-controller-secret to make the secret optional (add optional: true under the secret block) so the container can start without the secret and parsePullSecretToken() can execute its runtime fallback logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.template`:
- Around line 112-114: The pull-secret secret volume is currently required which
prevents the pod from starting when the secret is absent and blocks
parsePullSecretToken() from using its PULL_SECRET_TOKEN fallback; update the
manifest's volume entry for the "pull-secret" secretName
assisted-installer-controller-secret to make the secret optional (add optional:
true under the secret block) so the container can start without the secret and
parsePullSecretToken() can execute its runtime fallback logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6e06692-1eee-4943-80ff-5000d05e662a
📒 Files selected for processing (4)
deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.templatesrc/assisted_installer_controller/assisted_installer_controller.gosrc/main/assisted-installer-controller/assisted_installer_main.gosrc/main/assisted-installer-controller/assisted_installer_main_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- src/assisted_installer_controller/assisted_installer_controller.go
- src/main/assisted-installer-controller/assisted_installer_main_test.go
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Im holding it because I need to figure out if it's okay to have an empty secret On my local test (sno) it seems to be the case |
pastequo
force-pushed
the
feat/mount-secrets-as-file-not-env-vars
branch
from
17b2d3a to
f3bf117
Compare
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
1 similar comment
|
/retest |
|
/override ci/prow/edge-e2e-ai-operator-ztp |
|
@pastequo: Overrode contexts on behalf of pastequo: ci/prow/edge-e2e-ai-operator-ztp 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 kubernetes-sigs/prow repository. |
|
|
||
| if err != nil { | ||
| switch { | ||
| case os.IsNotExist(err): |
There was a problem hiding this comment.
So if we have wrong permissions or empty file, we do not fallback to env var? Is this intended?
There was a problem hiding this comment.
It's definitively arguable, but I think so. If the user (or something else) didn't configure everything correctly, I would rather fail "fast" instead of trying to make it work. I think it could be misleading for the user
PS empty file is not an error, we can have an empty environment variable today (maybe just for the dev stack tho)
| path: /etc/resolv.conf | ||
| - name: pull-secret | ||
| secret: | ||
| secretName: assisted-installer-controller-secret |
There was a problem hiding this comment.
items:
- key: pull-secret-token
path: token
defaultMode: 0400
shouldn't we add that? This way we would make sure the key matches at deploy time, the error would be obvious if they doesn't match (not present in the secret). WDYT?
There was a problem hiding this comment.
it shouldn't hurt, I will give a try
There was a problem hiding this comment.
I installed a SNO cluster with my local setup, it was successful even if the token was empty
I analyzed the pod spec, it contains "runAsUser" at the container spec level (and also "fsGroup" at the pod spec level, but it's not used here), so AFAIU it is guarantee that the container will run with that user and that the file will be owned by that user. So 0400 makes sense to me
pastequo
force-pushed
the
feat/mount-secrets-as-file-not-env-vars
branch
from
f3bf117 to
9ea2a1f
Compare
pastequo
force-pushed
the
feat/mount-secrets-as-file-not-env-vars
branch
from
9ea2a1f to
48ec42d
Compare
|
@pastequo: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pastequo, rccrdpccl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Mount pull secret token as a file
There is still a fallback to the environment variable in the code that tries to get the value of this secret