fix: all client #121
fix: all client #121
Conversation
WalkthroughReplaces cluster-scoped client usage with a cross-cluster Kubernetes client in the authorization model subroutine and its constructor; controller builds and passes this wildcard-cluster client and a logger; tests updated to supply the new client and logger and relax some mock expectations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/controller/store_controller.go (3)
⏰ 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)
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: 0
🧹 Nitpick comments (3)
internal/subroutine/authorization_model.go (3)
87-101: Make cluster path rewrite robust (split on "/clusters/" and ensure absolute path)Splitting on bare "clusters" can mis-split if that substring appears in another segment, and empty base paths can produce non-absolute paths. Use "/clusters/" as a boundary and ensure the base starts with "/".
Apply this diff:
- parts := strings.Split(parsed.Path, "clusters") - - parsed.Path, err = url.JoinPath(parts[0], "clusters", logicalcluster.Wildcard.String()) + // Derive base path before the clusters segment, if present, and ensure it is absolute. + base := "/" + if strings.Contains(parsed.Path, "/clusters/") { + base = strings.SplitN(parsed.Path, "/clusters/", 2)[0] + if base == "" { + base = "/" + } + } else if parsed.Path != "" { + base = parsed.Path + } + + parsed.Path, err = url.JoinPath(base, "clusters", logicalcluster.Wildcard.String()) if err != nil { log.Error().Err(err).Msg("unable to join path") return ctrl.Result{}, errors.NewOperatorError(err, true, false) } allCfg.Host = parsed.String()
103-104: Reduce log level for host to debugHost is useful for troubleshooting but noisy at info; consider debug to avoid log spam.
- log.Info().Str("host", allCfg.Host).Msg("using host") + log.Debug().Str("host", allCfg.Host).Msg("using host")
105-111: Add a User-Agent to the wildcard client (optional)Tagging requests helps tracing and debugging across clusters.
allClient, err := client.New(allCfg, client.Options{ Scheme: cluster.GetScheme(), })Add above:
+ // Tag requests for observability + allCfg = rest.AddUserAgent(allCfg, "security-operator/authorization-model:wildcard")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/subroutine/authorization_model.go(3 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). (1)
- GitHub Check: pipe / dockerBuild / docker
🔇 Additional comments (2)
internal/subroutine/authorization_model.go (2)
6-7: Imports look goodNecessary additions for URL manipulation, logical cluster handling, and client config.
Also applies to: 9-9, 18-18
113-114: Confirm context cluster selection with a direct client
allClientis pinned to the wildcard host; themccontext.WithCluster(ctx, "")insidegetRelatedAuthorizationModelsmay be redundant or confusing with a non-multicluster wrapper client. Consider using the originalctx(no empty cluster override) inside that function, or explicitly document that empty selects “all”.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/subroutine/authorization_model.go (1)
30-35: Remove unused logger parameter or store it in the struct.The constructor accepts
log *logger.Loggeras a parameter but neither stores it in the struct nor uses it within the constructor. This creates an inconsistency with the API surface.Apply this diff to remove the unused parameter:
-func NewAuthorizationModelSubroutine(fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager, allClient client.Client, log *logger.Logger) *authorizationModelSubroutine { +func NewAuthorizationModelSubroutine(fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager, allClient client.Client) *authorizationModelSubroutine { return &authorizationModelSubroutine{ fga: fga, mgr: mgr, allClient: allClient, } }Alternatively, if the logger is intended for future use, add it to the struct:
type authorizationModelSubroutine struct { fga openfgav1.OpenFGAServiceClient mgr mcmanager.Manager allClient client.Client + log *logger.Logger } func NewAuthorizationModelSubroutine(fga openfgav1.OpenFGAServiceClient, mgr mcmanager.Manager, allClient client.Client, log *logger.Logger) *authorizationModelSubroutine { return &authorizationModelSubroutine{ fga: fga, mgr: mgr, allClient: allClient, + log: log, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/store_controller.go(3 hunks)internal/subroutine/authorization_model.go(2 hunks)internal/subroutine/authorization_model_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/subroutine/authorization_model_test.go (1)
internal/subroutine/authorization_model.go (1)
NewAuthorizationModelSubroutine(30-36)
internal/controller/store_controller.go (3)
internal/subroutine/store.go (1)
NewStoreSubroutine(27-32)internal/subroutine/authorization_model.go (1)
NewAuthorizationModelSubroutine(30-36)api/v1alpha1/authorizationmodel_types.go (1)
AuthorizationModel(31-37)
⏰ 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 (3)
internal/subroutine/authorization_model_test.go (2)
330-331: LGTM! Mock expectations appropriately reflect optional flows.The use of
Maybe()onClusterFromContextandGetClientexpectations correctly reflects that these calls may not occur in all test scenarios, since the subroutine now uses the injectedallClientdirectly instead of deriving it from the manager context.
333-335: LGTM! Test properly initializes logger and updated constructor.The test correctly creates a logger using
testlogger.New()and passes it along with theclientto the updatedNewAuthorizationModelSubroutineconstructor, aligning with the new four-parameter signature.internal/subroutine/authorization_model.go (1)
78-78: LGTM! Correctly uses injected client for cross-cluster operations.The change to use
a.allClientinstead of deriving the client from the cluster context enables cross-cluster authorization model retrieval, which aligns with the PR objective to support wildcard cluster operations.
| parts := strings.Split(parsed.Path, "clusters") | ||
|
|
||
| parsed.Path, err = url.JoinPath(parts[0], "clusters", logicalcluster.Wildcard.String()) | ||
| if err != nil { | ||
| log.Fatal().Err(err).Msg("unable to join path") | ||
| } |
There was a problem hiding this comment.
Validate string split result before accessing array elements.
The code splits on "clusters" and accesses parts[0] without validation. If the URL doesn't contain "clusters" or has an unexpected format, this could cause a panic due to index out of bounds.
Apply this diff to add validation:
parts := strings.Split(parsed.Path, "clusters")
+ if len(parts) < 2 {
+ log.Fatal().Str("path", parsed.Path).Msg("unexpected URL path format: expected 'clusters' segment")
+ }
parsed.Path, err = url.JoinPath(parts[0], "clusters", logicalcluster.Wildcard.String())
if err != nil {
log.Fatal().Err(err).Msg("unable to join path")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parts := strings.Split(parsed.Path, "clusters") | |
| parsed.Path, err = url.JoinPath(parts[0], "clusters", logicalcluster.Wildcard.String()) | |
| if err != nil { | |
| log.Fatal().Err(err).Msg("unable to join path") | |
| } | |
| parts := strings.Split(parsed.Path, "clusters") | |
| if len(parts) < 2 { | |
| log.Fatal().Str("path", parsed.Path).Msg("unexpected URL path format: expected 'clusters' segment") | |
| } | |
| parsed.Path, err = url.JoinPath(parts[0], "clusters", logicalcluster.Wildcard.String()) | |
| if err != nil { | |
| log.Fatal().Err(err).Msg("unable to join path") | |
| } |
🤖 Prompt for AI Agents
In internal/controller/store_controller.go around lines 47 to 52, the code
splits parsed.Path with strings.Split(parsed.Path, "clusters") and then uses
parts[0] without checking the split result; this can panic if "clusters" is
missing or the path is malformed. Fix by validating the parts slice length and
contents before using parts[0]: check that len(parts) >= 1 (and ideally >=2 if
you expect content after "clusters"), handle the error case by logging and
returning an appropriate error (or falling back to a safe default) instead of
proceeding, and only call url.JoinPath with the validated segment(s) once
confirmed safe.
|
|
||
| parsed.Path, err = url.JoinPath(parts[0], "clusters", logicalcluster.Wildcard.String()) | ||
| if err != nil { | ||
| log.Fatal().Err(err).Msg("unable to join path") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Reconsider using Fatal logging in initialization code.
Using log.Fatal() terminates the entire process, which may not be appropriate for a library or controller initialization. This prevents graceful degradation and makes the code difficult to test.
Consider returning errors instead of calling Fatal() so that callers can handle failures appropriately. If terminating the process is intentional for this specific use case, please confirm that this behavior is acceptable for production deployments.
Also applies to: 62-62
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' internal/controller/store_controller.goLength of output: 3750
🏁 Script executed:
#!/bin/bash
rg -n "NewStoreReconciler" -g "!**/test/**"Length of output: 243
🏁 Script executed:
#!/bin/bash
sed -n '140,160p' cmd/operator.goLength of output: 811
Refactor initialization to return errors instead of log.Fatal
Replace the two log.Fatal().Err(…).Msg(…) calls in NewStoreReconciler (lines 51 & 62) with error returns: update its signature to (*StoreReconciler, error) and handle these errors in cmd/operator.go to allow graceful startup failures.
🤖 Prompt for AI Agents
In internal/controller/store_controller.go around lines 51 and 62, replace the
two log.Fatal().Err(...).Msg(...) calls with returning an error from
NewStoreReconciler by changing its signature to (*StoreReconciler, error); on
any failure build and return a wrapped error instead of calling log.Fatal, and
update all callers (notably cmd/operator.go) to handle the error on
initialization—propagate the error upward or log and exit there so startup
failures are handled gracefully.
| lifecycle: builder.NewBuilder("store", "StoreReconciler", []lifecyclesubroutine.Subroutine{ | ||
| subroutine.NewStoreSubroutine(fga, mcMgr), | ||
| subroutine.NewAuthorizationModelSubroutine(fga, mcMgr), | ||
| subroutine.NewAuthorizationModelSubroutine(fga, mcMgr, allClient, log), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify that the logger parameter is utilized in the subroutine.
The constructor call passes log to NewAuthorizationModelSubroutine, but reviewing the implementation in internal/subroutine/authorization_model.go (line 30) shows the logger parameter is accepted but never stored in the struct or used.
Run the following script to verify logger usage in the authorization model subroutine:
🏁 Script executed:
#!/bin/bash
# Description: Check if the logger parameter is used in the authorization model subroutine
# Search for logger field in the struct
ast-grep --pattern $'type authorizationModelSubroutine struct {
$$$
log $$$
$$$
}'
# Search for logger usage in the constructor
ast-grep --pattern $'func NewAuthorizationModelSubroutine($$$) $_ {
$$$
log$$$
$$$
}'Length of output: 0
Summary by CodeRabbit
New Features
Improvements
Tests