Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow Operator Generated bootstrap token #12520

Merged
merged 71 commits into from
Jun 3, 2022
Merged

Allow Operator Generated bootstrap token #12520

merged 71 commits into from
Jun 3, 2022

Conversation

lhaig
Copy link
Contributor

@lhaig lhaig commented Apr 8, 2022 •
edited
Loading

This PR provides an operator the ability to provide a generated management token which is then used to bootstrap the cluster ACLs.

TODO:

  • Tests
  • - Input Token Validation
  • - Remove hacky code here

@lhaig lhaig requested review from tgross and jrasell April 8, 2022 16:24
@vercel vercel bot temporarily deployed to Preview – nomad April 11, 2022 06:59 Inactive
tgross
tgross requested changes Apr 11, 2022
View reviewed changes
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lhaig! This is a great first pass. Most of the implementation looks pretty good, but I've left some comments about the API.

There's currently a failing test you'll want to fix when you go thru and add tests, and make sure you add documentation to the bootstrap command docs

nomad/acl_endpoint.go Outdated Show resolved Hide resolved
api/acl.go Outdated Show resolved Hide resolved
api/acl.go Outdated Show resolved Hide resolved
api/acl.go Outdated Show resolved Hide resolved
api/acl.go Outdated Show resolved Hide resolved
api/acl.go Outdated Show resolved Hide resolved
command/acl_bootstrap.go Outdated Show resolved Hide resolved
command/agent/acl_endpoint.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – nomad April 11, 2022 19:47 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad April 11, 2022 19:49 Inactive
@apollo13 apollo13 mentioned this pull request Apr 29, 2022
Closed
@tgross tgross linked an issue Apr 29, 2022 that may be closed by this pull request
Add support for initial_management token like in Consul #7777
Closed
@tgross tgross added this to the 1.4.0 milestone Apr 29, 2022
@tgross tgross mentioned this pull request May 12, 2022
Closed
@lhaig lhaig requested a review from a team May 13, 2022 23:58
@lhaig lhaig requested a review from a team as a code owner May 13, 2022 23:58
@lhaig lhaig requested a review from tgross May 16, 2022 17:14
@lhaig
Copy link
Contributor Author

lhaig commented May 16, 2022 •
edited
Loading

Hi @tgross,
I was wondering if I need to do anything to the API endpoint to prepare this for use with the terraform provider?
I am not sure what would be the best way to add this as an option.

Currently the client sets a header in the request

Perhaps I need to add something for when we are using the API?

@tgross tgross removed request for a team May 16, 2022 19:05
lhaig added 2 commits May 28, 2022 10:31
@lhaig
Merge branch 'f-bootstrap-token' of github.com:hashicorp/nomad into f…
5bccf03 
…-bootstrap-token
@lhaig
Update acl-tokens.mdx
16ec53b 
Make it more clear that the root token is based on the JSON above.
@lhaig @schmichael
Update api/acl.go
2d7b315 
Co-authored-by: Michael Schurter <mschurter@hashicorp.com>
schmichael
schmichael reviewed Jun 2, 2022
View reviewed changes
command/agent/acl_endpoint_test.go
Comment on lines +244 to +246
// Since we're not actually writing this HTTP request, we have
// to manually set ContentLength
req.ContentLength = -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow this took me a while to figure out: since we're never actually writing this HTTP request, the stdlib code that sets ContentLength is never set!

Therefore in tests we need to set ContentLength to some nonzero value since tests do not use a real HTTP server.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am glad it is finally solved it kept me up a few nights :-)

schmichael
schmichael approved these changes Jun 2, 2022
View reviewed changes
Copy link
Member

@schmichael schmichael left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Sorry our test harness breaks the ContentLength check! What a hassle.

I'm not sure why the lint tests are failing. They pass for me locally. Maybe someone else on the team will have an idea.

Update: Fixed lint tests! I think I just needed to upgrade hclfmt.

@schmichael schmichael added the backport/1.3.x backport to 1.3.x release line label Jun 2, 2022
tgross
tgross approved these changes Jun 3, 2022
View reviewed changes
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Kudos for sticking this one out @lhaig! Reviews for a big feature can sometimes be a good bit of work but Nomad is going to be that much better for this PR. Thanks!

@tgross tgross merged commit eafc939 into main Jun 3, 2022
@tgross tgross deleted the f-bootstrap-token branch June 3, 2022 11:37
@tgross tgross modified the milestones: 1.4.0, 1.3.2 Jun 3, 2022
@hc-github-team-nomad-core hc-github-team-nomad-core mentioned this pull request Jun 3, 2022
Merged
3 tasks
tgross pushed a commit that referenced this pull request Jun 3, 2022
@lhaig @tgross
Allow Operator Generated bootstrap token (#12520)
65b6afa
tgross pushed a commit that referenced this pull request Jun 3, 2022
@hc-github-team-nomad-core @lhaig
Allow Operator Generated bootstrap token (#12520) (#13217)
25b01aa 
Co-authored-by: Lance Haig <lnhaig@gmail.com>
ChaiWithJai pushed a commit that referenced this pull request Jun 3, 2022
@lhaig @ChaiWithJai
Allow Operator Generated bootstrap token (#12520)
7e6f107
@apollo13 apollo13 mentioned this pull request Sep 1, 2022
Merged
3 tasks
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apollo13 apollo13 apollo13 left review comments

@schmichael schmichael schmichael approved these changes

@tgross tgross tgross approved these changes

@jrasell jrasell Awaiting requested review from jrasell

@picatz picatz Awaiting requested review from picatz

Assignees
No one assigned
Labels
backport/1.3.x backport to 1.3.x release line
Projects
None yet
Milestone
1.3.2
Development

Successfully merging this pull request may close these issues.

Add support for initial_management token like in Consul
5 participants
@lhaig @apollo13 @schmichael @tgross @picatz