OCPBUGS-65622: add controller for downloads service account deletion logic#1111
OCPBUGS-65622: add controller for downloads service account deletion logic#1111ehearne-redhat wants to merge 8 commits intoopenshift:mainfrom
Conversation
ehearne-redhat
changed the title
openshift-ci-robot
added
jira/severity-critical
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-65622, 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: 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. |
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
from
5c08afe to
7ba9a63
Compare
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-65622, which is valid. 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. |
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
from
7ba9a63 to
0504779
Compare
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
Thanks @ehearne-redhat for the PR. I would suggest to implement the SA logic into the downloadsdeployment controller since the logic is basically similar. Maybe we could rename it just to downloads folder wise.
We might need to be cautions with the DownloadsDeploymentSync condition if renaming, cause since we would need to add it into the StaleConditionsController.
|
@jhadvig thanks for the review! I may also need to add manual garbage collection for the |
|
@ehearne-redhat thanks for the changes. I was thinking about the change since we are changing the management process for downloads SA we should do a similar thing for the console SA.
I apologise for not suggesting this approach on the first pass, but I believe this is the idiomatic implementation. |
|
@jhadvig that's okay - I have moved everything over as asked. I am trying to figure out why the controller value in objectreference is being set to true when multiple ownerrefs exist, even though the code should ensure that is not the case. 🤔 |
|
/retest |
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
from
ab9c36f to
9a3e679
Compare
|
/retest |
|
Hey @jhadvig - all tests are passing. Could you take a look again, and let me know if this was what you were looking for? Then, I can tidy up commits. :) |
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
@ehearne-redhat thank you so much for addressing the comments. Adding a few suggestiongs. PTAL
|
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:
Note
|
| Cohort / File(s) | Summary |
|---|---|
ServiceAccount Manifests bindata/assets/serviceaccounts/console-sa.yaml, bindata/assets/serviceaccounts/downloads-sa.yaml |
Add dedicated console and downloads ServiceAccount YAMLs with include.release.openshift.io annotations and capability metadata. |
Deployment Configuration bindata/assets/deployments/downloads-deployment.yaml |
Moved serviceAccountName: "downloads" from Deployment.spec into Deployment.spec.template.spec so pods use the downloads SA. |
RBAC & Manifest Cleanup manifests/03-rbac-role-cluster.yaml, manifests/06-sa.yaml |
Added ClusterRole rules for serviceaccounts (list/watch/get/create; update/delete for resourceNames console,downloads); removed inline console and downloads SA entries from manifests/06-sa.yaml. |
API Constants pkg/api/api.go |
Added exported constants OpenShiftConsoleDownloadsServiceAccountName and OpenShiftConsoleServiceAccountName mapped to existing deployment name constants. |
ServiceAccount Controller pkg/console/controllers/serviceaccounts/controller.go |
New ServiceAccountSyncController with Sync, SyncServiceAccount, and removeServiceAccount to apply or delete SA manifests, manage ownerRefs, and react to Console management state. |
Integration & Startup pkg/console/starter/starter.go |
Wired and started two ServiceAccountSyncController instances (console and downloads) in operator startup and added them to the run list. |
Minor Edits & Tests pkg/console/controllers/downloadsdeployment/controller.go, pkg/console/subresource/deployment/deployment_test.go |
Small comment change in downloads deployment controller; test updated to set serviceAccountName: "downloads" in the downloads deployment PodSpec. |
Sequence Diagram(s)
sequenceDiagram
participant Operator as Operator (Run)
participant SAController as ServiceAccountSyncController
participant Config as Console Config
participant SAClient as ServiceAccount Client
participant ResourceApply as resourceapply
Operator->>SAController: Sync(ctx, controllerContext)
activate SAController
SAController->>Config: Get Console config
Config-->>SAController: Console config
alt managementState == Managed
SAController->>ResourceApply: Load SA manifest from assets
ResourceApply-->>SAController: SA resource
SAController->>SAController: Set owner refs (Console as controller)
SAController->>SAClient: Apply ServiceAccount
SAClient-->>SAController: Applied
else managementState == Removed
SAController->>SAClient: Delete ServiceAccount
SAClient-->>SAController: Deleted / NotFound
else managementState == Unmanaged
SAController-->>SAController: Skip sync
end
SAController-->>Operator: return (error if any)
deactivate SAController
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title specifically references the main objective of the PR: adding a controller for downloads service account deletion logic, which is directly reflected in the substantial code changes introducing the ServiceAccountSyncController. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
📝 Coding Plan
- Generate coding plan for human review comments
Comment @coderabbitai help to get the list of available commands and usage tips.
|
Thanks Jakub - I will re-ping once tests pass. I have made the requested changes. :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
pkg/console/subresource/deployment/deployment_test.go (1)
1612-1612: Prefer API constant for the service account name in test fixtures.Using the shared constant avoids drift if naming changes later.
♻️ Suggested tweak
- ServiceAccountName: "downloads", + ServiceAccountName: api.OpenShiftConsoleDownloadsServiceAccountName,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/console/subresource/deployment/deployment_test.go` at line 1612, The test fixture uses the literal service account name "downloads"; replace this literal with the shared API constant (e.g., api.ServiceAccountNameDownloads or the appropriate exported constant in the api package) so the test uses the canonical service account identifier; update the ServiceAccountName field in the deployment test (where ServiceAccountName: "downloads") to reference that API constant.manifests/03-rbac-role-cluster.yaml (1)
151-174: Consider namespace-scoping ServiceAccount permissions for tighter least privilege.The
console-operatorClusterRole is bound cluster-wide via ClusterRoleBinding, which means the list/watch/get/create permissions on serviceaccounts apply across all namespaces. The update/delete operations are properly least-privilege scoped to resourceNamesconsoleanddownloads. If the operator only needs to manage ServiceAccounts in specific namespaces (e.g.,openshift-console), move the SA list/watch/get/create rules to namespace-scoped Roles instead of the ClusterRole.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests/03-rbac-role-cluster.yaml` around lines 151 - 174, The ClusterRole "console-operator" currently grants list/watch/get/create on ServiceAccounts cluster-wide; remove those list/watch/get/create rules from the ClusterRole and instead create a namespace-scoped Role (e.g., "console-operator-sa-access") in the target namespace(s) such as "openshift-console" that grants verbs list, watch, get, create on serviceaccounts, then add a RoleBinding that binds the operator's ServiceAccount to that Role in each namespace; keep the existing update/delete rules with resourceNames "console" and "downloads" in the ClusterRole for any cluster-scoped needs and ensure your ClusterRoleBinding only grants the remaining cluster-scoped permissions to the operator.pkg/console/controllers/serviceaccounts/controller.go (3)
134-136: Minor: Inconsistent error message formatting.The error message uses a capital "No" (should be lowercase for consistency with Go conventions) and has an extra space before the period.
📝 Suggested fix
- return fmt.Errorf("No service account found for name %v .", c.serviceAccountName) + return fmt.Errorf("no service account found for name %s", c.serviceAccountName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/console/controllers/serviceaccounts/controller.go` around lines 134 - 136, Update the error string returned when serviceAccount.Name == "" in controller.go (the if block referencing serviceAccount.Name and c.serviceAccountName) to use a lowercase "no" and remove the extra space before the period; e.g. change the fmt.Errorf message to "no service account found for name %v." (optionally use %q for clearer quoting of c.serviceAccountName).
40-40: Unused field:infrastructureListeris stored but never accessed.The
infrastructureListeris initialized in the constructor (line 69) but no method reads from it. If infrastructure config changes should trigger reconciliation only (via the informer on line 81), consider removing the stored lister to reduce confusion. Otherwise, if it's needed for future logic, add a TODO comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/console/controllers/serviceaccounts/controller.go` at line 40, The struct field infrastructureLister is unused: either remove the infrastructureLister field from the controller struct and delete its initialization in NewController (and any related imports) to avoid dead state, or if you intend to use it later, keep the field but add a clear TODO comment above the infrastructureLister declaration and a short inline comment in NewController explaining why it's stored; reference the field name infrastructureLister and the constructor function NewController to locate and update the code accordingly.
86-86: Replace deprecatedstrings.Titlewithgolang.org/x/text/cases.Title.
strings.Titleis deprecated since Go 1.18 and can trigger linter warnings. Although it works for ASCII-only controller names, use the modern alternative from thegolang.org/x/textpackage (already a project dependency).Add imports:
import "golang.org/x/text/cases" import "golang.org/x/text/language"Then update line 86:
-ToController(fmt.Sprintf("%sServiceAccountController", strings.Title(controllerName)), ...) +ToController(fmt.Sprintf("%sServiceAccountController", cases.Title(language.English).String(controllerName)), ...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/console/controllers/serviceaccounts/controller.go` at line 86, Replace deprecated strings.Title usage in the ToController call: import "golang.org/x/text/cases" and "golang.org/x/text/language", then change the name construction in the call that uses strings.Title(controllerName) (found in the ToController call that builds "%sServiceAccountController" and recorder.WithComponentSuffix) to use cases.Title(language.Und).String(controllerName) so the controller name is formed with the modern title-casing API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@manifests/03-rbac-role-cluster.yaml`:
- Around line 151-174: The ClusterRole "console-operator" currently grants
list/watch/get/create on ServiceAccounts cluster-wide; remove those
list/watch/get/create rules from the ClusterRole and instead create a
namespace-scoped Role (e.g., "console-operator-sa-access") in the target
namespace(s) such as "openshift-console" that grants verbs list, watch, get,
create on serviceaccounts, then add a RoleBinding that binds the operator's
ServiceAccount to that Role in each namespace; keep the existing update/delete
rules with resourceNames "console" and "downloads" in the ClusterRole for any
cluster-scoped needs and ensure your ClusterRoleBinding only grants the
remaining cluster-scoped permissions to the operator.
In `@pkg/console/controllers/serviceaccounts/controller.go`:
- Around line 134-136: Update the error string returned when serviceAccount.Name
== "" in controller.go (the if block referencing serviceAccount.Name and
c.serviceAccountName) to use a lowercase "no" and remove the extra space before
the period; e.g. change the fmt.Errorf message to "no service account found for
name %v." (optionally use %q for clearer quoting of c.serviceAccountName).
- Line 40: The struct field infrastructureLister is unused: either remove the
infrastructureLister field from the controller struct and delete its
initialization in NewController (and any related imports) to avoid dead state,
or if you intend to use it later, keep the field but add a clear TODO comment
above the infrastructureLister declaration and a short inline comment in
NewController explaining why it's stored; reference the field name
infrastructureLister and the constructor function NewController to locate and
update the code accordingly.
- Line 86: Replace deprecated strings.Title usage in the ToController call:
import "golang.org/x/text/cases" and "golang.org/x/text/language", then change
the name construction in the call that uses strings.Title(controllerName) (found
in the ToController call that builds "%sServiceAccountController" and
recorder.WithComponentSuffix) to use
cases.Title(language.Und).String(controllerName) so the controller name is
formed with the modern title-casing API.
In `@pkg/console/subresource/deployment/deployment_test.go`:
- Line 1612: The test fixture uses the literal service account name "downloads";
replace this literal with the shared API constant (e.g.,
api.ServiceAccountNameDownloads or the appropriate exported constant in the api
package) so the test uses the canonical service account identifier; update the
ServiceAccountName field in the deployment test (where ServiceAccountName:
"downloads") to reference that API constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f111efba-42f6-498f-bbf8-ca1719b5be08
📒 Files selected for processing (10)
bindata/assets/deployments/downloads-deployment.yamlbindata/assets/serviceaccounts/console-sa.yamlbindata/assets/serviceaccounts/downloads-sa.yamlmanifests/03-rbac-role-cluster.yamlmanifests/06-sa.yamlpkg/api/api.gopkg/console/controllers/downloadsdeployment/controller.gopkg/console/controllers/serviceaccounts/controller.gopkg/console/starter/starter.gopkg/console/subresource/deployment/deployment_test.go
💤 Files with no reviewable changes (1)
- manifests/06-sa.yaml
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
from
d75e52d to
72b2a70
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/console/controllers/serviceaccounts/controller.go (1)
129-147:⚠️ Potential issue | 🟠 MajorRe-add the Console controller ownerRef before apply.
Line 129 passes
operatorConfigCopy, but Lines 137-147 only clear existingControllerflags and never attach a controller ownerRef. This can leave the ServiceAccount ownerless from a controller perspective.Proposed fix
func (c *ServiceAccountSyncController) SyncServiceAccount(ctx context.Context, operatorConfigCopy *operatorv1.Console, controllerContext factory.SyncContext) error { @@ ownerRefs := serviceAccount.GetOwnerReferences() for oR := range ownerRefs { falseBool := false ownerRefs[oR].Controller = &falseBool } + ownerRef := metav1.NewControllerRef(operatorConfigCopy, operatorv1.GroupVersion.WithKind("Console")) + ownerRefs = append(ownerRefs, *ownerRef) serviceAccount.SetOwnerReferences(ownerRefs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/console/controllers/serviceaccounts/controller.go` around lines 129 - 147, ServiceAccount ownerRefs are cleared but a Console controller ownerRef is never re-attached; in SyncServiceAccount build and append a metav1.OwnerReference for the Console using operatorConfigCopy (set Name from operatorConfigCopy.GetName(), UID from operatorConfigCopy.GetUID(), Kind "Console" and the correct APIVersion for operatorv1) with Controller=true and BlockOwnerDeletion=true, then set that ownerRef on serviceAccount before calling the apply logic; update the code in SyncServiceAccount (serviceAccount, ownerRefs, operatorConfigCopy) to create and append this controller ownerRef.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/console/controllers/serviceaccounts/controller.go`:
- Around line 129-147: ServiceAccount ownerRefs are cleared but a Console
controller ownerRef is never re-attached; in SyncServiceAccount build and append
a metav1.OwnerReference for the Console using operatorConfigCopy (set Name from
operatorConfigCopy.GetName(), UID from operatorConfigCopy.GetUID(), Kind
"Console" and the correct APIVersion for operatorv1) with Controller=true and
BlockOwnerDeletion=true, then set that ownerRef on serviceAccount before calling
the apply logic; update the code in SyncServiceAccount (serviceAccount,
ownerRefs, operatorConfigCopy) to create and append this controller ownerRef.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a26fd2a2-0e32-41a7-8a0f-d87ee172ab7e
📒 Files selected for processing (4)
manifests/03-rbac-role-cluster.yamlpkg/api/api.gopkg/console/controllers/serviceaccounts/controller.gopkg/console/starter/starter.go
🚧 Files skipped from review as they are similar to previous changes (1)
- manifests/03-rbac-role-cluster.yaml
|
@coderabbitai I cannot add an additional ownerref to the controller using event happened 193 times, something is wrong: namespace/openshift-console-operator deployment/console-operator hmsg/7e9628ad2d - reason/ServiceAccountUpdateFailed Failed to update ServiceAccount/console -n openshift-console: ServiceAccount "console" is invalid: metadata.ownerReferences: Invalid value: [{"apiVersion":"config.openshift.io/v1","kind":"ClusterVersion","name":"version","uid":"5764b29f-7749-4012-94ce-d8c3d4e72991","controller":true},{"apiVersion":"operator.openshift.io/v1","kind":"Console","name":"cluster","uid":"44ea12d6-4e25-47cb-916f-5127bf971ea2","controller":true}]: Only one reference can have Controller set to true. Found "true" in references for ClusterVersion/version and Console/cluster (17:36:30Z) result=reject
event happened 192 times, something is wrong: namespace/openshift-console-operator deployment/console-operator hmsg/9ba732befd - reason/ServiceAccountUpdateFailed Failed to update ServiceAccount/downloads -n openshift-console: ServiceAccount "downloads" is invalid: metadata.ownerReferences: Invalid value: [{"apiVersion":"config.openshift.io/v1","kind":"ClusterVersion","name":"version","uid":"5764b29f-7749-4012-94ce-d8c3d4e72991","controller":true},{"apiVersion":"operator.openshift.io/v1","kind":"Console","name":"cluster","uid":"44ea12d6-4e25-47cb-916f-5127bf971ea2","controller":true}]: Only one reference can have Controller set to true. Found "true" in references for ClusterVersion/version and Console/cluster (17:36:30Z) result=reject }However, the logic in the service account controller should ensure that even if we used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me examine the service account controller code to understand the ownerReference logic. 🏁 Script executed: Length of output: 6090 Let me dig deeper into how 🏁 Script executed: Length of output: 7926 Now let me check how 🏁 Script executed: Length of output: 195 🏁 Script executed: Length of output: 7485
Root causeThe clearing loop in ownerRefs := serviceAccount.GetOwnerReferences() // empty — YAML has no ownerRefs
for oR := range ownerRefs { // iterates over nothing
ownerRefs[oR].Controller = &falseBool // never executed
}So the clearing does nothing. Meanwhile, // It only iterates over ownerRefs in *required* (the manifest SA — empty list)
for _, o := range required { // required is empty, loop body never runs
...
}Because FixYou need to fetch the live SA from the cluster, clear func (c *ServiceAccountSyncController) SyncServiceAccount(ctx context.Context, operatorConfigCopy *operatorv1.Console, controllerContext factory.SyncContext) error {
serviceAccount := resourceread.ReadServiceAccountV1OrDie(
bindata.MustAsset(fmt.Sprintf("assets/serviceaccounts/%s-sa.yaml", c.serviceAccountName)),
)
if serviceAccount.Name == "" {
return fmt.Errorf("No service account found for name %v .", c.serviceAccountName)
}
// Fetch the live SA to get its actual ownerRefs (e.g. ClusterVersion/version
// set by CVO from the old manifests/06-sa.yaml, or any previously set
// Console/cluster ref). We need these on the *required* object so that
// MergeOwnerRefs will update them — it only modifies refs that appear in
// required (matched by Name+Kind+APIVersion.Group).
existing, err := c.serviceAccountClient.ServiceAccounts(api.OpenShiftConsoleNamespace).Get(ctx, c.serviceAccountName, metav1.GetOptions{})
if err != nil && !apierrors.IsNotFound(err) {
return fmt.Errorf("failed to get service account %s: %w", c.serviceAccountName, err)
}
if err == nil {
// Clear controller:true from all existing ownerRefs to satisfy the
// Kubernetes invariant that only one ownerRef may have controller:true.
// https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/
falseBool := false
ownerRefs := existing.DeepCopy().GetOwnerReferences()
for i := range ownerRefs {
ownerRefs[i].Controller = &falseBool
}
serviceAccount.SetOwnerReferences(ownerRefs)
}
_, _, err = resourceapply.ApplyServiceAccount(ctx,
c.serviceAccountClient,
controllerContext.Recorder(),
serviceAccount,
)
if err != nil {
return fmt.Errorf("failed to apply service account %s: %w", c.serviceAccountName, err)
}
return nil
}Why this works:
This also makes the |
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
from
72b2a70 to
245427b
Compare
|
@coderabbitai I made that change, but I did change one thing. Your suggestion of |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/console/controllers/serviceaccounts/controller.go (1)
121-123: Avoid namespace drift between apply and delete paths.Line 122 hard-codes
openshift-consolefor deletion, while apply/get use the namespace from the manifest object. If manifest namespace changes, removed-state cleanup can miss the ServiceAccount.♻️ Proposed adjustment
func (c *ServiceAccountSyncController) removeServiceAccount(ctx context.Context) error { - err := c.serviceAccountClient.ServiceAccounts(api.OpenShiftConsoleNamespace).Delete(ctx, c.serviceAccountName, metav1.DeleteOptions{}) + serviceAccount := resourceread.ReadServiceAccountV1OrDie( + bindata.MustAsset(fmt.Sprintf("assets/serviceaccounts/%s-sa.yaml", c.serviceAccountName)), + ) + if serviceAccount.Namespace == "" { + return fmt.Errorf("service account namespace is empty for %s", c.serviceAccountName) + } + err := c.serviceAccountClient.ServiceAccounts(serviceAccount.Namespace).Delete(ctx, c.serviceAccountName, metav1.DeleteOptions{}) if apierrors.IsNotFound(err) { return nil } return err }Also applies to: 129-132, 142-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/console/controllers/serviceaccounts/controller.go` around lines 121 - 123, The delete path in removeServiceAccount uses a hard-coded api.OpenShiftConsoleNamespace when calling c.serviceAccountClient.ServiceAccounts(...).Delete which can cause namespace drift versus the apply/get paths that use the manifest's namespace; change the Delete calls (and the similar calls around lines referenced) to use the ServiceAccount namespace derived from the manifest or controller state (e.g., the manifest's namespace via c.manifest.GetNamespace() or the controller's namespace field) instead of api.OpenShiftConsoleNamespace, keeping c.serviceAccountName as-is so deletion targets the same resource created by apply.
🤖 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/console/controllers/serviceaccounts/controller.go`:
- Around line 121-123: The delete path in removeServiceAccount uses a hard-coded
api.OpenShiftConsoleNamespace when calling
c.serviceAccountClient.ServiceAccounts(...).Delete which can cause namespace
drift versus the apply/get paths that use the manifest's namespace; change the
Delete calls (and the similar calls around lines referenced) to use the
ServiceAccount namespace derived from the manifest or controller state (e.g.,
the manifest's namespace via c.manifest.GetNamespace() or the controller's
namespace field) instead of api.OpenShiftConsoleNamespace, keeping
c.serviceAccountName as-is so deletion targets the same resource created by
apply.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bb2957d-ec8c-4144-a595-ec3870a9c804
📒 Files selected for processing (4)
manifests/03-rbac-role-cluster.yamlpkg/api/api.gopkg/console/controllers/serviceaccounts/controller.gopkg/console/starter/starter.go
🚧 Files skipped from review as they are similar to previous changes (1)
- manifests/03-rbac-role-cluster.yaml
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
from
245427b to
aa70bf3
Compare
|
@coderabbitai I decided to keep it consistent instead. |
|
Tip For best results, initiate chat on the files or code changes.
|
|
@jhadvig - tests are now passing. Feel free to review the requested changes. :) |
| - list | ||
| - watch | ||
| - delete | ||
| - apiGroups: |
There was a problem hiding this comment.
We just need one
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- get
- list
- watch
- create
- update
- delete
resourceNames:
- console
- downloadsThere was a problem hiding this comment.
Looking here , if we did squash it all together then we may have issues with looking for service accounts. I kept it separate so we can list, watch and get any service accounts. However, there is no reason why we cannot merge the first two rules.
From what I understand, the minimum number of rules we can have is two. We can move list, watch and get to the third rule. However, we cannot restrict create .
You cannot restrict deletecollection or top-level create requests by resource name. For create, this limitation is because the name of the new object may not be known at authorization time.
As for the third rule, if we wanted to make modifications to the service account, we wouldn't want for the controller to be able to delete just any service account, so having resourceNames specified here allows us to restrict where possible.
So, the compromise we can do is keep create without restriction, and we can restrict everything else. I'll go ahead and update to reflect this, and feel free to correct me if I misunderstood anything.
There was a problem hiding this comment.
I also need to double check if the get used in the controller looks for these service accounts specifically or not and make adjustments.
There was a problem hiding this comment.
Hey @jhadvig - sadly we cannot use it like this. Found this in console-operator logs
026-03-11T13:25:17.260764907Z E0311 13:25:17.260731 1 reflector.go:205] "Failed to watch" err="failed to list *v1.ServiceAccount: serviceaccounts is forbidden: User \"system:serviceaccount:openshift-console-operator:console-operator\" cannot list resource \"serviceaccounts\" in API group \"\" in the namespace \"openshift-console\"" This is the current rule formation:
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- create
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- list
- watch
- get
- update
- delete
resourceNames:
- console
- downloadsI will probably need to move it to this
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- create
- list
- watch
- get
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- update
- delete
resourceNames:
- console
- downloadsLet me know if this suits. :)
There was a problem hiding this comment.
nit:
its a bit weird that we have the create action but not delete.
if anything I would add the create to the second list, sicne we are only creating these two SAs
| return nil | ||
| case operatorv1.Removed: | ||
| klog.V(4).Infoln("console is in a removed state: removing synced serviceaccount") | ||
| statusHandler := status.NewStatusHandler(c.operatorClient) |
There was a problem hiding this comment.
lets not initialize the statushandler in this case
return c.removeServiceAccount(ctx)| } | ||
|
|
||
| func (c *ServiceAccountSyncController) SyncServiceAccount(ctx context.Context, operatorConfigCopy *operatorv1.Console, controllerContext factory.SyncContext) error { | ||
| serviceAccount := resourceread.ReadServiceAccountV1OrDie( |
There was a problem hiding this comment.
Let pull this block into a helper
func DefaultServiceAccount(cr *operatorv1.Console) (*corev1.ServiceAccount, error) {
serviceAccount := resourceread.ReadServiceAccountV1OrDie(
bindata.MustAsset(fmt.Sprintf("assets/serviceaccounts/%s-sa.yaml", c.serviceAccountName)),
)
if serviceAccount.Name == "" {
return nil, fmt.Errorf("No service account found for name %v .", c.serviceAccountName)
}
util.AddOwnerRef(serviceAccount, util.OwnerRefFrom(cr))
return serviceAccount, nil
}| return fmt.Errorf("No service account found for name %v .", c.serviceAccountName) | ||
| } | ||
|
|
||
| // Fetch the live SA to get its actual ownerRefs (e.g. ClusterVersion/version |
There was a problem hiding this comment.
dont think this is quite right, eg is the SA will be missing this block will get skipped.
instead:
required, err := DefaultServiceAccount() // declared above
if err != nil {
return err
}
_, _, err = resourceapply.ApplyServiceAccount(ctx,
c.serviceAccountClient,
controllerContext.Recorder(),
serviceAccount,
)
if err != nil {
return fmt.Errorf("failed to apply service account %s: %w", c.serviceAccountName, err)
}
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
from
aa70bf3 to
9cd12ec
Compare
|
/retest |
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
2 times, most recently
from
3f7011f to
ac6818f
Compare
ehearne-redhat
force-pushed
the
downloads-service-account-controller
branch
from
ac6818f to
1ee9a0a
Compare
|
@ehearne-redhat: 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. |
| - list | ||
| - watch | ||
| - delete | ||
| - apiGroups: |
There was a problem hiding this comment.
nit:
its a bit weird that we have the create action but not delete.
if anything I would add the create to the second list, sicne we are only creating these two SAs
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ehearne-redhat, jhadvig 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
|
/approved |
3 participants
@yapei pointed out correctly that the service account
downloadsis not deleted once the poddownloadscompletes/deletes.This PR addresses this by adding a controller, similar to how downloads deployment has its own controller in
pkg/console/controllers/downloadsdeployment/controller.go.It also moves the service account in
bindata/assets/deployments/downloads-deployment.yamldown tospec.specso it is placed correctly.The service account is now managed dynamically, so static reference to it in
manifests/06-sa.yamlhas been removed. Now, it has its own file inbindata/assets/serviceaccounts/downloads-sa.yaml.Permissions for the controller to perform CRUD on service account objects has been given to
console-operatorClusterRole.Summary by CodeRabbit
New Features
Infrastructure & Operations
Tests