Skip to content

OSAC-1546: Add tenancy tests for explicit tenant handling#708

Open
jhernand wants to merge 1 commit into
osac-project:mainfrom
jhernand:add_tenancy_tests_for_explicit_tenant_handling
Open

OSAC-1546: Add tenancy tests for explicit tenant handling#708
jhernand wants to merge 1 commit into
osac-project:mainfrom
jhernand:add_tenancy_tests_for_explicit_tenant_handling

Conversation

@jhernand

@jhernand jhernand commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

This increases the coverage of the tenancy tests to make sure that when a user of the public API
specifies a tenant explicitly it is honoured during both creation and update operations. The new
tests verify three scenarios:

  • When a user creates an object with an explicit tenant that differs from the tenancy logic's
    default, the explicitly specified tenant is stored rather than being overridden by the default.
  • When an object is updated without mentioning the tenant in the update mask, the original tenant
    is preserved. The test intentionally configures a default tenant that differs from the one used
    during creation, so it can distinguish between genuine preservation and accidental re-defaulting.
  • When a user explicitly sets a different tenant during an update (via metadata.tenant in the
    update mask), the new tenant is persisted to the database.

Related: https://redhat.atlassian.net/browse/OSAC-1546
Related: #704

Test plan

  • New tests pass: ginkgo run --focus="Tenancy logic" -v internal/servers
  • All existing tenancy tests continue to pass (8/8)
  • Pre-commit hooks pass (including golangci-lint)

Summary by CodeRabbit

  • Tests
    • Enhanced test coverage for tenant-related operations, including test cases for tenant selection precedence, tenant preservation during updates, and explicit tenant specification with verification.

This increases the coverage of the tenancy tests to make sure that when
a user of the public API specifies a tenant explicitly it is honoured
during both creation and update operations. The new tests verify three
scenarios:

The first checks that when a user creates an object with an explicit
tenant that differs from the tenancy logic's default, the explicitly
specified tenant is stored rather than being overridden by the default.

The second checks that when an object is updated without mentioning the
tenant in the update mask, the original tenant is preserved. The test
intentionally configures a default tenant that differs from the one used
during creation, so it can distinguish between genuine preservation and
accidental re-defaulting.

The third checks that when a user explicitly sets a different tenant
during an update (via `metadata.tenant` in the update mask), the new
tenant is persisted to the database.

Related: https://redhat.atlassian.net/browse/OSAC-1546
Related: osac-project#704
Assisted-by: Cursor
Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
@openshift-ci-robot

openshift-ci-robot commented Jun 16, 2026

Copy link
Copy Markdown

@jhernand: This pull request references OSAC-1546 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 feature to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

This increases the coverage of the tenancy tests to make sure that when a user of the public API
specifies a tenant explicitly it is honoured during both creation and update operations. The new
tests verify three scenarios:

  • When a user creates an object with an explicit tenant that differs from the tenancy logic's
    default, the explicitly specified tenant is stored rather than being overridden by the default.
  • When an object is updated without mentioning the tenant in the update mask, the original tenant
    is preserved. The test intentionally configures a default tenant that differs from the one used
    during creation, so it can distinguish between genuine preservation and accidental re-defaulting.
  • When a user explicitly sets a different tenant during an update (via metadata.tenant in the
    update mask), the new tenant is persisted to the database.

Related: https://redhat.atlassian.net/browse/OSAC-1546
Related: #704

Test plan

  • New tests pass: ginkgo run --focus="Tenancy logic" -v internal/servers
  • All existing tenancy tests continue to pass (8/8)
  • Pre-commit hooks pass (including golangci-lint)

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.

@openshift-ci openshift-ci Bot requested review from adriengentil and trewest June 16, 2026 12:32
@openshift-ci

openshift-ci Bot commented Jun 16, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: bf756c88-ece5-4152-af9a-6327fb7bfcd5

📥 Commits

Reviewing files that changed from the base of the PR and between c05068c and 6647a41.

📒 Files selected for processing (1)
  • internal/servers/servers_tenancy_test.go

Walkthrough

Adds a fieldmaskpb import and renames the second BeforeEach tenant from "my-tenant" to "your-tenant". Introduces three new test cases in servers_tenancy_test.go that verify tenant selection precedence on cluster creation, tenant preservation during updates when the tenant field is absent from UpdateMask, and tenant override when metadata.tenant is explicitly included in UpdateMask.

Changes

Tenancy Test Coverage for Cluster Create/Update

Layer / File(s) Summary
Test fixture rename and import addition
internal/servers/servers_tenancy_test.go
fieldmaskpb import is added and the second BeforeEach tenant is renamed from "my-tenant" to "your-tenant" to create a distinguishable default vs. explicit tenant pair for new tests.
Tenancy precedence and UpdateMask test cases
internal/servers/servers_tenancy_test.go
Three test cases added: explicit tenant on create overrides the tenancy default; update without metadata.tenant in UpdateMask preserves the existing tenant (confirmed via subsequent GET); update with metadata.tenant in UpdateMask overrides and persists the new tenant.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • osac-project/fulfillment-service#504: Extends the same servers_tenancy_test.go file with tenancy-behavior test cases focused on visibility enforcement on create, directly overlapping test infrastructure.
  • osac-project/fulfillment-service#439: Updates tenancy behavior expectations in servers_tenancy_test.go using auth-subject context and tenant fields, including update-mask and tenant-persistence checks that mirror this PR's approach.

Suggested labels

approved, lgtm

Suggested reviewers

  • CrystalChun

Poem

🔐 Two tenants walk in — who takes the seat?
The mask says "override," the default must retreat.
A GET confirms the tenant stuck around,
No silent overwrite, no ownership unsound.
Test by test, the tenancy holds firm. ✅

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and concisely summarizes the main change: adding new tenancy tests for explicit tenant handling in compliance with OSAC-1546.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed No hardcoded secrets detected. All string literals are legitimate test data (tenant identifiers, template IDs, test descriptions). The new fieldmaskpb import is a standard protobuf library.
No-Weak-Crypto ✅ Passed No weak cryptographic algorithms (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto implementations, or non-constant-time secret comparisons detected. Only standard protobuf fieldmaskpb util...
No-Injection-Vectors ✅ Passed Test file uses only safe Ginkgo/Gomega patterns with protobuf builders and hardcoded test data; no SQL concatenation, shell injection, eval/exec, deserialization, YAML, or dynamic command construct...
Container-Privileges ✅ Passed PR modifies only a Go unit test file (servers_tenancy_test.go), adding test cases for tenancy logic. No container manifests, K8s deployments, Helm charts, Dockerfiles, or security contexts were mod...
No-Sensitive-Data-In-Logs ✅ Passed No logging calls expose sensitive data. Test file contains zero logging function calls; only logger configuration. Test data uses non-sensitive tenant/template names with no passwords, tokens, API...
Ai-Attribution ✅ Passed AI tool (Cursor) usage properly attributed with Red Hat-compliant "Assisted-by" trailer; Co-Authored-By not misused.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants