From 214dcf8d0de724aca43b6fb01925994021b3618e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 2 Dec 2021 17:10:25 -0500 Subject: [PATCH] ca: use the real FSM operation in tests Previously we had a couple copies that reproduced the FSM operation. These copies introduce risk that the test does not accurately match production. This PR removes the test versions of the FSM operation, and exports the real production FSM operation so that it can be used in tests. The consul provider tests did need to change because of this. Previously we would return a hardcoded value of 2, but in production this value is always incremented. --- agent/connect/ca/provider_consul_test.go | 18 +++++++-- agent/connect/ca/testing.go | 28 -------------- agent/consul/fsm/commands_oss.go | 26 ++++++++----- agent/consul/leader_connect_ca_test.go | 48 +++--------------------- 4 files changed, 37 insertions(+), 83 deletions(-) diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index 6b4f318377..c9fcc28d1d 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/agent/connect/ca/provider_consul_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/agent/consul/fsm" "github.com/hashicorp/consul/agent/consul/state" "github.com/hashicorp/consul/agent/structs" ) @@ -23,7 +24,16 @@ func (c *consulCAMockDelegate) ProviderState(id string) (*structs.CAConsulProvid } func (c *consulCAMockDelegate) ApplyCARequest(req *structs.CARequest) (interface{}, error) { - return ApplyCARequestToStore(c.state, req) + idx, _, err := c.state.CAConfig(nil) + if err != nil { + return nil, err + } + + result := fsm.ApplyConnectCAOperationFromRequest(c.state, req, idx+1) + if err, ok := result.(error); ok && err != nil { + return nil, err + } + return result, nil } func newMockDelegate(t *testing.T, conf *structs.CAConfiguration) *consulCAMockDelegate { @@ -176,7 +186,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.NoError(err) require.Equal(spiffeService.URI(), parsed.URIs[0]) require.Empty(parsed.Subject.CommonName) - require.Equal(uint64(2), parsed.SerialNumber.Uint64()) + require.Equal(uint64(3), parsed.SerialNumber.Uint64()) subjectKeyID, err := connect.KeyId(csr.PublicKey) require.NoError(err) require.Equal(subjectKeyID, parsed.SubjectKeyId) @@ -205,7 +215,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.NoError(err) require.Equal(spiffeService.URI(), parsed.URIs[0]) require.Empty(parsed.Subject.CommonName) - require.Equal(parsed.SerialNumber.Uint64(), uint64(2)) + require.Equal(uint64(4), parsed.SerialNumber.Uint64()) requireNotEncoded(t, parsed.SubjectKeyId) requireNotEncoded(t, parsed.AuthorityKeyId) @@ -233,7 +243,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.NoError(err) require.Equal(spiffeAgent.URI(), parsed.URIs[0]) require.Empty(parsed.Subject.CommonName) - require.Equal(uint64(2), parsed.SerialNumber.Uint64()) + require.Equal(uint64(5), parsed.SerialNumber.Uint64()) requireNotEncoded(t, parsed.SubjectKeyId) requireNotEncoded(t, parsed.AuthorityKeyId) diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go index 6ba9df791a..a1bbc6a4e2 100644 --- a/agent/connect/ca/testing.go +++ b/agent/connect/ca/testing.go @@ -12,8 +12,6 @@ import ( "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" ) @@ -222,32 +220,6 @@ func (v *TestVaultServer) Stop() error { return nil } -func ApplyCARequestToStore(store *state.Store, req *structs.CARequest) (interface{}, error) { - idx, _, err := store.CAConfig(nil) - if err != nil { - return nil, err - } - - switch req.Op { - case structs.CAOpSetProviderState: - _, err := store.CASetProviderState(idx+1, req.ProviderState) - if err != nil { - return nil, err - } - - return true, nil - case structs.CAOpDeleteProviderState: - if err := 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 requireTrailingNewline(t testing.T, leafPEM string) { t.Helper() if len(leafPEM) == 0 { diff --git a/agent/consul/fsm/commands_oss.go b/agent/consul/fsm/commands_oss.go index 27050113b3..b07d12c14f 100644 --- a/agent/consul/fsm/commands_oss.go +++ b/agent/consul/fsm/commands_oss.go @@ -378,10 +378,19 @@ func (c *FSM) applyConnectCAOperation(buf []byte, index uint64) interface{} { []metrics.Label{{Name: "op", Value: string(req.Op)}}) defer metrics.MeasureSinceWithLabels([]string{"fsm", "ca"}, time.Now(), []metrics.Label{{Name: "op", Value: string(req.Op)}}) + + result := ApplyConnectCAOperationFromRequest(c.state, &req, index) + if err, ok := result.(error); ok && err != nil { + c.logger.Warn("Failed to apply CA operation", "operation", req.Op) + } + return result +} + +func ApplyConnectCAOperationFromRequest(state *state.Store, req *structs.CARequest, index uint64) interface{} { switch req.Op { case structs.CAOpSetConfig: if req.Config.ModifyIndex != 0 { - act, err := c.state.CACheckAndSetConfig(index, req.Config.ModifyIndex, req.Config) + act, err := state.CACheckAndSetConfig(index, req.Config.ModifyIndex, req.Config) if err != nil { return err } @@ -389,29 +398,29 @@ func (c *FSM) applyConnectCAOperation(buf []byte, index uint64) interface{} { return act } - return c.state.CASetConfig(index, req.Config) + return state.CASetConfig(index, req.Config) case structs.CAOpSetRoots: - act, err := c.state.CARootSetCAS(index, req.Index, req.Roots) + act, err := state.CARootSetCAS(index, req.Index, req.Roots) if err != nil { return err } return act case structs.CAOpSetProviderState: - act, err := c.state.CASetProviderState(index, req.ProviderState) + act, err := state.CASetProviderState(index, req.ProviderState) if err != nil { return err } return act case structs.CAOpDeleteProviderState: - if err := c.state.CADeleteProviderState(index, req.ProviderState.ID); err != nil { + if err := state.CADeleteProviderState(index, req.ProviderState.ID); err != nil { return err } return true case structs.CAOpSetRootsAndConfig: - act, err := c.state.CARootSetCAS(index, req.Index, req.Roots) + act, err := state.CARootSetCAS(index, req.Index, req.Roots) if err != nil { return err } @@ -419,20 +428,19 @@ func (c *FSM) applyConnectCAOperation(buf []byte, index uint64) interface{} { return act } - act, err = c.state.CACheckAndSetConfig(index, req.Config.ModifyIndex, req.Config) + act, err = state.CACheckAndSetConfig(index, req.Config.ModifyIndex, req.Config) if err != nil { return err } return act case structs.CAOpIncrementProviderSerialNumber: - sn, err := c.state.CAIncrementProviderSerialNumber(index) + sn, err := state.CAIncrementProviderSerialNumber(index) if err != nil { return err } return sn default: - c.logger.Warn("Invalid CA operation", "operation", req.Op) return fmt.Errorf("Invalid CA operation '%s'", req.Op) } } diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 846144460d..82dabe87cf 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -14,18 +14,18 @@ import ( "testing" "time" - "github.com/hashicorp/consul/testrpc" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent/connect" ca "github.com/hashicorp/consul/agent/connect/ca" + "github.com/hashicorp/consul/agent/consul/fsm" "github.com/hashicorp/consul/agent/consul/state" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/token" "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" + "github.com/hashicorp/consul/testrpc" ) // TODO(kyhavlov): replace with t.Deadline() @@ -83,47 +83,11 @@ func (m *mockCAServerDelegate) ApplyCARequest(req *structs.CARequest) (interface 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 - } - - 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) + result := fsm.ApplyConnectCAOperationFromRequest(m.store, req, idx+1) + if err, ok := result.(error); ok && err != nil { + return nil, err } + return result, nil } func (m *mockCAServerDelegate) forwardDC(method, dc string, args interface{}, reply interface{}) error {