-
Notifications
You must be signed in to change notification settings - Fork 24
HYPERFLEET-1471 - feat: Server-set tenancy on the resource write path #339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -264,6 +264,57 @@ func TestPresentResource_WithReferences(t *testing.T) { | |
| Expect(*refs["wif_config"][1].Id).To(Equal("wif-2")) | ||
| } | ||
|
|
||
| func TestPresentResource_WithTenancy(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| now := time.Now() | ||
| resource := &api.Resource{ | ||
| Meta: api.Meta{ID: "id", CreatedTime: now, UpdatedTime: now}, | ||
| Kind: "Channel", | ||
| Name: "test", | ||
| Spec: datatypes.JSON(`{}`), | ||
| Tenancy: datatypes.JSON(`{"org":"acme"}`), | ||
| CreatedBy: "user@test.com", | ||
| UpdatedBy: "user@test.com", | ||
| } | ||
|
|
||
| resp := PresentResource(resource) | ||
| Expect(resp.Tenancy).ToNot(BeNil()) | ||
| Expect(*resp.Tenancy).To(HaveKeyWithValue("org", "acme")) | ||
| } | ||
|
|
||
| func TestPresentResource_EmptyTenancy(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| now := time.Now() | ||
|
Comment on lines
+267
to
+289
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider consolidating these into a table-driven test. The cases share the same setup and assertions, and a single table will scale better as we add more tenancy presentation scenarios. |
||
| resource := &api.Resource{ | ||
| Meta: api.Meta{ID: "id", CreatedTime: now, UpdatedTime: now}, | ||
| Kind: "Channel", | ||
| Name: "test", | ||
| Spec: datatypes.JSON(`{}`), | ||
| Tenancy: datatypes.JSON(`{}`), | ||
| CreatedBy: "user@test.com", | ||
| UpdatedBy: "user@test.com", | ||
| } | ||
|
|
||
| resp := PresentResource(resource) | ||
| Expect(resp.Tenancy).ToNot(BeNil(), "empty tenancy should present as explicit {}, not be omitted") | ||
| Expect(*resp.Tenancy).To(BeEmpty()) | ||
| } | ||
|
|
||
| func TestConvertResource_IgnoresTenancyInBody(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| body := []byte(`{"kind":"Channel","name":"stable","spec":{"is_default":true},"tenancy":{"org":"forged"}}`) | ||
| var req openapi.ResourceCreateRequest | ||
| err := json.Unmarshal(body, &req) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| resource, convErr := ConvertResource(&req) | ||
| Expect(convErr).NotTo(HaveOccurred()) | ||
| Expect(resource.Tenancy).To(BeEmpty()) | ||
| } | ||
|
|
||
| func TestPresentResourceList(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,13 +9,15 @@ import ( | |
| "time" | ||
|
|
||
| . "github.com/onsi/gomega" | ||
| "gorm.io/datatypes" | ||
| "gorm.io/gorm" | ||
|
|
||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" | ||
| "github.com/openshift-hyperfleet/hyperfleet-api/pkg/tenant" | ||
| ) | ||
|
|
||
| const ( | ||
|
|
@@ -409,6 +411,55 @@ func TestResourceService_Create_SetsUserFromAuthContext(t *testing.T) { | |
| Expect(result.UpdatedBy).To(Equal("user@test.com")) | ||
| } | ||
|
|
||
| func TestResourceService_Create_StampsTenancyFromContext(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider a test that pre-sets |
||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
|
|
||
| ctx := tenant.WithTenant(context.Background(), &tenant.ResolvedTenant{ | ||
| Dimensions: map[string]string{"org": "acme"}, | ||
| }) | ||
| resource := testResource("Channel", "ch-1", "stable") | ||
|
|
||
| result, svcErr := svc.Create(ctx, "Channel", resource, nil) | ||
| Expect(svcErr).To(BeNil()) | ||
| Expect(string(result.Tenancy)).To(MatchJSON(`{"org":"acme"}`)) | ||
| } | ||
|
|
||
| func TestResourceService_Create_SystemIdentityGetsEmptyTenancy(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
|
|
||
| ctx := tenant.WithTenant(context.Background(), &tenant.ResolvedTenant{ | ||
| System: true, | ||
| Dimensions: map[string]string{"org": "acme"}, | ||
| }) | ||
| resource := testResource("Channel", "ch-1", "stable") | ||
|
|
||
| result, svcErr := svc.Create(ctx, "Channel", resource, nil) | ||
| Expect(svcErr).To(BeNil()) | ||
| Expect(string(result.Tenancy)).To(MatchJSON(`{}`)) | ||
| } | ||
|
|
||
| func TestResourceService_Create_NoTenantContext_GetsEmptyTenancy(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
Comment on lines
+414
to
+450
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider consolidating the three Create tenancy tests into one table-driven test. They share the same setup and only differ by context input and expected Tenancy JSON, similar to TestTenancyJSON in pkg/tenant/context_test.go. |
||
| setupTestDescriptors() | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
|
|
||
| resource := testResource("Channel", "ch-1", "stable") | ||
|
|
||
| result, svcErr := svc.Create(context.Background(), "Channel", resource, nil) | ||
| Expect(svcErr).To(BeNil()) | ||
| Expect(string(result.Tenancy)).To(MatchJSON(`{}`)) | ||
| } | ||
|
|
||
| func TestResourceService_Create_PreservesExplicitValues(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
@@ -564,6 +615,23 @@ func TestResourceService_Patch_SpecChanged_IncrementsGeneration(t *testing.T) { | |
| Expect(result.Generation).To(Equal(int32(2))) | ||
| } | ||
|
|
||
| func TestResourceService_Patch_DoesNotModifyTenancy(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| mockDao := newMockResourceDao() | ||
| svc, _, _ := newTestResourceService(mockDao) | ||
|
|
||
| existing := testResource("Channel", "ch-1", "stable") | ||
| existing.Tenancy = datatypes.JSON(`{"org":"acme"}`) | ||
| mockDao.addResource(existing) | ||
|
|
||
| patch := &api.ResourcePatch{Spec: map[string]interface{}{"key": "new-value"}} | ||
| result, svcErr := svc.Patch(context.Background(), "Channel", "ch-1", patch) | ||
| Expect(svcErr).To(BeNil()) | ||
| Expect(string(result.Tenancy)).To(MatchJSON(`{"org":"acme"}`)) | ||
| } | ||
|
|
||
| func TestResourceService_Patch_LabelsChanged_IncrementsGeneration(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package tenant | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
|
|
||
| "gorm.io/datatypes" | ||
| ) | ||
|
|
||
| type contextKey struct{} | ||
|
|
||
| // ResolvedTenant holds the tenant identity resolved from gateway-injected request headers. | ||
| type ResolvedTenant struct { | ||
| Dimensions map[string]string | ||
| System bool | ||
| } | ||
|
|
||
| // WithTenant attaches a resolved tenant identity to the context. | ||
| func WithTenant(ctx context.Context, t *ResolvedTenant) context.Context { | ||
| return context.WithValue(ctx, contextKey{}, t) | ||
| } | ||
|
|
||
| // FromContext returns the tenant identity attached to ctx, or nil if none was resolved. | ||
| func FromContext(ctx context.Context) *ResolvedTenant { | ||
| if t, ok := ctx.Value(contextKey{}).(*ResolvedTenant); ok { | ||
| return t | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // TenancyJSON returns the caller's tenancy map as JSONB for storage on created resources. | ||
| // System and unscoped/absent callers get an empty map, which no tenant-scoped query can ever match. | ||
| func TenancyJSON(ctx context.Context) datatypes.JSON { | ||
| t := FromContext(ctx) | ||
| if t == nil || t.System || len(t.Dimensions) == 0 { | ||
| return datatypes.JSON([]byte("{}")) | ||
| } | ||
| b, err := json.Marshal(t.Dimensions) | ||
| if err != nil { | ||
| return datatypes.JSON([]byte("{}")) | ||
| } | ||
| return datatypes.JSON(b) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package tenant | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| func TestFromContext_NoTenant(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| got := FromContext(context.Background()) | ||
| Expect(got).To(BeNil()) | ||
| } | ||
|
|
||
| func TestWithTenant_RoundTrip(t *testing.T) { | ||
| RegisterTestingT(t) | ||
|
|
||
| want := &ResolvedTenant{Dimensions: map[string]string{"org": "acme"}} | ||
| ctx := WithTenant(context.Background(), want) | ||
|
|
||
| got := FromContext(ctx) | ||
| Expect(got).To(Equal(want)) | ||
| } | ||
|
|
||
| func TestTenancyJSON(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| ctx context.Context | ||
| want string | ||
| }{ | ||
| { | ||
| name: "no tenant in context", | ||
| ctx: context.Background(), | ||
| want: "{}", | ||
| }, | ||
| { | ||
| name: "system identity", | ||
| ctx: WithTenant(context.Background(), &ResolvedTenant{System: true, Dimensions: map[string]string{"org": "acme"}}), | ||
| want: "{}", | ||
| }, | ||
| { | ||
| name: "empty dimensions", | ||
| ctx: WithTenant(context.Background(), &ResolvedTenant{Dimensions: map[string]string{}}), | ||
| want: "{}", | ||
| }, | ||
| { | ||
| name: "nil dimensions", | ||
| ctx: WithTenant(context.Background(), &ResolvedTenant{}), | ||
| want: "{}", | ||
| }, | ||
| { | ||
| name: "tenant with single dimension", | ||
| ctx: WithTenant(context.Background(), &ResolvedTenant{Dimensions: map[string]string{"org": "acme"}}), | ||
| want: `{"org":"acme"}`, | ||
| }, | ||
| { | ||
| name: "tenant with multiple dimensions", | ||
| ctx: WithTenant(context.Background(), &ResolvedTenant{ | ||
| Dimensions: map[string]string{"org": "acme", "project": "project-1"}, | ||
| }), | ||
| want: `{"org":"acme","project":"project-1"}`, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| got := TenancyJSON(tt.ctx) | ||
| Expect(string(got)).To(MatchJSON(tt.want)) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
presentTenancy returns nil for zero-length JSON but {} for stored "{}". Worth normalizing so empty tenancy always serializes the same way.