OSAC-496: Remove subnet/tenant fallback dead code#704
Conversation
|
Skipping CI for Draft Pull Request. |
|
@tchughesiv: This pull request references OSAC-496 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
Walkthrough
ChangesTenancy enforcement and shared metadata propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
tchughesiv
force-pushed
the
OSAC-496-remove-fallback-dead-code
branch
from
c619acb to
72f4545
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/servers/generic_server.go`:
- Around line 1243-1251: The error handling for DetermineDefaultTenant() in the
generic_server.go file currently returns a gRPC Internal status code for all
errors, but when the error indicates a contract violation (explicit tenant
required but omitted), it should return a permission-level or client-fixable
status code like PermissionDenied or InvalidArgument instead. Inspect the error
returned by s.tenancyLogic.DetermineDefaultTenant(ctx) and conditionally map it
to the appropriate gRPC status code using grpcstatus.Errorf: if the error
indicates a missing or invalid tenant requirement, use PermissionDenied or
InvalidArgument, otherwise fall back to Internal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0f0284bc-f406-4e8f-bc0b-69ee01c79ce3
📒 Files selected for processing (23)
internal/auth/default_tenancy_logic.gointernal/auth/default_tenancy_logic_test.gointernal/auth/guest_tenancy_logic.gointernal/auth/guest_tenancy_logic_test.gointernal/auth/tenancy_logic.gointernal/servers/generic_server.gointernal/servers/servers_tenancy_test.goit/it_annotations_test.goit/it_cluster_reconciler_test.goit/it_compute_subnet_test.goit/it_emergency_access_test.goit/it_labels_test.goit/it_metadata.goit/it_multitenancy_test.goit/it_nodeset_removal_test.goit/it_private_cluster_templates_test.goit/it_private_host_types_test.goit/it_public_clusters_test.goit/it_rest_gateway_test.goit/it_role_binding_reconciler_test.goit/it_role_reconciler_test.goit/it_tool.goit/it_version_test.go
💤 Files with no reviewable changes (1)
- internal/auth/tenancy_logic.go
|
Addressed the CodeRabbit review on
Fixed in |
Remove the SharedTenants variable and stop defaulting universal-access subjects to the shared tenant when no explicit tenant is provided. Defer default-tenant resolution in the generic server until the request and current object both omit a tenant, and update integration tests to set shared tenant explicitly on admin creates.
Introduce auth.ErrExplicitTenantRequired and map it to PermissionDenied in determineAssignedTenant so universal-access callers get a client-actionable status instead of Internal.
tchughesiv
force-pushed
the
OSAC-496-remove-fallback-dead-code
branch
from
02c95ec to
ce63eca
Compare
|
/retest-required |
Hub registration in CI and installer scripts uses the admin token, which now requires an explicit tenant. Default the create hub CLI to the shared tenant and accept an optional --tenant override.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@it/it_private_host_types_test.go`:
- Line 170: This is an optional refactor for consistency. If you choose to
improve consistency with other tests that use sharedMetadata(), consider either
extending the sharedMetadata() helper to support additional custom fields like
Finalizers, or creating a local builder pattern at the test site that starts
with sharedMetadata() and adds the custom Finalizers field. However, the current
approach of manually setting Tenant: auth.SharedTenant is acceptable and doesn't
require changes since this test has specific custom metadata requirements that
differ from other tests.
In `@it/it_tool.go`:
- Around line 2102-2104: Replace the inline metadata construction in the
Metadata field assignment with a call to the sharedMetadata() helper function.
Instead of using privatev1.Metadata_builder{Tenant: auth.SharedTenant}.Build(),
call sharedMetadata() to ensure consistency with the rest of the test suite and
reduce duplication in metadata construction patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7b91aad2-aadd-4be1-958a-aff63827c372
📒 Files selected for processing (24)
internal/auth/default_tenancy_logic.gointernal/auth/default_tenancy_logic_test.gointernal/auth/guest_tenancy_logic.gointernal/auth/guest_tenancy_logic_test.gointernal/auth/tenancy_logic.gointernal/cmd/cli/create/hub/create_hub_cmd.gointernal/servers/generic_server.gointernal/servers/servers_tenancy_test.goit/it_annotations_test.goit/it_cluster_reconciler_test.goit/it_compute_subnet_test.goit/it_emergency_access_test.goit/it_labels_test.goit/it_metadata.goit/it_multitenancy_test.goit/it_nodeset_removal_test.goit/it_private_cluster_templates_test.goit/it_private_host_types_test.goit/it_public_clusters_test.goit/it_rest_gateway_test.goit/it_role_binding_reconciler_test.goit/it_role_reconciler_test.goit/it_tool.goit/it_version_test.go
Address CodeRabbit nitpick for consistent metadata construction in registerHub.
Review responses (OSAC-496)Fixed / addressed
Skipped (with reason)
e2e-vmaas re-running on latest commits. |
Remove the --tenant flag and always set hub metadata tenant to shared. Platform hub registration only needs the shared tenant for now; a configurable tenant flag can follow in a separate change.
|
@coderabbitai review |
✅ Action performedReview finished.
|
jhernand
left a comment
jhernand
left a comment
There was a problem hiding this comment.
This kind of change requires careful review by the core team, as the tenancy logic is a key piece of the authorization infrastructure, used by all objects, not just subnets. So please don't merge this till we have approved it.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchughesiv 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 |
|
@tchughesiv: 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. |
|
Regarding the assignation of the default tenant and respecting the tenant explicitly provided by the user, I think it is already working as intended. I added more tests to verify it here: #708 . |
|
@jhernand are we saying this PR should be closed? or should i leave it open until decisions are made by core? |
|
awaiting tenancy decisions via OSAC-1546 |
3 participants
OSAC-496: Remove subnet/tenant fallback dead code (fulfillment-service)
Jira: https://redhat.atlassian.net/browse/OSAC-496
Follow-up (CLI/AAP tenancy design): https://redhat.atlassian.net/browse/OSAC-1546
Related PRs
metadata.tenant=sharedinpublish_templates(required for e2e-vmaas)Review status
Core team review requested — tenancy logic affects all objects, not just subnets. Please do not merge until approved (review).
Summary
Removes dead tenancy fallback code after subnet became mandatory on ComputeInstance creation. Universal-access subjects no longer default to the
"shared"tenant when no tenant is specified — they must provide an explicit tenant on the request. TheSharedTenantsset variable is removed; visibility logic usescollections.NewSet(SharedTenant)inline.Also fixes
determineAssignedTenant()to resolve an explicit request tenant before callingDetermineDefaultTenant(), so admin creates with an explicit tenant are not rejected solely because the subject has universal access.Changes
DetermineDefaultTenant()returns error when assignable tenants are universalSharedTenantsvariable fromtenancy_logic.gogeneric_server.gountil request/current object omit tenantauth.ErrExplicitTenantRequiredtoPermissionDeniedindetermineAssignedTenant()osac create hub: hardcode hub metadata tenant toshared(client-side explicit tenant for admin bootstrap)sharedtenant on admin creates (it_metadata.go, hub registration viasharedMetadata())Testing
ginkgo run -r internal— passginkgo run it— passbuf lint,golangci-lint,uv run dev.py lint— passAcceptance Criteria
SharedTenantsvariable removed from fulfillment-serviceSummary by CodeRabbit