feat: automatic core type generation #136
Conversation
WalkthroughInjects a discovery-client factory into authorization-model generation to discover and render Kubernetes API resources; changes invite.Process to no-op when Account.Spec.Creator is nil; replaces mockery config and regenerates many mocks with new constructors; adds a direct go.mod dependency and several test updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/subroutine/invite.go (1)
77-86: I need to verify the reconciler setup more comprehensively to confirm whether Account changes trigger reconciliation through any mechanism.Now let me check the reconciler's setup method to confirm the watch configuration:
Let me now search the repository for the reconciler setup to confirm the watch configuration:
Based on my verification:
- Script execution confirmed: No explicit Watches or Owns on Account objects found in the codebase.
- Controllers require Watches to be configured to enqueue reconcile.Requests in response to events, and without an Account watch, RequeueAfter duration can be set in the Reconciler return object and will run again after that duration even if the object is unchanged.
- Go validation idiom confirmed: To check for strings that contain only whitespace, use strings.TrimSpace(str) == "", which is exactly what the suggested diff does after nil-checking the
*stringpointer.The original review comment is well-founded. Without a Watch on Account, if Creator is set after the nil check, no automatic reconciliation occurs. Adding
RequeueAfter: time.Minuteensures eventual consistency through periodic retries, and the TrimSpace check prevents silent failures with whitespace-only emails.
Add RequeueAfter and validate non-empty email to ensure eventual consistency and prevent stalled Invite creation.
When
account.Spec.Creatoris nil, the reconciler returns early. Without a watch on Account updates, if Creator is set later, reconciliation won't auto-trigger. Additionally, an empty or whitespace-only email bypasses validation. Apply this fix:- if account.Spec.Creator == nil { - log.Info().Str("workspace", wsName).Msg("account creator is nil, skipping invite creation") - return ctrl.Result{}, nil - } + if account.Spec.Creator == nil || strings.TrimSpace(*account.Spec.Creator) == "" { + log.Info().Str("workspace", wsName).Msg("account creator is nil/empty, skipping invite creation") + // Light backoff so we eventually pick up a late Creator without flooding the queue. + return ctrl.Result{RequeueAfter: time.Minute}, nil + }Add imports:
import ( // ... "strings" "time" )
🧹 Nitpick comments (1)
internal/subroutine/authorization_model.go (1)
154-161: Preserve any existing host path prefix when scoping to a workspace.Overwriting parsed.Path may drop an existing reverse-proxy prefix. Prefer joining with the existing path.
- parsed.Path, err = url.JoinPath("clusters", fmt.Sprintf("root:orgs:%s", store.Name)) + parsed.Path, err = url.JoinPath(parsed.Path, "clusters", fmt.Sprintf("root:orgs:%s", store.Name))If you need stricter control, derive from a clean URL base and set cfg.PathPrefix instead.
Please confirm your cluster config contains no required path prefix; otherwise requests may 404 behind proxies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(1 hunks)internal/subroutine/authorization_model.go(4 hunks)internal/subroutine/invite.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / lint / lint
🔇 Additional comments (3)
go.mod (1)
18-18: Module hygiene confirmed; dependency bump is clean.The v0.7.3 bump is already properly synced in go.sum (lines 162-163). The release only includes a bug fix for status condition updates, with no breaking changes. No further action needed.
internal/subroutine/authorization_model.go (2)
30-32: Schema version "1.2" is correct and compatible.OpenFGA supports schema version 1.2, and the openfga/language Go package v0.2.0-beta.2 examples demonstrate compatibility with schema "1.2". No changes needed.
268-293: The review comment misdiagnoses the actual issue and suggests an ineffective fix.The real problem is not filename collision—it's that the template generates type definitions with group-prefixed names (e.g.,
authorization_k8s_io_pod) but hardcodes parent type references to onlycore_namespaceandcore_platform-mesh_io_account(lines 41, 58). These parent types don't exist in the codebase for non-core groups, causing generated modules from groups likeauthentication.k8s.ioorauthorization.k8s.ioto reference undefined parent types.Changing filenames does not affect the type names generated by the template—it only changes the module name wrapping. The suggestion leaves the actual bug (hardcoded parent references) unresolved. The template logic needs fixing, not the filename pattern.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/subroutine/authorization_model.go (2)
258-264: Fallback when SingularName is empty and sanitize identifiers.Some APIResources have empty SingularName; fall back to lowercased Kind and sanitize.
- err := tpl.Execute(&buffer, modelInput{ - Name: resource.Name, - Group: strings.ReplaceAll(group, ".", "_"), // TODO: group name length capping - Singular: resource.SingularName, + singular := resource.SingularName + if singular == "" { + singular = strings.ToLower(resource.Kind) + } + err := tpl.Execute(&buffer, modelInput{ + Name: resource.Name, + Group: strings.ReplaceAll(group, ".", "_"), // TODO: group name length capping + Singular: singular, Scope: string(scope), })
38-71: Avoid module/file name collisions across groups (include group in names).Current module header and file name use only resource.Name, which can collide across API groups. Include sanitized group.
- priviledgedTemplate = template.Must(template.New("model").Parse(`module internal_core_types_{{ .Name }} + priviledgedTemplate = template.Must(template.New("model").Parse(`module internal_core_types_{{ .Group }}_{{ .Name }}And for generated files:
- files = append(files, language.ModuleFile{ - Name: fmt.Sprintf("internal_core_types_%s.fga", apiRes.Name), + group := apiRes.Group + if group == "" { group = "core" } + group = strings.ReplaceAll(group, ".", "_") + files = append(files, language.ModuleFile{ + Name: fmt.Sprintf("internal_core_types_%s_%s.fga", group, apiRes.Name), Contents: buf.String(), })Apply the same module-name change to the core template referenced by modelTpl to keep headers and filenames aligned.
Also applies to: 290-294
🧹 Nitpick comments (5)
internal/subroutine/mocks/mock_OpenFGAServiceClient.go (1)
42-51: Standardize variadic grpc.CallOption handling (avoid mismatched expectations).The methods package opts as a single []grpc.CallOption (Called(ctx, in, opts)), while the Expecter helpers expand them (append(..., opts...)...). If any production code passes options, expectations won’t match and Run handlers will panic on args[2].([]grpc.CallOption).
Align the Expecter to pass a single []grpc.CallOption (as the methods do). Example for BatchCheck:
func (_e *MockOpenFGAServiceClient_Expecter) BatchCheck(ctx interface{}, in interface{}, opts ...interface{}) *MockOpenFGAServiceClient_BatchCheck_Call { - return &MockOpenFGAServiceClient_BatchCheck_Call{Call: _e.mock.On("BatchCheck", - append([]interface{}{ctx, in}, opts...)...)} + // Pack variadic opts into a single slice to match method invocation. + var packed interface{} + if len(opts) > 0 { + packed = opts + return &MockOpenFGAServiceClient_BatchCheck_Call{Call: _e.mock.On("BatchCheck", ctx, in, packed)} + } + return &MockOpenFGAServiceClient_BatchCheck_Call{Call: _e.mock.On("BatchCheck", ctx, in)} }Please apply the same pattern across all methods with grpc.CallOption varargs.
Also applies to: 85-93, 125-134
internal/subroutine/invite_test.go (1)
65-84: Restore coverage for “Creator == nil” no-op path.A test for Account.Spec.Creator == nil was removed. Please add a case asserting we do not create an Invite and return nil error. I can draft it if helpful.
internal/subroutine/mocks/mock_CTRLManager.go (1)
25-37: LGTM; add compile-time interface check to catch drift early.Looks correct for controller-runtime’s Manager. Optionally add an interface assertion to fail fast if upstream signatures change.
+// Compile-time check: ensure CTRLManager satisfies controller-runtime manager.Manager. +var _ manager.Manager = (*CTRLManager)(nil)internal/subroutine/mocks/mock_Manager.go (1)
23-35: LGTM; add compile-time interface check to guard against upstream changes.The surface matches multicluster-runtime Manager. Add assertion to fail fast if the API drifts.
+// Compile-time check: ensure MockManager satisfies multicluster manager.Manager. +var _ manager.Manager = (*MockManager)(nil)internal/subroutine/mocks/mock_DiscoveryInterface.go (1)
17-29: LGTM; add compile-time interface check for discovery.Add a satisfaction assertion to catch future API drift.
+// Compile-time check: ensure MockDiscoveryInterface satisfies discovery.DiscoveryInterface. +var _ discovery.DiscoveryInterface = (*MockDiscoveryInterface)(nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.mockery.yaml(1 hunks)go.mod(1 hunks)internal/controller/store_controller.go(2 hunks)internal/subroutine/authorization_model.go(4 hunks)internal/subroutine/authorization_model_test.go(3 hunks)internal/subroutine/invite_test.go(1 hunks)internal/subroutine/mocks/mock_CTRLManager.go(1 hunks)internal/subroutine/mocks/mock_Client.go(19 hunks)internal/subroutine/mocks/mock_Cluster.go(20 hunks)internal/subroutine/mocks/mock_DiscoveryInterface.go(1 hunks)internal/subroutine/mocks/mock_Manager.go(24 hunks)internal/subroutine/mocks/mock_OpenFGAServiceClient.go(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (2)
internal/controller/store_controller.go (1)
internal/subroutine/authorization_model.go (1)
NewAuthorizationModelSubroutine(83-90)
internal/subroutine/authorization_model_test.go (3)
internal/subroutine/authorization_model.go (1)
NewAuthorizationModelSubroutine(83-90)internal/subroutine/mocks/mock_CTRLManager.go (1)
NewCTRLManager(27-37)internal/subroutine/mocks/mock_DiscoveryInterface.go (1)
NewMockDiscoveryInterface(19-29)
🔇 Additional comments (5)
internal/subroutine/authorization_model_test.go (1)
57-70: Update tests for discovery factory returning (client, error).If you adopt the error-returning factory, adjust constructor usages:
- subroutine := subroutine.NewAuthorizationModelSubroutine(nil, nil, nil, nil, nil) + subroutine := subroutine.NewAuthorizationModelSubroutine(nil, nil, nil, nil, nil)(no change needed when passing nil)
And where a mock is provided:
- subroutine := subroutine.NewAuthorizationModelSubroutine(fga, manager, client, func(cfg *rest.Config) discovery.DiscoveryInterface { return discoveryMock }, logger.Logger) + subroutine := subroutine.NewAuthorizationModelSubroutine( + fga, manager, client, + func(cfg *rest.Config) (discovery.DiscoveryInterface, error) { return discoveryMock, nil }, + logger.Logger, + )Also applies to: 337-345
internal/subroutine/mocks/mock_Cluster.go (1)
20-32: LGTM on mock constructor and expecter pattern.Constructor wiring and cleanup look correct.
.mockery.yaml (1)
1-9: YAML syntax is valid; manual verification of regeneration still required.The
.mockery.yamlconfiguration parses correctly with proper structure across all six package definitions. However, mockery CLI is unavailable in this environment, so you must manually verify that running mockery produces no unintended drift by executing:mockery --config .mockery.yaml --all --keeptree --dry-run git status --porcelainThis will confirm the current config regenerates the exact existing mock files without changes.
internal/subroutine/mocks/mock_Client.go (2)
44-66: Double-check obj type for Apply: runtime.ApplyConfiguration may not exist in apimachinery.If you’re targeting controller-runtime v0.22.x, verify the exact obj type required by Client.Apply (if present). Mismatch will break builds.
If Apply is not on client.Client (or uses a different obj type), regenerate this mock against the correct interface version to avoid compile errors.
72-79: Fix variadic option mismatch: expectations expand opts, invocations pass a slice — leads to unmatched mocks.Today:
- Methods call
_mock.Called(..., opts)(single slice).- Expecters call
.On(..., append(..., opts...)...)(expanded).- Run handlers expect
args[n].([]client.<Option>).Result: when options are provided, the call signatures don’t match and tests will fail.
Minimal, safe fix: keep invocations passing a single typed slice; change Expecter helpers to also pass a single typed slice by converting
...interface{}to the correct[]client.<Option>type.Apply this pattern across all affected Expecters:
--- a/internal/subroutine/mocks/mock_Client.go +++ b/internal/subroutine/mocks/mock_Client.go @@ func (_e *MockClient_Expecter) Apply(ctx interface{}, obj interface{}, opts ...interface{}) *MockClient_Apply_Call { - return &MockClient_Apply_Call{Call: _e.mock.On("Apply", - append([]interface{}{ctx, obj}, opts...)...)} + // Pack variadics into a single typed []client.ApplyOption to match invocation. + packed := make([]client.ApplyOption, 0, len(opts)) + for _, o := range opts { + packed = append(packed, o.(client.ApplyOption)) + } + return &MockClient_Apply_Call{Call: _e.mock.On("Apply", ctx, obj, packed)} } @@ func (_e *MockClient_Expecter) Create(ctx interface{}, obj interface{}, opts ...interface{}) *MockClient_Create_Call { - return &MockClient_Create_Call{Call: _e.mock.On("Create", - append([]interface{}{ctx, obj}, opts...)...)} + packed := make([]client.CreateOption, 0, len(opts)) + for _, o := range opts { + packed = append(packed, o.(client.CreateOption)) + } + return &MockClient_Create_Call{Call: _e.mock.On("Create", ctx, obj, packed)} } @@ func (_e *MockClient_Expecter) Delete(ctx interface{}, obj interface{}, opts ...interface{}) *MockClient_Delete_Call { - return &MockClient_Delete_Call{Call: _e.mock.On("Delete", - append([]interface{}{ctx, obj}, opts...)...)} + packed := make([]client.DeleteOption, 0, len(opts)) + for _, o := range opts { + packed = append(packed, o.(client.DeleteOption)) + } + return &MockClient_Delete_Call{Call: _e.mock.On("Delete", ctx, obj, packed)} } @@ func (_e *MockClient_Expecter) DeleteAllOf(ctx interface{}, obj interface{}, opts ...interface{}) *MockClient_DeleteAllOf_Call { - return &MockClient_DeleteAllOf_Call{Call: _e.mock.On("DeleteAllOf", - append([]interface{}{ctx, obj}, opts...)...)} + packed := make([]client.DeleteAllOfOption, 0, len(opts)) + for _, o := range opts { + packed = append(packed, o.(client.DeleteAllOfOption)) + } + return &MockClient_DeleteAllOf_Call{Call: _e.mock.On("DeleteAllOf", ctx, obj, packed)} } @@ func (_e *MockClient_Expecter) Get(ctx interface{}, key interface{}, obj interface{}, opts ...interface{}) *MockClient_Get_Call { - return &MockClient_Get_Call{Call: _e.mock.On("Get", - append([]interface{}{ctx, key, obj}, opts...)...)} + packed := make([]client.GetOption, 0, len(opts)) + for _, o := range opts { + packed = append(packed, o.(client.GetOption)) + } + return &MockClient_Get_Call{Call: _e.mock.On("Get", ctx, key, obj, packed)} } @@ func (_e *MockClient_Expecter) List(ctx interface{}, list interface__, opts ...interface{}) *MockClient_List_Call { - return &MockClient_List_Call{Call: _e.mock.On("List", - append([]interface{}{ctx, list}, opts...)...)} + packed := make([]client.ListOption, 0, len(opts)) + for _, o := range opts { + packed = append(packed, o.(client.ListOption)) + } + return &MockClient_List_Call{Call: _e.mock.On("List", ctx, list, packed)} } @@ func (_e *MockClient_Expecter) Patch(ctx interface{}, obj interface{}, patch interface{}, opts ...interface{}) *MockClient_Patch_Call { - return &MockClient_Patch_Call{Call: _e.mock.On("Patch", - append([]interface{}{ctx, obj, patch}, opts...)...)} + packed := make([]client.PatchOption, 0, len(opts)) + for _, o := range opts { + packed = append(packed, o.(client.PatchOption)) + } + return &MockClient_Patch_Call{Call: _e.mock.On("Patch", ctx, obj, patch, packed)} } @@ func (_e *MockClient_Expecter) Update(ctx interface{}, obj interface{}, opts ...interface{}) *MockClient_Update_Call { - return &MockClient_Update_Call{Call: _e.mock.On("Update", - append([]interface{}{ctx, obj}, opts...)...)} + packed := make([]client.UpdateOption, 0, len(opts)) + for _, o := range opts { + packed = append(packed, o.(client.UpdateOption)) + } + return &MockClient_Update_Call{Call: _e.mock.On("Update", ctx, obj, packed)} }Add a compile-time interface check to catch future drift:
+// Compile-time check: ensure MockClient satisfies controller-runtime client.Client. +var _ client.Client = (*MockClient)(nil)To ensure the Apply signature matches your controller-runtime version, please confirm:
Also applies to: 148-151, 220-223, 293-295, 365-368, 563-565, 636-638, 904-906
Signed-off-by: aaronschweig <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/subroutine/authorization_model.go (1)
235-259: Handle empty SingularName to prevent invalid type identifiers.Some Kubernetes API resources have an empty
SingularNamefield, which would result in invalid type names in the generated schema (e.g.,core_instead ofcore_namespace).Apply this diff to fall back to the resource Kind when SingularName is empty:
func processAPIResourceIntoModel(resource metav1.APIResource, tpl *template.Template) (bytes.Buffer, error) { scope := apiextensionsv1.ClusterScoped if resource.Namespaced { scope = apiextensionsv1.NamespaceScoped } group := "core" if resource.Group != "" { group = resource.Group } + singular := resource.SingularName + if singular == "" { + singular = strings.ToLower(resource.Kind) + } + var buffer bytes.Buffer err := tpl.Execute(&buffer, modelInput{ Name: resource.Name, Group: strings.ReplaceAll(group, ".", "_"), - Singular: resource.SingularName, + Singular: singular, Scope: string(scope), })
🧹 Nitpick comments (2)
internal/subroutine/authorization_model_test.go (1)
212-249: Consider reusing actual templates to reduce test brittleness.The hardcoded module structure duplicates the logic from the main code. If the templates in
authorization_model.gochange, this test setup must be manually updated.Consider importing and executing the actual templates to generate the expected module content dynamically, which would keep tests aligned with production code:
// Import the template from the main package or expose it for testing buf, err := processAPIResourceIntoModel(metav1.APIResource{ Name: "namespaces", SingularName: "namespace", Namespaced: false, Group: "", }, priviledgedTemplate) assert.NoError(t, err) moduleFiles = append(moduleFiles, language.ModuleFile{ Name: "internal_core_types_namespaces.fga", Contents: buf.String(), })This would eliminate the hardcoded string and ensure tests stay synchronized with template changes.
internal/subroutine/authorization_model.go (1)
149-149: Consider extracting magic string to a named constant.The hard-coded string
"orgs"represents a special store name that controls whether discovery is enabled.Consider defining it as a constant for clarity:
const ( schemaVersion = "1.2" organizationStoreName = "orgs" )Then use it:
- if store.Name != "orgs" { + if store.Name != organizationStoreName {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/subroutine/authorization_model.go(5 hunks)internal/subroutine/authorization_model_test.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-24T06:53:47.110Z
Learnt from: aaronschweig
PR: platform-mesh/security-operator#136
File: internal/subroutine/authorization_model.go:0-0
Timestamp: 2025-10-24T06:53:47.110Z
Learning: In internal/subroutine/authorization_model.go, the privileged template extends parent types (core_platform-mesh_io_account or core_namespace) with create/list/watch permissions for privileged resources. These permissions are defined at the parent level (e.g., create_{{ .Group }}_{{ .Name }}) rather than directly on the resource types, delegating control to the account or namespace scope.
Applied to files:
internal/subroutine/authorization_model.gointernal/subroutine/authorization_model_test.go
📚 Learning: 2025-10-24T06:20:22.771Z
Learnt from: aaronschweig
PR: platform-mesh/security-operator#136
File: internal/subroutine/authorization_model.go:74-81
Timestamp: 2025-10-24T06:20:22.771Z
Learning: In internal/subroutine/authorization_model.go, the NewDiscoveryClientFunc factory does not need to return an error because the *rest.Config passed to it is copied from an already validated manager config (a.mgr.GetLocalManager().GetConfig()), so the config is guaranteed to be valid at that point.
Applied to files:
internal/subroutine/authorization_model.go
🧬 Code graph analysis (1)
internal/subroutine/authorization_model_test.go (3)
internal/subroutine/authorization_model.go (1)
NewAuthorizationModelSubroutine(83-90)internal/subroutine/mocks/mock_CTRLManager.go (1)
NewCTRLManager(27-37)internal/subroutine/mocks/mock_DiscoveryInterface.go (1)
NewMockDiscoveryInterface(19-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / lint / lint
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (2)
internal/subroutine/authorization_model.go (1)
198-216: Improved model comparison approach.Using protobuf JSON serialization for comparison avoids DSL ordering issues and provides more reliable semantic equality checking.
internal/subroutine/authorization_model_test.go (1)
94-98: Fix compilation error: testing.T has no Context() method.The
testing.Ttype does not have aContext()method in standard Go. This will cause a compilation error.Apply this diff to fix the error:
func TestAuthorizationModelFinalize(t *testing.T) { subroutine := subroutine.NewAuthorizationModelSubroutine(nil, nil, nil, nil, nil) - _, err := subroutine.Finalize(t.Context(), nil) + _, err := subroutine.Finalize(context.Background(), nil) assert.Nil(t, err) }Likely an incorrect or invalid review comment.
| cmds: | ||
| - echo "https://openmfp:[email protected]" >> $HOME/.git-credentials | ||
| - git config --global url."https://${PLATFORM_MESH_TOKEN}@github.com/".insteadOf "https://github.com/" | ||
| - go test -count=1 ./... {{.ADDITIONAL_COMMAND_ARGS}} |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/subroutine/authorization_model.go (2)
235-259: Handle empty SingularName to avoid invalid type identifiers.Some Kubernetes APIResources have an empty
SingularName, which would produce invalid type names in the generated schema.Apply this diff to add a fallback:
func processAPIResourceIntoModel(resource metav1.APIResource, tpl *template.Template) (bytes.Buffer, error) { scope := apiextensionsv1.ClusterScoped if resource.Namespaced { scope = apiextensionsv1.NamespaceScoped } group := "core" if resource.Group != "" { group = resource.Group } + + singular := resource.SingularName + if singular == "" { + singular = strings.ToLower(resource.Kind) + } var buffer bytes.Buffer err := tpl.Execute(&buffer, modelInput{ Name: resource.Name, Group: strings.ReplaceAll(group, ".", "_"), - Singular: resource.SingularName, + Singular: singular, Scope: string(scope), })
158-158: Ensure absolute path when rewriting API server URL.
url.JoinPath("clusters", ...)may produce a relative path. Preserve the base path or ensure a leading slash.Apply this diff:
-parsed.Path, err = url.JoinPath("clusters", fmt.Sprintf("root:orgs:%s", store.Name)) +parsed.Path, err = url.JoinPath("/", "clusters", fmt.Sprintf("root:orgs:%s", store.Name))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/subroutine/authorization_model.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-24T06:53:47.110Z
Learnt from: aaronschweig
PR: platform-mesh/security-operator#136
File: internal/subroutine/authorization_model.go:0-0
Timestamp: 2025-10-24T06:53:47.110Z
Learning: In internal/subroutine/authorization_model.go, the privileged template extends parent types (core_platform-mesh_io_account or core_namespace) with create/list/watch permissions for privileged resources. These permissions are defined at the parent level (e.g., create_{{ .Group }}_{{ .Name }}) rather than directly on the resource types, delegating control to the account or namespace scope.
Applied to files:
internal/subroutine/authorization_model.go
📚 Learning: 2025-10-24T06:20:22.771Z
Learnt from: aaronschweig
PR: platform-mesh/security-operator#136
File: internal/subroutine/authorization_model.go:74-81
Timestamp: 2025-10-24T06:20:22.771Z
Learning: In internal/subroutine/authorization_model.go, the NewDiscoveryClientFunc factory does not need to return an error because the *rest.Config passed to it is copied from an already validated manager config (a.mgr.GetLocalManager().GetConfig()), so the config is guaranteed to be valid at that point.
Applied to files:
internal/subroutine/authorization_model.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (4)
internal/subroutine/authorization_model.go (4)
4-9: LGTM! Import additions support template rendering and API discovery.The new imports appropriately support the dynamic FGA schema generation from discovered Kubernetes resources.
Also applies to: 19-22
30-72: LGTM! Template structure correctly delegates create/list to parent types.The privileged template appropriately extends parent types with scoped create/list/watch permissions, consistent with the architectural pattern confirmed in previous discussions.
Based on learnings
74-90: LGTM! Discovery client factory integration is clean.The factory injection enables testability while keeping the production path simple with the validated config assumption.
Based on learnings
198-213: LGTM! JSON comparison is more robust than DSL string comparison.Switching to JSON-based proto comparison correctly handles ordering differences and ensures semantic equivalence checking.
|
|
||
| discoveryClient := a.newDiscoveryClientFunc(cfg) | ||
|
|
||
| coreModules, err := discoverAndRender(discoveryClient, modelTpl, groupVersions) |
There was a problem hiding this comment.
Fix compilation error: undefined variable modelTpl.
Line 168 references modelTpl which is not defined in this file, causing a compilation error.
You need to define a template for non-privileged core types. Add this definition in the var block (after line 36):
coreModelTemplate = template.Must(template.New("model").Parse(`module internal_core_types_{{ .Name }}
type {{ .Group }}_{{ .Singular }}
relations
define parent: [{{ if eq .Scope "Namespaced" }}core_namespace{{ else }}core_platform-mesh_io_account{{ end }}]
define member: [role#assignee] or owner or member from parent
define owner: [role#assignee] or owner from parent
define get: member
define update: member
define delete: member
define patch: member
define watch: member
define manage_iam_roles: owner
define get_iam_roles: member
define get_iam_users: member
`))Then update line 168:
-coreModules, err := discoverAndRender(discoveryClient, modelTpl, groupVersions)
+coreModules, err := discoverAndRender(discoveryClient, coreModelTemplate, groupVersions)🤖 Prompt for AI Agents
In internal/subroutine/authorization_model.go around lines 36 and 168, define a
new template variable in the var block after line 36 named coreModelTemplate
containing the provided template for non-privileged core types, then replace the
undefined modelTpl on line 168 with coreModelTemplate when calling
discoverAndRender; this ensures the template is defined and used correctly.
| func discoverAndRender(dc discovery.DiscoveryInterface, tpl *template.Template, groupVersions []string) ([]language.ModuleFile, error) { | ||
| var files []language.ModuleFile | ||
| for _, gv := range groupVersions { | ||
| resourceList, err := dc.ServerResourcesForGroupVersion(gv) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("discover resources for %s: %w", gv, err) | ||
| } | ||
|
|
||
| for _, apiRes := range resourceList.APIResources { | ||
| if strings.Contains(apiRes.Name, "/") { // skip subresources | ||
| continue | ||
| } | ||
|
|
||
| buf, err := processAPIResourceIntoModel(apiRes, tpl) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("process api resource %s in %s: %w", apiRes.Name, gv, err) | ||
| } | ||
|
|
||
| files = append(files, language.ModuleFile{ | ||
| Name: fmt.Sprintf("internal_core_types_%s.fga", apiRes.Name), | ||
| Contents: buf.String(), | ||
| }) | ||
| } | ||
| } | ||
| return files, nil | ||
| } |
There was a problem hiding this comment.
Include group in ModuleFile name to prevent collisions.
Resources with the same plural name in different API groups will overwrite each other's module files.
Modify discoverAndRender to return group information and update line 280:
- files = append(files, language.ModuleFile{
- Name: fmt.Sprintf("internal_core_types_%s.fga", apiRes.Name),
- Contents: buf.String(),
- })
+ group := "core"
+ if apiRes.Group != "" {
+ group = strings.ReplaceAll(apiRes.Group, ".", "_")
+ }
+ files = append(files, language.ModuleFile{
+ Name: fmt.Sprintf("internal_core_types_%s_%s.fga", group, apiRes.Name),
+ Contents: buf.String(),
+ })Committable suggestion skipped: line range outside the PR's diff.
It generates a complete and valid schema with all kubernetes core types included:
Rendered FGA schema
Summary by CodeRabbit
Chores
New Features
Bug Fixes
Tests