OCPBUGS-84955: Add render-sensitive flag to hypershift install render #8436
OCPBUGS-84955: Add render-sensitive flag to hypershift install render #8436clebs wants to merge 3 commits intoopenshift:mainfrom
hypershift install render #8436Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/invalid-bug
|
@clebs: This pull request references Jira Issue OCPBUGS-84955, which is invalid:
Comment 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. |
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughA new boolean Options field Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as InstallCmd
participant Renderer
participant Store as ManifestObjects
participant Output
User->>CLI: invoke install command (flags: --template?, --render-sensitive?)
CLI->>Renderer: render(Template, RenderSensitive)
alt Template true and RenderSensitive false
Renderer-->>CLI: return error "templates require --render-sensitive when secrets are present"
CLI-->>User: print error
else
Renderer->>Store: load manifests (Secrets + other objects)
alt RenderSensitive false
Renderer->>Store: filter out Secret objects
end
Renderer->>Output: emit rendered manifests
Output-->>User: write manifests
end
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci
Bot
added
area/cli
|
/area cli |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@clebs: This pull request references Jira Issue OCPBUGS-84955, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@clebs: This pull request references Jira Issue OCPBUGS-84955, which is valid. 3 validation(s) were run on this bug
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. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: clebs, patjlm 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
removed
the
do-not-merge/work-in-progress
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8436 +/- ##
==========================================
+ Coverage 37.44% 37.46% +0.02%
==========================================
Files 751 751
Lines 91969 91980 +11
==========================================
+ Hits 34435 34461 +26
+ Misses 54894 54881 -13
+ Partials 2640 2638 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/install/install_render.go`:
- Around line 135-144: The current filter only removes top-level *corev1.Secret
entries from objectsToRender when opts.RenderSensitive is false but misses
secrets embedded inside Template objects; update the filtering loop to detect
template objects (the Template type used by the codebase, e.g.
templatev1.Template or the project's Template wrapper) and either remove any
inner *corev1.Secret entries from the template's Objects slice or skip emitting
the entire Template if it contains secrets; ensure you still build a new
filtered objectsToRender slice and reference opts.RenderSensitive,
objectsToRender, corev1.Secret and the Template type/name to locate and modify
the logic.
In `@cmd/install/install_test.go`:
- Around line 514-558: The test is bypassing the real rendering by calling
hyperShiftOperatorManifests and re-implementing secret filtering; replace that
with an end-to-end call to RenderHyperShiftOperator using the same Options (pass
tc.renderSensitive) and assert directly on the returned objects so the
sensitive-rendering flag behavior is exercised; remove the manual filtering
logic (the loop that builds rendered and renderedSecrets) and instead inspect
the objects slice returned by RenderHyperShiftOperator for corev1.Secret
instances and their names, then use g.Expect on that result according to
tc.expectSecrets.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6f7dd5cc-ab34-4576-bc90-82d8f68906d1
📒 Files selected for processing (3)
cmd/install/install.gocmd/install/install_render.gocmd/install/install_test.go
|
New changes are detected. LGTM label has been removed. |
Introduce the --render-sensitive flag to the hypershift install render command to avoid leaking secrets. This is now needed because the new webhookcerts controller introduces secrets that should not be rendered by default. Signed-off-by: Borja Clemente <bclement@redhat.com>
The hypershift install render --template command still leaks secrets after introducing the --render-sensitive flag. This makes it required to have the flag set to true to acknowledge that the template contains sensitive data. Signed-off-by: Borja Clemente <bclement@redhat.com>
clebs
force-pushed
the
install-render-sensitive
branch
from
db4c56f to
c41aeae
Compare
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 `@cmd/install/install_test.go`:
- Around line 529-544: The test currently swallows Decode errors from
hyperapi.YamlSerializer.Decode while iterating over strings.SplitSeq output,
which can hide parsing failures; update the loop that builds secretNames to fail
the test on any non-nil err returned by hyperapi.YamlSerializer.Decode (use
g.Expect(err).ToNot(HaveOccurred() or equivalent) so decode errors fail fast)
and additionally assert that at least one manifest document was decoded (e.g.,
track a decodedCount and g.Expect(decodedCount).To(BeNumerically(">", 0)) before
checking tc.expectSecrets) so the test cannot pass vacuously when parsing fails.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1425c809-dd28-4dbd-adbc-7505f933e0f5
📒 Files selected for processing (4)
cmd/install/install.gocmd/install/install_render.gocmd/install/install_render_test.gocmd/install/install_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/install/install_test.go (1)
549-553: ⚡ Quick winRemove duplicated secret assertion block.
Lines 549-553 repeat the same
tc.expectSecretsassertions already done at Lines 543-547. Keeping one block avoids redundant checks and future drift.♻️ Proposed cleanup
- if tc.expectSecrets { - g.Expect(secretNames).ToNot(BeEmpty(), "expected secrets in rendered output") - } else { - g.Expect(secretNames).To(BeEmpty(), "expected no secrets in rendered output") - } }) } }🤖 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 `@cmd/install/install_test.go` around lines 549 - 553, Remove the duplicated assertion block that checks tc.expectSecrets against secretNames (the second g.Expect block using secretNames and tc.expectSecrets); keep the original assertions (the first g.Expect checks on secretNames at the earlier block) and delete the repeated g.Expect(...) else branch starting with "if tc.expectSecrets { g.Expect(secretNames)..." to avoid redundant checks and drift.
🤖 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.
Nitpick comments:
In `@cmd/install/install_test.go`:
- Around line 549-553: Remove the duplicated assertion block that checks
tc.expectSecrets against secretNames (the second g.Expect block using
secretNames and tc.expectSecrets); keep the original assertions (the first
g.Expect checks on secretNames at the earlier block) and delete the repeated
g.Expect(...) else branch starting with "if tc.expectSecrets {
g.Expect(secretNames)..." to avoid redundant checks and drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d3a36dcb-290e-4404-ac37-3eecf3a0a0bf
📒 Files selected for processing (1)
cmd/install/install_test.go
Fail on decode errors and assert at least one manifest was decoded.
clebs
force-pushed
the
install-render-sensitive
branch
from
6c6bd93 to
2376a26
Compare
clebs
changed the title
hypershift install render
3 participants
What this PR does / why we need it:
Introduce the --render-sensitive flag to the hypershift install render command to avoid leaking secrets.
This is now needed because the new webhookcerts controller introduces secrets that should not be rendered by default.
Which issue(s) this PR fixes:
Fixes OCPBUGS-84955
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Tests
Chores