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

Support sockaddr templates in start/retry join #4102

Merged
merged 3 commits into from
May 10, 2018
Merged

Support sockaddr templates in start/retry join #4102

merged 3 commits into from
May 10, 2018

Conversation

banks
Copy link
Member

@banks banks commented May 10, 2018

This supersedes #3865 from @dominik-lekse.

See the original for rationale and discussion. I had to rebase a lot of stuff from the original fork the PR was based on and ended up in a mess so I choose to start a new branch from fresh master and cherry-pick @dominik-lekse's commit into it.

In the process of adding tests, I found a couple of issues which are fixed here:

  • only LAN addresses were supported and by the same rationale as the original PR that we should support sockaddr consistently across config it seems very confusing to have support in start_join but not in start_join_wan.
  • There was a bug in the code as added that would double the number of addresses with empty strings due to using make([]string, len(...)) and then appending instead of make([]string, 0, len(...))
  • the config docs should be updated to reflect this new feature.

// the result list will contain the input string as a single element with no
// error set. In contrast to expandAddrs, expandOptionalAddrs does not validate
// if the result contains valid addresses and returns a list of strings.
// However, if the expansion of the go-sockaddr template fails an error is set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why you don't want to validate? for example if the resolved template returns a unix socket address we should throw an error because that's not going to work for retry-join/retry-join-wan.

Copy link
Member Author

@banks banks May 10, 2018

Choose a reason for hiding this comment

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

This was from @dominik-lekse's original PR but I think the rationale is that in this case the RuntimeConfig members we are outputting to are []string not []net.Addr.

We could add the parsing here just to catch errors but on the basis that before this PR passing an invalid IP to these config params is not validated here in the builder it seems reasonable not to jump through that extra hoop.

Presumably the agent itself will error if the expanded (or just non-template) value provided was invalid just like it does before this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did a quick test on master and looks like an invalid address in retry join gets all the way upto the join call in memberlist before it errors out. We can do better with this validation in the future though, agree with you on the rationale to leave it as is for this PR.

@banks banks merged commit c1374a5 into master May 10, 2018
@banks banks deleted the join-sockaddr branch May 10, 2018 16:42
@dominik-lekse
Copy link
Contributor

Thanks everyone involved for picking this feature up and including it in the next release.

@banks
Copy link
Member Author

banks commented May 11, 2018

@dominik-lekse thanks for writing it! Sorry it ended up being a PR with my name on :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants