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

Create our own KubernetesClient (instead of using fabric8) #80

Merged
merged 7 commits into from Jul 17, 2018

Conversation

leszko
Copy link

@leszko leszko commented Jul 13, 2018

Create our own Hazelcast KubernetesClient to use Kubernetes REST API. The motivation for this work was to get rid of the dependency to fabric8.io:kubernetes-client, which is ~11MB. We actually make only one call to Kubernetes API, so there is no good reason to import the whole library.

Tested on: Minikube, Pivotal PKS, Google Cloud Platform, and OpenShift Online.

Benefits from this change:

  • Decreases significantly the size of hazelcast-kubernetes: 11MB -> 334KB
  • Allows to include hazelcast-kubernetes in hazelcast-all
  • Decreases the Docker image size by 9MB
  • Simplifies the code of ServiceEndpointResolver

There are also some drawbacks of this change (but I believe they are minor comparing to the benefits):

  • We need to maintain KubernetesClient (minor, since it's just one class)
  • We lose some of the configuration options, which fabric8 provides (e.g. turning off CA Cert verification, etc.) - I believe it's not super important since we allow using hazelcast-kubernetes only from inside Kubernetes

Related to: #75

@devOpsHazelcast
Copy link
Contributor

Test PASSed.

Copy link
Collaborator

@hasancelik hasancelik left a comment

Choose a reason for hiding this comment

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

great work @leszko 💯 👍

Copy link
Contributor

@googlielmo googlielmo left a comment

Choose a reason for hiding this comment

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

Super neat and test-complete too, this is exactly what we need!
Thanks @leszko LGTM

connection.setRequestProperty("Authorization", String.format("Bearer %s", apiToken));

if (connection.getResponseCode() != HTTP_OK) {
throw new KubernetesClientException(String.format("Failure executing: GET at: %s. Message: %s,", urlString,
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we reasonably expect exactly 200 for our GETs (not other 2xx codes or even 3xx redirects etc.)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, according to the Kubernetes API Spec [1], 200 is the only valid response.

[1] https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.11/#read-142

Copy link
Contributor

Choose a reason for hiding this comment

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

super!

return "Forbidden!Configured service account doesn't have access. Service account may have been revoked. "
+ "endpoints is forbidden: User \"system:serviceaccount:default:default\" cannot list endpoints "
+ "in the namespace \"default\"";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

yep we know this one 👍

Copy link
Author

Choose a reason for hiding this comment

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

hehe, yeah, that's why I added a test specifically for this case ;)

Endpoints endpoints = createNotReadyEndpoints(2);
given(client.endpointsByName(NAMESPACE, SERVICE_NAME)).willReturn(endpoints);

ServiceEndpointResolver sut = new ServiceEndpointResolver(LOGGER, SERVICE_NAME, 0, null, null, NAMESPACE, null,
Copy link
Contributor

Choose a reason for hiding this comment

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

This would seriously benefit from a builder pattern (in general, I don't mean in this PR btw!)

Copy link
Author

Choose a reason for hiding this comment

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

I can't agree more. But definitely not part of this PR, it's already big.

@leszko leszko merged commit b7768ab into hazelcast:master Jul 17, 2018
@leszko leszko deleted the remove_dependencies_client branch July 17, 2018 14:44
@mesutcelik mesutcelik mentioned this pull request Jul 25, 2018
@leszko leszko added this to the 1.2 milestone Aug 27, 2018
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

4 participants