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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ehearne-redhat 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 |
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 |
| _, _, serviceAccountErr := c.SyncDownloadsServiceAccount(ctx, operatorConfigCopy, controllerContext) | ||
| statusHandler.AddConditions(status.HandleProgressingOrDegraded("DownloadsServiceAccountSync", "FailedApply", serviceAccountErr)) | ||
| if serviceAccountErr != nil { | ||
| return statusHandler.FlushAndReturn(serviceAccountErr) | ||
| } | ||
|
|
||
| actualDownloadsDownloadsDeployment, _, downloadsDeploymentErr := c.SyncDownloadsDeployment(ctx, operatorConfigCopy, infrastructureConfig, controllerContext) |
There was a problem hiding this comment.
@jhadvig do you want me to add these conditions to the stale conditions operator?
|
@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
|
| Cohort / File(s) | Summary |
|---|---|
ServiceAccount Manifests bindata/assets/serviceaccounts/console-sa.yaml, bindata/assets/serviceaccounts/downloads-sa.yaml |
Add dedicated console and downloads ServiceAccount manifests with include.release.openshift.io annotations and capability metadata. |
Deployment Configuration bindata/assets/deployments/downloads-deployment.yaml |
Moved serviceAccountName: "downloads" from Deployment.spec to Deployment.spec.template.spec so pods are assigned the downloads SA. |
RBAC & Manifest Cleanup manifests/03-rbac-role-cluster.yaml, manifests/06-sa.yaml |
Extended console-operator ClusterRole with list/watch/get/create/update/delete rules for serviceaccounts (including resourceNames console, downloads); removed inline console and downloads ServiceAccount entries from manifests/06-sa.yaml. |
API Constants pkg/api/api.go |
Added exported constants OpenShiftConsoleDownloadsServiceAccountName and OpenShiftConsoleServiceAccountName mapped to deployment name constants. |
ServiceAccount Controller pkg/console/controllers/serviceaccounts/controller.go |
New ServiceAccountSyncController implementing Sync, SyncServiceAccount, and removeServiceAccount to apply or delete ServiceAccount manifests based on Console operator management state and set owner references. |
Integration & Startup pkg/console/starter/starter.go |
Registered and started two ServiceAccountSyncController instances (console and downloads) during 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 |
Comment changed (clients) in downloads deployment controller; test updated to include serviceAccountName: "downloads" in the downloads deployment PodSpec. |
Sequence Diagram(s)
sequenceDiagram
participant Operator as Operator (Run)
participant SAController as ServiceAccount SyncController
participant Config as Console Config Client
participant SAClient as ServiceAccount Client
participant ResourceApply as resourceapply Helper
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
🎯 3 (Moderate) | ⏱️ ~25 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 PR title accurately describes the main change: adding a controller for managing the downloads service account lifecycle, which is the primary objective addressed across multiple files. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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. :) |
|
@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
manifests/03-rbac-role-cluster.yaml (1)
153-163:⚠️ Potential issue | 🟠 MajorMissing resourceNames restriction on serviceaccounts rule.
Per jhadvig's earlier review comment, the serviceaccounts rule should be restricted to only the
consoleanddownloadsServiceAccounts usingresourceNames. This follows the principle of least privilege.🔒 Proposed fix to restrict serviceaccounts RBAC
- apiGroups: - "" resources: - serviceaccounts verbs: - get - list - delete - create - update - watch + resourceNames: + - console + - downloadsNote: With
resourceNames, thecreateverb won't be effective (can't specify resourceNames when creating), butlistwithout resourceNames is needed to discover SAs. Consider splitting into two rules if create capability is required.🤖 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 153 - 163, The RBAC rule granting verbs on the "serviceaccounts" resource must be restricted to specific ServiceAccount names: add resourceNames: ["console", "downloads"] to the rule that currently lists serviceaccounts so it only applies to those two SAs (reference the existing rule that mentions resources: - serviceaccounts and verbs: - get - list - delete - create - update - watch); because resourceNames cannot be used with create/list semantics, split into two rules if needed: one rule with resourceNames ["console","downloads"] for get/update/delete/watch, and a separate unrestricted rule (without resourceNames) that only includes list (or create if you actually need create) to preserve discovery functionality.
🧹 Nitpick comments (2)
pkg/console/controllers/serviceaccounts/controller.go (2)
94-114: Handle removed-state failures through status conditions too.The
Removedbranch returns early, so delete failures skipstatus.Handle*reporting and won’t be reflected in operator conditions.As per coding guidelines, "Status conditions should use status.Handle* functions."Proposed change
func (c *ServiceAccountSyncController) Sync(ctx context.Context, controllerContext factory.SyncContext) error { operatorConfig, err := c.consoleOperatorLister.Get(api.ConfigResourceName) if err != nil { return err } operatorConfigCopy := operatorConfig.DeepCopy() + statusHandler := status.NewStatusHandler(c.operatorClient) switch operatorConfigCopy.Spec.ManagementState { case operatorv1.Managed: klog.V(4).Infoln("console is in a managed state: syncing serviceaccount") case operatorv1.Unmanaged: klog.V(4).Infoln("console is in an unmanaged state: skipping serviceaccount sync") return nil case operatorv1.Removed: klog.V(4).Infoln("console is in a removed state: removing synced serviceaccount") - return c.removeServiceAccount(ctx) + removeErr := c.removeServiceAccount(ctx) + statusHandler.AddConditions(status.HandleProgressingOrDegraded("ServiceAccountSync", "FailedDelete", removeErr)) + return statusHandler.FlushAndReturn(removeErr) default: return fmt.Errorf("unknown state: %v", operatorConfigCopy.Spec.ManagementState) } - statusHandler := status.NewStatusHandler(c.operatorClient) serviceAccountErr := c.SyncServiceAccount(ctx, operatorConfigCopy, controllerContext) statusHandler.AddConditions(status.HandleProgressingOrDegraded("ServiceAccountSync", "FailedApply", serviceAccountErr))🤖 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 94 - 114, The Removed branch returns early from the management-state switch so failures from removeServiceAccount(ctx) are never passed through the status handler; change the Removed case to set a serviceAccountErr by calling c.removeServiceAccount(ctx) (instead of returning), then proceed to create the status.NewStatusHandler(c.operatorClient), call statusHandler.AddConditions(status.HandleProgressingOrDegraded("ServiceAccountSync", "FailedApply", serviceAccountErr)) and finally return statusHandler.FlushAndReturn(serviceAccountErr) (mirroring how SyncServiceAccount results are handled) so all remove failures are reflected in operator conditions.
88-91: Wrap returned API errors with operation context.Raw
return errmakes failure triage harder in controller logs; include resource/action context.As per coding guidelines, "use ... meaningful error messages" and "wrap errors with context".Proposed change
operatorConfig, err := c.consoleOperatorLister.Get(api.ConfigResourceName) if err != nil { - return err + return fmt.Errorf("get Console %q: %w", api.ConfigResourceName, err) } ... err := c.serviceAccountClient.ServiceAccounts(api.OpenShiftConsoleNamespace).Delete(ctx, c.serviceAccountName, metav1.DeleteOptions{}) if apierrors.IsNotFound(err) { return nil } -return err +if err != nil { + return fmt.Errorf("delete ServiceAccount %s/%s: %w", api.OpenShiftConsoleNamespace, c.serviceAccountName, err) +} +return nil ... if err != nil { - return err + return fmt.Errorf("apply ServiceAccount %s/%s: %w", api.OpenShiftConsoleNamespace, c.serviceAccountName, err) }Also applies to: 118-123, 155-157
🤖 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 88 - 91, The controller returns raw errors from lister Get calls; wrap those with contextual messages to aid triage by using fmt.Errorf to include the operation and resource name. Replace direct returns like "return err" after c.consoleOperatorLister.Get(api.ConfigResourceName) with a wrapped error such as fmt.Errorf("failed to get console operator config %q: %w", api.ConfigResourceName, err), and apply the same pattern to the other lister/Get sites noted (the calls around lines 118-123 and 155-157) so every failure includes the action and resource identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manifests/03-rbac-role-cluster.yaml`:
- Around line 140-143: The resourceNames list under the
customresourcedefinitions rule contains invalid short names ("console" and
"downloads"); replace those with the actual fully-qualified CRD names (e.g.,
"consoleplugins.console.openshift.io", "consoles.operator.openshift.io",
"consoleclidownloads.console.openshift.io") if you intend to restrict specific
CRDs, or remove "console" and "downloads" from resourceNames and move them to
the serviceaccounts rule if the intent was to restrict ServiceAccount names;
update the resourceNames array in the customresourcedefinitions rule accordingly
and ensure only fully-qualified CRD names remain there.
In `@pkg/api/api.go`:
- Around line 68-69: Rename the incorrectly cased constant
OpenshiftConsoleServiceAccountName to OpenShiftConsoleServiceAccountName to
match the existing naming convention used by OpenShiftConsoleName and
OpenShiftConsoleDeploymentName; update any usages (e.g., the reference that
currently imports/uses OpenshiftConsoleServiceAccountName) to the new
OpenShiftConsoleServiceAccountName symbol so all references compile and remain
consistent with the rest of the constants.
In `@pkg/console/controllers/serviceaccounts/controller.go`:
- Around line 133-147: The code appends an ownerRef (from
subresourceutil.OwnerRefFrom) which may have Controller=true but then
unconditionally sets Controller=false on every entry in
serviceAccount.OwnerReferences, nullifying the controller; update the loop to
skip clearing Controller for the newly added ownerRef (compare by UID or
Name/Kind/APIVersion—use the local ownerRef variable returned by OwnerRefFrom)
so only other existing ownerRefs get Controller set to false, ensuring exactly
one Controller=true remains (or explicitly set ownerRef.Controller = pointer to
true before appending and clear Controller on all others).
---
Duplicate comments:
In `@manifests/03-rbac-role-cluster.yaml`:
- Around line 153-163: The RBAC rule granting verbs on the "serviceaccounts"
resource must be restricted to specific ServiceAccount names: add resourceNames:
["console", "downloads"] to the rule that currently lists serviceaccounts so it
only applies to those two SAs (reference the existing rule that mentions
resources: - serviceaccounts and verbs: - get - list - delete - create - update
- watch); because resourceNames cannot be used with create/list semantics, split
into two rules if needed: one rule with resourceNames ["console","downloads"]
for get/update/delete/watch, and a separate unrestricted rule (without
resourceNames) that only includes list (or create if you actually need create)
to preserve discovery functionality.
---
Nitpick comments:
In `@pkg/console/controllers/serviceaccounts/controller.go`:
- Around line 94-114: The Removed branch returns early from the management-state
switch so failures from removeServiceAccount(ctx) are never passed through the
status handler; change the Removed case to set a serviceAccountErr by calling
c.removeServiceAccount(ctx) (instead of returning), then proceed to create the
status.NewStatusHandler(c.operatorClient), call
statusHandler.AddConditions(status.HandleProgressingOrDegraded("ServiceAccountSync",
"FailedApply", serviceAccountErr)) and finally return
statusHandler.FlushAndReturn(serviceAccountErr) (mirroring how
SyncServiceAccount results are handled) so all remove failures are reflected in
operator conditions.
- Around line 88-91: The controller returns raw errors from lister Get calls;
wrap those with contextual messages to aid triage by using fmt.Errorf to include
the operation and resource name. Replace direct returns like "return err" after
c.consoleOperatorLister.Get(api.ConfigResourceName) with a wrapped error such as
fmt.Errorf("failed to get console operator config %q: %w",
api.ConfigResourceName, err), and apply the same pattern to the other lister/Get
sites noted (the calls around lines 118-123 and 155-157) so every failure
includes the action and resource identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3577197-ea4a-49e3-9ada-e696fd29b6cc
📒 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
332bd0e to
216d657
Compare
|
@coderabbitai please review new changes |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
manifests/03-rbac-role-cluster.yaml (1)
159-169:⚠️ Potential issue | 🔴 CriticalRemove
createfrom theresourceNames-scoped RBAC rule.In Kubernetes RBAC,
resourceNamesrestricts only requests where the object name is part of the authorization URL (e.g.,GET /serviceaccounts/console). Top-levelcreateis a collection request (POST /serviceaccounts) with no name in the URL, soresourceNameshas no effect on it. Thecreateverb must be granted in a separate rule withoutresourceNames.Suggested RBAC split
- apiGroups: - "" resources: - serviceaccounts verbs: - - create + - create + - apiGroups: + - "" + resources: + - serviceaccounts + verbs: - update - delete resourceNames: - console - downloads🤖 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 159 - 169, The current RBAC rule that lists verbs: [create, update, delete] with resources: [serviceaccounts] and resourceNames: [console, downloads] is incorrect because create is a collection-level verb and cannot be scoped by resourceNames; remove "create" from that resourceNames-scoped rule (leave update and delete for resourceNames: console, downloads) and add a new rule granting verbs: [create] for resources: [serviceaccounts] without any resourceNames entry so POST/collection create requests are permitted; update the Role/ClusterRole manifests accordingly (refer to the resourceNames, verbs, and serviceaccounts entries to locate the rules).
🧹 Nitpick comments (1)
pkg/console/controllers/serviceaccounts/controller.go (1)
88-90: Wrap returned errors with operation context.Several direct
return errpaths (Line 90, Line 122, Line 158) lose actionable context during troubleshooting. Add operation/resource context in these returns.As per coding guidelines, "Wrap errors with context; provide meaningful messages."
Also applies to: 117-123, 151-159
🤖 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 88 - 90, Wrap direct returns of err with contextual messages for the resource fetches: replace plain "return err" after the call to c.consoleOperatorLister.Get(api.ConfigResourceName) (the operatorConfig retrieval) with a wrapped error such as fmt.Errorf("failed to get console operator config %s: %w", api.ConfigResourceName, err); do the same for the other lister Get return paths referenced (the returns in the 117-123 and 151-159 blocks) by adding descriptive operation/resource context and using %w to preserve the original error.
🤖 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/console/controllers/serviceaccounts/controller.go`:
- Around line 94-103: The Removed branch currently returns immediately and
bypasses status updates; change the operatorConfigCopy.Spec.ManagementState
switch so it calls c.removeServiceAccount(ctx) and captures its error instead of
returning early, then use the controller's status.Handle* helpers (e.g.,
status.HandleDegraded and/or status.HandleProgressing) to report delete failures
or success accordingly (invoke the appropriate status.Handle* functions with the
error and context) rather than skipping status updates; update any call sites
around c.removeServiceAccount(ctx) to follow this pattern so delete failures
emit the proper degraded/progressing conditions.
- Around line 44-59: The two controllers are publishing the same condition type
ServiceAccountSync which causes status collisions; change the condition type to
be unique per controller (e.g., derive it from controllerName or append/prepend
controllerName to ServiceAccountSync) in NewServiceAccountSyncController and any
place that currently references ServiceAccountSync (around the
ServiceAccountSync publication at lines ~106-110) so each controller (console vs
downloads) uses a distinct condition type, update any uses in status updates and
tests to reference the new per-controller condition name.
In `@pkg/console/starter/starter.go`:
- Line 341: The code uses the wrong constant name
api.OpenshiftConsoleServiceAccountName; update this reference to the correctly
exported constant api.OpenShiftConsoleServiceAccountName (capital 'S') wherever
it's used (e.g., the occurrence in starter.go) so it matches the definition in
pkg/api/api.go and resolves the compile-time error.
---
Duplicate comments:
In `@manifests/03-rbac-role-cluster.yaml`:
- Around line 159-169: The current RBAC rule that lists verbs: [create, update,
delete] with resources: [serviceaccounts] and resourceNames: [console,
downloads] is incorrect because create is a collection-level verb and cannot be
scoped by resourceNames; remove "create" from that resourceNames-scoped rule
(leave update and delete for resourceNames: console, downloads) and add a new
rule granting verbs: [create] for resources: [serviceaccounts] without any
resourceNames entry so POST/collection create requests are permitted; update the
Role/ClusterRole manifests accordingly (refer to the resourceNames, verbs, and
serviceaccounts entries to locate the rules).
---
Nitpick comments:
In `@pkg/console/controllers/serviceaccounts/controller.go`:
- Around line 88-90: Wrap direct returns of err with contextual messages for the
resource fetches: replace plain "return err" after the call to
c.consoleOperatorLister.Get(api.ConfigResourceName) (the operatorConfig
retrieval) with a wrapped error such as fmt.Errorf("failed to get console
operator config %s: %w", api.ConfigResourceName, err); do the same for the other
lister Get return paths referenced (the returns in the 117-123 and 151-159
blocks) by adding descriptive operation/resource context and using %w to
preserve the original error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e73724c8-b84e-4c51-b41c-e53dfb8b7361
📒 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)
- pkg/api/api.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/console/starter/starter.go (1)
330-343:⚠️ Potential issue | 🔴 CriticalFix incorrect constant name casing on line 341.
The constant
api.OpenshiftConsoleServiceAccountName(lowercase 's') does not match the definition inpkg/api/api.gowhich isOpenShiftConsoleServiceAccountName(capital 'S'). This will cause a compile-time error.🔧 Proposed fix
consoleServiceAccountController := serviceaccounts.NewServiceAccountSyncController( // clients operatorClient, configInformers, // operator operatorConfigInformers.Operator().V1().Consoles(), kubeClient.CoreV1(), // ServiceAccount kubeInformersNamespaced.Core().V1().ServiceAccounts(), // ServiceAccount recorder, - api.OpenshiftConsoleServiceAccountName, + api.OpenShiftConsoleServiceAccountName, api.OpenShiftConsoleName, // controller name )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/console/starter/starter.go` around lines 330 - 343, The code uses the wrong-cased constant name; replace api.OpenshiftConsoleServiceAccountName with the correctly cased api.OpenShiftConsoleServiceAccountName in the call to serviceaccounts.NewServiceAccountSyncController (the block that constructs consoleServiceAccountController) so the symbol matches the definition in pkg/api/api.go.pkg/console/controllers/serviceaccounts/controller.go (2)
109-109:⚠️ Potential issue | 🟠 MajorCondition type is shared across both SA controllers and can overwrite status.
Using a hardcoded
"ServiceAccountSync"means console/downloads controller updates can mask each other. Make the condition type instance-specific.Suggested fix
type ServiceAccountSyncController struct { serviceAccountName string + conditionType string operatorClient v1helpers.OperatorClient @@ ctrl := &ServiceAccountSyncController{ serviceAccountName: serviceAccountName, + conditionType: fmt.Sprintf("%sServiceAccountSync", controllerName), @@ - statusHandler.AddConditions(status.HandleProgressingOrDegraded("ServiceAccountSync", "FailedApply", serviceAccountErr)) + statusHandler.AddConditions(status.HandleProgressingOrDegraded(c.conditionType, "FailedApply", serviceAccountErr))🤖 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 109, The condition type "ServiceAccountSync" is too generic and shared by both SA controllers, causing status overwrites; update the call site that invokes statusHandler.AddConditions with status.HandleProgressingOrDegraded to use an instance-specific condition type (e.g., include the ServiceAccount name or UID in the condition string) instead of the hardcoded "ServiceAccountSync" so the condition becomes unique per ServiceAccount (for example build a conditionType like fmt.Sprintf("ServiceAccountSync/%s", sa.Name) or similar) and apply the same pattern wherever "ServiceAccountSync" is used so controllers cannot mask each other's status.
94-114:⚠️ Potential issue | 🟠 MajorRemoved-state delete path still skips status publication on failure.
The
Removedbranch returns immediately, so delete failures bypassstatus.Handle*updates and won’t surface degraded/progressing conditions for this controller.Suggested fix
func (c *ServiceAccountSyncController) Sync(ctx context.Context, controllerContext factory.SyncContext) error { + statusHandler := status.NewStatusHandler(c.operatorClient) + operatorConfig, err := c.consoleOperatorLister.Get(api.ConfigResourceName) if err != nil { return err } operatorConfigCopy := operatorConfig.DeepCopy() + var serviceAccountErr error switch operatorConfigCopy.Spec.ManagementState { case operatorv1.Managed: klog.V(4).Infoln("console is in a managed state: syncing serviceaccount") + serviceAccountErr = c.SyncServiceAccount(ctx, operatorConfigCopy, controllerContext) case operatorv1.Unmanaged: klog.V(4).Infoln("console is in an unmanaged state: skipping serviceaccount sync") return nil case operatorv1.Removed: klog.V(4).Infoln("console is in a removed state: removing synced serviceaccount") - return c.removeServiceAccount(ctx) + serviceAccountErr = c.removeServiceAccount(ctx) default: return fmt.Errorf("unknown state: %v", operatorConfigCopy.Spec.ManagementState) } - statusHandler := status.NewStatusHandler(c.operatorClient) - - serviceAccountErr := c.SyncServiceAccount(ctx, operatorConfigCopy, controllerContext) statusHandler.AddConditions(status.HandleProgressingOrDegraded("ServiceAccountSync", "FailedApply", serviceAccountErr)) - if serviceAccountErr != nil { - return statusHandler.FlushAndReturn(serviceAccountErr) - } - - return statusHandler.FlushAndReturn(nil) + return statusHandler.FlushAndReturn(serviceAccountErr) }As per coding guidelines, "Status conditions should use status.Handle* functions."
🤖 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 94 - 114, The Removed branch returns early so failures from removeServiceAccount skip status updates; change it to call c.removeServiceAccount(ctx) without returning immediately (or capture its error into a variable), then create the status handler (status.NewStatusHandler), use status.HandleProgressingOrDegraded (or status.Handle* helpers) to add conditions for the delete operation, and call statusHandler.FlushAndReturn(err) (similar to how SyncServiceAccount is handled) so removeServiceAccount failures surface as progressing/degraded status; update references: operatorConfigCopy.Spec.ManagementState, c.removeServiceAccount, status.NewStatusHandler, status.HandleProgressingOrDegraded, and statusHandler.FlushAndReturn.
🧹 Nitpick comments (2)
manifests/03-rbac-role-cluster.yaml (1)
159-169:resourceNamesdoes not restrictcreateoperations in Kubernetes RBAC.For top-level
createrequests, the API server doesn't know the object's final name at authorization time, soresourceNamesconstraints cannot be matched and the restriction has no effect. This means the operator can create any ServiceAccount, not just "console" and "downloads". (Note: This limitation applies only to top-level resources; subresources likepods/execcan useresourceNameswithcreatebecause the parent object name is part of the request.)If restricting creation to specific names is required, you would need a ValidatingAdmissionPolicy or webhook. However, for an operator managing its own resources, this is typically acceptable since the operator code itself controls what gets created. The restriction still properly limits
updateanddeleteoperations.Consider either:
- Accept this RBAC behavior (operator code controls what's created anyway)
- Remove
createfrom this rule if unrestricted creation is not intended, and add a separate unrestricted rule if needed- Use a ValidatingAdmissionPolicy or webhook for name-based creation restrictions
🤖 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 159 - 169, The RBAC rule listing serviceaccounts with resourceNames: console, downloads incorrectly includes the "create" verb because Kubernetes cannot enforce name-based restrictions for top-level create requests; this means the operator can create any ServiceAccount. Remove "create" from the verbs list in the rule that references serviceaccounts (keep update and delete) or, if you truly need to restrict create by name, implement a ValidatingAdmissionPolicy/webhook instead; ensure you update the rule that contains resourceNames, verbs, and resources: serviceaccounts so create is no longer present if you choose removal.pkg/console/controllers/serviceaccounts/controller.go (1)
88-90: Wrap returned errors with action/resource context.A few direct
return errpaths lose crucial debugging context (which read/delete/apply failed).Suggested fix
operatorConfig, err := c.consoleOperatorLister.Get(api.ConfigResourceName) if err != nil { - return err + return fmt.Errorf("get Console %q: %w", api.ConfigResourceName, err) } @@ func (c *ServiceAccountSyncController) removeServiceAccount(ctx context.Context) error { err := c.serviceAccountClient.ServiceAccounts(api.OpenShiftConsoleNamespace).Delete(ctx, c.serviceAccountName, metav1.DeleteOptions{}) if apierrors.IsNotFound(err) { return nil } - return err + if err != nil { + return fmt.Errorf("delete ServiceAccount %s/%s: %w", api.OpenShiftConsoleNamespace, c.serviceAccountName, err) + } + return nil } @@ if err != nil { - return err + return fmt.Errorf("apply ServiceAccount %s/%s: %w", api.OpenShiftConsoleNamespace, c.serviceAccountName, err) }As per coding guidelines, "Wrap errors with context and use typed errors where appropriate."
Also applies to: 118-123, 157-159
🤖 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 88 - 90, Replace direct "return err" with wrapped errors that include the action and resource context; e.g., where you call c.consoleOperatorLister.Get(api.ConfigResourceName) wrap the error as fmt.Errorf("failed to get console operator config %q: %w", api.ConfigResourceName, err). Do the same for the other two spots (lines mentioned) — wrap delete/read/apply errors with messages like "failed to delete serviceaccount %s: %w" or "failed to apply rolebinding %s: %w" using the appropriate resource/identifier and the original err so callers get actionable context.
🤖 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`:
- Line 109: The condition type "ServiceAccountSync" is too generic and shared by
both SA controllers, causing status overwrites; update the call site that
invokes statusHandler.AddConditions with status.HandleProgressingOrDegraded to
use an instance-specific condition type (e.g., include the ServiceAccount name
or UID in the condition string) instead of the hardcoded "ServiceAccountSync" so
the condition becomes unique per ServiceAccount (for example build a
conditionType like fmt.Sprintf("ServiceAccountSync/%s", sa.Name) or similar) and
apply the same pattern wherever "ServiceAccountSync" is used so controllers
cannot mask each other's status.
- Around line 94-114: The Removed branch returns early so failures from
removeServiceAccount skip status updates; change it to call
c.removeServiceAccount(ctx) without returning immediately (or capture its error
into a variable), then create the status handler (status.NewStatusHandler), use
status.HandleProgressingOrDegraded (or status.Handle* helpers) to add conditions
for the delete operation, and call statusHandler.FlushAndReturn(err) (similar to
how SyncServiceAccount is handled) so removeServiceAccount failures surface as
progressing/degraded status; update references:
operatorConfigCopy.Spec.ManagementState, c.removeServiceAccount,
status.NewStatusHandler, status.HandleProgressingOrDegraded, and
statusHandler.FlushAndReturn.
In `@pkg/console/starter/starter.go`:
- Around line 330-343: The code uses the wrong-cased constant name; replace
api.OpenshiftConsoleServiceAccountName with the correctly cased
api.OpenShiftConsoleServiceAccountName in the call to
serviceaccounts.NewServiceAccountSyncController (the block that constructs
consoleServiceAccountController) so the symbol matches the definition in
pkg/api/api.go.
---
Nitpick comments:
In `@manifests/03-rbac-role-cluster.yaml`:
- Around line 159-169: The RBAC rule listing serviceaccounts with resourceNames:
console, downloads incorrectly includes the "create" verb because Kubernetes
cannot enforce name-based restrictions for top-level create requests; this means
the operator can create any ServiceAccount. Remove "create" from the verbs list
in the rule that references serviceaccounts (keep update and delete) or, if you
truly need to restrict create by name, implement a
ValidatingAdmissionPolicy/webhook instead; ensure you update the rule that
contains resourceNames, verbs, and resources: serviceaccounts so create is no
longer present if you choose removal.
In `@pkg/console/controllers/serviceaccounts/controller.go`:
- Around line 88-90: Replace direct "return err" with wrapped errors that
include the action and resource context; e.g., where you call
c.consoleOperatorLister.Get(api.ConfigResourceName) wrap the error as
fmt.Errorf("failed to get console operator config %q: %w",
api.ConfigResourceName, err). Do the same for the other two spots (lines
mentioned) — wrap delete/read/apply errors with messages like "failed to delete
serviceaccount %s: %w" or "failed to apply rolebinding %s: %w" using the
appropriate resource/identifier and the original err so callers get actionable
context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30f9bff8-3e6c-45dd-bc77-f446ac235cba
📒 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
216d657 to
d75e52d
Compare
|
@coderabbitai can we get another review? I just pushed the changes up. |
|
✅ Actions performedFull review triggered. |
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: The following test 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. |
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