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.

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

feature(config): Add go-sockaddr templating support to nomad consul address #12084

Merged
merged 1 commit into from Feb 24, 2022

Conversation

Nkmol
Copy link
Contributor

@Nkmol Nkmol commented Feb 17, 2022

This PR adds support for go-sockaddr/template in the consul address Stanza.

As noted in the issue, this becomes especially useful when you want to pair your nomad agent to the consul private network agent in a dynamic fashion.

Fixes: #11062

Special note to the reviewer

A check has been added to only support one single IP in the Stanza, as was done in other places using ParseSingleIPTemplate. However, I think in the years Hashicorp defines this in their shared package: https://github.com/hashicorp/go-secure-stdlib/blob/7166b1cdf6a6eb1311247e34feae17eaf7b657bf/listenerutil/parse.go#L374
Adding an extra dependency to the repo of go-secure-stdlib might not be something preferable. If this is still preferable, I could add this extra import.


This is my first contribution to this project and the first time touching Go, so any tips or directions for improvement is always welcome :)

@hashicorp-cla
Copy link

hashicorp-cla commented Feb 17, 2022

CLA assistant check
All committers have signed the CLA.

@DerekStrickland
Copy link
Contributor

Hi @Nkmol . Thanks for contributing!

I'm going to take a look today and talk to the team about your contribution.

Copy link
Contributor

@DerekStrickland DerekStrickland left a comment

Choose a reason for hiding this comment

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

I found one thing right away that is a blocker. See comment about panic.

nomad/structs/config/consul.go Outdated Show resolved Hide resolved
@Nkmol Nkmol force-pushed the nomad-consul-sockaddr-address-support branch from c7880b9 to a092991 Compare February 19, 2022 16:17
@Nkmol Nkmol changed the title feature: add go-sockaddr/template support for nomad consul address feature(config): Add go-sockaddr templating support to nomad consul address Feb 19, 2022
helper/config/config.go Outdated Show resolved Hide resolved
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 @Nkmol! Thanks for this PR, this is looking really good. I've left a few comments but once those are resolved I imagine we can get this shipped as part of Nomad 1.3.0

If you'd like, you can add a changelog file too. That'd be a text file at .changelog/12084.txt formatted similar to .changelog/12079.txt.

website/content/docs/configuration/consul.mdx Outdated Show resolved Hide resolved
helper/config/config.go Outdated Show resolved Hide resolved
nomad/structs/config/consul_test.go Outdated Show resolved Hide resolved
helper/config/config.go Outdated Show resolved Hide resolved
helper/config/config.go Outdated Show resolved Hide resolved
nomad/structs/config/consul_test.go Outdated Show resolved Hide resolved
@Nkmol Nkmol force-pushed the nomad-consul-sockaddr-address-support branch from a7683f1 to 7f4e241 Compare February 23, 2022 20:12
@Nkmol
Copy link
Contributor Author

Nkmol commented Feb 23, 2022

Thanks @shoenig and @tgross for the thorough review. Based on the feedback, I changed the following:

  • Imported the https://github.com/hashicorp/go-secure-stdlib/releases/tag/listenerutil%2Fv0.1.4 module
    Importing this module, however, introduced more dependencies than I was expecting. My gut feeling is telling me the dependencies added and changed might be too much out of scope of this PR. I am having a hard time assessing the mod.go changes :) Let me know what you think about this.
  • Implemented table-driven tests
    TIL :) I did not know about this and tried to make them as parallel runnable as possible being aware of the Go Routine bug.
  • Migrated from Go asserts to testify

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! I'll get this merged once CI is done and it'll ship in Nomad 1.3.0. Thanks again @Nkmol!

@tgross tgross merged commit 0ae76b1 into hashicorp:main Feb 24, 2022
@tgross tgross added this to the 1.3.0 milestone Feb 24, 2022
@Nkmol Nkmol deleted the nomad-consul-sockaddr-address-support branch February 24, 2022 15:09
@github-actions
Copy link

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 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consul.address config doesn't interpolate GetInterfaceIP, or use CONSUL_HTTP_ADDR
5 participants