status-im
/
consul
mirror of https://github.com/status-im/consul.git
2
0
Fork 0
Code Issues Projects Releases Wiki Activity

move deny action to enterprise only for traffic permissions (#20313) 

Add missing import

Add explicit enum case for deny action

Remove extra comments

Add build tags to ent and ce tests

Add copyright headers for the ce files

Fix case statements for ce validator

Remove ce tests with Deny traffic permissions

Fix more integration tests

Split more ce and ent tests, add back ent deny tests for traffic permissions controller

temp rename before rebase

Readd ent deny tests for traffic permissions controller
This commit is contained in:
John Landa 2024-01-24 13:01:22 -07:00 committed by GitHub
parent 4ce4dd1492
commit 65920dccf4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 133 additions and 129 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
internal/auth/internal/controllers/trafficpermissions/controller_test.go
View File

@ -114,7 +114,7 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi
Destination: &pbauth.Destination{
IdentityName: "wi1",
},
Action: pbauth.Action_ACTION_DENY,
Action: pbauth.Action_ACTION_ALLOW,
Permissions: []*pbauth.Permission{p1},
}).
WithTenancy(tenancy).
@ -154,7 +154,7 @@ func (suite *controllerSuite) TestReconcile_CTPCreate_ReferencingTrafficPermissi
// Ensure that the CTP was created
ctp := suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctp, []*pbauth.Permission{p2}, []*pbauth.Permission{p1})
suite.requireCTP(ctp, []*pbauth.Permission{p1, p2}, []*pbauth.Permission{})
rtest.RequireOwner(suite.T(), ctp, wi.Id, true)
})
}
@ -174,7 +174,7 @@ func (suite *controllerSuite) TestReconcile_WorkloadIdentityDelete_ReferencingTr
Destination: &pbauth.Destination{
IdentityName: "wi1",
},
Action: pbauth.Action_ACTION_DENY,
Action: pbauth.Action_ACTION_ALLOW,
Permissions: []*pbauth.Permission{p1},
}).
WithTenancy(tenancy).
@ -270,7 +270,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination
Destination: &pbauth.Destination{
IdentityName: "wi1",
},
Action: pbauth.Action_ACTION_DENY,
Action: pbauth.Action_ACTION_ALLOW,
Permissions: []*pbauth.Permission{p1},
}).Write(suite.T(), suite.client)
suite.requireTrafficPermissionsTracking(tp1, id)
@ -298,7 +298,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination
// Ensure that the CTP was updated
ctpResource = suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctpResource, []*pbauth.Permission{p2}, []*pbauth.Permission{p1})
suite.requireCTP(ctpResource, []*pbauth.Permission{p1, p2}, []*pbauth.Permission{})
rtest.RequireOwner(suite.T(), ctpResource, wi.Id, true)
assertCTPDefaultStatus(suite.T(), ctpResource, false)
@ -316,7 +316,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination
Destination: &pbauth.Destination{
IdentityName: "wi1",
},
Action: pbauth.Action_ACTION_DENY,
Action: pbauth.Action_ACTION_ALLOW,
Permissions: []*pbauth.Permission{p3},
}).Write(suite.T(), suite.client)
suite.requireTrafficPermissionsTracking(tp3, id)
@ -326,7 +326,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsCreate_Destination
// Ensure that the CTP was updated
ctpResource = suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctpResource, []*pbauth.Permission{p2}, []*pbauth.Permission{p1, p3})
suite.requireCTP(ctpResource, []*pbauth.Permission{p1, p2, p3}, []*pbauth.Permission{})
rtest.RequireOwner(suite.T(), ctpResource, wi.Id, true)
assertCTPDefaultStatus(suite.T(), ctpResource, false)
@ -368,7 +368,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsDelete_Destination
Destination: &pbauth.Destination{
IdentityName: "wi1",
},
Action: pbauth.Action_ACTION_DENY,
Action: pbauth.Action_ACTION_ALLOW,
Permissions: []*pbauth.Permission{p1},
}).
WithTenancy(tenancy).
@ -398,7 +398,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsDelete_Destination
require.NoError(suite.T(), err)
ctp := suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctp, []*pbauth.Permission{p2}, []*pbauth.Permission{p1})
suite.requireCTP(ctp, []*pbauth.Permission{p1, p2}, []*pbauth.Permission{})
rtest.RequireOwner(suite.T(), ctp, wi.Id, true)
// Delete TP2
@ -409,7 +409,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsDelete_Destination
// Ensure that the CTP was updated
ctp = suite.client.RequireResourceExists(suite.T(), id)
suite.requireCTP(ctp, []*pbauth.Permission{}, []*pbauth.Permission{p1})
suite.requireCTP(ctp, []*pbauth.Permission{p1}, []*pbauth.Permission{})
// Ensure TP2 is untracked
newTps := suite.mapper.GetTrafficPermissionsForCTP(ctp.Id)
@ -434,7 +434,7 @@ func (suite *controllerSuite) TestReconcile_TrafficPermissionsDelete_Destination
Destination: &pbauth.Destination{
IdentityName: "wi1",
},
Action: pbauth.Action_ACTION_DENY,
Action: pbauth.Action_ACTION_ALLOW,
Permissions: []*pbauth.Permission{p1},
}).
WithTenancy(tenancy).

1
internal/auth/internal/types/errors.go
View File

@ -6,7 +6,6 @@ package types
import "errors"
var (
errInvalidAction = errors.New("action must be either allow or deny")
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.")
errSourceExcludes = errors.New("must be defined on wildcard sources")

20
internal/auth/internal/types/traffic_permissions.go
View File

@ -106,20 +106,18 @@ func firstNonEmptyString(a, b, c string) (string, bool) {
var ValidateTrafficPermissions = resource.DecodeAndValidate(validateTrafficPermissions)
// validator takes a traffic permission and ensures that it conforms to the actions allowed in
// either CE or Enterprise versions of Consul
type validator interface {
ValidateAction(res *DecodedTrafficPermissions) error
}
func validateTrafficPermissions(res *DecodedTrafficPermissions) error {
var merr error
// enumcover:pbauth.Action
switch res.Data.Action {
case pbauth.Action_ACTION_ALLOW:
case pbauth.Action_ACTION_DENY:
case pbauth.Action_ACTION_UNSPECIFIED:
fallthrough
default:
merr = multierror.Append(merr, resource.ErrInvalidField{
Name: "data.action",
Wrapped: errInvalidAction,
})
err := v.ValidateAction(res)
if err != nil {
merr = multierror.Append(merr, err)
}
if res.Data.Destination == nil || (len(res.Data.Destination.IdentityName) == 0) {

73
internal/auth/internal/types/traffic_permissions_ce_test.go Normal file
View File

@ -0,0 +1,73 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1
//go:build !consulent
package types
import (
"testing"
"github.com/hashicorp/consul/internal/resource/resourcetest"
pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1"
"github.com/hashicorp/consul/sdk/testutil"
"github.com/stretchr/testify/require"
)
func TestValidateTrafficPermissionsActionCE(t *testing.T) {
cases := map[string]struct {
tp *pbauth.TrafficPermissions
expectErr string
}{
"ok-minimal": {
tp: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{IdentityName: "wi-1"},
Action: pbauth.Action_ACTION_ALLOW,
},
},
"unspecified-action": {
// Any type other than the TrafficPermissions type would work
// to cause the error we are expecting
tp: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{
IdentityName: "wi1",
},
Action: pbauth.Action_ACTION_UNSPECIFIED,
Permissions: nil,
},
expectErr: `invalid "data.action" field: action must be allow`,
},
"deny-action": {
tp: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{IdentityName: "wi-1"},
Action: pbauth.Action_ACTION_DENY,
},
expectErr: `invalid "data.action" field: action must be allow`,
},
"invalid-action": {
tp: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{
IdentityName: "wi1",
},
Action: pbauth.Action(50),
Permissions: nil,
},
expectErr: `invalid "data.action" field: action must be allow`,
},
}
for n, tc := range cases {
t.Run(n, func(t *testing.T) {
res := resourcetest.Resource(pbauth.TrafficPermissionsType, "tp").
WithData(t, tc.tp).
Build()
err := ValidateTrafficPermissions(res)
if tc.expectErr == "" {
require.NoError(t, err)
} else {
testutil.RequireErrorContains(t, err, tc.expectErr)
}
})
}
}

4
internal/auth/internal/types/traffic_permissions_test.go
View File

@ -52,7 +52,7 @@ func TestValidateTrafficPermissions(t *testing.T) {
Action: pbauth.Action_ACTION_UNSPECIFIED,
Permissions: nil,
},
expectErr: `invalid "data.action" field: action must be either allow or deny`,
expectErr: `invalid "data.action" field`,
},
"invalid-action": {
tp: &pbauth.TrafficPermissions{
@ -62,7 +62,7 @@ func TestValidateTrafficPermissions(t *testing.T) {
Action: pbauth.Action(50),
Permissions: nil,
},
expectErr: `invalid "data.action" field: action must be either allow or deny`,
expectErr: `invalid "data.action" field`,
},
"no-destination": {
tp: &pbauth.TrafficPermissions{

36
internal/auth/internal/types/traffic_permissions_validator_ce.go Normal file
View File

@ -0,0 +1,36 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1
//go:build !consulent
package types
import (
"errors"
"github.com/hashicorp/consul/internal/resource"
pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1"
)
var v validator = &actionValidator{}
type actionValidator struct{}
func (v *actionValidator) ValidateAction(res *DecodedTrafficPermissions) error {
// enumcover:pbauth.Action
switch res.Data.Action {
case pbauth.Action_ACTION_ALLOW:
case pbauth.Action_ACTION_UNSPECIFIED:
fallthrough
case pbauth.Action_ACTION_DENY:
fallthrough
default:
return resource.ErrInvalidField{
Name: "data.action",
Wrapped: errors.New("action must be allow"),
}
}
return nil
}
var _ validator = (*actionValidator)(nil)

106
test/integration/consul-container/test/trafficpermissions/tcp_test.go
View File

@ -11,6 +11,8 @@ import (
"github.com/hashicorp/consul/sdk/testutil/retry"
"github.com/stretchr/testify/require"
rtest "github.com/hashicorp/consul/internal/resource/resourcetest"
pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
@ -20,7 +22,6 @@ import (
libservice "github.com/hashicorp/consul/test/integration/consul-container/libs/service"
"github.com/hashicorp/consul/test/integration/consul-container/libs/topology"
"github.com/hashicorp/consul/test/integration/consul-container/libs/utils"
"github.com/stretchr/testify/require"
)
const (
@ -192,97 +193,6 @@ func TestTrafficPermission_TCP_DefaultDeny(t *testing.T) {
client2TCPSuccess: true,
client2EchoSuccess: true,
},
"deny takes precedence over allow": {
tp1: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{
IdentityName: staticServerIdentity,
},
Action: pbauth.Action_ACTION_DENY,
Permissions: []*pbauth.Permission{
{
Sources: []*pbauth.Source{
{
IdentityName: "static-client-1-identity",
Namespace: "default",
Partition: "default",
Peer: "local",
},
},
},
},
},
tp2: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{
IdentityName: staticServerIdentity,
},
Action: pbauth.Action_ACTION_ALLOW,
Permissions: []*pbauth.Permission{
{
Sources: []*pbauth.Source{
{
IdentityName: "static-client-1-identity",
Namespace: "default",
Partition: "default",
Peer: "local",
},
},
},
},
},
client1TCPSuccess: false,
client1EchoSuccess: false,
client2TCPSuccess: false,
client2EchoSuccess: false,
},
"deny all exclude service + allow on that service": {
tp1: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{
IdentityName: staticServerIdentity,
},
Action: pbauth.Action_ACTION_DENY,
Permissions: []*pbauth.Permission{
{
Sources: []*pbauth.Source{
{
Namespace: "default",
Partition: "default",
Peer: "local",
Exclude: []*pbauth.ExcludeSource{
{
IdentityName: "static-client-1-identity",
Namespace: "default",
Partition: "default",
Peer: "local",
},
},
},
},
},
},
},
tp2: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{
IdentityName: staticServerIdentity,
},
Action: pbauth.Action_ACTION_ALLOW,
Permissions: []*pbauth.Permission{
{
Sources: []*pbauth.Source{
{
IdentityName: "static-client-1-identity",
Namespace: "default",
Partition: "default",
Peer: "local",
},
},
},
},
},
client1TCPSuccess: true,
client1EchoSuccess: true,
client2TCPSuccess: false,
client2EchoSuccess: false,
},
}
runTrafficPermissionsTests(t, true, cases)
@ -309,18 +219,6 @@ func TestTrafficPermission_TCP_DefaultAllow(t *testing.T) {
client2TCPSuccess: false,
client2EchoSuccess: false,
},
"empty deny denies everything": {
tp1: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{
IdentityName: staticServerIdentity,
},
Action: pbauth.Action_ACTION_DENY,
},
client1TCPSuccess: false,
client1EchoSuccess: false,
client2TCPSuccess: false,
client2EchoSuccess: false,
},
"allow everything": {
tp1: &pbauth.TrafficPermissions{
Destination: &pbauth.Destination{