OCPBUGS-97605: Use static ServiceAccount for operand#535
Conversation
openshift-ci-robot
added
jira/severity-low
|
@bentito: This pull request references Jira Issue OCPBUGS-97605, 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. |
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe ExternalDNS controller now uses a fixed operand ServiceAccount name for lookups and desired resources. Tests, source RBAC, and generated RBAC manifests reflect the explicit ServiceAccount subject in the 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
openshift-ci
Bot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/jira refresh |
When the operator is installed in a custom namespace, OLM fails to rewrite
Group subjects in the ClusterRoleBinding. By standardizing the operand to use
a single static ServiceAccount ('external-dns') and referencing it as a
ServiceAccount subject, OLM successfully dynamically rewrites the namespace
field during custom namespace installs, allowing operand pods to function correctly.
openshift-ci-robot
added
jira/valid-bug
|
@bentito: This pull request references Jira Issue OCPBUGS-97605, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
84d558d to
80a5ece
Compare
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/controller/externaldns/service_account.go (1)
35-40: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid controller ownership on the shared ServiceAccount.
ExternalDNSOperandServiceAccountName()is static across all ExternalDNS instances, but this path still sets the current CR as the controller owner. If more than one ExternalDNS exists, deleting one can garbage-collect the shared ServiceAccount and break the others. Use shared ownership semantics or enforce a singleton CR.🤖 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/operator/controller/externaldns/service_account.go` around lines 35 - 40, The service account identified by ExternalDNSOperandServiceAccountName is shared across ExternalDNS instances, so do not assign the current CR as its controller owner in the reconcile path. Update the ownership handling around SetControllerReference to use shared, non-controller ownership semantics, or enforce a singleton ExternalDNS resource before applying controller ownership.
🧹 Nitpick comments (1)
hack/golangci-lint.sh (1)
40-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnsure temporary directory is cleaned up on verification failure.
If the checksum verification fails, the script will exit immediately due to the
set -edirective, bypassing the manualrm -rf "$TEMPDIR"command at the end of the script. This will result in a leaked temporary directory. Consider using atrapto guarantee cleanup regardless of how the script terminates.Add a trap immediately after the temporary directory is created, and remove the manual cleanup at the bottom of the script:
TEMPDIR=$(mktemp -d) trap 'rm -rf "$TEMPDIR"' EXIT🤖 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 `@hack/golangci-lint.sh` around lines 40 - 44, Add an EXIT trap immediately after TEMPDIR is created to remove the temporary directory on all termination paths, including checksum verification failures. Remove the manual rm -rf cleanup at the script’s end while preserving the existing verification logic.
🤖 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 `@config/rbac/operand_rolebinding.yaml`:
- Around line 11-13: The ClusterRoleBinding still grants access to every
ServiceAccount in the external-dns-operator namespace, defeating the fixed
least-privilege ServiceAccount subject. Remove or explicitly justify the
system:serviceaccounts:external-dns-operator Group in
config/rbac/operand_rolebinding.yaml (anchor, lines 11-13), then apply the same
RBAC correction in
bundle/manifests/external-dns_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml
(sibling, lines 11-13), preserving access only for the intended external-dns
ServiceAccount.
---
Outside diff comments:
In `@pkg/operator/controller/externaldns/service_account.go`:
- Around line 35-40: The service account identified by
ExternalDNSOperandServiceAccountName is shared across ExternalDNS instances, so
do not assign the current CR as its controller owner in the reconcile path.
Update the ownership handling around SetControllerReference to use shared,
non-controller ownership semantics, or enforce a singleton ExternalDNS resource
before applying controller ownership.
---
Nitpick comments:
In `@hack/golangci-lint.sh`:
- Around line 40-44: Add an EXIT trap immediately after TEMPDIR is created to
remove the temporary directory on all termination paths, including checksum
verification failures. Remove the manual rm -rf cleanup at the script’s end
while preserving the existing verification logic.
🪄 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: eb9949b5-ae8b-48e9-ad24-76669777d883
📒 Files selected for processing (7)
bundle/manifests/external-dns_rbac.authorization.k8s.io_v1_clusterrolebinding.yamlconfig/rbac/operand_rolebinding.yamlhack/golangci-lint.shpkg/operator/controller/externaldns/controller_test.gopkg/operator/controller/externaldns/service_account.gopkg/operator/controller/externaldns/service_account_test.gopkg/operator/controller/names.go
|
@bentito: 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. |
|
@bentito: We never really declared (== tested) the support of the installation of EDO outside of the suggested namespace. The install mode is own and single namespace and we always suggested |
Sorry about delayed response. @alebedev87 so are you suggesting just reject the bug as not a bug? |
3 participants
When the operator is installed in a custom namespace, OLM fails to rewrite Group subjects in the ClusterRoleBinding. By standardizing the operand to use a single static ServiceAccount ('external-dns') and referencing it as a ServiceAccount subject, OLM successfully dynamically rewrites the namespace field during custom namespace installs, allowing operand pods to function correctly.
This aligns with Red Hat's philosophy that operators should be namespace-agnostic and resolves the issue where OLM fails to rewrite Group subjects for custom namespace installations.
Includes a build fix to allow the golangci-lint script to run on Apple Silicon.
Resolves: https://issues.redhat.com/browse/OCPBUGS-97605