From 6228c4a53c384380f3d41aa03f03117f9e29053e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 22 Jun 2021 17:25:38 -0400 Subject: [PATCH] ca: fix mockCAServerDelegate to work with the new interface raftApply was removed so ApplyCARequest needs to handle all the possible operations Also set the providerShim to use the mock provider. other changes are small test improvements that were necessary to debug the failures. --- agent/connect/ca/provider_consul.go | 12 +--- agent/consul/leader_connect_ca.go | 8 +-- agent/consul/leader_connect_ca_test.go | 92 ++++++++++++++++++-------- 3 files changed, 71 insertions(+), 41 deletions(-) diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index d2453a609b..928c6adf9a 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -113,23 +113,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 } diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 74d89be769..950ea35871 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -23,10 +23,10 @@ type caState string const ( caStateUninitialized caState = "UNINITIALIZED" - caStateInitializing = "INITIALIZING" - caStateInitialized = "INITIALIZED" - caStateRenewIntermediate = "RENEWING" - caStateReconfig = "RECONFIGURING" + caStateInitializing caState = "INITIALIZING" + caStateInitialized caState = "INITIALIZED" + caStateRenewIntermediate caState = "RENEWING" + caStateReconfig caState = "RECONFIGURING" ) // caServerDelegate is an interface for server operations for facilitating diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 643cc1983d..502dda28d8 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/go-version" "github.com/hashicorp/serf/serf" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent/connect" @@ -61,15 +62,57 @@ func (m *mockCAServerDelegate) CheckServers(datacenter string, fn func(*metadata }) } +// ApplyCARequest mirrors FSM.applyConnectCAOperation because that functionality +// is not exported. func (m *mockCAServerDelegate) ApplyCARequest(req *structs.CARequest) (interface{}, error) { - return ca.ApplyCARequestToStore(m.store, req) -} + idx, _, err := m.store.CAConfig(nil) + if err != nil { + return nil, err + } + + m.callbackCh <- fmt.Sprintf("raftApply/ConnectCA") + + switch req.Op { + case structs.CAOpSetConfig: + if req.Config.ModifyIndex != 0 { + act, err := m.store.CACheckAndSetConfig(idx+1, req.Config.ModifyIndex, req.Config) + if err != nil { + return nil, err + } + + return act, nil + } + + return nil, m.store.CASetConfig(idx+1, req.Config) + case structs.CAOpSetRootsAndConfig: + act, err := m.store.CARootSetCAS(idx, req.Index, req.Roots) + if err != nil || !act { + return act, err + } + + act, err = m.store.CACheckAndSetConfig(idx+1, req.Config.ModifyIndex, req.Config) + if err != nil { + return nil, err + } + return act, nil + case structs.CAOpSetProviderState: + _, err := m.store.CASetProviderState(idx+1, req.ProviderState) + if err != nil { + return nil, err + } -func (m *mockCAServerDelegate) createCAProvider(conf *structs.CAConfiguration) (ca.Provider, error) { - return &mockCAProvider{ - callbackCh: m.callbackCh, - rootPEM: m.primaryRoot.RootCert, - }, nil + return true, nil + case structs.CAOpDeleteProviderState: + if err := m.store.CADeleteProviderState(idx+1, req.ProviderState.ID); err != nil { + return nil, err + } + + return true, nil + case structs.CAOpIncrementProviderSerialNumber: + return uint64(2), nil + default: + return nil, fmt.Errorf("Invalid CA operation '%s'", req.Op) + } } func (m *mockCAServerDelegate) forwardDC(method, dc string, args interface{}, reply interface{}) error { @@ -98,23 +141,6 @@ func (m *mockCAServerDelegate) generateCASignRequest(csr string) *structs.CASign } } -func (m *mockCAServerDelegate) raftApply(t structs.MessageType, msg interface{}) (interface{}, error) { - if t == structs.ConnectCARequestType { - req := msg.(*structs.CARequest) - act, err := m.store.CARootSetCAS(1, req.Index, req.Roots) - require.NoError(m.t, err) - require.True(m.t, act) - - act, err = m.store.CACheckAndSetConfig(1, req.Config.ModifyIndex, req.Config) - require.NoError(m.t, err) - require.True(m.t, act) - } else { - return nil, fmt.Errorf("got invalid MessageType %v", t) - } - m.callbackCh <- fmt.Sprintf("raftApply/%s", t) - return nil, nil -} - // mockCAProvider mocks an empty provider implementation with a channel in order to coordinate // waiting for certain methods to be called. type mockCAProvider struct { @@ -147,6 +173,7 @@ func (m *mockCAProvider) SupportsCrossSigning() (bool, error) func (m *mockCAProvider) Cleanup(_ bool, _ map[string]interface{}) error { return nil } func waitForCh(t *testing.T, ch chan string, expected string) { + t.Helper() select { case op := <-ch: if op != expected { @@ -179,6 +206,7 @@ func testCAConfig() *structs.CAConfiguration { // initTestManager initializes a CAManager with a mockCAServerDelegate, consuming // the ops that come through the channels and returning when initialization has finished. func initTestManager(t *testing.T, manager *CAManager, delegate *mockCAServerDelegate) { + t.Helper() initCh := make(chan struct{}) go func() { require.NoError(t, manager.InitializeCA()) @@ -209,13 +237,19 @@ func TestCAManager_Initialize(t *testing.T) { conf.Datacenter = "dc2" delegate := NewMockCAServerDelegate(t, conf) manager := NewCAManager(delegate, nil, testutil.Logger(t), conf) + manager.providerShim = &mockCAProvider{ + callbackCh: delegate.callbackCh, + rootPEM: delegate.primaryRoot.RootCert, + } // Call InitializeCA and then confirm the RPCs and provider calls // happen in the expected order. - require.EqualValues(t, caStateUninitialized, manager.state) + require.Equal(t, caStateUninitialized, manager.state) errCh := make(chan error) go func() { - errCh <- manager.InitializeCA() + err := manager.InitializeCA() + assert.NoError(t, err) + errCh <- err }() waitForCh(t, delegate.callbackCh, "forwardDC/ConnectCA.Roots") @@ -234,7 +268,7 @@ func TestCAManager_Initialize(t *testing.T) { t.Fatal("never got result from errCh") } - require.EqualValues(t, caStateInitialized, manager.state) + require.Equal(t, caStateInitialized, manager.state) } func TestCAManager_UpdateConfigWhileRenewIntermediate(t *testing.T) { @@ -259,6 +293,10 @@ func TestCAManager_UpdateConfigWhileRenewIntermediate(t *testing.T) { conf.Datacenter = "dc2" delegate := NewMockCAServerDelegate(t, conf) manager := NewCAManager(delegate, nil, testutil.Logger(t), conf) + manager.providerShim = &mockCAProvider{ + callbackCh: delegate.callbackCh, + rootPEM: delegate.primaryRoot.RootCert, + } initTestManager(t, manager, delegate) // Wait half the TTL for the cert to need renewing.