Update HCP bootstrapping to support existing clusters by freddygv · Pull Request #16916 · hashicorp/consul

freddygv · 2023-04-07T03:42:54Z

Replaces #16788

Description

We want to allow users to link Consul clusters that already exist to
HCP. Existing clusters need care when bootstrapped by HCP, since we do
not want to do things like change ACL/TLS settings for a running
cluster.

The plan is to only send down a management token to existing clusters
and nothing else (at least initially).

This PR adds a few commits to that end:

Add an HCP management token to server config so that it can be persisted
through Raft as we do for the initial management token.
Update the HCP SDK to be able to receive this new management token.
Update the bootstrapping logic to account for the different types of
clusters that are going to be supported.

Best reviewed by commit

Testing & Reproduction steps

Unit tests

PR Checklist

updated test coverage
external facing docs updated
not a security concern

TODO

Manual testing

jjacobson93

Just one non-blocking comment/question. Looks good

jjacobson93 · 2023-04-10T17:15:50Z

 // IsCloudEnabled returns true if a cloud.resource_id is set and the server mode is enabled
 func (c *RuntimeConfig) IsCloudEnabled() bool {
+	if c == nil {
+		return false


What's the case where RuntimeConfig is nil here?

There are some tests in command/agent that hit that and panic'd: https://app.circleci.com/pipelines/github/hashicorp/consul/56048/workflows/a6e357e8-4c0f-49a7-a22b-6872e2f391f8/jobs/1349145/parallel-runs/3

jmurret

👏 nice work. just had two small things, but one could possibly cause a subtle bug, so asking for changes/discussion.

jmurret · 2023-04-20T18:09:02Z

 }

 func (b *builder) cloudConfigVal(v *CloudConfigRaw) (val hcpconfig.CloudConfig) {
+	val.ResourceID = os.Getenv("HCP_RESOURCE_ID")


I think you will need to use LookupEnv here similar to what the SDK uses or else this will set an empty string if it is null. Previous iterations of the SDK also checked for null rather than empty string and would fail. So, probably best to only set this when the env var is actually configured.

The val variable here is already a zero-value of hcp.CloudConfig because of the named return, so if HCP_RESOURCE_ID is not set then it stays empty. The SDK is checking that it's set first because that function is mutating potentially non-empty config.

I updated this to make the val variable declaration clearer. This isn't really a case to use named returns: style guide.

hanshasselberg

Thanks, this looks great! I like the combined flow and the tests!

I left a couple nits and I have one question: When a cluster with consul 1.14 with CCM integration is upgraded to Consul 1.15, will it rebootstrap because of the missing sucess marker?

hanshasselberg · 2023-04-24T12:55:28Z


-func persistBootstrapConfig(dataDir, cfgJSON string) error {
+func persistBootstrapConfig(dir, cfgJSON string) error {
+	if cfgJSON == "" {


nit: to further remove differences between new and existing cluster flow, an empty config {} could be stored here.

What if we send that empty JSON string down from HCP? It would simplify some of the conditional logic in persistAndProcessConfig

Take a look at the last commit, I updated that bit to expect {} from CCM

I like the idea to always send valid json 👍

jmurret

Nice work! 👍

We want to move away from injecting an initial management token into Consul clusters linked to HCP. The reasoning is that by using a separate class of token we can have more flexibility in terms of allowing HCP's token to co-exist with the user's management token. Down the line we can also more easily adjust the permissions attached to HCP's token to limit it's scope. With these changes, the cloud management token is like the initial management token in that iit has the same global management policy and if it is created it effectively bootstraps the ACL system.

The HCP management token will now be sent in a special field rather than as Consul's "initial management" token configuration. This commit also updates the mock HCP server to more accurately reflect the behavior of the CCM backend.

We want to allow users to link Consul clusters that already exist to HCP. Existing clusters need care when bootstrapped by HCP, since we do not want to do things like change ACL/TLS settings for a running cluster. Additional changes: * Deconstruct MaybeBootstrap so that it can be tested. The HCP Go SDK requires HTTPS to fetch a token from the Auth URL, even if the backend server is mocked. By pulling the hcp.Client creation out we can modify its TLS configuration in tests while keeping the secure behavior in production code. * Add light validation for data received/loaded. * Sanitize initial_management token from received config, since HCP will only ever use the CloudConfig.MangementToken.

freddygv requested review from hanshasselberg and jjacobson93 April 7, 2023 03:42

github-actions Bot added pr/dependencies PR specifically updates dependencies of project theme/cli Flags and documentation for the CLI interface theme/config Relating to Consul Agent configuration, including reloading labels Apr 7, 2023

freddygv mentioned this pull request Apr 7, 2023

Merge Bootstrap Config from CCM #16788

Closed

3 tasks

freddygv added backport/1.14 labels Apr 7, 2023

jjacobson93 approved these changes Apr 10, 2023

View reviewed changes

freddygv force-pushed the cc-4716-link-existing-clusters branch from a8e7574 to 7956dbc Compare April 10, 2023 23:06

freddygv requested review from a team and JadhavPoonam and removed request for a team April 10, 2023 23:38

lkysow mentioned this pull request Apr 11, 2023

Bump hashicorp/hcp-scada-provider from 0.2.0 to 0.2.3 #16972

Closed

jmurret requested changes Apr 20, 2023

View reviewed changes

freddygv requested a review from jmurret April 21, 2023 19:51

hanshasselberg approved these changes Apr 24, 2023

View reviewed changes

jmurret approved these changes Apr 24, 2023

View reviewed changes

freddygv force-pushed the cc-4716-link-existing-clusters branch from 9b3b0ed to 97b080b Compare April 24, 2023 23:04

jmurret force-pushed the cc-4716-link-existing-clusters branch from 22c7a30 to 9a28f9e Compare April 25, 2023 19:09

freddygv added 4 commits April 27, 2023 21:41

Update SDK and mock HCP server

a5a08cc

The HCP management token will now be sent in a special field rather than as Consul's "initial management" token configuration. This commit also updates the mock HCP server to more accurately reflect the behavior of the CCM backend.

Add changelog entry

f798be1

hanshasselberg force-pushed the cc-4716-link-existing-clusters branch from 9a28f9e to f798be1 Compare April 27, 2023 19:41

hanshasselberg merged commit e02ef16 into main Apr 27, 2023

hanshasselberg deleted the cc-4716-link-existing-clusters branch April 27, 2023 20:27

This was referenced Apr 27, 2023

Backport of Update HCP bootstrapping to support existing clusters into release/1.14.x #17174

Closed

Backport of Update HCP bootstrapping to support existing clusters into release/1.15.x #17175

Closed

hanshasselberg mentioned this pull request May 10, 2023

Update HCP bootstrapping to support existing clusters 1.14.x version #17303

Merged

4 tasks

p-linnane mentioned this pull request Jun 1, 2023

consul 1.15.3 Homebrew/homebrew-core#132601

Merged

Conversation

freddygv commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing & Reproduction steps

PR Checklist

TODO

Uh oh!

jjacobson93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmurret left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hanshasselberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmurret left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

freddygv commented Apr 7, 2023 •

edited

Loading