-
Notifications
You must be signed in to change notification settings - Fork 922
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
Add KubernetesEndpointGroup
#5001
Conversation
I managed to finish implementing our own Kubernetes authentications by porting https://github.com/kubernetes-client/java. However, I'm not sure the code is maintainable when there are new changes on the Kubernetes side. On second thought, it would be much better to use a well-known Kubernetes client implementation instead. https://github.com/kubernetes-client/java is hard-coded with OkHttp but https://github.com/fabric8io/kubernetes-client has an integration layer for various client implementations. I filed fabric8io/kubernetes-client#5307 to support for Armeria backend. I will continue working on this PR once the Armeria backend for the Fabric Kubernetes client is implemented. It will make this PR simple and easy to review for maintainers. |
7d6222f
to
fe3ad3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 🚀 🚀 🚀
.../src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java
Show resolved
Hide resolved
.../src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroup.java
Show resolved
Hide resolved
...in/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroupBuilder.java
Show resolved
Hide resolved
...test/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesAvailableCondition.java
Show resolved
Hide resolved
...in/java/com/linecorp/armeria/client/kubernetes/endpoints/KubernetesEndpointGroupBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nits for me 👍 Thanks @ikhoon 🙇 👍 🙇
public KubernetesEndpointGroup build() { | ||
checkState(serviceName != null, "serviceName not set"); | ||
return new KubernetesEndpointGroup(kubernetesClient, namespace, serviceName, portName, autoClose, | ||
selectionStrategy, shouldAllowEmptyEndpoints(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks likely that either Watcher<Pod>
or Watcher<Node>
will emit an event.
However, both events are required to construct endpoints in maybeUpdateEndpoints
.
Since most users wait for EndpointGroup#whenReady
to validate the endpoint before using it, I wonder if setting allowEmptyEndpoints=false
is a more sensitive default. I think as it stands, the initial endpoints will almost always be an empty list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
.map(NodeAddress::getAddress) | ||
.findFirst().orElse(null); | ||
if (nodeIp == null) { | ||
logger.debug("No 'InternalIP' is found in {}. node: {}", nodeName, node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) Is there no need to delete the nodeToIp
entry in this case since there is no valid nodeip?
case ADDED: | ||
case MODIFIED: | ||
final String nodeIp = node.getStatus().getAddresses().stream() | ||
.filter(address -> "InternalIP".equals(address.getType())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) Does ExternalIP
/HostName
not work? I think it's fine to only handle InternaIP
for now, but just in case someone asks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I don't know much about how Kubernetes operates and which IP is preferred.
Let me move this filter to the builder. InternalIP
will be chosen by default but overridable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good 👍 👍 👍
There are leak reports in CI builds which turned out to be bugs in tests of https://github.com/fabric8io/kubernetes-client. I will disable the tests until the upstream fixes it. |
I found another leak in tests fabric8io/kubernetes-client#5854 |
Motivation:
It is tricky to send requests to a Kubernetes cluster from outside servers without ingress.
There is no way to send traffic directly to the pod, but we can send traffic to the port of nodes (NodePort) where the pods are located.
This PR proposes a new EndpointGroup that can send requests with CSLB using NodeIP and NodePort to pods in Kubernetes. This way is not an ideal CSLB where servers and clients communicate directly, but it will be a safer way to send traffic without going through ingress which can be SPOF.
Modifications:
KubernetesEndpointGroup
on top ofKubernetesClient
to dynamically obtain Kubernetes resources.services
,nodes
,pods
is required to fetch endpoints.service.ports[*].nodePort
is used to create the port ofEndpoint
.ADDED
andMODIFIED
events are used to update resouces.DELETED
is used to remove the resouce.BOOKMARK
event is not used andERROR
may be ignorable.KubernetesEndpointGroup
with both a real Kubernetes cluster and a mock Kubernetes server.Result:
KubernetesEndpointGroup
to perform client-side load-balancing when sending requests.