From 51419de72c2c3551a419a246ddc3ef7689db2de3 Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Tue, 6 Feb 2024 09:48:50 -0800 Subject: [PATCH] Backport of Panic for unregistered types into release/1.18.x (#20504) * backport of commit 392b8d7573b841827517cbcadfcd699e2321c25b * backport of commit b4716599ae56f5220eddc6511d674b5da205b440 * backport of commit a03cb97cb0c9b5e861b5db6e86fee48b7603c445 * backport of commit 73b277cdef2d77a57bbdaa1f9fd63c31ba71554f * backport of commit e53b9794c8f0a8b231cea239db1f2b6b89b8c1c4 --------- Co-authored-by: Matt Keeler --- agent/consul/client_test.go | 11 +---- agent/consul/server_test.go | 45 +++++++++---------- .../testdata/v2-resource-dependencies.md | 9 +++- agent/rpc/peering/service_test.go | 4 +- internal/controller/runner.go | 13 ++++++ .../mesh/internal/controllers/register.go | 6 ++- 6 files changed, 47 insertions(+), 41 deletions(-) diff --git a/agent/consul/client_test.go b/agent/consul/client_test.go index a3967e8c63..b308450800 100644 --- a/agent/consul/client_test.go +++ b/agent/consul/client_test.go @@ -7,9 +7,6 @@ import ( "bytes" "context" "fmt" - "github.com/hashicorp/consul/internal/catalog" - "github.com/hashicorp/consul/internal/mesh" - "github.com/hashicorp/consul/internal/resource/demo" "net" "os" "strings" @@ -17,8 +14,6 @@ import ( "testing" "time" - "github.com/hashicorp/consul/internal/resource" - "github.com/hashicorp/go-hclog" "github.com/hashicorp/serf/serf" "github.com/stretchr/testify/require" @@ -562,10 +557,6 @@ func newDefaultDeps(t testutil.TestingTB, c *Config) Deps { RPCHoldTimeout: c.RPCHoldTimeout, } connPool.SetRPCClientTimeout(c.RPCClientTimeout) - registry := resource.NewRegistry() - demo.RegisterTypes(registry) - mesh.RegisterTypes(registry) - catalog.RegisterTypes(registry) return Deps{ EventPublisher: stream.NewEventPublisher(10 * time.Second), Logger: logger, @@ -585,7 +576,7 @@ func newDefaultDeps(t testutil.TestingTB, c *Config) Deps { GetNetRPCInterceptorFunc: middleware.GetNetRPCInterceptor, EnterpriseDeps: newDefaultDepsEnterprise(t, logger, c), XDSStreamLimiter: limiter.NewSessionLimiter(), - Registry: registry, + Registry: NewTypeRegistry(), } } diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 1e884e609e..19cbd56849 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -7,11 +7,9 @@ import ( "context" "crypto/tls" "crypto/x509" - "flag" "fmt" "net" "os" - "path/filepath" "reflect" "strings" "sync" @@ -2296,37 +2294,34 @@ func TestServer_addServerTLSInfo(t *testing.T) { } } -// goldenMarkdown reads and optionally writes the expected data to the goldenMarkdown file, -// returning the contents as a string. -func goldenMarkdown(t *testing.T, name, got string) string { - t.Helper() - - golden := filepath.Join("testdata", name+".md") - update := flag.Lookup("update").Value.(flag.Getter).Get().(bool) - if update && got != "" { - err := os.WriteFile(golden, []byte(got), 0644) - require.NoError(t, err) - } - - expected, err := os.ReadFile(golden) - require.NoError(t, err) - - return string(expected) -} - func TestServer_ControllerDependencies(t *testing.T) { - t.Parallel() + // The original goal of this test was to track controller/resource type dependencies + // as they change over time. However, the test is difficult to maintain and provides + // only limited value as we were not even performing validations on them. The Server + // type itself will validate that no cyclical dependencies exist so this test really + // only produces a visual representation of the dependencies. That comes at the expense + // of having to maintain the golden files. What further complicates this is that + // Consul Enterprise will have potentially different dependencies that don't exist + // in CE. Therefore if we want to maintain this test, we would need to have a separate + // Enterprise and CE golden files and any CE PR which causes regeneration of the golden + // file would require another commit in enterprise to regen the enterprise golden file + // even if no new enterprise watches were added. + // + // Therefore until we have a better way of managing this, the test will be skipped. + t.Skip("This test would be very difficult to maintain and provides limited value") _, conf := testServerConfig(t) deps := newDefaultDeps(t, conf) - deps.Experiments = []string{"resource-apis"} + deps.Experiments = []string{"resource-apis", "v2tenancy"} deps.LeafCertManager = &leafcert.Manager{} s1, err := newServerWithDeps(t, conf, deps) require.NoError(t, err) waitForLeaderEstablishment(t, s1) - actual := fmt.Sprintf("```mermaid\n%s\n```", s1.controllerManager.CalculateDependencies(s1.registry.Types()).ToMermaid()) - expected := goldenMarkdown(t, "v2-resource-dependencies", actual) - require.Equal(t, expected, actual) + // gotest.tools/v3 defines CLI flags which are incompatible wit the golden package + // Once we eliminate gotest.tools/v3 from usage within Consul we could uncomment this + // actual := fmt.Sprintf("```mermaid\n%s\n```", s1.controllerManager.CalculateDependencies(s1.registry.Types()).ToMermaid()) + // expected := golden.Get(t, actual, "v2-resource-dependencies") + // require.Equal(t, expected, actual) } diff --git a/agent/consul/testdata/v2-resource-dependencies.md b/agent/consul/testdata/v2-resource-dependencies.md index 8c5e7c7776..68a67d28a0 100644 --- a/agent/consul/testdata/v2-resource-dependencies.md +++ b/agent/consul/testdata/v2-resource-dependencies.md @@ -4,6 +4,10 @@ flowchart TD auth/v2beta1/computedtrafficpermissions --> auth/v2beta1/partitiontrafficpermissions auth/v2beta1/computedtrafficpermissions --> auth/v2beta1/trafficpermissions auth/v2beta1/computedtrafficpermissions --> auth/v2beta1/workloadidentity + auth/v2beta1/namespacetrafficpermissions + auth/v2beta1/partitiontrafficpermissions + auth/v2beta1/trafficpermissions + auth/v2beta1/workloadidentity catalog/v2beta1/computedfailoverpolicy --> catalog/v2beta1/failoverpolicy catalog/v2beta1/computedfailoverpolicy --> catalog/v2beta1/service catalog/v2beta1/failoverpolicy @@ -25,7 +29,6 @@ flowchart TD hcp/v2/link hcp/v2/telemetrystate --> hcp/v2/link internal/v1/tombstone - mesh/v2beta1/apigateway mesh/v2beta1/computedexplicitdestinations --> catalog/v2beta1/service mesh/v2beta1/computedexplicitdestinations --> catalog/v2beta1/workload mesh/v2beta1/computedexplicitdestinations --> mesh/v2beta1/computedroutes @@ -57,4 +60,8 @@ flowchart TD multicluster/v2/computedexportedservices --> multicluster/v2/exportedservices multicluster/v2/computedexportedservices --> multicluster/v2/namespaceexportedservices multicluster/v2/computedexportedservices --> multicluster/v2/partitionexportedservices + multicluster/v2/exportedservices + multicluster/v2/namespaceexportedservices + multicluster/v2/partitionexportedservices + tenancy/v2beta1/namespace ``` \ No newline at end of file diff --git a/agent/rpc/peering/service_test.go b/agent/rpc/peering/service_test.go index 3d1e63d684..efc3bff697 100644 --- a/agent/rpc/peering/service_test.go +++ b/agent/rpc/peering/service_test.go @@ -15,8 +15,6 @@ import ( "testing" "time" - "github.com/hashicorp/consul/internal/resource" - "github.com/google/tcpproxy" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-uuid" @@ -1969,7 +1967,7 @@ func newDefaultDeps(t *testing.T, c *consul.Config) consul.Deps { NewRequestRecorderFunc: middleware.NewRequestRecorder, GetNetRPCInterceptorFunc: middleware.GetNetRPCInterceptor, XDSStreamLimiter: limiter.NewSessionLimiter(), - Registry: resource.NewRegistry(), + Registry: consul.NewTypeRegistry(), } } diff --git a/internal/controller/runner.go b/internal/controller/runner.go index c4240c3cf0..7092654b71 100644 --- a/internal/controller/runner.go +++ b/internal/controller/runner.go @@ -10,6 +10,8 @@ import ( "time" "golang.org/x/sync/errgroup" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/hashicorp/go-hclog" @@ -189,6 +191,7 @@ func (c *controllerRunner) primeCache(ctx context.Context, typ *pbresource.Type) }, }) if err != nil { + c.handleInvalidControllerWatch(err) c.logger.Error("failed to create cache priming watch", "error", err) return err } @@ -196,6 +199,7 @@ func (c *controllerRunner) primeCache(ctx context.Context, typ *pbresource.Type) for { event, err := wl.Recv() if err != nil { + c.handleInvalidControllerWatch(err) c.logger.Warn("error received from cache priming watch", "error", err) return err } @@ -224,6 +228,7 @@ func (c *controllerRunner) watch(ctx context.Context, typ *pbresource.Type, add }, }) if err != nil { + c.handleInvalidControllerWatch(err) c.logger.Error("failed to create watch", "error", err) return err } @@ -231,6 +236,7 @@ func (c *controllerRunner) watch(ctx context.Context, typ *pbresource.Type, add for { event, err := wl.Recv() if err != nil { + c.handleInvalidControllerWatch(err) c.logger.Warn("error received from watch", "error", err) return err } @@ -396,6 +402,13 @@ func (c *controllerRunner) runtime(logger hclog.Logger) Runtime { } } +func (c *controllerRunner) handleInvalidControllerWatch(err error) { + st, ok := status.FromError(err) + if ok && st.Code() == codes.InvalidArgument { + panic(fmt.Sprintf("controller %s attempted to initiate an invalid watch: %q. This is a bug within the controller.", c.ctrl.name, err.Error())) + } +} + type mapperRequest struct{ res *pbresource.Resource } // Key satisfies the queue.ItemType interface. It returns a string which will be diff --git a/internal/mesh/internal/controllers/register.go b/internal/mesh/internal/controllers/register.go index a543fd66d6..1729a51498 100644 --- a/internal/mesh/internal/controllers/register.go +++ b/internal/mesh/internal/controllers/register.go @@ -5,7 +5,6 @@ package controllers import ( "context" - "github.com/hashicorp/consul/internal/mesh/internal/controllers/apigateways" "github.com/hashicorp/consul/internal/mesh/internal/controllers/gatewayproxy" "github.com/hashicorp/consul/internal/mesh/internal/controllers/meshconfiguration" @@ -58,5 +57,8 @@ func Register(mgr *controller.Manager, deps Dependencies) { mgr.Register(meshgateways.Controller()) mgr.Register(meshconfiguration.Controller()) - mgr.Register(apigateways.Controller()) + + // This controller is currently configured to watch types which aren't registered and produces infinite + // errors because of this. Once the watched types are in place we should uncomment this. + // mgr.Register(apigateways.Controller()) }