NO-ISSUE: Remove SystemTenancyLogic in favor of DefaultTenancyLogic - #439
openshift-merge-bot[bot] merged 1 commit into
Conversation
The `SystemTenancyLogic` was a separate implementation used by the private API and controllers that bypassed tenant filtering entirely. Now that admin users receive the universal tenant set from Authorino (via the `["*"]` value), the `DefaultTenancyLogic` can handle both regular users and admins uniformly. `DetermineDefaultTenants` now falls back to `SharedTenants` when the subject's tenant set is universal, since an infinite set cannot be stored as an object's tenants. This preserves the old `SystemTenancyLogic` behavior where objects created through the private API defaulted to the shared tenant. All server tests are updated to set a system subject with universal tenants in the context, which was previously unnecessary because `SystemTenancyLogic` ignored the context entirely. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
|
@jhernand: This pull request explicitly references no jira issue. 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. |
WalkthroughThis pull request removes the Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
🧹 Nitpick comments (2)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go (1)
346-373: Nit: stale "public" label now that tenancy logic is shared.
tenancyLogicis now wired into both public and private server builders (clusters, hubs, leases, events, …), but the surrounding comment (// Create the public tenancy logic:) and log message ("Creating public tenancy logic") still describe it as public-only. Consider generalizing for clarity.✏️ Proposed wording tweak
- // Create the public tenancy logic: + // Create the tenancy logic (shared by public and private servers): c.logger.InfoContext( ctx, - "Creating public tenancy logic", + "Creating tenancy logic", slog.String("type", c.args.tenancyLogic), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go` around lines 346 - 373, The comment and log call misleadingly say "public" even though the tenancyLogic (variable tenancyLogic built via auth.NewDefaultTenancyLogic()/auth.NewGuestTenancyLogic() and keyed by c.args.tenancyLogic) is shared across public and private builders; update the inline comment and the c.logger.InfoContext message to a generalized phrase such as "Creating tenancy logic" or "Creating shared tenancy logic" (leave the switch and error handling unchanged) so log and comment match actual usage.internal/servers/servers_suite_test.go (1)
65-74: Optional: suite-default mock diverges from realDefaultTenancyLogic.With
DetermineAssignableTenantsreturning a universal set, the realDefaultTenancyLogic.DetermineDefaultTenantswould now returnSharedTenants, but this suite-level default returnsNewSet("system"). Tests that exercise default-tenant assignment through the suite mock (rather than their own local mock) could therefore pass assertions that wouldn't hold in production. Consider aligning the default toSharedTenants(or a value derived from the assignable mock) so suite-wide behavior better mirrors runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/servers_suite_test.go` around lines 65 - 74, The suite-level mock for auth.TenancyLogic diverges from the real DefaultTenancyLogic by returning NewUniversalSet for DetermineAssignableTenants while DetermineDefaultTenants returns NewSet("system"); update the mock so DetermineDefaultTenants returns the same SharedTenants set the real DefaultTenancyLogic would derive (or compute it from the mocked DetermineAssignableTenants result) instead of NewSet("system"), i.e. change the tenancy.EXPECT().DetermineDefaultTenants(...) stub to return SharedTenants (or a value built from collections.NewUniversalSet[string]()) so suite behavior matches DefaultTenancyLogic's runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 346-373: The comment and log call misleadingly say "public" even
though the tenancyLogic (variable tenancyLogic built via
auth.NewDefaultTenancyLogic()/auth.NewGuestTenancyLogic() and keyed by
c.args.tenancyLogic) is shared across public and private builders; update the
inline comment and the c.logger.InfoContext message to a generalized phrase such
as "Creating tenancy logic" or "Creating shared tenancy logic" (leave the switch
and error handling unchanged) so log and comment match actual usage.
In `@internal/servers/servers_suite_test.go`:
- Around line 65-74: The suite-level mock for auth.TenancyLogic diverges from
the real DefaultTenancyLogic by returning NewUniversalSet for
DetermineAssignableTenants while DetermineDefaultTenants returns
NewSet("system"); update the mock so DetermineDefaultTenants returns the same
SharedTenants set the real DefaultTenancyLogic would derive (or compute it from
the mocked DetermineAssignableTenants result) instead of NewSet("system"), i.e.
change the tenancy.EXPECT().DetermineDefaultTenants(...) stub to return
SharedTenants (or a value built from collections.NewUniversalSet[string]()) so
suite behavior matches DefaultTenancyLogic's runtime behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 336e2d6f-5ed2-4e1c-92ef-f15e0e0eab68
📒 Files selected for processing (12)
internal/auth/default_tenancy_logic.gointernal/auth/default_tenancy_logic_test.gointernal/auth/system_tenancy_logic.gointernal/auth/system_tenancy_logic_test.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/coordination/leader_elector_test.gointernal/servers/clusters_server_test.gointernal/servers/private_hubs_server_test.gointernal/servers/servers_suite_test.gointernal/servers/servers_tenancy_test.gointernal/servers/subnets_server_test.gointernal/servers/virtual_networks_server_test.go
💤 Files with no reviewable changes (2)
- internal/auth/system_tenancy_logic_test.go
- internal/auth/system_tenancy_logic.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4c571a6
into
osac-project:main
3 participants
Summary
SystemTenancyLogicand useDefaultTenancyLogicfor both public and private APIs,eliminating the separate tenancy logic that was previously used for the private API.
DefaultTenancyLogic.DetermineDefaultTenantsto return the shared tenant when thesubject has access to all tenants (universal set), since an infinite set cannot be stored as
the tenants of an object.
SystemTenancyLogicandSystemAttributionLogicusages in tests with mocks,and add auth subject context where needed.
Test plan
DefaultTenancyLogicto verify shared tenant behaviorginkgo run -r internalpassesSummary by CodeRabbit
Refactor
Tests