Merge pull request #10445 from hashicorp/dnephin/ca-provider-explore

ca: isolate more of the CA logic in CAManager
hashicorp · Jul 12, 2021 · b89ec82 · b89ec82
2 parents 570eac3 + bf292cb
commit b89ec82
Show file tree

Hide file tree

Showing 16 changed files with 168 additions and 205 deletions.
diff --git a/agent/connect/ca/provider.go b/agent/connect/ca/provider.go
@@ -3,8 +3,6 @@ package ca
 import (
 	"crypto/x509"
 	"errors"
-
-	"github.com/hashicorp/go-hclog"
 )
 
 //go:generate mockery -name Provider -inpkg
@@ -171,13 +169,6 @@ type Provider interface {
 	Cleanup(providerTypeChange bool, otherConfig map[string]interface{}) error
 }
 
-// NeedsLogger is an optional interface that allows a CA provider to use the
-// Consul logger to output diagnostic messages.
-type NeedsLogger interface {
-	// SetLogger will pass a configured Logger to the provider.
-	SetLogger(logger hclog.Logger)
-}
-
 // NeedsStop is an optional interface that allows a CA to define a function
 // to be called when the CA instance is no longer in use. This is different
 // from Cleanup(), as only the local provider instance is being shut down

diff --git a/agent/connect/ca/provider_aws.go b/agent/connect/ca/provider_aws.go
@@ -18,7 +18,6 @@ import (
 
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/logging"
 )
 
 const (
@@ -78,12 +77,9 @@ type AWSProvider struct {
 	logger          hclog.Logger
 }
 
-// SetLogger implements NeedsLogger
-func (a *AWSProvider) SetLogger(logger hclog.Logger) {
-	a.logger = logger.
-		ResetNamed(logging.Connect).
-		Named(logging.CA).
-		Named(logging.AWS)
+// NewAWSProvider returns a new AWSProvider
+func NewAWSProvider(logger hclog.Logger) *AWSProvider {
+	return &AWSProvider{logger: logger}
 }
 
 // Configure implements Provider

diff --git a/agent/connect/ca/provider_aws_test.go b/agent/connect/ca/provider_aws_test.go
@@ -7,9 +7,10 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/acmpca"
+	"github.com/stretchr/testify/require"
+
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/sdk/testutil"
-	"github.com/stretchr/testify/require"
 )
 
 // skipIfAWSNotConfigured skips the test unless ENABLE_AWS_PCA_TESTS=true.
@@ -375,9 +376,7 @@ func TestAWSProvider_Cleanup(t *testing.T) {
 }
 
 func testAWSProvider(t *testing.T, cfg ProviderConfig) *AWSProvider {
-	p := &AWSProvider{}
-	logger := testutil.Logger(t)
-	p.SetLogger(logger)
+	p := NewAWSProvider(testutil.Logger(t))
 	require.NoError(t, p.Configure(cfg))
 	return p
 }

diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go
@@ -19,7 +19,6 @@ import (
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/logging"
 )
 
 var (
@@ -51,6 +50,11 @@ type ConsulProvider struct {
 	sync.RWMutex
 }
 
+// NewConsulProvider returns a new ConsulProvider that is ready to be used.
+func NewConsulProvider(delegate ConsulProviderStateDelegate, logger hclog.Logger) *ConsulProvider {
+	return &ConsulProvider{Delegate: delegate, logger: logger}
+}
+
 type ConsulProviderStateDelegate interface {
 	State() *state.Store
 	ApplyCARequest(*structs.CARequest) (interface{}, error)
@@ -114,23 +118,15 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error {
 		return nil
 	}
 
-	// Write the provider state to the state store.
-	newState := structs.CAConsulProviderState{
-		ID: c.id,
-	}
-
 	args := &structs.CARequest{
 		Op:            structs.CAOpSetProviderState,
-		ProviderState: &newState,
+		ProviderState: &structs.CAConsulProviderState{ID: c.id},
 	}
 	if _, err := c.Delegate.ApplyCARequest(args); err != nil {
 		return err
 	}
 
-	c.logger.Debug("consul CA provider configured",
-		"id", c.id,
-		"is_primary", c.isPrimary,
-	)
+	c.logger.Debug("consul CA provider configured", "id", c.id, "is_primary", c.isPrimary)
 
 	return nil
 }
@@ -667,14 +663,6 @@ func (c *ConsulProvider) generateCA(privateKey string, sn uint64) (string, error
 	return buf.String(), nil
 }
 
-// SetLogger implements the NeedsLogger interface so the provider can log important messages.
-func (c *ConsulProvider) SetLogger(logger hclog.Logger) {
-	c.logger = logger.
-		ResetNamed(logging.Connect).
-		Named(logging.CA).
-		Named(logging.Consul)
-}
-
 func (c *ConsulProvider) parseTestState(rawConfig map[string]interface{}, state map[string]string) {
 	c.testState = nil
 	if rawTestState, ok := rawConfig["test_state"]; ok {

diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go
@@ -17,7 +17,6 @@ import (
 
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/logging"
 )
 
 const VaultCALeafCertRole = "leaf-cert"
@@ -38,8 +37,11 @@ type VaultProvider struct {
 	logger                       hclog.Logger
 }
 
-func NewVaultProvider() *VaultProvider {
-	return &VaultProvider{shutdown: func() {}}
+func NewVaultProvider(logger hclog.Logger) *VaultProvider {
+	return &VaultProvider{
+		shutdown: func() {},
+		logger:   logger,
+	}
 }
 
 func vaultTLSConfig(config *structs.VaultCAProviderConfig) *vaultapi.TLSConfig {
@@ -143,14 +145,6 @@ func (v *VaultProvider) renewToken(ctx context.Context, watcher *vaultapi.Lifeti
 	}
 }
 
-// SetLogger implements the NeedsLogger interface so the provider can log important messages.
-func (v *VaultProvider) SetLogger(logger hclog.Logger) {
-	v.logger = logger.
-		ResetNamed(logging.Connect).
-		Named(logging.CA).
-		Named(logging.Vault)
-}
-
 // State implements Provider. Vault provider needs no state other than the
 // user-provided config currently.
 func (v *VaultProvider) State() (map[string]string, error) {

diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go
@@ -8,12 +8,13 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/consul/agent/connect"
-	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/go-hclog"
 	vaultapi "github.com/hashicorp/vault/api"
 	"github.com/stretchr/testify/require"
+
+	"github.com/hashicorp/consul/agent/connect"
+	"github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/consul/sdk/testutil/retry"
 )
 
 func TestVaultCAProvider_VaultTLSConfig(t *testing.T) {
@@ -485,7 +486,7 @@ func createVaultProvider(t *testing.T, isPrimary bool, addr, token string, rawCo
 		conf[k] = v
 	}
 
-	provider := NewVaultProvider()
+	provider := NewVaultProvider(hclog.New(nil))
 
 	cfg := ProviderConfig{
 		ClusterID:  connect.TestClusterID,
@@ -494,11 +495,6 @@ func createVaultProvider(t *testing.T, isPrimary bool, addr, token string, rawCo
 		RawConfig:  conf,
 	}
 
-	logger := hclog.New(&hclog.LoggerOptions{
-		Output: ioutil.Discard,
-	})
-	provider.SetLogger(logger)
-
 	if !isPrimary {
 		cfg.IsPrimary = false
 		cfg.Datacenter = "dc2"

diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go
@@ -7,14 +7,15 @@ import (
 	"os/exec"
 	"sync"
 
+	"github.com/hashicorp/go-hclog"
+	vaultapi "github.com/hashicorp/vault/api"
+	"github.com/mitchellh/go-testing-interface"
+
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/sdk/freeport"
 	"github.com/hashicorp/consul/sdk/testutil/retry"
-	"github.com/hashicorp/go-hclog"
-	vaultapi "github.com/hashicorp/vault/api"
-	"github.com/mitchellh/go-testing-interface"
 )
 
 // KeyTestCases is a list of the important CA key types that we should test
@@ -75,13 +76,9 @@ func CASigningKeyTypeCases() []CASigningKeyTypes {
 
 // TestConsulProvider creates a new ConsulProvider, taking care to stub out it's
 // Logger so that logging calls don't panic. If logging output is important
-// SetLogger can be called again with another logger to capture logs.
 func TestConsulProvider(t testing.T, d ConsulProviderStateDelegate) *ConsulProvider {
-	provider := &ConsulProvider{Delegate: d}
-	logger := hclog.New(&hclog.LoggerOptions{
-		Output: ioutil.Discard,
-	})
-	provider.SetLogger(logger)
+	logger := hclog.New(&hclog.LoggerOptions{Output: ioutil.Discard})
+	provider := &ConsulProvider{Delegate: d, logger: logger}
 	return provider
 }
 

diff --git a/agent/consul/consul_ca_delegate.go b/agent/consul/consul_ca_delegate.go
diff --git a/agent/consul/leader.go b/agent/consul/leader.go
@@ -387,9 +387,6 @@ func (s *Server) revokeLeadership() {
 
 	s.stopConnectLeader()
 
-	s.caManager.setCAProvider(nil, nil)
-	s.caManager.setState(caStateUninitialized, false)
-
 	s.stopACLTokenReaping()
 
 	s.stopACLUpgrade()

diff --git a/agent/consul/leader_connect.go b/agent/consul/leader_connect.go
@@ -2,12 +2,10 @@ package consul
 
 import (
 	"context"
-	"fmt"
 	"time"
 
 	"golang.org/x/time/rate"
 
-	"github.com/hashicorp/consul/agent/connect/ca"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/logging"
 )
@@ -49,36 +47,6 @@ func (s *Server) stopConnectLeader() {
 	s.leaderRoutineManager.Stop(caRootPruningRoutineName)
 	s.leaderRoutineManager.Stop(caRootMetricRoutineName)
 	s.leaderRoutineManager.Stop(caSigningMetricRoutineName)
-
-	// If the provider implements NeedsStop, we call Stop to perform any shutdown actions.
-	provider, _ := s.caManager.getCAProvider()
-	if provider != nil {
-		if needsStop, ok := provider.(ca.NeedsStop); ok {
-			needsStop.Stop()
-		}
-	}
-}
-
-// createProvider returns a connect CA provider from the given config.
-func (s *Server) createCAProvider(conf *structs.CAConfiguration) (ca.Provider, error) {
-	var p ca.Provider
-	switch conf.Provider {
-	case structs.ConsulCAProvider:
-		p = &ca.ConsulProvider{Delegate: &consulCADelegate{s}}
-	case structs.VaultCAProvider:
-		p = ca.NewVaultProvider()
-	case structs.AWSCAProvider:
-		p = &ca.AWSProvider{}
-	default:
-		return nil, fmt.Errorf("unknown CA provider %q", conf.Provider)
-	}
-
-	// If the provider implements NeedsLogger, we give it our logger.
-	if needsLogger, ok := p.(ca.NeedsLogger); ok {
-		needsLogger.SetLogger(s.logger)
-	}
-
-	return p, nil
 }
 
 func (s *Server) runCARootPruning(ctx context.Context) error {
@@ -213,11 +181,3 @@ func lessThanHalfTimePassed(now, notBefore, notAfter time.Time) bool {
 	t := notBefore.Add(halfTime(notBefore, notAfter))
 	return t.Sub(now) > 0
 }
-
-func (s *Server) generateCASignRequest(csr string) *structs.CASignRequest {
-	return &structs.CASignRequest{
-		Datacenter:   s.config.PrimaryDatacenter,
-		CSR:          csr,
-		WriteRequest: structs.WriteRequest{Token: s.tokens.ReplicationToken()},
-	}
-}