From b1056a06716934adcd449fbcc431890dc97387ef Mon Sep 17 00:00:00 2001 From: Iryna Shustava Date: Fri, 13 Oct 2023 16:27:17 -0600 Subject: [PATCH] review comments --- .../testhelpers/acl_hooks_test_helpers.go | 10 ++++++++++ .../catalog/internal/types/dns_policy_test.go | 5 ++--- .../internal/types/health_checks_test.go | 5 ++--- .../internal/types/health_status_test.go | 18 ++++++++++++++++++ .../catalog/internal/types/service_test.go | 5 ++--- .../types/destinations_configuration_test.go | 5 ++--- .../mesh/internal/types/destinations_test.go | 5 ++--- .../internal/types/proxy_configuration_test.go | 10 ++++++---- internal/resource/resourcetest/acls.go | 6 +++++- internal/resource/resourcetest/tenancy.go | 15 +++++++++++++++ 10 files changed, 64 insertions(+), 20 deletions(-) diff --git a/internal/catalog/internal/testhelpers/acl_hooks_test_helpers.go b/internal/catalog/internal/testhelpers/acl_hooks_test_helpers.go index c1a00edf53ef..c2e8947ac545 100644 --- a/internal/catalog/internal/testhelpers/acl_hooks_test_helpers.go +++ b/internal/catalog/internal/testhelpers/acl_hooks_test_helpers.go @@ -93,6 +93,16 @@ func RunWorkloadSelectingTypeACLsTests[T WorkloadSelecting](t *testing.T, typ *p WriteOK: resourcetest.ALLOW, ListOK: resourcetest.DEFAULT, }, + "service test write with prefixed selectors and a policy with a specific service": { + Rules: `service "test" { policy = "write" } service "workload" { policy = "read" }`, + Data: getData(&pbcatalog.WorkloadSelector{Prefixes: []string{"workload"}}), + Typ: typ, + ReadOK: resourcetest.ALLOW, + // TODO (ishustava): this is wrong and should be fixed in a follow up PR. We should not allow + // a policy for a specific service when only prefixes are specified in the selector. + WriteOK: resourcetest.ALLOW, + ListOK: resourcetest.DEFAULT, + }, } for name, tc := range cases { diff --git a/internal/catalog/internal/types/dns_policy_test.go b/internal/catalog/internal/types/dns_policy_test.go index 9f74366592e5..1303d2878cf7 100644 --- a/internal/catalog/internal/types/dns_policy_test.go +++ b/internal/catalog/internal/types/dns_policy_test.go @@ -175,7 +175,6 @@ func TestDNSPolicyACLs(t *testing.T) { Weights: &pbcatalog.Weights{Passing: 1, Warning: 0}, } }, - func(registry resource.Registry) { - RegisterDNSPolicy(registry) - }) + RegisterDNSPolicy, + ) } diff --git a/internal/catalog/internal/types/health_checks_test.go b/internal/catalog/internal/types/health_checks_test.go index 26c09a419d1d..c9cdf01ae84c 100644 --- a/internal/catalog/internal/types/health_checks_test.go +++ b/internal/catalog/internal/types/health_checks_test.go @@ -203,7 +203,6 @@ func TestHealthChecksACLs(t *testing.T) { func(selector *pbcatalog.WorkloadSelector) *pbcatalog.HealthChecks { return &pbcatalog.HealthChecks{Workloads: selector} }, - func(registry resource.Registry) { - RegisterHealthChecks(registry) - }) + RegisterHealthChecks, + ) } diff --git a/internal/catalog/internal/types/health_status_test.go b/internal/catalog/internal/types/health_status_test.go index 644d9effb347..9482e4770e40 100644 --- a/internal/catalog/internal/types/health_status_test.go +++ b/internal/catalog/internal/types/health_status_test.go @@ -292,6 +292,24 @@ func TestHealthStatusACLs(t *testing.T) { WriteOK: resourcetest.ALLOW, ListOK: resourcetest.DEFAULT, }, + "node test read with workload owner": { + Rules: `node "test" { policy = "read" }`, + Data: healthStatusData, + Owner: workload, + Typ: pbcatalog.HealthStatusType, + ReadOK: resourcetest.DENY, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, + }, + "node test write with workload owner": { + Rules: `node "test" { policy = "write" }`, + Data: healthStatusData, + Owner: workload, + Typ: pbcatalog.HealthStatusType, + ReadOK: resourcetest.DENY, + WriteOK: resourcetest.DENY, + ListOK: resourcetest.DEFAULT, + }, } for name, tc := range cases { diff --git a/internal/catalog/internal/types/service_test.go b/internal/catalog/internal/types/service_test.go index 1de151568e12..18649dda9a0a 100644 --- a/internal/catalog/internal/types/service_test.go +++ b/internal/catalog/internal/types/service_test.go @@ -282,7 +282,6 @@ func TestServiceACLs(t *testing.T) { func(selector *pbcatalog.WorkloadSelector) *pbcatalog.Service { return &pbcatalog.Service{Workloads: selector} }, - func(registry resource.Registry) { - RegisterService(registry) - }) + RegisterService, + ) } diff --git a/internal/mesh/internal/types/destinations_configuration_test.go b/internal/mesh/internal/types/destinations_configuration_test.go index 5a6d3ff73289..11af0732d5d9 100644 --- a/internal/mesh/internal/types/destinations_configuration_test.go +++ b/internal/mesh/internal/types/destinations_configuration_test.go @@ -22,9 +22,8 @@ func TestDestinationsConfigurationACLs(t *testing.T) { func(selector *pbcatalog.WorkloadSelector) *pbmesh.DestinationsConfiguration { return &pbmesh.DestinationsConfiguration{Workloads: selector} }, - func(registry resource.Registry) { - RegisterDestinationsConfiguration(registry) - }) + RegisterDestinationsConfiguration, + ) } func TestValidateDestinationsConfiguration(t *testing.T) { diff --git a/internal/mesh/internal/types/destinations_test.go b/internal/mesh/internal/types/destinations_test.go index 89c44963e854..c286af022f53 100644 --- a/internal/mesh/internal/types/destinations_test.go +++ b/internal/mesh/internal/types/destinations_test.go @@ -255,7 +255,6 @@ func TestDestinationsACLs(t *testing.T) { func(selector *pbcatalog.WorkloadSelector) *pbmesh.Destinations { return &pbmesh.Destinations{Workloads: selector} }, - func(registry resource.Registry) { - RegisterDestinations(registry) - }) + RegisterDestinations, + ) } diff --git a/internal/mesh/internal/types/proxy_configuration_test.go b/internal/mesh/internal/types/proxy_configuration_test.go index 3d332ca0d377..f5c52d474c35 100644 --- a/internal/mesh/internal/types/proxy_configuration_test.go +++ b/internal/mesh/internal/types/proxy_configuration_test.go @@ -24,11 +24,13 @@ import ( func TestProxyConfigurationACLs(t *testing.T) { catalogtesthelpers.RunWorkloadSelectingTypeACLsTests[*pbmesh.ProxyConfiguration](t, pbmesh.ProxyConfigurationType, func(selector *pbcatalog.WorkloadSelector) *pbmesh.ProxyConfiguration { - return &pbmesh.ProxyConfiguration{Workloads: selector} + return &pbmesh.ProxyConfiguration{ + Workloads: selector, + DynamicConfig: &pbmesh.DynamicConfig{}, + } }, - func(registry resource.Registry) { - RegisterProxyConfiguration(registry) - }) + RegisterProxyConfiguration, + ) } func TestMutateProxyConfiguration(t *testing.T) { diff --git a/internal/resource/resourcetest/acls.go b/internal/resource/resourcetest/acls.go index c73f3c4752d4..4aff9e30327b 100644 --- a/internal/resource/resourcetest/acls.go +++ b/internal/resource/resourcetest/acls.go @@ -52,11 +52,15 @@ func RunACLTestCase(t *testing.T, tc ACLTestCase, registry resource.Registry) { reg, ok := registry.Resolve(tc.Typ) require.True(t, ok) + resolvedType, ok := registry.Resolve(tc.Typ) + require.True(t, ok) + res := Resource(tc.Typ, "test"). - WithTenancy(resource.DefaultNamespacedTenancy()). + WithTenancy(DefaultTenancyForType(t, resolvedType)). WithOwner(tc.Owner). WithData(t, tc.Data). Build() + ValidateAndNormalize(t, registry, res) config := acl.Config{ diff --git a/internal/resource/resourcetest/tenancy.go b/internal/resource/resourcetest/tenancy.go index 838379cebbed..5f5c0525b6f4 100644 --- a/internal/resource/resourcetest/tenancy.go +++ b/internal/resource/resourcetest/tenancy.go @@ -5,6 +5,7 @@ package resourcetest import ( "strings" + "testing" "github.com/hashicorp/consul/internal/resource" "github.com/hashicorp/consul/proto-public/pbresource" @@ -35,3 +36,17 @@ func Tenancy(s string) *pbresource.Tenancy { return &pbresource.Tenancy{Partition: "BAD", Namespace: "BAD", PeerName: "BAD"} } } + +func DefaultTenancyForType(t *testing.T, reg resource.Registration) *pbresource.Tenancy { + switch reg.Scope { + case resource.ScopeNamespace: + return resource.DefaultNamespacedTenancy() + case resource.ScopePartition: + return resource.DefaultPartitionedTenancy() + case resource.ScopeCluster: + return resource.DefaultClusteredTenancy() + default: + t.Fatalf("unsupported resource scope: %v", reg.Scope) + return nil + } +}