OCPBUGS-80925: Remove the CRB for the default openshift-cluster-version SA#1366
OCPBUGS-80925: Remove the CRB for the default openshift-cluster-version SA#1366DavidHurta wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/test all |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting WalkthroughValidates and honors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DavidHurta 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 |
openshift-ci
bot
added
the
approved
DavidHurta
force-pushed
the
add-delete-annotation-to-default-crb
branch
from
5fe6918 to
f64426f
Compare
DavidHurta
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@DavidHurta: This pull request references Jira Issue OCPBUGS-80925, 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. |
To ensure the default SA does not have cluster admin permissions. To achieve this, rendering logic needs to be updated as well. During cluster bootstrap, the installer calls rendering commands of specific components required for the bootstrap [1]. These rendered manifests are then applied by the cluster-bootstrap component [2]. The cluster-bootstrap component applies all the non-bootstrap manifests as they are [3]. At no stage is the delete annotation [4] taken into account, and thus the CRB would keep getting applied during installations and getting removed only during cluster upgrades due to the annotation. This would prohibit us from ever removing the manifest file from the repository, as a freshly installed cluster upgrading to a version where manifest does not exist would result in the CRB being applied till manually removed, causing a security concern. Teach the rendering command to respect the delete annotation to allow us to remove such manifests. Add the delete annotation and move the CRB into a lower run-level where we can safely remove manifests and not take up space in the used levels. [1]: https://github.com/openshift/installer/blob/c93ae9fc74d7fa0f478fa250de2ba702f84a0a21/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template [2]: https://github.com/openshift/installer/blob/c93ae9fc74d7fa0f478fa250de2ba702f84a0a21/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L576 [3]: https://github.com/openshift/cluster-bootstrap/blob/b23c6ce3df43aed15158e999239694ec75371f18/pkg/start/start.go#L142 [4]: https://github.com/openshift/enhancements/blob/master/enhancements/update/object-removal-manifest-annotation.md
The installer renders the CVO bootstrap manifests into its bootstrap-manifests directory [1], where bootstrap manifests of other related componenets are rendered as well by their respective bootstrap commands. The directory is then consumed by the cluster-bootstrap component [2]. The cluster-bootstrap component copies these manifests to the static Pod path of the node's kubelet [3]. As such, static Pods have some notable details, such as: > The spec of a static Pod cannot refer to other API objects > (e.g., ServiceAccount, ConfigMap, Secret, etc). [4] Mention this in the manifest to save some time for future developers. [1]: https://github.com/openshift/installer/blob/c93ae9fc74d7fa0f478fa250de2ba702f84a0a21/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L192 [2]: https://github.com/openshift/installer/blob/c93ae9fc74d7fa0f478fa250de2ba702f84a0a21/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L576 [3]: https://github.com/openshift/cluster-bootstrap/blob/dc0d4a5cdaf8a7477cab584208dc99352f46efe2/pkg/start/bootstrap.go#L52-L60 [4]: https://kubernetes.io/docs/tasks/configure-pod-container/static-pod/
DavidHurta
force-pushed
the
add-delete-annotation-to-default-crb
branch
from
f64426f to
b7b86df
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/payload/render_test.go (1)
299-303: Prefer early-fail when an expected error is missing.Using
t.Fatalfhere avoids cascading assertion noise in inclusion/exclusion checks after a failed precondition.Optional tidy-up
if tt.expectError { if err == nil { - t.Errorf("expected error but got none") + t.Fatalf("expected error but got none") } } else { if err != nil { t.Fatalf("renderDir failed: %v", err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/payload/render_test.go` around lines 299 - 303, Replace the non-fatal assertion that checks for a missing expected error (currently using t.Errorf when tt.expectError is true and err == nil) with a fatal assertion so the test stops immediately; specifically change the t.Errorf("expected error but got none") to t.Fatalf(...) in the test where tt.expectError and err are checked (the block referencing tt.expectError and err) to prevent subsequent inclusion/exclusion assertions from running after the precondition fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/payload/render_test.go`:
- Around line 299-303: Replace the non-fatal assertion that checks for a missing
expected error (currently using t.Errorf when tt.expectError is true and err ==
nil) with a fatal assertion so the test stops immediately; specifically change
the t.Errorf("expected error but got none") to t.Fatalf(...) in the test where
tt.expectError and err are checked (the block referencing tt.expectError and
err) to prevent subsequent inclusion/exclusion assertions from running after the
precondition fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4447c6b7-2046-44f4-9be6-3c41a2db645f
📒 Files selected for processing (5)
bootstrap/bootstrap-pod.yamlcmd/cluster-version-operator/render.goinstall/0000_00_cluster-version-operator_90_roles-default.yamlpkg/payload/render.gopkg/payload/render_test.go
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 `@pkg/payload/render.go`:
- Around line 151-156: Reorder the validation so the delete-annotation is
checked before feature/profile/version exclusion: call
resourcedelete.ValidDeleteAnnotation(manifest.Obj.GetAnnotations()) first (using
the same error handling pattern that appends to errs with fmt.Errorf("invalid
delete annotation in %s from %s: %w", manifest.String(), file.Name(), err)),
handle the found==true case with the same klog.Infof exclusion message, and only
afterwards call manifest.Include(nil, requiredFeatureSet, clusterProfile, nil,
overrides, enabledFeatureGates, majorVersion); keep existing log messages and
error appending behavior but swap the order of
resourcedelete.ValidDeleteAnnotation and manifest.Include so malformed delete
annotations are always validated.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3fa4623d-f2a6-484c-801f-8489a6427297
📒 Files selected for processing (5)
bootstrap/bootstrap-pod.yamlcmd/cluster-version-operator/render.goinstall/0000_00_cluster-version-operator_90_roles-default.yamlpkg/payload/render.gopkg/payload/render_test.go
✅ Files skipped from review due to trivial changes (3)
- install/0000_00_cluster-version-operator_90_roles-default.yaml
- bootstrap/bootstrap-pod.yaml
- cmd/cluster-version-operator/render.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/payload/render_test.go
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai summary Also note the added delete annotation to remove the CRB. |
|
The PR removes the ClusterRoleBinding (CRB) for the default ✅ Actions performedSummary regeneration triggered. |
|
/jira refresh |
|
@DavidHurta: This pull request references Jira Issue OCPBUGS-80925, which is invalid:
Comment 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. |
|
/retest-required |
|
@DavidHurta: The following tests failed, say
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. |
2 participants
The PR aims to remove the default CRB by adding the delete annotation and updating the rendering logic to respect the annotation.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests