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

Add first version of a Fabric8 Kubernetes Client extension #2910

Merged
merged 1 commit into from
Jun 22, 2019

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jun 20, 2019

TODO

Fixes: #1488

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added 2 very minor comments. Code looks good otherwise.

@geoand geoand marked this pull request as ready for review June 21, 2019 09:50
@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

PR is now ready. I ran the native test locally and it worked wonderfully.

@ricardozanini
Copy link

@geoand seems very nice! Many thanks for that. Have you tested this client using SA token/cert inside OpenShift? This is the next step of the tests that I'm about to run today in native environment to make sure that the CertUtils won't break anything because of the EC Keys thing. I guess won't. 😄

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

@ricardozanini Thanks :)

Yeah, I explicitly disabled the EC thing so --allow-incomplete-classpath isn't necessary.
Also I tested this against a proper Openshift cluster and it did work :). I didn't test inside Openshift (which would then bring in the token/cert stuff), but only from outside. I am assuming it will continue to work, but even if it doesn't I don't think it will be hard to fix

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

@ricardozanini I also tried using the certificate of the Kubernetes API and it worked great in native mode as well.

@ricardozanini
Copy link

ricardozanini commented Jun 21, 2019

@geoand many thanks for your feedback. So, I'm assuming you copied the ssl required native libs into your image? Do you mind sharing which libs have you copied? And what base image? I'm about to write a blog post about this setup also to help others having SSL working on native images.

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

@ricardozanini well, I sort of cheated to be honest :). I just copied the cluster certificate locally and instructed the Kubernetes Client to use it (while also telling it to explicitly not trust unknown certificates).
Then I built the native binary locally and just tried it against the cluster.

So essentially I didn't create a docker image and run a pod in the cluster, I just made sure the native binary works with the certificates.

I think @cescoffier would know a lot more about which base image to use and which libs are needed

@ricardozanini
Copy link

@geoand I'm about to upload an image with a demo working, I'll share with you the results afterwards.

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

@ricardozanini cool, looking forward to it :)

@fstab
Copy link
Contributor

fstab commented Jun 21, 2019

I tried it. It compiles, but when I run the Docker image I get this error:

./application: error while loading shared libraries: libcrypt.so.1: cannot open shared object file: No such file or directory

I used the Dockerfile.native image from the maven archetype.

@ricardozanini
Copy link

@geoand here's the working docker image:
https://quay.io/repository/ricardozanini/kogito-svc-discovery

Then:

docker run -i --rm -p 8080:8080 -e KUBERNETES_MASTER=<your cluster URL> -e KUBERNETES_AUTH_TOKEN=$(oc whoami -t) -e KUBERNETES_TRUST_CERTIFICATES=true  quay.io/ricardozanini/kogito-svc-discovery

Go to:
http://localhost:8080/services/default

Assuming your user has permission to query the default namespace. Otherwise, just replace default for some other namespace.

You should see the services in JSON format.

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

@fstab that is a typical issue when using the wrong docker base image. @gsmet @cescoffier @ricardozanini can you assist @fstab with the error perhaps?

@ricardozanini
Copy link

@fstab that's because you need to copy the native lib to your docker image, see:
https://github.com/ricardozanini/k8s-svc-discovery/blob/master/kogito-client/src/main/docker/Dockerfile.native

This image already have what you need to run the client. Just copy the native lib from cp $GRAALVM_HOME/jre/lib/amd64/libsunec.so

@gsmet
Copy link
Member

gsmet commented Jun 21, 2019

so about this libcrypt.so.1 issue, the issue is when building things with libcrypt.so.1 while running with libcrypt.so.2 or the opposite (typically Fedora 30 uses .so.2).

This is definitely annoying. I don't know how we can find an acceptable solution to this issue while all the distributions/users transition to the new lib.

@ricardozanini
Copy link

ricardozanini commented Jun 21, 2019

@geoand the base image from the maven archetype (fedora-minimal) doesn't have ssl support, My bad: does have, but uses another version of the lib like pointed by @gsmet. Besides copying the SunEC lib, you need the ssl and crypto libs that are not available in some base images.

Also, I'll work on something to not use the native library. See:
oracle/graal#951

Thanks to @ruromero for pointing this issue 😃

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

Thanks for the info @ricardozanini and @gsmet!

@geoand geoand requested a review from gsmet June 21, 2019 15:30
@fstab
Copy link
Contributor

fstab commented Jun 21, 2019

Thank for your help with the libcrypt.so.1 issue. This seems to work now, but I ran into the next one: I'm using a custom resource, and the spec is implemented with Jackson's @JsonDeserialize similar to the fabric8 example code. As soon as the fabric8 client attempts to read the custom resource I am seeing this:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `org.acme.quickstart.cr.DemoResourceSpec` (no Creators, like default construct, exist): cannot de
serialize from Object value (no delegate- or property-based Creator)                           
 at [Source: UNKNOWN; line: -1, column: -1] (through reference chain: io.fabric8.kubernetes.api.model.WatchEvent["object"]->org.acme.quickstart.cr.DemoResource["spec"])
        at com.fasterxml.jackson.databind.DeserializationContext.reportBadDefinition(DeserializationContext.java:1452)
        at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1028)
        at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1297)
        at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:326)
        at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:159)
        at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129)
        at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:288)
        at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:151)
        at com.fasterxml.jackson.databind.ObjectMapper._readValue(ObjectMapper.java:3984)
        at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2276)
        at com.fasterxml.jackson.databind.ObjectMapper.treeToValue(ObjectMapper.java:2758)
        at io.fabric8.kubernetes.internal.KubernetesDeserializer.deserialize(KubernetesDeserializer.java:80)
        at io.fabric8.kubernetes.internal.KubernetesDeserializer.deserialize(KubernetesDeserializer.java:32)
        at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129)
        at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:288)
        at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:151)
        at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4013)
        at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3004)
        at io.fabric8.kubernetes.client.dsl.internal.WatchHTTPManager.readWatchEvent(WatchHTTPManager.java:293)
        at io.fabric8.kubernetes.client.dsl.internal.WatchConnectionManager$1.onMessage(WatchConnectionManager.java:224)
        at okhttp3.internal.ws.RealWebSocket.onReadMessage(RealWebSocket.java:323)
        at okhttp3.internal.ws.WebSocketReader.readMessageFrame(WebSocketReader.java:219)
        at okhttp3.internal.ws.WebSocketReader.processNextFrame(WebSocketReader.java:105)
        at okhttp3.internal.ws.RealWebSocket.loopReader(RealWebSocket.java:274)
        at okhttp3.internal.ws.RealWebSocket$2.onResponse(RealWebSocket.java:214)
        at okhttp3.RealCall$AsyncCall.execute(RealCall.java:206)
        at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)
        at com.oracle.svm.core.thread.JavaThreads.threadStartRoutine(JavaThreads.java:473)
        at com.oracle.svm.core.posix.thread.PosixJavaThreads.pthreadStartRoutine(PosixJavaThreads.java:193)

Is this supposed to work, or is Jackson deserialization not possible with native images?

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

@fstab so you are using a k8s custom resource, right? Serialization and Deserialization definitely work with the standard model classes and I'll look into CR handling over the weekend (shouldn't be hard to fix I assume - but I would prefer the PR be merged anyway since it covers the most common use cases)

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

@fstab could you please share what exactly your failing class looks like? I am basically interested if it extends KubernetesResource

@fstab
Copy link
Contributor

fstab commented Jun 21, 2019

@geoand I uploaded a minimal operator example on https://github.com/fstab/operator-example

  • The master branch uses quarkus 0.17.0 and works with the JVM image as described in README.md
  • The native branch uses quarkus 999-SNAPSHOT created from this PR, the steps described in README.md lead to the Jackson serialization exception above.

Thanks a lot for looking into this!

@geoand
Copy link
Contributor Author

geoand commented Jun 21, 2019

@geoand I uploaded a minimal operator example on https://github.com/fstab/operator-example

  • The master branch uses quarkus 0.17.0 and works with the JVM image as described in README.md
  • The native branch uses quarkus 999-SNAPSHOT created from this PR, the steps described in README.md lead to the Jackson serialization exception above.

Thanks a lot for looking into this!

Thanks for the reproducer! Looking into it now

@geoand
Copy link
Contributor Author

geoand commented Jun 22, 2019

@fstab the problem in your example is that ExampleResourceSpec has not been registered for reflection, therefore GraalVM removed it's constructor when creating the native binary.
For the standard Kubernetes Model classes we overcome this problem because we register all classes that implement KubernetesResource as (possible) targets for reflection.

So if your ExampleResourceSpec has no reason to implement said Interface (which I guess makes sense for this use case), what you have to do is annotate it with @RegisterForReflection as described in some detail here. This annotation is a marker that makes Quarkus register the class for reflection during GraalVM native image generation.

Would you like to give it a shot and let me know how it goes?

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added a few comments/questions inline.

I think it's an interesting addition and a nice first step. I'm wondering though if it would be worth to go a bit further and allow to configure the client via Quarkus properties and so on. Not sure if it's a good idea or not?

I think this PR has value as is so I'm in favor of merging it and then build on it.

@geoand
Copy link
Contributor Author

geoand commented Jun 22, 2019

I was actually also thinking the same thing about configuring a client via properties. I think it will be great, but I also believe we should first think about which properties would be useful to expose - probably something similar to what we have done in Spring Cloud Kubernetes.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Nice contribution, thanks!

@gsmet gsmet added this to the 0.18.0 milestone Jun 22, 2019
@gsmet gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes release/noteworthy-feature labels Jun 22, 2019
@geoand
Copy link
Contributor Author

geoand commented Jun 22, 2019

You are very welcome!

@geoand
Copy link
Contributor Author

geoand commented Jun 22, 2019

@fstab One more thing you will need to do in your example to get it work (I tried it), is to ensure that the native binary contains daemonset.yaml (by default GraalVM will not include resources).
To do that, all you need to do is add

<additionalBuildArgs>-H:IncludeResources=daemonset.yaml</additionalBuildArgs>

to the configuration section of the quarkus-maven-plugin

@geoand geoand merged commit d16be7f into quarkusio:master Jun 22, 2019
@geoand geoand deleted the kubernetes-client branch June 22, 2019 14:39
@gsmet
Copy link
Member

gsmet commented Jun 22, 2019

@geoand is it a standard file or something custom?

@geoand
Copy link
Contributor Author

geoand commented Jun 22, 2019

@gsmet it's custom to the example application in question. I was thinking that we could perhaps add all yaml or json files when this extension is added, but it will still be a heuristic - there are no certainties that these files are actual Kubernetes files (although I suppose we could parse them to determine that).

@fstab
Copy link
Contributor

fstab commented Jun 22, 2019

@geoand I can confirm that my operator example works in native mode with the changes you described above. Great job, thank you very much!!!

@geoand
Copy link
Contributor Author

geoand commented Jun 22, 2019

Awesome to hear it @fstab! Thanks for your help!

@geoand
Copy link
Contributor Author

geoand commented Jun 24, 2019

@fstab I was thinking, would you be up to contributing a guide for using the Kubernetes Client?

@fstab
Copy link
Contributor

fstab commented Jun 25, 2019

Yes, I could do that, but it might take two weeks or so until I find the time. Will it be a new document in docs/src/main/asciidoc?

@geoand
Copy link
Contributor Author

geoand commented Jun 25, 2019

@fstab yes exactly! That would be awesome, thanks!

@fstab
Copy link
Contributor

fstab commented Jul 4, 2019

@geoand regarding the guide for the Kubernetes client extension: I wrote a series of Blog posts about it:

I think it should be possible to condense these into a tutorial. I will try to find the time for this next week.

@geoand
Copy link
Contributor Author

geoand commented Jul 4, 2019

That's awesome, thanks @fstab !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/noteworthy-feature triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce a Kubernetes client
4 participants