From a8c6609f3cde3f31fda5e894dd7a30b30e1336f2 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Fri, 14 Jul 2023 12:25:25 -0700 Subject: [PATCH 1/6] [CC-5718] Remove HCP token requirement during bootstrap --- agent/hcp/bootstrap/bootstrap.go | 19 ++++++++------ agent/hcp/bootstrap/bootstrap_test.go | 37 +++++++++++++++------------ 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/agent/hcp/bootstrap/bootstrap.go b/agent/hcp/bootstrap/bootstrap.go index 38fa7d22746b..47410192b429 100644 --- a/agent/hcp/bootstrap/bootstrap.go +++ b/agent/hcp/bootstrap/bootstrap.go @@ -298,21 +298,23 @@ func persistAndProcessConfig(dataDir string, devMode bool, bsCfg *hcpclient.Boot return "", fmt.Errorf("failed to persist bootstrap config: %w", err) } - if err := validateManagementToken(bsCfg.ManagementToken); err != nil { - return "", fmt.Errorf("invalid management token: %w", err) - } - if err := persistManagementToken(dir, bsCfg.ManagementToken); err != nil { - return "", fmt.Errorf("failed to persist HCP management token: %w", err) + if bsCfg.ManagementToken != "" { + if err := validateManagementToken(bsCfg.ManagementToken); err != nil { + return "", fmt.Errorf("invalid management token: %w", err) + } + if err := persistManagementToken(dir, bsCfg.ManagementToken); err != nil { + return "", fmt.Errorf("failed to persist HCP management token: %w", err) + } } - if err := persistSucessMarker(dir); err != nil { + if err := persistSuccessMarker(dir); err != nil { return "", fmt.Errorf("failed to persist success marker: %w", err) } } return cfgJSON, nil } -func persistSucessMarker(dir string) error { +func persistSuccessMarker(dir string) error { name := filepath.Join(dir, successFileName) return os.WriteFile(name, []byte(""), 0600) @@ -454,7 +456,8 @@ func loadManagementToken(dir string) (string, error) { name := filepath.Join(dir, tokenFileName) bytes, err := os.ReadFile(name) if os.IsNotExist(err) { - return "", errors.New("configuration files on disk are incomplete, missing: " + name) + // A missing management token is not an error, if none was provided by HCP + return "", nil } if err != nil { return "", fmt.Errorf("failed to read: %w", err) diff --git a/agent/hcp/bootstrap/bootstrap_test.go b/agent/hcp/bootstrap/bootstrap_test.go index 7cb46a32bf2f..fa3202c4473a 100644 --- a/agent/hcp/bootstrap/bootstrap_test.go +++ b/agent/hcp/bootstrap/bootstrap_test.go @@ -305,9 +305,10 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) { warning string } type testCase struct { - existingCluster bool - mutateFn func(t *testing.T, dir string) - expect expect + existingCluster bool + disableManagementToken bool + mutateFn func(t *testing.T, dir string) + expect expect } run := func(t *testing.T, tc testCase) { @@ -319,7 +320,7 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) { // Do some common setup as if we received config from HCP and persisted it to disk. require.NoError(t, lib.EnsurePath(dir, true)) - require.NoError(t, persistSucessMarker(dir)) + require.NoError(t, persistSuccessMarker(dir)) if !tc.existingCluster { caCert, caKey, err := tlsutil.GenerateCA(tlsutil.CAOpts{}) @@ -333,9 +334,12 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) { require.NoError(t, persistBootstrapConfig(dir, cfgJSON)) } - token, err := uuid.GenerateUUID() - require.NoError(t, err) - require.NoError(t, persistManagementToken(dir, token)) + var token string + if !tc.disableManagementToken { + token, err = uuid.GenerateUUID() + require.NoError(t, err) + require.NoError(t, persistManagementToken(dir, token)) + } // Optionally mutate the persisted data to trigger errors while loading. if tc.mutateFn != nil { @@ -348,7 +352,6 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) { if loaded { require.Equal(t, token, cfg.ManagementToken) require.Empty(t, ui.ErrorWriter.String()) - } else { require.Nil(t, cfg) require.Contains(t, ui.ErrorWriter.String(), tc.expect.warning) @@ -365,15 +368,11 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) { warning: "", }, }, - "existing cluster missing token": { - existingCluster: true, - mutateFn: func(t *testing.T, dir string) { - // Remove the token file while leaving the existing cluster marker. - require.NoError(t, os.Remove(filepath.Join(dir, tokenFileName))) - }, + "existing cluster no token": { + existingCluster: true, + disableManagementToken: true, expect: expect{ - loaded: false, - warning: "configuration files on disk are incomplete", + loaded: true, }, }, "existing cluster no files": { @@ -396,6 +395,12 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) { warning: "", }, }, + "new cluster with no token": { + disableManagementToken: true, + expect: expect{ + loaded: true, + }, + }, "new cluster some files": { mutateFn: func(t *testing.T, dir string) { // Remove one of the required files From f1225f5d474406073ae9f46ddd3c51e93aee3d8a Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 17 Jul 2023 10:23:43 -0700 Subject: [PATCH 2/6] Re-add error for loading HCP management token --- agent/hcp/bootstrap/bootstrap.go | 2 +- agent/hcp/bootstrap/bootstrap_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/agent/hcp/bootstrap/bootstrap.go b/agent/hcp/bootstrap/bootstrap.go index 47410192b429..1c7ae190a36b 100644 --- a/agent/hcp/bootstrap/bootstrap.go +++ b/agent/hcp/bootstrap/bootstrap.go @@ -457,7 +457,7 @@ func loadManagementToken(dir string) (string, error) { bytes, err := os.ReadFile(name) if os.IsNotExist(err) { // A missing management token is not an error, if none was provided by HCP - return "", nil + return "", errors.New("configuration files on disk are incomplete, missing: " + name) } if err != nil { return "", fmt.Errorf("failed to read: %w", err) diff --git a/agent/hcp/bootstrap/bootstrap_test.go b/agent/hcp/bootstrap/bootstrap_test.go index fa3202c4473a..74b57e5f50ab 100644 --- a/agent/hcp/bootstrap/bootstrap_test.go +++ b/agent/hcp/bootstrap/bootstrap_test.go @@ -372,7 +372,7 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) { existingCluster: true, disableManagementToken: true, expect: expect{ - loaded: true, + loaded: false, }, }, "existing cluster no files": { @@ -398,7 +398,7 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) { "new cluster with no token": { disableManagementToken: true, expect: expect{ - loaded: true, + loaded: false, }, }, "new cluster some files": { From fc680e806ee6428c3a363d066f6b093fbe2fdd20 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Mon, 17 Jul 2023 11:38:31 -0700 Subject: [PATCH 3/6] Remove old comment --- agent/hcp/bootstrap/bootstrap.go | 1 - 1 file changed, 1 deletion(-) diff --git a/agent/hcp/bootstrap/bootstrap.go b/agent/hcp/bootstrap/bootstrap.go index 1c7ae190a36b..29080d4b4643 100644 --- a/agent/hcp/bootstrap/bootstrap.go +++ b/agent/hcp/bootstrap/bootstrap.go @@ -456,7 +456,6 @@ func loadManagementToken(dir string) (string, error) { name := filepath.Join(dir, tokenFileName) bytes, err := os.ReadFile(name) if os.IsNotExist(err) { - // A missing management token is not an error, if none was provided by HCP return "", errors.New("configuration files on disk are incomplete, missing: " + name) } if err != nil { From 5958ae0921522707652794668233741298863ade Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Wed, 19 Jul 2023 09:49:25 -0700 Subject: [PATCH 4/6] Add changelog entry --- .changelog/18140.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/18140.txt diff --git a/.changelog/18140.txt b/.changelog/18140.txt new file mode 100644 index 000000000000..e02ab33bceb2 --- /dev/null +++ b/.changelog/18140.txt @@ -0,0 +1,3 @@ +```release-note:improvement +cloud: Removes requirement for HCP to provide a management token +``` From 536e6f3b3b68754cb9958f83e225e6f7d1565c08 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Wed, 19 Jul 2023 09:52:05 -0700 Subject: [PATCH 5/6] Remove extra validation line --- agent/hcp/bootstrap/bootstrap.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/agent/hcp/bootstrap/bootstrap.go b/agent/hcp/bootstrap/bootstrap.go index 29080d4b4643..320b36107313 100644 --- a/agent/hcp/bootstrap/bootstrap.go +++ b/agent/hcp/bootstrap/bootstrap.go @@ -354,12 +354,9 @@ func persistTLSCerts(dir string, serverCert, serverKey string, caCerts []string) return nil } -// Basic validation to ensure a UUID was loaded. +// Basic validation to ensure a UUID was loaded and assumes the token is non-empty func validateManagementToken(token string) error { - if token == "" { - return errors.New("missing HCP management token") - } - + // note: we assume that the token is not an empty string if _, err := uuid.ParseUUID(token); err != nil { return errors.New("management token is not a valid UUID") } From a99dd82929013249c8f57f1867da8a8ed4fb93d9 Mon Sep 17 00:00:00 2001 From: Jeremy Jacobson Date: Thu, 20 Jul 2023 09:42:28 -0700 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: lornasong --- .changelog/18140.txt | 2 +- agent/hcp/bootstrap/bootstrap.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.changelog/18140.txt b/.changelog/18140.txt index e02ab33bceb2..fabd9fc2916b 100644 --- a/.changelog/18140.txt +++ b/.changelog/18140.txt @@ -1,3 +1,3 @@ ```release-note:improvement -cloud: Removes requirement for HCP to provide a management token +hcp: Removes requirement for HCP to provide a management token ``` diff --git a/agent/hcp/bootstrap/bootstrap.go b/agent/hcp/bootstrap/bootstrap.go index 320b36107313..191859ea002b 100644 --- a/agent/hcp/bootstrap/bootstrap.go +++ b/agent/hcp/bootstrap/bootstrap.go @@ -298,6 +298,8 @@ func persistAndProcessConfig(dataDir string, devMode bool, bsCfg *hcpclient.Boot return "", fmt.Errorf("failed to persist bootstrap config: %w", err) } + // HCP only returns the management token if it requires Consul to + // initialize it if bsCfg.ManagementToken != "" { if err := validateManagementToken(bsCfg.ManagementToken); err != nil { return "", fmt.Errorf("invalid management token: %w", err)