Merged
Conversation
|
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:
WalkthroughAdds manifest-level service permissions and end-to-end RBAC: permission model and registry, builders for ServiceAccount/Role/RoleBinding, client orchestration to create/apply/cleanup RBAC before workloads, workload builders updated to set serviceAccount/automount, CRD/applyconfig updates, tests and SDL fixtures. Changes
Sequence DiagramsequenceDiagram
participant Builder as Workload Builder
participant Orchestrator as Cluster Client
participant K8s as Kubernetes API
Builder->>Builder: detect permissions (GetWorkloadPermissions / HasPermissions)
Orchestrator->>Builder: check HasPermissions()
alt permissions present
Orchestrator->>Builder: BuildServiceAccount()
Builder-->>Orchestrator: ServiceAccount
Orchestrator->>K8s: applyServiceAccount (Get -> Update/Create)
Orchestrator->>Builder: BuildRole()
Builder-->>Orchestrator: Role (policy rules)
Orchestrator->>K8s: applyRole (Get -> Update/Create)
Orchestrator->>Builder: BuildRoleBinding()
Builder-->>Orchestrator: RoleBinding
Orchestrator->>K8s: applyRoleBinding (Get -> Update/Create)
else no permissions
Orchestrator->>Orchestrator: skip RBAC resources
end
Orchestrator->>Builder: Build Deployment/StatefulSet (serviceAccountName & automount)
Builder-->>Orchestrator: PodSpec
Orchestrator->>K8s: apply Deployment/StatefulSet
K8s-->>Orchestrator: resources applied
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/client/clientset/versioned/fake/clientset_generated.go (1)
17-17:⚠️ Potential issue | 🟠 MajorResolve the manual modifications in generated code.
This file is marked as generated by
client-genwith a "DO NOT EDIT" directive, yet it contains manual modifications forListOptionssupport in the watch reactors. These changes will be overwritten if the code is regenerated. Additionally, there is a typo on lines 55 and 104:watchActcionshould bewatchAction.Consider one of the following approaches:
- Update the code generator template to include the
ListOptionshandling permanently.- Remove the "DO NOT EDIT" comment if manual maintenance is intended.
- If this is a temporary workaround for the feature branch, document it and fix the typo (
watchActcion→watchAction).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/clientset/versioned/fake/clientset_generated.go` at line 17, The file is a generated client ("DO NOT EDIT") but contains manual changes to support ListOptions in watch reactors and two typos: watchActcion should be watchAction; fix by either (A) reverting manual changes and updating the client-gen template to emit ListOptions-aware watch reactors so generated code is correct, or (B) if you intend to keep manual edits temporarily, remove or clearly document the deviation from the "DO NOT EDIT" directive and correct the misspelt identifier occurrences (rename watchActcion → watchAction wherever used, e.g., in the watch reactor setup) and ensure the reactor code properly accepts metav1.ListOptions; pick one approach and apply consistently.cluster/kube/builder/builder.go (1)
54-62:⚠️ Potential issue | 🟡 MinorRun gofmt to fix CI failure.
CI reports this file isn’t gofmt’d; the new env-var constants block is the likely culprit. Please re-run gofmt on this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/builder.go` around lines 54 - 62, The file fails gofmt; reformat builder.go so the env var constants block (symbols like envVarAkashGroupSequence, envVarAkashDeploymentSequence, envVarAkashOrderSequence, envVarAkashOwner, envVarAkashProvider, envVarAkashClusterPublicHostname, envVarKubernetesNamespaceOverride, envVarKubernetesServiceHost, envVarKubernetesServicePort) follows gofmt spacing and alignment. Run `gofmt -w` (or `gofmt -s -w`) on the file and commit the re-formatted file so CI passes.
🧹 Nitpick comments (3)
pkg/apis/akash.network/v2beta2/manifest.go (1)
289-293: Consider copying permissions slices to avoid aliasing.Both conversions assign the
Readslice directly, which can couple the CRD and provider representations if either is mutated later. A defensive copy avoids that shared backing array.♻️ Suggested defensive copies
- if ms.Params.Permissions != nil { - ams.Params.Permissions = &mani.ServicePermissions{ - Read: ms.Params.Permissions.Read, - } - } + if ms.Params.Permissions != nil { + ams.Params.Permissions = &mani.ServicePermissions{ + Read: append([]string(nil), ms.Params.Permissions.Read...), + } + }- if ams.Params.Permissions != nil { - ms.Params.Permissions = &ManifestServicePermissions{ - Read: ams.Params.Permissions.Read, - } - } + if ams.Params.Permissions != nil { + ms.Params.Permissions = &ManifestServicePermissions{ + Read: append([]string(nil), ams.Params.Permissions.Read...), + } + }Also applies to: 334-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/apis/akash.network/v2beta2/manifest.go` around lines 289 - 293, The conversion currently assigns Permissions.Read slices directly (ms.Params.Permissions -> ams.Params.Permissions.Read), creating shared backing arrays between the CRD and provider structs; change these assignments in the conversion functions that touch mani.ServicePermissions (e.g., where ms.Params.Permissions is mapped to ams.Params.Permissions and the analogous block at lines ~334-338) to perform defensive copies of the slice (e.g., allocate a new slice and copy values or use append to copy into a nil slice) so the resulting mani.ServicePermissions.Read does not alias the original slice.cluster/kube/builder/role_test.go (1)
52-71: Tests rely on rule index order, but current implementation guarantees it.While these tests do assert specific rule positions (
role.Rules[0],[1], etc.), the rules are appended in a deterministic, hardcoded order within the switch statement inbuildPolicyRules()(role.go). Map iteration is not involved. However, the tests remain brittle to future refactoring. For better resilience, consider matching rules by resource name or using ElementsMatch on a normalized list instead of positional indices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/role_test.go` around lines 52 - 71, The test is brittle because it assumes rule ordering; update the assertions to locate rules by resource name instead of positional indices or use require.ElementsMatch on a normalized representation. Specifically, in the test referencing role.Rules (the checks against role.Rules[0], [1], [2]), find the rule whose Resources contains "pods", the one with "pods/log", and the one with "deployments" (or build a slice of expected rule structs and use require.ElementsMatch) so the assertions validate verbs and APIGroups per rule without depending on ordering from buildPolicyRules().cluster/kube/builder/role.go (1)
30-76: LGTM - PolicyRule generation is well-structured with appropriate read-only permissions.The implementation correctly:
- Deduplicates permission entries to avoid redundant rules
- Grants minimal read-only verbs (
get,list,watch)- Uses correct API groups (empty string for core,
appsfor deployments)- Returns
nilfor empty permissions (resulting in a Role with no rules)Minor consideration: Unknown permission strings in
GetPermissions()are silently ignored. This is likely acceptable if validation occurs at manifest parsing time, but consider logging a warning for unrecognized permissions to aid debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/role.go` around lines 30 - 76, buildPolicyRules currently ignores unknown permission strings silently; update the function (role.buildPolicyRules) to log a warning when a permission value from permissions := b.GetPermissions() is not handled by the switch (i.e., in the default case for the switch over resource) so maintainers can debug unexpected entries—use the existing logger available in this context (or b.logger/processLogger if present) to emit a concise warning including the unrecognized resource string and continue deduplication via the seen map as now.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cluster/kube/apply.go`:
- Around line 323-349: In applyRoleBinding, remove the comparison of RoleRef
from the update-condition logic: do not check reflect.DeepEqual(&uobj.RoleRef,
&oobj.RoleRef) when deciding to call kc.RbacV1().RoleBindings(...).Update;
RoleRef is immutable and that comparison will make Update attempts fail. Keep
the existing checks for labels/subjects/revision and, if you need to handle a
differing RoleRef you must implement delete-and-recreate logic (use
b.Create()/kc.RbacV1().RoleBindings(...).Create after deleting the old
RoleBinding) rather than attempting an Update.
In `@cluster/kube/builder/statefulset.go`:
- Around line 68-72: The file has gofmt formatting issues around the StatefulSet
spec construction (fields like AutomountServiceAccountToken, ServiceAccountName,
Containers from b.container(), ImagePullSecrets b.secretsRefs, and Volumes
b.volumesObjs) — run gofmt (or go fmt ./...) on the file to reformat it and
commit the changes so the struct field alignment and spacing meet gofmt rules;
ensure you also reformat the other affected block where similar fields are
listed (including usages around the container/secrets/volumes declarations)
before pushing.
In `@cluster/kube/builder/workload.go`:
- Around line 422-465: The change to automountServiceAccountToken() is breaking
because it returns a pointer to false when no permissions exist, overriding
Kubernetes' default (nil) and disabling auto-mount; update
automountServiceAccountToken() so it returns nil unless hasPermissions() is true
(i.e., if b.hasPermissions() return pointer to true, otherwise return nil) so
the field is omitted and cluster default behavior is preserved; check related
helpers hasPermissions() and serviceAccountName() to ensure they still drive
behavior when permissions are present.
In `@cluster/kube/client.go`:
- Around line 572-606: The RBAC resources created by applyServiceAccount,
applyRole, and applyRoleBinding are not being recorded in previousObj nor
handled in recover(); update the apply block to append the returned
nobj/uobj/oobj (or at least the created/updated objects and their kinds/keys)
into previousObj so they can be rolled back, and extend recover() to mirror
Service/Secret recovery by deleting newly created RBAC objects and restoring
prior versions (using the same logic you use for Services/Deployments/Secrets)
when nobj indicates creation or oobj indicates prior state; ensure you reference
applyServiceAccount, applyRole, applyRoleBinding, previousObj, and recover()
when implementing this tracking and cleanup.
In `@go.mod`:
- Around line 12-13: The go.mod replace directives for
github.com/cometbft/cometbft and github.com/cosmos/cosmos-sdk are still pinned
to older Akash fork versions (v0.38.19-akash.1 and v0.53.4-akash.b.10) while the
require section was bumped to v0.38.21 and v0.53.5; update the replace
directives so they point to the matching Akash-tagged equivalents (e.g.,
v0.38.21-akash.1 and v0.53.5-akash.1) to ensure the build uses the same API
versions as the requires for cometbft and cosmos-sdk.
In `@pkg/client/clientset/versioned/fake/clientset_generated.go`:
- Around line 54-60: There is a typo in the watch reactor variable name; rename
watchActcion to watchAction where the code does type assertion (if watchAction,
ok := action.(testing.WatchActionImpl); ok { opts = watchAction.ListOptions })
in both NewSimpleClientset and NewClientset so subsequent code using opts, gvr
:= action.GetResource(), ns := action.GetNamespace(), and watch, err :=
o.Watch(gvr, ns, opts) compiles and uses the correct identifier.
---
Outside diff comments:
In `@cluster/kube/builder/builder.go`:
- Around line 54-62: The file fails gofmt; reformat builder.go so the env var
constants block (symbols like envVarAkashGroupSequence,
envVarAkashDeploymentSequence, envVarAkashOrderSequence, envVarAkashOwner,
envVarAkashProvider, envVarAkashClusterPublicHostname,
envVarKubernetesNamespaceOverride, envVarKubernetesServiceHost,
envVarKubernetesServicePort) follows gofmt spacing and alignment. Run `gofmt -w`
(or `gofmt -s -w`) on the file and commit the re-formatted file so CI passes.
In `@pkg/client/clientset/versioned/fake/clientset_generated.go`:
- Line 17: The file is a generated client ("DO NOT EDIT") but contains manual
changes to support ListOptions in watch reactors and two typos: watchActcion
should be watchAction; fix by either (A) reverting manual changes and updating
the client-gen template to emit ListOptions-aware watch reactors so generated
code is correct, or (B) if you intend to keep manual edits temporarily, remove
or clearly document the deviation from the "DO NOT EDIT" directive and correct
the misspelt identifier occurrences (rename watchActcion → watchAction wherever
used, e.g., in the watch reactor setup) and ensure the reactor code properly
accepts metav1.ListOptions; pick one approach and apply consistently.
---
Nitpick comments:
In `@cluster/kube/builder/role_test.go`:
- Around line 52-71: The test is brittle because it assumes rule ordering;
update the assertions to locate rules by resource name instead of positional
indices or use require.ElementsMatch on a normalized representation.
Specifically, in the test referencing role.Rules (the checks against
role.Rules[0], [1], [2]), find the rule whose Resources contains "pods", the one
with "pods/log", and the one with "deployments" (or build a slice of expected
rule structs and use require.ElementsMatch) so the assertions validate verbs and
APIGroups per rule without depending on ordering from buildPolicyRules().
In `@cluster/kube/builder/role.go`:
- Around line 30-76: buildPolicyRules currently ignores unknown permission
strings silently; update the function (role.buildPolicyRules) to log a warning
when a permission value from permissions := b.GetPermissions() is not handled by
the switch (i.e., in the default case for the switch over resource) so
maintainers can debug unexpected entries—use the existing logger available in
this context (or b.logger/processLogger if present) to emit a concise warning
including the unrecognized resource string and continue deduplication via the
seen map as now.
In `@pkg/apis/akash.network/v2beta2/manifest.go`:
- Around line 289-293: The conversion currently assigns Permissions.Read slices
directly (ms.Params.Permissions -> ams.Params.Permissions.Read), creating shared
backing arrays between the CRD and provider structs; change these assignments in
the conversion functions that touch mani.ServicePermissions (e.g., where
ms.Params.Permissions is mapped to ams.Params.Permissions and the analogous
block at lines ~334-338) to perform defensive copies of the slice (e.g.,
allocate a new slice and copy values or use append to copy into a nil slice) so
the resulting mani.ServicePermissions.Read does not alias the original slice.
vertex451
reviewed
Feb 19, 2026
vertex451
reviewed
Feb 19, 2026
| } | ||
| lid := testutil.LeaseID(t) | ||
|
|
||
| t.Run("defaults to false when no permissions specified", func(t *testing.T) { |
Contributor
There was a problem hiding this comment.
🟢 When test fails, the title is formatted with underscore instead of space. It is easier to find the failed test in code when you have test title with underscore.
vertex451
reviewed
Feb 19, 2026
vertex451
reviewed
Feb 19, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cluster/kube/builder/permissions.go (1)
85-121: Consider returning empty slice instead of nil for consistency.
GenerateRulesreturnsnilfor nil/empty permissions but appends to anilslice for valid permissions. When no mappings match, it returns an empty slice. This inconsistency could cause issues for callers checkinglen(rules) == 0vsrules == nil.♻️ Optional: Initialize slice for consistent return type
func (r *PolicyRuleRegistry) GenerateRules(permissions *WorkloadPermissions) []rbacv1.PolicyRule { - if permissions == nil { + if permissions == nil || !permissions.HasAny() { return nil } - var rules []rbacv1.PolicyRule + rules := make([]rbacv1.PolicyRule, 0) seen := make(map[string]map[PermissionType]bool)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/permissions.go` around lines 85 - 121, The GenerateRules function currently returns nil when permissions is nil but an empty slice when no mappings match; make the return type consistent by initializing rules as an empty slice up front and returning that for nil/empty input. Specifically, in GenerateRules (and before the seen map), set rules to an explicitly allocated zero-length slice of rbacv1.PolicyRule and change the early nil check for permissions to return that empty slice instead of nil; keep using seen and mapping.Generator as-is and simply ensure the function always returns the initialized rules slice.cluster/kube/builder/permissions_test.go (1)
43-62: Incomplete test coverage forHasRead.The
HasReadtest only covers the nil case. Consider adding test cases for empty permissions and permissions with read values to mirror theHasAnytest structure.♻️ Suggested additional test cases
func TestWorkloadPermissions_HasRead(t *testing.T) { tests := []struct { name string perms *WorkloadPermissions expected bool }{ { name: "nil permissions returns false", perms: nil, expected: false, }, + { + name: "empty permissions returns false", + perms: &WorkloadPermissions{}, + expected: false, + }, + { + name: "with read permissions returns true", + perms: &WorkloadPermissions{Read: []string{"logs"}}, + expected: true, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/permissions_test.go` around lines 43 - 62, Add missing test cases for WorkloadPermissions.HasRead by expanding TestWorkloadPermissions_HasRead to include at least: (1) an empty WorkloadPermissions{} instance that should return false, and (2) a WorkloadPermissions with Read set to true that should return true (optionally also include Read:false to assert false). Use the same table-driven pattern as existing tests (iterate over tests, call tt.perms.HasRead(), and assert equality), referencing the WorkloadPermissions type and the HasRead method to locate the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cluster/kube/builder/statefulset.go`:
- Around line 68-72: Run gofmt on the file and apply its fixes: reformat the
struct literal that includes AutomountServiceAccountToken, ServiceAccountName,
Containers, ImagePullSecrets and Volumes (the code that uses
b.automountServiceAccountToken(), b.serviceAccountName(), b.container(),
b.secretsRefs, b.volumesObjs) so it matches gofmt output (or run gofmt -w) and
commit the formatted file; re-run the provided gofmt check to verify no
remaining diffs.
---
Nitpick comments:
In `@cluster/kube/builder/permissions_test.go`:
- Around line 43-62: Add missing test cases for WorkloadPermissions.HasRead by
expanding TestWorkloadPermissions_HasRead to include at least: (1) an empty
WorkloadPermissions{} instance that should return false, and (2) a
WorkloadPermissions with Read set to true that should return true (optionally
also include Read:false to assert false). Use the same table-driven pattern as
existing tests (iterate over tests, call tt.perms.HasRead(), and assert
equality), referencing the WorkloadPermissions type and the HasRead method to
locate the code to modify.
In `@cluster/kube/builder/permissions.go`:
- Around line 85-121: The GenerateRules function currently returns nil when
permissions is nil but an empty slice when no mappings match; make the return
type consistent by initializing rules as an empty slice up front and returning
that for nil/empty input. Specifically, in GenerateRules (and before the seen
map), set rules to an explicitly allocated zero-length slice of
rbacv1.PolicyRule and change the early nil check for permissions to return that
empty slice instead of nil; keep using seen and mapping.Generator as-is and
simply ensure the function always returns the initialized rules slice.
cloud-j-luna
force-pushed
the
feature/automount-sa-token
branch
2 times, most recently
from
25af23a to
71b5b17
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cluster/kube/builder/role_test.go (2)
52-72: Test assertions are order-dependent.The test assumes
role.Rules[0]is pods,role.Rules[1]is pods/log, androle.Rules[2]is deployments. If the rule generation order changes (e.g., due to map iteration or refactoring), this test will fail even if the correct rules are generated.💡 Consider order-independent assertions
// Helper to find rule by resource findRule := func(rules []rbacv1.PolicyRule, resource string) *rbacv1.PolicyRule { for i := range rules { if slices.Contains(rules[i].Resources, resource) { return &rules[i] } } return nil } podsRule := findRule(role.Rules, "pods") require.NotNil(t, podsRule) require.Contains(t, podsRule.Verbs, "get") // ... etc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/role_test.go` around lines 52 - 72, The test currently assumes a fixed order in role.Rules (accessing role.Rules[0], [1], [2]), which makes it flaky; update role_test.go to perform order-independent assertions by locating rules by resource name (e.g., find the rule that contains "pods", the rule that contains "pods/log", and the rule that contains "deployments") and then assert the expected Verbs and APIGroups for each found rule (verify pods has get/list/watch, pods/log has get/list, and deployments has APIGroups == []string{"apps"} plus get/list/watch); add a small helper (e.g., findRule) to scan role.Rules for a given resource and require the returned rule is not nil before asserting its fields.
22-24: Variable shadowing:sdlshadows the imported package.The variable
sdlshadows the importedpkg.akt.dev/go/sdlpackage. While this compiles, it prevents subsequent use of the package within the same scope and reduces code clarity.♻️ Suggested fix
- sdl, err := sdl.ReadFile("../../../testdata/sdl/permissions.yaml") + sdlData, err := sdl.ReadFile("../../../testdata/sdl/permissions.yaml") require.NoError(t, err) - mani, err := sdl.Manifest() + mani, err := sdlData.Manifest()Apply the same pattern at line 116.
Also applies to: 115-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/role_test.go` around lines 22 - 24, The test declares a local variable named `sdl` which shadows the imported package `sdl` (e.g., the call `sdl.ReadFile(...)`), so rename the local buffer variable to something like `sdlBytes` or `content` (e.g., `sdlBytes, err := sdl.ReadFile(...); require.NoError(t, err)`) so the package symbol remains available; apply the same rename pattern to the other occurrence referenced in the comment (the similar declaration around lines 115-117) so no local variable shadows the imported `sdl` package.cluster/kube/builder/permissions.go (1)
154-180: Consider removing the commented-out events stub.Keeping large commented code blocks tends to rot; prefer tracking this in an issue or a dedicated follow‑up PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/permissions.go` around lines 154 - 180, Remove the commented-out events stub block (the eventsResourceMapping function and its TODO) from permissions.go; instead open or reference a follow-up issue/PR to reintroduce event RBAC when implemented. Locate the commented block containing eventsResourceMapping, ResourcePolicyMapping, and PermissionType cases (PermissionTypeRead/Write/Delete) and delete it so only active RBAC mappings remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cluster/kube/builder/permissions.go`:
- Around line 136-140: The RBAC rule that grants access to the pods/log
subresource incorrectly includes the "list" verb; update the permission block
(the rule with APIGroups: []string{""} and Resources: []string{"pods/log"}) to
remove "list" from the Verbs slice so it only grants "get" (i.e., change Verbs:
[]string{"get", "list"} to only include "get") to follow Kubernetes subresource
capabilities and least-privilege practices.
In `@cluster/kube/client.go`:
- Around line 543-551: The code currently only creates/apply RBAC via
applyServiceAccount/applyRole/applyRoleBinding when workload.HasPermissions() is
true, leaving orphaned RBAC objects when permissions are removed; update the
branch where HasPermissions() is false to proactively clean up existing RBAC for
that service by (1) resolving the ServiceAccount/Role/RoleBinding names or
labels (use the same builder.BuildServiceAccount/BuildRole/BuildRoleBinding
naming conventions or check svc.serviceAccount/svc.role/svc.roleBinding fields)
and calling the corresponding delete/cleanup functions (or call apply* with an
explicit delete) so existing objects are removed, and (2) extend
cleanupStaleResources to include label-based deletion for RBAC resources
(ServiceAccount, Role, RoleBinding) so stale RBAC objects are cleaned up like
Deployments/Services; ensure you reference
applyServiceAccount/applyRole/applyRoleBinding, HasPermissions(),
svc.serviceAccount/svc.role/svc.roleBinding, and cleanupStaleResources when
making the changes.
In `@pkg/apis/akash.network/v2beta2/zz_generated.deepcopy.go`:
- Around line 523-542: The DeepCopyInto implementation for
ManifestServicePermissions only deep-copies the Read slice and leaves Write and
Delete sharing underlying arrays due to the shallow *out = *in; update
DeepCopyInto (function ManifestServicePermissions.DeepCopyInto) to deep-copy the
Write and Delete slice fields the same way Read is handled (allocate new slices
and copy elements) so all three slices are independent; regenerate deepcopy code
with deepcopy-gen after ensuring the ManifestServicePermissions type includes
Read, Write, Delete so the generator emits proper copying for each slice.
---
Duplicate comments:
In `@go.mod`:
- Around line 68-76: The replace directives for github.com/cometbft/cometbft and
github.com/cosmos/cosmos-sdk point to older fork versions (v0.38.19-akash.1 and
v0.53.4-akash.b.10) that conflict with the versions pinned in the require
section (v0.38.21 and v0.53.5), causing API drift; update the replace entries to
match the required versions (or update the require pins to match the forked
versions) so that github.com/cometbft/cometbft and github.com/cosmos/cosmos-sdk
use consistent versions during build, ensuring the replace lines reference
v0.38.21 (or the intended forked tag) and v0.53.5 (or the intended forked tag)
respectively and keep the github.com/99designs/keyring =>
github.com/cosmos/keyring line consistent if needed.
---
Nitpick comments:
In `@cluster/kube/builder/permissions.go`:
- Around line 154-180: Remove the commented-out events stub block (the
eventsResourceMapping function and its TODO) from permissions.go; instead open
or reference a follow-up issue/PR to reintroduce event RBAC when implemented.
Locate the commented block containing eventsResourceMapping,
ResourcePolicyMapping, and PermissionType cases
(PermissionTypeRead/Write/Delete) and delete it so only active RBAC mappings
remain.
In `@cluster/kube/builder/role_test.go`:
- Around line 52-72: The test currently assumes a fixed order in role.Rules
(accessing role.Rules[0], [1], [2]), which makes it flaky; update role_test.go
to perform order-independent assertions by locating rules by resource name
(e.g., find the rule that contains "pods", the rule that contains "pods/log",
and the rule that contains "deployments") and then assert the expected Verbs and
APIGroups for each found rule (verify pods has get/list/watch, pods/log has
get/list, and deployments has APIGroups == []string{"apps"} plus
get/list/watch); add a small helper (e.g., findRule) to scan role.Rules for a
given resource and require the returned rule is not nil before asserting its
fields.
- Around line 22-24: The test declares a local variable named `sdl` which
shadows the imported package `sdl` (e.g., the call `sdl.ReadFile(...)`), so
rename the local buffer variable to something like `sdlBytes` or `content`
(e.g., `sdlBytes, err := sdl.ReadFile(...); require.NoError(t, err)`) so the
package symbol remains available; apply the same rename pattern to the other
occurrence referenced in the comment (the similar declaration around lines
115-117) so no local variable shadows the imported `sdl` package.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cluster/kube/builder/role_test.go`:
- Around line 52-71: The test is brittle because it indexes role.Rules by
position and expects verbs for "pods/log" that the generator doesn't emit;
change the assertions to find the rule whose Resources contains "pods/log"
(iterate role.Rules and match on Resources) and assert the expected verbs
exactly (e.g., only "get" per the generator) rather than using role.Rules[1];
likewise remove positional coupling for other checks by locating rules by
resource name when asserting APIGroups/Verbs on role.Rules.
In `@cluster/kube/cleanup.go`:
- Around line 83-92: The selector building uses labels.NewRequirement(...,
selection.NotIn, servicesWithPermissions) without guarding an empty
servicesWithPermissions slice, which causes Kubernetes errors when the slice is
empty; update the code that creates req1/req2 (labels.NewRequirement calls
referencing builder.AkashManifestServiceLabelName and
builder.AkashManagedLabelName) to first check if servicesWithPermissions is
empty and only create/add the NotIn requirement when it contains elements,
otherwise build rbacSelector using only the Equals requirement for
builder.AkashManagedLabelName (or skip the NotIn addition), so rbacSelector :=
labels.NewSelector().Add(*req2).String() in the empty case.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cluster/kube/builder/builder.gocluster/kube/builder/permissions.gocluster/kube/builder/role.gocluster/kube/builder/role_test.gocluster/kube/builder/rolebinding.gocluster/kube/builder/serviceaccount.gocluster/kube/cleanup.gocluster/kube/cleanup_test.gocluster/kube/client.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cluster/kube/builder/serviceaccount.go
- cluster/kube/builder/builder.go
- cluster/kube/builder/role.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cluster/kube/builder/role_test.go (1)
52-70:⚠️ Potential issue | 🟡 MinorAvoid order-dependent rule assertions.
Index-based checks on
role.Rulesare brittle if rule ordering changes; prefer lookup by resource and assert verbs/APIGroups on the matched rule.🔧 Suggested update
@@ import ( "testing" "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" @@ - // Check pods rule - require.Contains(t, role.Rules[0].Resources, "pods") - require.Contains(t, role.Rules[0].Verbs, "get") - require.Contains(t, role.Rules[0].Verbs, "list") - require.Contains(t, role.Rules[0].Verbs, "watch") - - // Check pods/log rule - require.Contains(t, role.Rules[1].Resources, "pods/log") - require.Contains(t, role.Rules[1].Verbs, "get") - - // Check deployments rule - require.Contains(t, role.Rules[2].Resources, "deployments") - require.Equal(t, []string{"apps"}, role.Rules[2].APIGroups) - require.Contains(t, role.Rules[2].Verbs, "get") - require.Contains(t, role.Rules[2].Verbs, "list") - require.Contains(t, role.Rules[2].Verbs, "watch") + podsRule := findRuleByResource(t, role.Rules, "pods") + require.ElementsMatch(t, []string{"get", "list", "watch"}, podsRule.Verbs) + + podsLogRule := findRuleByResource(t, role.Rules, "pods/log") + require.ElementsMatch(t, []string{"get"}, podsLogRule.Verbs) + + deploymentsRule := findRuleByResource(t, role.Rules, "deployments") + require.Equal(t, []string{"apps"}, deploymentsRule.APIGroups) + require.ElementsMatch(t, []string{"get", "list", "watch"}, deploymentsRule.Verbs) }) } + +func findRuleByResource(t *testing.T, rules []rbacv1.PolicyRule, resource string) rbacv1.PolicyRule { + t.Helper() + for _, rule := range rules { + for _, res := range rule.Resources { + if res == resource { + return rule + } + } + } + t.Fatalf("rule for resource %q not found", resource) + return rbacv1.PolicyRule{} +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/role_test.go` around lines 52 - 70, The tests currently assert on role.Rules by fixed indices which is brittle; change the assertions to find rules by resource name instead of assuming order: iterate/search role.Rules for the rule whose Resources contains "pods", assert its Verbs include "get","list","watch"; find the rule with Resources containing "pods/log" and assert Verbs include "get"; find the rule with Resources containing "deployments" and assert its APIGroups equals []string{"apps"} and Verbs include "get","list","watch". Use the existing role.Rules, rule.Resources, rule.Verbs and rule.APIGroups symbols to locate and validate the matching rules.
🧹 Nitpick comments (1)
cluster/kube/builder/role_test.go (1)
73-112: Avoid commented-out test blocks; prefert.Skipor an issue link.The disabled test will rot; consider re-enabling with
t.Skip(...)and referencing a tracking issue, or remove the block and track separately.If you want, I can draft a skipped test template or open a tracking issue for the events permissions case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cluster/kube/builder/role_test.go` around lines 73 - 112, Replace the large commented-out test block with an active skipped test so it doesn't rot: restore the t.Run("generates logs and events permissions correctly", ...) block and at the top call t.Skip with a short message and an issue/PR reference; keep the existing setup and assertions (references: NewWorkloadBuilder, BuildRole, ClusterDeployment, roleBuilder.Create) so the test is runnable and documents the missing work, or alternatively remove the block entirely and add a one-line comment linking to a tracking issue number for the events permissions case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cluster/kube/builder/role_test.go`:
- Around line 52-70: The tests currently assert on role.Rules by fixed indices
which is brittle; change the assertions to find rules by resource name instead
of assuming order: iterate/search role.Rules for the rule whose Resources
contains "pods", assert its Verbs include "get","list","watch"; find the rule
with Resources containing "pods/log" and assert Verbs include "get"; find the
rule with Resources containing "deployments" and assert its APIGroups equals
[]string{"apps"} and Verbs include "get","list","watch". Use the existing
role.Rules, rule.Resources, rule.Verbs and rule.APIGroups symbols to locate and
validate the matching rules.
---
Nitpick comments:
In `@cluster/kube/builder/role_test.go`:
- Around line 73-112: Replace the large commented-out test block with an active
skipped test so it doesn't rot: restore the t.Run("generates logs and events
permissions correctly", ...) block and at the top call t.Skip with a short
message and an issue/PR reference; keep the existing setup and assertions
(references: NewWorkloadBuilder, BuildRole, ClusterDeployment,
roleBuilder.Create) so the test is runnable and documents the missing work, or
alternatively remove the block entirely and add a one-line comment linking to a
tracking issue number for the events permissions case.
vertex451
previously approved these changes
Feb 23, 2026
cloud-j-luna
force-pushed
the
feature/automount-sa-token
branch
from
65135d4 to
ee9d30f
Compare
cloud-j-luna
force-pushed
the
feature/automount-sa-token
branch
from
ee9d30f to
fb3f813
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.