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

Nodeport syncing updates #45

Merged
merged 5 commits into from
Jan 8, 2019
Merged

Nodeport syncing updates #45

merged 5 commits into from
Jan 8, 2019

Conversation

adilyse
Copy link
Contributor

@adilyse adilyse commented Dec 20, 2018

This updates the NodePort sync logic to use the node's ip address, rather than the pod's ip. This puts a fully routable address into the sync, making it possible to externally sync with these services.

Additionally, it updates the port determination logic to take the first defined NodePort when there are multiples. It also now takes the first address when the ports are unnamed, as well as adding tests for this case.

The original plan was to only sync NodePort service instances for the
nodes they were running on. Currently, they are being synced with the
pod's ip address instead, which is not guaranteed to be routable. For
users running in this type of environment, this is causing these services
to be reaped by the anti-entropy mechanism since their health checks fail
due to the unroutability. Additionally, this provides the incorrect
ip address for routing to NodePort services, which makes the sync ineffective.

Given that there is not an easy way to get the node info from a service,
the easiest solution to this is to sync all of the kubernetes nodes
for the NodePort service. This has the benefit of distributing traffic
as Kuberentes expects it for this type of service.
It *is* possible to find the node from an endpoint, so this turns into
a straight up bug fix to match the intended design. It gets the routable
node info into the sync, allowing folks to use it for external connections.
This should also fix the anti-entropy issue that was removing NodePort
services because of the nodes failing health checks.
If more than one NodePort was defined for a service, rather than
taking the first, the last was taken instead. This update takes the
first as expected.

It additionally now takes the first port when using unnamed ports,
as well as adds tests for unnamed ports for both NodePort and
ClusterIP type services.
@adilyse adilyse requested review from mitchellh, anubhavmishra and a team December 20, 2018 01:37
@hidaen
Copy link

hidaen commented Dec 29, 2018

Hi @adilyse I found some problems while testing the nodeport-syncing branch:
When I tested nodeport, I found that some nodeport service address.Type is NodeInternalIP. This situation is not registered. Will this be considered for entry? Or why there is only reason to register NodeExternalIP.

@adilyse
Copy link
Contributor Author

adilyse commented Jan 7, 2019

@hidaen

Thanks for taking a look! The InternalIP issue is a good find. I was working from the assumption that people were wanting to access these services from outside of their Kubernetes cluster, which would make the ExternalIP more appropriate.

For your InternalIP use case, would using an InternalIP if an ExternalIP isn't found be enough? Or do you need more control?

Some users may not be exposing any external ip addresses, or may
prefer to have all the routing happen within their network. This
allows users to choose whether to sync NodePort services with either
the node's External or Internal IP address.
@adilyse
Copy link
Contributor Author

adilyse commented Jan 7, 2019

Thinking about this further, adding an option seems to support a wider array of use cases, so I've opted for that.

@hidaen
Copy link

hidaen commented Jan 8, 2019

Thinking about this further, adding an option seems to support a wider array of use cases, so I've opted for that.

@adilyse
I think there is a problem with my description, I think adding the sync-node-external-ip parameter is not a good choice.
External access to the Kubernetes service, using ExternalIP is indeed more appropriate.
But Kubernetes NodePort service should register ExternalIP or Node IP whether it is ExternalIP or InternalIP
I am going to try to write this piece of code, so I will ask again.

Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

This looks really good. I made one comment but otherwise the impl and testing is great 👍

catalog/from-k8s/resource.go Outdated Show resolved Hide resolved
This allows for a variety of sync options with NodePort. At this time,
ExternalOnly, InternalOnly and ExternalFirst are implemented options.
The first two limit the node ip's address to either the ExternalIP or
the InternalIP. The third option uses the node's ExternalIP if it
exists. If not, it instead uses the node's InternalIP.
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

New commit looks great!

@adilyse
Copy link
Contributor Author

adilyse commented Jan 8, 2019

Fixes #8

@adilyse adilyse merged commit 947bc1a into master Jan 8, 2019
@adilyse adilyse deleted the nodeport-syncing branch January 8, 2019 21:02
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
update CHANGELOG to show connect injector RBAC support
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.

None yet

3 participants