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

Removed Kubernetes self contact points hack #433

Merged
merged 1 commit into from
Jan 11, 2019

Conversation

jroper
Copy link
Contributor

@jroper jroper commented Jan 7, 2019

Fixes #424.

Issue #238 introduced a hack that meant that cluster bootstrap used the pod host name when the kubernetes configuration was present, rather than the IP address, since Kubernetes discovery returned host names rather than IP addresses to work with Istio. That hack was made redundant however by #242, which introduced matching by either hostname, or IP address, so it didn't matter if the self host name was an IP address or hostname. However, the hack wasn't removed and stayed there, benign, until #415 changed the way namespace configuration is consumed. At that point, the hack was still benign, but resulted in some confusing log messages. This removes the hack.

Fixes akka#424.

Issue akka#238 introduced a hack that meant that cluster bootstrap used the
pod host name when the kubernetes configuration was present, rather than
the IP address, since Kubernetes discovery returned host names rather
than IP addresses to work with Istio. That hack was made redundant
however by akka#242, which introduced matching by either hostname, or IP
address, so it didn't matter if the self host name was an IP address or
hostname. However, the hack wasn't removed and stayed there, benign,
until akka#415 changed the way namespace configuration is consumed. At that
point, the hack was still benign, but resulted in some confusing log
messages. This removes the hack.
Copy link
Member

@chbatey chbatey left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@raboof
Copy link
Member

raboof commented Jan 7, 2019

I'm not completely confident #242 makes this hack obsolete for the Istio case (though I'd definitely be happy to see it go). I'll look into adding an Istio test to travis (related: #352).

@jroper
Copy link
Contributor Author

jroper commented Jan 7, 2019

That'd be great.

From what I understand, Istio communication should work because the Kubernetes API discovery returns the pod host name in the host field, which is what cluster bootstrap uses for HTTP and remoting connections, meanwhile Istio self host matching should work because the IP address returned by getLocalHost.getAddress is the public IP of the pod (even though it might not be accessible), and that will be matched against the IP address returned by the Kubernetes API. Previously, the matching worked because both the Kubernetes API discovery converted this IP address to the pod hostname, and the feature being deleted converted the discovered IP address via getLocalHost.getAddress to a pod hostname, and so they matched. So, matching the discovered address against the unconverted address will also work.

@jroper
Copy link
Contributor Author

jroper commented Jan 7, 2019

There was basically two ways to solve the problem, one was to return both the IP address and the host name from the self contact points (that's what the hack was) and match against either, the other was to return both the IP address and the host name from the discovery API (that's what #242 did) and match against either. Both were done, but only one was needed.

@raboof
Copy link
Member

raboof commented Jan 11, 2019

I'm afraid I won't get to adding an Istio test in the near future, so this is OK to merge if you're confident the hack is no longer necessary.

@patriknw
Copy link
Member

Let's merge and we can look into it again if it's reported as a problem with Istio (so far it hasn't been top priority for us to look into Istio)

@patriknw patriknw merged commit 3ed10d7 into akka:master Jan 11, 2019
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.

4 participants