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

Document how clients not on K8s can join a DC in K8s #9438

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

ndhanushkodi
Copy link
Contributor

@ndhanushkodi ndhanushkodi commented Dec 18, 2020

Documents hashicorp/consul-helm#740.

Also resolves/helps with hashicorp/consul-helm#743.

@@ -35,14 +48,68 @@ annotation `consul.hashicorp.com/auto-join-port` to an integer value or
a named port to specify the port for the auto-join to return. This enables
different pods to have different exposed ports.
Copy link
Contributor Author

@ndhanushkodi ndhanushkodi Dec 18, 2020

Choose a reason for hiding this comment

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

In my testing, I did not need to add this annotation, even though I configured my server gossip port to be 9301. consul members still successfully showed all members of the cluster.

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 think it's still useful documentation to have, but that's why I didn't include this annotation in the instructions below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that's interesting. Did you change the gossip ports on the servers or just the external client? I definitely see in the code that it's being used.

Copy link
Member

Choose a reason for hiding this comment

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

weird, I don't know how the join could have worked without this? Wouldn't it try 8301 unless explicitly configured?

Copy link
Contributor Author

@ndhanushkodi ndhanushkodi Jan 6, 2021

Choose a reason for hiding this comment

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

@ishustava Yup, I did change the gossip port on the server to 9301. And the client just used -retry-join 'provider=k8s host_network=true label_selector="app=consul,component=server"' to join the servers. I know its super weird I would expect it to use that code as well!

@lkysow Yup, I would have expected the external client agent fail to join because it would try 8301. However, when trying the exactly the steps here while replacing the retry-join with -retry-join 'provider=k8s host_network=true label_selector="app=consul,component=server"' I observed it joining just fine and on port 9301, which is definitely mind boggling.

I will retry those steps one more time tomorrow morning just to make sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I ran through the steps with -retry-join 'provider=k8s host_network=true label_selector="app=consul,component=server"' on the external agent without the annotation and it works. In fact, when I do add the annotation, it doesn't accept an integer value, and if I add the port "9301" as a string, the consul clients on k8s itself fail to join saying no consul servers found.

I think for these docs I should remove this annotation, since it doesn't seem to work. wdyt @ishustava @lkysow?

Copy link
Member

Choose a reason for hiding this comment

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

As discussed lets:

  1. Add a warning that if you're using 9301 and your consul servers are on different nodes (which is unlikely because in that case you wouldn't need to use 9301) then you can't use autojoin
  2. Create a bug in go-discover about the annotation issue

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 removed the note about the annotation since its currently broken (we can add it back once the bug is fixed in go-discover), and added the note/warning about default port. I phrased it a bit differently, let me know if you think its clear or not

Copy link
Contributor Author

@ndhanushkodi ndhanushkodi Jan 7, 2021

Choose a reason for hiding this comment

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

Writing the bug right now. EDIT: hashicorp/go-discover#166

@ndhanushkodi ndhanushkodi marked this pull request as ready for review December 18, 2020 21:27
@ndhanushkodi ndhanushkodi requested review from lkysow, a team and ishustava and removed request for a team December 18, 2020 21:27
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looking pretty good! Thanks so much for doing this work and writing the docs; I like that the networking requirements sound much more clear now! I've left a few edits and suggestions, and once those are resolved, it's good to go!

@@ -35,14 +48,68 @@ annotation `consul.hashicorp.com/auto-join-port` to an integer value or
a named port to specify the port for the auto-join to return. This enables
different pods to have different exposed ports.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that's interesting. Did you change the gossip ports on the servers or just the external client? I definitely see in the code that it's being used.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

This is looking great!

@@ -35,14 +48,68 @@ annotation `consul.hashicorp.com/auto-join-port` to an integer value or
a named port to specify the port for the auto-join to return. This enables
different pods to have different exposed ports.
Copy link
Member

Choose a reason for hiding this comment

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

weird, I don't know how the join could have worked without this? Wouldn't it try 8301 unless explicitly configured?

@ndhanushkodi
Copy link
Contributor Author

@lkysow @ishustava Thank you so much for the thorough reviews, it really helped. I think I've left open only the stuff worth taking another look at and resolved anything else. Going to re-request your reviews.

gke-external-agent-default-pool-32d15192-vo7k 10.138.0.42:8301 alive client 1.9.1 2 dc1 <default>
```

### Auto-join on via host ports
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Auto-join on via host ports
### Auto-join via host ports

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

💯 looks great! Just comment about not setting -client 0.0.0.0 since that exposes the HTTP API.

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

🎉

@ndhanushkodi ndhanushkodi deleted the docs-external-clients branch January 8, 2021 00:14
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/307179.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants