Add ACL system config on `run_consul` on consul >= 1.4.0

[devvesa]

I miss some options to configure and bootstrap ACL system since Consul v1.4.0 and later (https://www.consul.io/docs/agent/acl-system.html) when running run_consul.

[devvesa]

If you accept the request and probably with some guidance I can help.

[brikis98]

Yes, we've definitely wanted to add better support for ACLs, but have not had time to do it yet. PR is very welcome!

[devvesa]

Gotcha! Time not guaranteed either, but I'll try to find some :)

[brikis98]

Thx!

[yardbirdsax]

I see this issue hasn't gotten any movement and would like to pick it up since we have a requirement for this. Are there any general thoughts / guidelines by which this should be handled? The tricky parts to me are 1) making sure the bootstrap ACL command is only run once (i.e. it can't be run on all the ASG provisioned instances, only one), and 2) making sure it's smart enough to see that it's been done already and retrieve the value for subsequent launches.

I was thinking to use AWS SSM Parameter Store to securely store the ACL token, which instances could retrieve upon launch. I'd welcome any thoughts on other approaches and how to solve the challenges noted above.

[brikis98]

I see this issue hasn't gotten any movement and would like to pick it up since we have a requirement for this.

Great, thank you!

1. making sure the bootstrap ACL command is only run once (i.e. it can't be run on all the ASG provisioned instances, only one),

There are multiple ways to solve this. With an ASG, I think you can pick a unique "rally point" where all the nodes run the same code to figure out which one of them should do this. We did this in a Bash script for Couchbase: https://github.com/gruntwork-io/terraform-aws-couchbase/blob/master/modules/couchbase-commons/couchbase-rally-point.

2. making sure it's smart enough to see that it's been done already and retrieve the value for subsequent launches.

I was thinking to use AWS SSM Parameter Store to securely store the ACL token, which instances could retrieve upon launch. I'd welcome any thoughts on other approaches and how to solve the challenges noted above.

My guess is S3 would be the most universal store for this in AWS? With encryption, of course. SSM Param Store and AWS Secrets Manager are options too, but not everyone uses those, whereas almost everyone uses S3.

[yardbirdsax]

There are multiple ways to solve this. With an ASG, I think you can pick a unique "rally point" where all the nodes run the same code to figure out which one of them should do this. We did this in a Bash script for Couchbase: https://github.com/gruntwork-io/terraform-aws-couchbase/blob/master/modules/couchbase-commons/couchbase-rally-point.

Oooh, that's some cool stuff there. I was taking a similar approach but will happily try to re-use this. I see that you've got a common scripts repo (https://github.com/gruntwork-io/bash-commons); perhaps we should pluck this functionality and put it there, then reference it? It seems like something that would be useful in other places.

My guess is S3 would be the most universal store for this in AWS? With encryption, of course. SSM Param Store and AWS Secrets Manager are options too, but not everyone uses those, whereas almost everyone uses S3.

It's true that S3 is pretty universal. My thought around using SSM was that it doesn't require any setup outside the module itself (i.e. you don't have to already have an S3 bucket, or create one as part of the module). Still it's a valid point. Would you be alright if I did it using SSM for now and then made it able to use S3 if requested as well?

[brikis98]

There are multiple ways to solve this. With an ASG, I think you can pick a unique "rally point" where all the nodes run the same code to figure out which one of them should do this. We did this in a Bash script for Couchbase: https://github.com/gruntwork-io/terraform-aws-couchbase/blob/master/modules/couchbase-commons/couchbase-rally-point.

Oooh, that's some cool stuff there. I was taking a similar approach but will happily try to re-use this. I see that you've got a common scripts repo (https://github.com/gruntwork-io/bash-commons); perhaps we should pluck this functionality and put it there, then reference it? It seems like something that would be useful in other places.

Moving it to bash-commons sounds great!

My guess is S3 would be the most universal store for this in AWS? With encryption, of course. SSM Param Store and AWS Secrets Manager are options too, but not everyone uses those, whereas almost everyone uses S3.

It's true that S3 is pretty universal. My thought around using SSM was that it doesn't require any setup outside the module itself (i.e. you don't have to already have an S3 bucket, or create one as part of the module). Still it's a valid point. Would you be alright if I did it using SSM for now and then made it able to use S3 if requested as well?

Yup, that's fine, as long as the API is designed where we could add support for other storage locations in the future.

[yardbirdsax]

Sounds good. I’ll work on putting the bash code in the common repo first. After that I think I’ll need to update the tests here since this is a pretty significant change. Should be a good exercise for my Go muscles. 🤣 Once I have those in a good place I’ll open a draft PR so you all can make sure I’m doing it correctly.

[yardbirdsax]

I've begun looking at the tests and before digging in too much wanted to ask for some feedback. I'm still a bit of a padawan level Golang coder so thank you in advance for what may be some newb-ish questions. I'd rather ask up front than submit a dumpster of a PR that needs to be reworked. 😄

I think that we'll have to make it so that methods like this one accept a token, or create new methods that mirror this but accept a token. If it was the former then if the token was passed as an empty string I'd not initialize the Token parameter of the Config object. Something like:

if token != "" {
  config.Token = token
}

This is so that when we're testing a cluster that has had the ACL bootstrapped we can still do so in the same way. I'd probably also do things like make sure the parameter value was saved, etc.

In a previous life in something like Python or others I'd use an optional parameter with a default value so the signature of the method call didn't change (necessitating upstream updates). But since Golang doesn't have that, and this method doesn't accept a struct as its input, I can't make it accept this value without changing the outside interface. Ideally I don't want to duplicate all the logic in there just for this, though perhaps that is the best way (or refactor it further).

I welcome your input so I can make sure the implementation here is clean.

[brikis98]

Your understanding seems correct to me. Feel free to update the signature of that method; it's only used in this repo, and it's just test code, and not some public API we maintain, so backwards incompatible changes are just fine there.