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

Support Nomad Variables #1632

Merged
merged 1 commit into from
Oct 26, 2022
Merged

Support Nomad Variables #1632

merged 1 commit into from
Oct 26, 2022

Conversation

angrycub
Copy link
Contributor

@angrycub angrycub commented Aug 15, 2022 •
edited
Loading

This PR extends consul-template to support NomadVariables. It adds 4 additional functions:

  • nomadVars
  • nomadVarsSafe
  • nomadVar
  • nomadVarExists

There is a circular dependency issue with Nomad, so the tests won't pass until Nomad has released a version that contains secure variables. I'm not sure how exactly to manage this, but I need to get the PR up at least.

@angrycub angrycub added enhancement nomad Related to ingetration in Nomad labels Aug 15, 2022
@angrycub angrycub requested a review from a team August 15, 2022 19:06
@angrycub angrycub self-assigned this Aug 15, 2022
@angrycub angrycub mentioned this pull request Aug 15, 2022
Closed
@eikenb
Copy link
Contributor

eikenb commented Aug 15, 2022

[..] so the tests won't pass until Nomad has released a version that contains secure variables.

Is it upstream yet? We can do a commit-id update for now if necessary.

Do you want these merged before the 0.29.2 release I'm planning for this week?

@tgross
Copy link
Member

tgross commented Aug 16, 2022 •
edited
Loading

Is it upstream yet? We can do a commit-id update for now if necessary.

Everything we need to support this in the API is on main. We're expecting to release Nomad 1.4.0-beta in the next few weeks.

Do you want these merged before the 0.29.2 release I'm planning for this week?

Maybe we hold off on that? When I sent @angrycub off on this particular adventure 😀 I wasn't keeping in mind the beta. We can always pin to a commit-id on the Nomad side for the beta release and then if there's anything that comes up in the beta window where we really hate the API and need to change it, we're not causing regressions on a released version of consul-template.

So what do you (@eikenb and @angrycub) think about the following:

  • For this PR we pin the go.mod to a commit-id on Nomad.
  • consul-template 0.29.2 ships without this PR.
  • We ship Nomad 1.4.0-beta in the next few weeks, pinning our CT dependency on the head of this PR's branch.
  • Sometime during the beta window we merge this PR.
  • Nomad 1.4.0 GA pins CT to a commit-id on CT's main at that point, or a hypothetical 0.29.x that lands afterwards.

That'll prevent Nomad from causing extra release schedule pressure for y'all, and give us a little breathing room around the Nomad 1.4.0 beta as well.

@eikenb
Copy link
Contributor

eikenb commented Aug 16, 2022

Thanks @tgross. Sounds like a good plan.

tgross
tgross reviewed Aug 17, 2022
View reviewed changes
dependency/nomad_secure_variables_get_test.go Outdated Show resolved Hide resolved
template/template_test.go Show resolved Hide resolved
docs/templating-language.md Outdated Show resolved Hide resolved
dependency/nomad_secure_variables_structs.go Outdated Show resolved Hide resolved
dependency/dependency_test.go Outdated Show resolved Hide resolved
dependency/nomad_secure_variables_list_test.go Outdated Show resolved Hide resolved
@tgross tgross mentioned this pull request Aug 26, 2022
Closed
@angrycub angrycub changed the title Support Nomad Secure Variables Support Nomad Variables Aug 30, 2022
@angrycub angrycub mentioned this pull request Aug 30, 2022
Merged
@tgross tgross mentioned this pull request Sep 9, 2022
Merged
tgross
tgross reviewed Sep 22, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
This was referenced Sep 29, 2022
Closed
Closed
@tgross
Copy link
Member

tgross commented Sep 30, 2022

I've opened hashicorp/nomad#14762 with a reminder to ourselves to update Nomad to point to the new release of CT once this has been merged.

eikenb
eikenb reviewed Sep 30, 2022
View reviewed changes
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@angrycub
Copy link
Contributor Author

angrycub commented Oct 17, 2022 •
edited
Loading

Rebased on main, Updated the Nomad API dependency to 1.4.1's /api version. Reverted the test shenanigans. Things look to be green and ready to rock @eikenb, @tgross

tgross
tgross approved these changes Oct 17, 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.

👍

docs/templating-language.md Outdated Show resolved Hide resolved
eikenb
eikenb reviewed Oct 17, 2022
View reviewed changes
.go-version Outdated Show resolved Hide resolved
eikenb
eikenb suggested changes Oct 18, 2022
View reviewed changes
Copy link
Contributor

@eikenb eikenb left a comment

Choose a reason for hiding this comment

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

Everything looks good except that one file. Thanks.

@tgross tgross requested a review from eikenb October 25, 2022 17:15
@eikenb
Copy link
Contributor

eikenb commented Oct 25, 2022

@angrycub... could you rebase on main/HEAD to get the go.mod build changes? This should allow the tests to pass (or at least fix what's currently failing). Thanks.

@tgross
Copy link
Member

tgross commented Oct 26, 2022

@angrycub @eikenb I've just pushed a branch https://github.com/hashicorp/consul-template/tree/pinned-for-nomad-1.4.1 which is at commit 61e288a, which is what Nomad's go.mod is pinned to as of right now. That'll let us rebase/squash this PR and get the tests green before merging without interfering with the ongoing Nomad builds before we have a PR to bump the dependency. We can delete the branch so the commit gets GC'd after we've finished shipping 1.4.2.

@angrycub
Support Nomad Variables
5244d28 
Nomad SV: Add Test helpers
Nomad SV: Secure Variables List dependency
Nomad SV: Secure Variables Get dependency
Nomad SV: Add template functions

Add test for variable value leak via error
Add variable values to redactinator

Support Namespace and Region for SV
Upgrading Nomad API version to v1.4.1
Fix copypasta in Nomad configuration
@angrycub angrycub merged commit 90370e0 into main Oct 26, 2022
@angrycub angrycub deleted the nomad-secure-vars branch October 26, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eikenb eikenb eikenb approved these changes

@tgross tgross tgross approved these changes

Assignees

@angrycub angrycub

Labels
enhancement nomad Related to ingetration in Nomad
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@angrycub @eikenb @tgross