Skip to content
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

resource: Require scope for resource registration #18635

Merged
merged 2 commits into from
Sep 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion agent/grpc-external/services/resource/testing/testing.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"context"
"testing"

"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
Expand All @@ -16,6 +17,7 @@ import (
"github.com/hashicorp/consul/acl"
"github.com/hashicorp/consul/acl/resolver"
svc "github.com/hashicorp/consul/agent/grpc-external/services/resource"
"github.com/hashicorp/consul/agent/grpc-external/testutils"
internal "github.com/hashicorp/consul/agent/grpc-internal"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/internal/resource"
Expand Down Expand Up @@ -52,7 +54,27 @@ func AuthorizerFrom(t *testing.T, policyStrs ...string) resolver.Result {
// returns a client to interact with it. ACLs will be disabled and only the
// default partition and namespace are available.
func RunResourceService(t *testing.T, registerFns ...func(resource.Registry)) pbresource.ResourceServiceClient {
return RunResourceServiceWithACL(t, resolver.DANGER_NO_AUTH{}, registerFns...)
// Provide a resolver which will default partition and namespace when not provided. This is similar to user
// initiated requests.
//
// Controllers under test should be providing full tenancy since they will run with the DANGER_NO_AUTH.
mockACLResolver := &svc.MockACLResolver{}
mockACLResolver.On("ResolveTokenAndDefaultMeta", mock.Anything, mock.Anything, mock.Anything).
Return(testutils.ACLsDisabled(t), nil).
Run(func(args mock.Arguments) {
// Caller expecting passed in tokenEntMeta and authorizerContext to be filled in.
tokenEntMeta := args.Get(1).(*acl.EnterpriseMeta)
if tokenEntMeta != nil {
FillEntMeta(tokenEntMeta)
}

authzContext := args.Get(2).(*acl.AuthorizerContext)
if authzContext != nil {
FillAuthorizerContext(authzContext)
}
})

return RunResourceServiceWithACL(t, mockACLResolver, registerFns...)
}

func RunResourceServiceWithACL(t *testing.T, aclResolver svc.ACLResolver, registerFns ...func(resource.Registry)) pbresource.ResourceServiceClient {
Expand Down
19 changes: 19 additions & 0 deletions agent/grpc-external/services/resource/testing/testing_ce.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

//go:build !consulent
// +build !consulent

package testing

import (
"github.com/hashicorp/consul/acl"
)

func FillEntMeta(entMeta *acl.EnterpriseMeta) {
// nothing to to in CE.
}

func FillAuthorizerContext(authzContext *acl.AuthorizerContext) {
// nothing to to in CE.
}
5 changes: 3 additions & 2 deletions internal/catalog/catalogtest/test_integration_v1alpha1.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/hashicorp/consul/internal/catalog/internal/controllers/nodehealth"
"github.com/hashicorp/consul/internal/catalog/internal/controllers/workloadhealth"
"github.com/hashicorp/consul/internal/catalog/internal/types"
"github.com/hashicorp/consul/internal/resource"
rtest "github.com/hashicorp/consul/internal/resource/resourcetest"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v1alpha1"
"github.com/hashicorp/consul/proto-public/pbresource"
Expand Down Expand Up @@ -68,15 +69,15 @@ func VerifyCatalogV1Alpha1IntegrationTestResults(t *testing.T, client pbresource
c.RequireResourceExists(t, rtest.Resource(catalog.ServiceV1Alpha1Type, "foo").ID())

for i := 1; i < 5; i++ {
nodeId := rtest.Resource(catalog.NodeV1Alpha1Type, fmt.Sprintf("node-%d", i)).ID()
nodeId := rtest.Resource(catalog.NodeV1Alpha1Type, fmt.Sprintf("node-%d", i)).WithTenancy(resource.DefaultNamespacedTenancy()).ID()
c.RequireResourceExists(t, nodeId)

res := c.RequireResourceExists(t, rtest.Resource(catalog.HealthStatusV1Alpha1Type, fmt.Sprintf("node-%d-health", i)).ID())
rtest.RequireOwner(t, res, nodeId, true)
}

for i := 1; i < 21; i++ {
workloadId := rtest.Resource(catalog.WorkloadV1Alpha1Type, fmt.Sprintf("api-%d", i)).ID()
workloadId := rtest.Resource(catalog.WorkloadV1Alpha1Type, fmt.Sprintf("api-%d", i)).WithTenancy(resource.DefaultNamespacedTenancy()).ID()
c.RequireResourceExists(t, workloadId)

res := c.RequireResourceExists(t, rtest.Resource(catalog.HealthStatusV1Alpha1Type, fmt.Sprintf("api-%d-health", i)).ID())
Expand Down
1 change: 0 additions & 1 deletion internal/catalog/catalogtest/test_lifecycle_v1alpha1.go
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,6 @@ func RunCatalogV1Alpha1EndpointsLifecycleIntegrationTest(t *testing.T, client pb
// Now delete the service and ensure that the endpoints eventually are deleted as well
c.MustDelete(t, service.Id)
c.WaitForDeletion(t, endpointsID)

}

func setHealthStatus(t *testing.T, client *rtest.Client, owner *pbresource.ID, name string, health pbcatalog.Health) *pbresource.Resource {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func (suite *reconciliationDataSuite) SetupTest() {
WithData(suite.T(), &pbcatalog.ServiceEndpoints{
Endpoints: []*pbcatalog.Endpoint{
{
TargetRef: rtest.Resource(types.WorkloadType, "api-1").ID(),
TargetRef: rtest.Resource(types.WorkloadType, "api-1").WithTenancy(resource.DefaultNamespacedTenancy()).ID(),
Addresses: []*pbcatalog.WorkloadAddress{
{
Host: "127.0.0.1",
Expand All @@ -150,7 +150,7 @@ func (suite *reconciliationDataSuite) SetupTest() {
func (suite *reconciliationDataSuite) TestGetServiceData_NotFound() {
// This test's purposes is to ensure that NotFound errors when retrieving
// the service data are ignored properly.
data, err := getServiceData(suite.ctx, suite.rt, rtest.Resource(types.ServiceType, "not-found").ID())
data, err := getServiceData(suite.ctx, suite.rt, rtest.Resource(types.ServiceType, "not-found").WithTenancy(resource.DefaultNamespacedTenancy()).ID())
require.NoError(suite.T(), err)
require.Nil(suite.T(), data)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ func (suite *controllerSuite) TestController() {
go mgr.Run(suite.ctx)

// Create an advance pointer to some services.
apiServiceRef := resource.Reference(rtest.Resource(types.ServiceType, "api").ID(), "")
otherServiceRef := resource.Reference(rtest.Resource(types.ServiceType, "other").ID(), "")
apiServiceRef := resource.Reference(rtest.Resource(types.ServiceType, "api").WithTenancy(resource.DefaultNamespacedTenancy()).ID(), "")
otherServiceRef := resource.Reference(rtest.Resource(types.ServiceType, "other").WithTenancy(resource.DefaultNamespacedTenancy()).ID(), "")

// create a failover without any services
failoverData := &pbcatalog.FailoverPolicy{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,31 +24,37 @@ func TestMapper_Tracking(t *testing.T) {

// Create an advance pointer to some services.
randoSvc := rtest.Resource(types.ServiceType, "rando").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, &pbcatalog.Service{}).
Build()
rtest.ValidateAndNormalize(t, registry, randoSvc)

apiSvc := rtest.Resource(types.ServiceType, "api").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, &pbcatalog.Service{}).
Build()
rtest.ValidateAndNormalize(t, registry, apiSvc)

fooSvc := rtest.Resource(types.ServiceType, "foo").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, &pbcatalog.Service{}).
Build()
rtest.ValidateAndNormalize(t, registry, fooSvc)

barSvc := rtest.Resource(types.ServiceType, "bar").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, &pbcatalog.Service{}).
Build()
rtest.ValidateAndNormalize(t, registry, barSvc)

wwwSvc := rtest.Resource(types.ServiceType, "www").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, &pbcatalog.Service{}).
Build()
rtest.ValidateAndNormalize(t, registry, wwwSvc)

fail1 := rtest.Resource(types.FailoverPolicyType, "api").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, &pbcatalog.FailoverPolicy{
Config: &pbcatalog.FailoverConfig{
Destinations: []*pbcatalog.FailoverDestination{
Expand All @@ -62,6 +68,7 @@ func TestMapper_Tracking(t *testing.T) {
failDec1 := rtest.MustDecode[*pbcatalog.FailoverPolicy](t, fail1)

fail2 := rtest.Resource(types.FailoverPolicyType, "www").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, &pbcatalog.FailoverPolicy{
Config: &pbcatalog.FailoverConfig{
Destinations: []*pbcatalog.FailoverDestination{
Expand All @@ -75,6 +82,7 @@ func TestMapper_Tracking(t *testing.T) {
failDec2 := rtest.MustDecode[*pbcatalog.FailoverPolicy](t, fail2)

fail1_updated := rtest.Resource(types.FailoverPolicyType, "api").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, &pbcatalog.FailoverPolicy{
Config: &pbcatalog.FailoverConfig{
Destinations: []*pbcatalog.FailoverDestination{
Expand Down Expand Up @@ -178,13 +186,7 @@ func requireServicesTracked(t *testing.T, mapper *Mapper, svc *pbresource.Resour
}

func newRef(typ *pbresource.Type, name string) *pbresource.Reference {
return rtest.Resource(typ, name).Reference("")
}

func defaultTenancy() *pbresource.Tenancy {
return &pbresource.Tenancy{
Partition: "default",
Namespace: "default",
PeerName: "local",
}
return rtest.Resource(typ, name).
WithTenancy(resource.DefaultNamespacedTenancy()).
Reference("")
}
1 change: 1 addition & 0 deletions internal/catalog/internal/types/dns_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func RegisterDNSPolicy(r resource.Registry) {
r.Register(resource.Registration{
Type: DNSPolicyV1Alpha1Type,
Proto: &pbcatalog.DNSPolicy{},
Scope: resource.ScopeNamespace,
Validate: ValidateDNSPolicy,
})
}
Expand Down
1 change: 1 addition & 0 deletions internal/catalog/internal/types/failover_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func RegisterFailoverPolicy(r resource.Registry) {
r.Register(resource.Registration{
Type: FailoverPolicyV1Alpha1Type,
Proto: &pbcatalog.FailoverPolicy{},
Scope: resource.ScopeNamespace,
Mutate: MutateFailoverPolicy,
Validate: ValidateFailoverPolicy,
})
Expand Down
7 changes: 5 additions & 2 deletions internal/catalog/internal/types/failover_policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ func TestValidateFailoverPolicy(t *testing.T) {

run := func(t *testing.T, tc testcase) {
res := resourcetest.Resource(FailoverPolicyType, "api").
WithTenancy(resource.DefaultNamespacedTenancy()).
WithData(t, tc.failover).
Build()

Expand Down Expand Up @@ -228,7 +229,7 @@ func TestValidateFailoverPolicy(t *testing.T) {
"dest: ref with section": {
config: &pbcatalog.FailoverConfig{
Destinations: []*pbcatalog.FailoverDestination{
{Ref: resourcetest.Resource(ServiceType, "api").Reference("blah")},
{Ref: resourcetest.Resource(ServiceType, "api").WithTenancy(resource.DefaultNamespacedTenancy()).Reference("blah")},
},
},
expectErr: `invalid element at index 0 of list "destinations": invalid "ref" field: invalid "section" field: section not supported for failover policy dest refs`,
Expand Down Expand Up @@ -589,7 +590,9 @@ func TestSimplifyFailoverPolicy(t *testing.T) {
}

func newRef(typ *pbresource.Type, name string) *pbresource.Reference {
return resourcetest.Resource(typ, name).Reference("")
return resourcetest.Resource(typ, name).
WithTenancy(resource.DefaultNamespacedTenancy()).
Reference("")
}

func newRefWithPeer(typ *pbresource.Type, name string, peer string) *pbresource.Reference {
Expand Down
1 change: 1 addition & 0 deletions internal/catalog/internal/types/health_checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func RegisterHealthChecks(r resource.Registry) {
r.Register(resource.Registration{
Type: HealthChecksV1Alpha1Type,
Proto: &pbcatalog.HealthChecks{},
Scope: resource.ScopeNamespace,
Validate: ValidateHealthChecks,
})
}
Expand Down
1 change: 1 addition & 0 deletions internal/catalog/internal/types/health_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func RegisterHealthStatus(r resource.Registry) {
r.Register(resource.Registration{
Type: HealthStatusV1Alpha1Type,
Proto: &pbcatalog.HealthStatus{},
Scope: resource.ScopeNamespace,
Validate: ValidateHealthStatus,
})
}
Expand Down
10 changes: 8 additions & 2 deletions internal/catalog/internal/types/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,14 @@ var (

func RegisterNode(r resource.Registry) {
r.Register(resource.Registration{
Type: NodeV1Alpha1Type,
Proto: &pbcatalog.Node{},
Type: NodeV1Alpha1Type,
Proto: &pbcatalog.Node{},
// TODO: A node should be partition scoped. However its HealthStatus which is
// namespace scoped has Node as an owner. We do not support ownership between resources
// of differing scope at this time. HealthStatus will probably be split out into two different
// types, one for namespace scoped owners and the other for partition scoped owners.
// Until that time, Node will remain namespace scoped.
Scope: resource.ScopeNamespace,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I needed to set this to ScopePartition to test partition. Maybe we need to do that split soon 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @ishustava regarding tracking/prioritizing this change to Node/HealthStatus since it seems to fall under catalog and mesh.

Validate: ValidateNode,
})
}
Expand Down
2 changes: 1 addition & 1 deletion internal/catalog/internal/types/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ func RegisterService(r resource.Registry) {
r.Register(resource.Registration{
Type: ServiceV1Alpha1Type,
Proto: &pbcatalog.Service{},
Validate: ValidateService,
Scope: resource.ScopeNamespace,
Validate: ValidateService,
})
}

Expand Down
1 change: 1 addition & 0 deletions internal/catalog/internal/types/service_endpoints.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ func RegisterServiceEndpoints(r resource.Registry) {
r.Register(resource.Registration{
Type: ServiceEndpointsV1Alpha1Type,
Proto: &pbcatalog.ServiceEndpoints{},
Scope: resource.ScopeNamespace,
Validate: ValidateServiceEndpoints,
Mutate: MutateServiceEndpoints,
})
Expand Down
6 changes: 5 additions & 1 deletion internal/catalog/internal/types/service_endpoints_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ func TestValidateServiceEndpoints_Ok(t *testing.T) {
}

res := rtest.Resource(ServiceEndpointsType, "test-service").
WithTenancy(defaultEndpointTenancy).
WithData(t, data).
Build()

Expand Down Expand Up @@ -231,6 +232,7 @@ func TestValidateServiceEndpoints_EndpointInvalid(t *testing.T) {
},
}
res := rtest.Resource(ServiceEndpointsType, "test-service").
WithTenancy(defaultEndpointTenancy).
WithOwner(tcase.owner).
WithData(t, data).
Build()
Expand All @@ -246,7 +248,9 @@ func TestValidateServiceEndpoints_EndpointInvalid(t *testing.T) {
}

func TestMutateServiceEndpoints_PopulateOwner(t *testing.T) {
res := rtest.Resource(ServiceEndpointsType, "test-service").Build()
res := rtest.Resource(ServiceEndpointsType, "test-service").
WithTenancy(defaultEndpointTenancy).
Build()

require.NoError(t, MutateServiceEndpoints(res))
require.NotNil(t, res.Owner)
Expand Down
1 change: 1 addition & 0 deletions internal/catalog/internal/types/virtual_ips.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func RegisterVirtualIPs(r resource.Registry) {
r.Register(resource.Registration{
Type: VirtualIPsV1Alpha1Type,
Proto: &pbcatalog.VirtualIPs{},
Scope: resource.ScopeNamespace,
Validate: ValidateVirtualIPs,
})
}
Expand Down
1 change: 1 addition & 0 deletions internal/catalog/internal/types/workload.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func RegisterWorkload(r resource.Registry) {
r.Register(resource.Registration{
Type: WorkloadV1Alpha1Type,
Proto: &pbcatalog.Workload{},
Scope: resource.ScopeNamespace,
Validate: ValidateWorkload,
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ func (suite *xdsControllerTestSuite) TestReconcile_PushChangeError() {
func (suite *xdsControllerTestSuite) TestReconcile_MissingEndpoint() {
// Set fooProxyStateTemplate with a reference to fooEndpoints, without storing fooEndpoints so the controller should
// notice it's missing.
fooEndpointsId := resourcetest.Resource(catalog.ServiceEndpointsType, "foo-service").ID()
fooEndpointsId := resourcetest.Resource(catalog.ServiceEndpointsType, "foo-service").WithTenancy(resource.DefaultNamespacedTenancy()).ID()
fooRequiredEndpoints := make(map[string]*pbproxystate.EndpointRef)
fooRequiredEndpoints["test-cluster-1"] = &pbproxystate.EndpointRef{
Id: fooEndpointsId,
Expand Down
1 change: 1 addition & 0 deletions internal/mesh/internal/types/computed_routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ func RegisterComputedRoutes(r resource.Registry) {
r.Register(resource.Registration{
Type: ComputedRoutesV1Alpha1Type,
Proto: &pbmesh.ComputedRoutes{},
Scope: resource.ScopeNamespace,
Validate: ValidateComputedRoutes,
})
}
Expand Down
1 change: 1 addition & 0 deletions internal/mesh/internal/types/destination_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func RegisterDestinationPolicy(r resource.Registry) {
r.Register(resource.Registration{
Type: DestinationPolicyV1Alpha1Type,
Proto: &pbmesh.DestinationPolicy{},
Scope: resource.ScopeNamespace,
Validate: ValidateDestinationPolicy,
})
}
Expand Down
1 change: 1 addition & 0 deletions internal/mesh/internal/types/grpc_route.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func RegisterGRPCRoute(r resource.Registry) {
r.Register(resource.Registration{
Type: GRPCRouteV1Alpha1Type,
Proto: &pbmesh.GRPCRoute{},
Scope: resource.ScopeNamespace,
// TODO(rb): normalize parent/backend ref tenancies in a Mutate hook
Validate: ValidateGRPCRoute,
})
Expand Down
1 change: 1 addition & 0 deletions internal/mesh/internal/types/http_route.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ func RegisterHTTPRoute(r resource.Registry) {
r.Register(resource.Registration{
Type: HTTPRouteV1Alpha1Type,
Proto: &pbmesh.HTTPRoute{},
Scope: resource.ScopeNamespace,
Mutate: MutateHTTPRoute,
Validate: ValidateHTTPRoute,
})
Expand Down
3 changes: 2 additions & 1 deletion internal/mesh/internal/types/http_route_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"google.golang.org/protobuf/types/known/durationpb"

"github.com/hashicorp/consul/internal/catalog"
"github.com/hashicorp/consul/internal/resource"
"github.com/hashicorp/consul/internal/resource/resourcetest"
pbmesh "github.com/hashicorp/consul/proto-public/pbmesh/v1alpha1"
"github.com/hashicorp/consul/proto-public/pbresource"
Expand Down Expand Up @@ -962,7 +963,7 @@ func getXRouteRetriesTestCases() map[string]xRouteRetriesTestcase {
}

func newRef(typ *pbresource.Type, name string) *pbresource.Reference {
return resourcetest.Resource(typ, name).Reference("")
return resourcetest.Resource(typ, name).WithTenancy(resource.DefaultNamespacedTenancy()).Reference("")
}

func newBackendRef(typ *pbresource.Type, name, port string) *pbmesh.BackendReference {
Expand Down
Loading