From 96bf7682133234f0d881b052870c18c6b9d27ad4 Mon Sep 17 00:00:00 2001 From: skpratt Date: Tue, 6 Feb 2024 21:40:48 -0600 Subject: [PATCH] review fixes --- agent/xdsv2/rbac_resources.go | 8 ++--- internal/auth/internal/types/errors.go | 2 +- .../internal/types/traffic_permissions.go | 15 +++------ .../types/traffic_permissions_test.go | 31 +++++++++++++++++++ .../catalogv2/traffic_permissions_test.go | 2 +- 5 files changed, 41 insertions(+), 17 deletions(-) diff --git a/agent/xdsv2/rbac_resources.go b/agent/xdsv2/rbac_resources.go index b2a36c73d2..ba4e1599d5 100644 --- a/agent/xdsv2/rbac_resources.go +++ b/agent/xdsv2/rbac_resources.go @@ -363,8 +363,8 @@ func translateRule(dr *pbproxystate.DestinationRule) *envoy_rbac_v3.Permission { func permissionsFromDestinationRules(drs []*pbproxystate.DestinationRule) []*envoy_rbac_v3.Permission { var perms []*envoy_rbac_v3.Permission for _, dr := range drs { - var subPerms []*envoy_rbac_v3.Permission - for _, er := range dr.Exclude { + subPerms := make([]*envoy_rbac_v3.Permission, len(dr.Exclude)) + for i, er := range dr.Exclude { translated := translateRule(&pbproxystate.DestinationRule{ PathExact: er.PathExact, PathPrefix: er.PathPrefix, @@ -372,9 +372,9 @@ func permissionsFromDestinationRules(drs []*pbproxystate.DestinationRule) []*env Methods: er.Methods, DestinationRuleHeader: er.Headers, }) - subPerms = append(subPerms, &envoy_rbac_v3.Permission{ + subPerms[i] = &envoy_rbac_v3.Permission{ Rule: &envoy_rbac_v3.Permission_NotRule{NotRule: translated}, - }) + } } subPerms = append([]*envoy_rbac_v3.Permission{translateRule(dr)}, subPerms...) perms = append(perms, combineAndPermissions(subPerms)) diff --git a/internal/auth/internal/types/errors.go b/internal/auth/internal/types/errors.go index 87732d5bd6..f3645ded6a 100644 --- a/internal/auth/internal/types/errors.go +++ b/internal/auth/internal/types/errors.go @@ -7,7 +7,7 @@ import "errors" var ( errSourcesTenancy = errors.New("permissions sources may not specify partitions, peers, and sameness_groups together") - errSourceWildcards = errors.New("permission sources may not have wildcard namespaces and explicit names.") + errSourceWildcards = errors.New("permission sources may not have wildcard namespaces and explicit names") errSourceExcludes = errors.New("must be defined on wildcard sources") errInvalidPrefixValues = errors.New("prefix values, regex values, and explicit names must not combined") errInvalidRule = errors.New("rules must contain path, method, header, or port fields") diff --git a/internal/auth/internal/types/traffic_permissions.go b/internal/auth/internal/types/traffic_permissions.go index 2bfa8359bb..71aa1b7d24 100644 --- a/internal/auth/internal/types/traffic_permissions.go +++ b/internal/auth/internal/types/traffic_permissions.go @@ -4,6 +4,8 @@ package types import ( + "golang.org/x/exp/slices" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/consul/acl" @@ -293,7 +295,7 @@ func validatePermission(p *pbauth.Permission, id *pbresource.ID, wrapErr func(er } } for _, m := range excl.Methods { - if len(dest.Methods) != 0 && !listContains(dest.Methods, m) { + if len(dest.Methods) != 0 && !slices.Contains(dest.Methods, m) { merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ Name: "exclude_permission_header_rule", Wrapped: errExclValuesMustBeSubset, @@ -301,7 +303,7 @@ func validatePermission(p *pbauth.Permission, id *pbresource.ID, wrapErr func(er } } for _, port := range excl.PortNames { - if len(dest.PortNames) != 0 && !listContains(dest.PortNames, port) { + if len(dest.PortNames) != 0 && !slices.Contains(dest.PortNames, port) { merr = multierror.Append(merr, wrapExclPermRuleErr(resource.ErrInvalidListElement{ Name: "exclude_permission_header_rule", Wrapped: errExclValuesMustBeSubset, @@ -321,15 +323,6 @@ func validatePermission(p *pbauth.Permission, id *pbresource.ID, wrapErr func(er return merr } -func listContains(list []string, str string) bool { - for _, item := range list { - if item == str { - return true - } - } - return false -} - func sourceHasIncompatibleTenancies(src pbauth.SourceToSpiffe, id *pbresource.ID) bool { if id.Tenancy == nil { id.Tenancy = &pbresource.Tenancy{} diff --git a/internal/auth/internal/types/traffic_permissions_test.go b/internal/auth/internal/types/traffic_permissions_test.go index 131a357e22..9d614809b8 100644 --- a/internal/auth/internal/types/traffic_permissions_test.go +++ b/internal/auth/internal/types/traffic_permissions_test.go @@ -42,6 +42,37 @@ func TestValidateTrafficPermissions(t *testing.T) { Action: pbauth.Action_ACTION_ALLOW, }, }, + "ok-permissions": { + tp: &pbauth.TrafficPermissions{ + Destination: &pbauth.Destination{IdentityName: "wi-1"}, + Action: pbauth.Action_ACTION_ALLOW, + Permissions: []*pbauth.Permission{ + { + Sources: []*pbauth.Source{ + { + IdentityName: "wi-2", + Namespace: "default", + Partition: "default", + }, + { + IdentityName: "wi-1", + Namespace: "default", + Partition: "ap1", + }, + }, + DestinationRules: []*pbauth.DestinationRule{ + { + PathPrefix: "/", + Methods: []string{"GET"}, + Headers: []*pbauth.DestinationRuleHeader{{Name: "X-Consul-Token", Present: false, Invert: true}}, + PortNames: []string{"https"}, + Exclude: []*pbauth.ExcludePermissionRule{{PathExact: "/admin"}}, + }, + }, + }, + }, + }, + }, "unspecified-action": { // Any type other than the TrafficPermissions type would work // to cause the error we are expecting diff --git a/test-integ/catalogv2/traffic_permissions_test.go b/test-integ/catalogv2/traffic_permissions_test.go index dd07536306..4e97462e33 100644 --- a/test-integ/catalogv2/traffic_permissions_test.go +++ b/test-integ/catalogv2/traffic_permissions_test.go @@ -284,7 +284,7 @@ func TestL7TrafficPermissions(t *testing.T) { } t.Log(initialTrafficPerms) // Wait for the resource updates to go through and Envoy to be ready - time.Sleep(500 * time.Millisecond) + time.Sleep(1 * time.Second) // Check the default server workload envoy config for RBAC filters matching testcase criteria serverWorkload := cluster.WorkloadsByID(topology.ID{ Partition: "default",