From f45be222bbb454b6165989f0ab133eb3b2263f50 Mon Sep 17 00:00:00 2001 From: Eric Haberkorn Date: Wed, 18 Oct 2023 10:55:32 -0400 Subject: [PATCH] Prevent circular dependencies between v2 resources and generate a mermaid diagram with their dependencies (#19230) --- agent/consul/server.go | 2 +- agent/consul/server_test.go | 42 +++++++- .../testdata/v2-resource-dependencies.md | 45 ++++++++ internal/controller/dependencies.go | 100 ++++++++++++++++++ internal/controller/dependencies_test.go | 66 ++++++++++++ .../controller/testdata/dependencies.golden | 5 + 6 files changed, 258 insertions(+), 2 deletions(-) create mode 100644 agent/consul/testdata/v2-resource-dependencies.md create mode 100644 internal/controller/dependencies.go create mode 100644 internal/controller/dependencies_test.go create mode 100644 internal/controller/testdata/dependencies.golden diff --git a/agent/consul/server.go b/agent/consul/server.go index 9540cbe1c0..1fe709b4b6 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -976,7 +976,7 @@ func (s *Server) registerControllers(deps Deps, proxyUpdater ProxyUpdater) error demo.RegisterControllers(s.controllerManager) } - return nil + return s.controllerManager.ValidateDependencies(s.registry.Types()) } func newGRPCHandlerFromConfig(deps Deps, config *Config, s *Server) connHandler { diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index e8058a468a..6cf396efe4 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -6,9 +6,11 @@ package consul import ( "context" "crypto/x509" + "flag" "fmt" "net" "os" + "path/filepath" "reflect" "strings" "sync" @@ -35,10 +37,12 @@ import ( external "github.com/hashicorp/consul/agent/grpc-external" grpcmiddleware "github.com/hashicorp/consul/agent/grpc-middleware" hcpclient "github.com/hashicorp/consul/agent/hcp/client" + "github.com/hashicorp/consul/agent/leafcert" "github.com/hashicorp/consul/agent/metadata" "github.com/hashicorp/consul/agent/rpc/middleware" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/token" + proxytracker "github.com/hashicorp/consul/internal/mesh/proxy-tracker" "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/consul/sdk/freeport" "github.com/hashicorp/consul/sdk/testutil" @@ -336,7 +340,8 @@ func newServerWithDeps(t *testing.T, c *Config, deps Deps) (*Server, error) { } } grpcServer := external.NewServer(deps.Logger.Named("grpc.external"), nil, deps.TLSConfigurator, rpcRate.NullRequestLimitsHandler()) - srv, err := NewServer(c, deps, grpcServer, nil, deps.Logger, nil) + proxyUpdater := proxytracker.NewProxyTracker(proxytracker.ProxyTrackerConfig{}) + srv, err := NewServer(c, deps, grpcServer, nil, deps.Logger, proxyUpdater) if err != nil { return nil, err } @@ -2100,3 +2105,38 @@ func TestServer_hcpManager(t *testing.T) { hcp1.AssertExpectations(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() + + _, conf := testServerConfig(t) + deps := newDefaultDeps(t, conf) + deps.Experiments = []string{"resource-apis"} + 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) +} diff --git a/agent/consul/testdata/v2-resource-dependencies.md b/agent/consul/testdata/v2-resource-dependencies.md new file mode 100644 index 0000000000..1e34812446 --- /dev/null +++ b/agent/consul/testdata/v2-resource-dependencies.md @@ -0,0 +1,45 @@ +```mermaid +flowchart TD + auth/v2beta1/computedtrafficpermissions --> auth/v2beta1/trafficpermissions + auth/v2beta1/computedtrafficpermissions --> auth/v2beta1/workloadidentity + catalog/v2beta1/failoverpolicy --> catalog/v2beta1/service + catalog/v2beta1/healthstatus + catalog/v2beta1/node --> catalog/v2beta1/healthstatus + catalog/v2beta1/service + catalog/v2beta1/serviceendpoints --> catalog/v2beta1/service + catalog/v2beta1/serviceendpoints --> catalog/v2beta1/workload + catalog/v2beta1/workload --> catalog/v2beta1/healthstatus + catalog/v2beta1/workload --> catalog/v2beta1/node + demo/v1/album + demo/v1/artist + demo/v1/concept + demo/v1/executive + demo/v1/recordlabel + demo/v2/album + demo/v2/artist + internal/v1/tombstone + mesh/v2beta1/computedexplicitdestinations --> catalog/v2beta1/service + mesh/v2beta1/computedexplicitdestinations --> catalog/v2beta1/workload + mesh/v2beta1/computedexplicitdestinations --> mesh/v2beta1/computedroutes + mesh/v2beta1/computedexplicitdestinations --> mesh/v2beta1/destinations + mesh/v2beta1/computedproxyconfiguration --> catalog/v2beta1/workload + mesh/v2beta1/computedproxyconfiguration --> mesh/v2beta1/proxyconfiguration + mesh/v2beta1/computedroutes --> catalog/v2beta1/failoverpolicy + mesh/v2beta1/computedroutes --> catalog/v2beta1/service + mesh/v2beta1/computedroutes --> mesh/v2beta1/destinationpolicy + mesh/v2beta1/computedroutes --> mesh/v2beta1/grpcroute + mesh/v2beta1/computedroutes --> mesh/v2beta1/httproute + mesh/v2beta1/computedroutes --> mesh/v2beta1/tcproute + mesh/v2beta1/destinationpolicy + mesh/v2beta1/destinations + mesh/v2beta1/grpcroute + mesh/v2beta1/httproute + mesh/v2beta1/proxyconfiguration + mesh/v2beta1/proxystatetemplate --> auth/v2beta1/computedtrafficpermissions + mesh/v2beta1/proxystatetemplate --> catalog/v2beta1/service + mesh/v2beta1/proxystatetemplate --> catalog/v2beta1/workload + mesh/v2beta1/proxystatetemplate --> mesh/v2beta1/computedexplicitdestinations + mesh/v2beta1/proxystatetemplate --> mesh/v2beta1/computedproxyconfiguration + mesh/v2beta1/proxystatetemplate --> mesh/v2beta1/computedroutes + mesh/v2beta1/tcproute +``` \ No newline at end of file diff --git a/internal/controller/dependencies.go b/internal/controller/dependencies.go new file mode 100644 index 0000000000..6a91d91ff7 --- /dev/null +++ b/internal/controller/dependencies.go @@ -0,0 +1,100 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package controller + +import ( + "fmt" + "sort" + "strings" + + "github.com/hashicorp/go-multierror" + + "github.com/hashicorp/consul/internal/resource" + "github.com/hashicorp/consul/proto-public/pbresource" +) + +func (m *Manager) ValidateDependencies(registrations []resource.Registration) error { + deps := m.CalculateDependencies(registrations) + + return deps.validate() +} + +type Dependencies map[string][]string + +func (deps Dependencies) validate() error { + var merr error + seen := make(map[string]map[string]struct{}) + + mkErr := func(src, dst string) error { + vals := []string{src, dst} + sort.Strings(vals) + return fmt.Errorf("circular dependency between %q and %q", vals[0], vals[1]) + } + + for src, dsts := range deps { + seenDsts := seen[src] + if len(seenDsts) == 0 { + seen[src] = make(map[string]struct{}) + } + + for _, dst := range dsts { + if _, ok := seenDsts[dst]; ok { + merr = multierror.Append(merr, mkErr(src, dst)) + } + + if inverseDsts := seen[dst]; len(inverseDsts) > 0 { + if _, ok := inverseDsts[src]; ok { + merr = multierror.Append(merr, mkErr(src, dst)) + } + } + seen[src][dst] = struct{}{} + } + } + + return merr +} + +func (m *Manager) CalculateDependencies(registrations []resource.Registration) Dependencies { + typeToString := func(t *pbresource.Type) string { + return strings.ToLower(fmt.Sprintf("%s/%s/%s", t.Group, t.GroupVersion, t.Kind)) + } + + out := make(map[string][]string) + for _, r := range registrations { + out[typeToString(r.Type)] = nil + } + + for _, c := range m.controllers { + watches := make([]string, 0, len(c.watches)) + for _, w := range c.watches { + watches = append(watches, typeToString(w.watchedType)) + } + + out[typeToString(c.managedType)] = watches + } + + return out +} + +func (deps Dependencies) ToMermaid() string { + depStrings := make([]string, 0, len(deps)) + + for src, dsts := range deps { + if len(dsts) == 0 { + depStrings = append(depStrings, fmt.Sprintf(" %s", src)) + continue + } + + for _, dst := range dsts { + depStrings = append(depStrings, fmt.Sprintf(" %s --> %s", src, dst)) + } + } + + sort.Slice(depStrings, func(a, b int) bool { + return depStrings[a] < depStrings[b] + }) + out := "flowchart TD\n" + strings.Join(depStrings, "\n") + + return out +} diff --git a/internal/controller/dependencies_test.go b/internal/controller/dependencies_test.go new file mode 100644 index 0000000000..18db58a285 --- /dev/null +++ b/internal/controller/dependencies_test.go @@ -0,0 +1,66 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package controller + +import ( + "testing" + + "github.com/hashicorp/consul/internal/testing/golden" + "github.com/stretchr/testify/require" +) + +func TestDependenciesGolden(t *testing.T) { + deps := Dependencies{ + "t1": []string{"t2", "t3"}, + "t2": []string{"t4"}, + "t4": []string{"t1"}, + } + mermaid := deps.ToMermaid() + expected := golden.Get(t, mermaid, "dependencies.golden") + require.Equal(t, expected, mermaid) +} + +func TestValidateDependencies(t *testing.T) { + type testCase struct { + dependencies Dependencies + expectErr string + } + + run := func(t *testing.T, tc testCase) { + err := tc.dependencies.validate() + if len(tc.expectErr) > 0 { + require.Contains(t, err.Error(), tc.expectErr) + } else { + require.NoError(t, err) + } + + } + + cases := map[string]testCase{ + "empty": { + dependencies: nil, + }, + "no circular dependencies": { + dependencies: Dependencies{ + "t1": []string{"t2", "t3"}, + "t2": []string{"t3"}, + "t3": []string{"t4"}, + "t4": nil, + }, + }, + "with circular dependency": { + dependencies: Dependencies{ + "t1": []string{"t2", "t3"}, + "t2": []string{"t1"}, + }, + expectErr: `circular dependency between "t1" and "t2"`, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + run(t, tc) + }) + } +} diff --git a/internal/controller/testdata/dependencies.golden b/internal/controller/testdata/dependencies.golden new file mode 100644 index 0000000000..f0ff372a16 --- /dev/null +++ b/internal/controller/testdata/dependencies.golden @@ -0,0 +1,5 @@ +flowchart TD + t1 --> t2 + t1 --> t3 + t2 --> t4 + t4 --> t1 \ No newline at end of file