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

stork-service-discovery-kubernetes extension requires some extra configutation that is not documented #24444

Closed
pjgg opened this issue Mar 21, 2022 · 16 comments · Fixed by #25374
Labels
Milestone

Comments

@pjgg
Copy link
Contributor

pjgg commented Mar 21, 2022

Describe the bug

stork-service-discovery-kubernetes requires org.bouncycastle:bctls-jdk15on dependency and also some extra configuration on your application.properties:

Example

# application properties should be here
quarkus.security.security-providers=BCJSSE

quarkus.http.ssl.certificate.key-store-file=server-keystore.jks
quarkus.http.ssl.certificate.key-store-password=password
quarkus.http.ssl.certificate.trust-store-file=server-truststore.jks
quarkus.http.ssl.certificate.trust-store-password=password

quarkus.native.additional-build-args=--allow-incomplete-classpath

Also, end-user must be sure that has the following K8s/ocp cluster roles binding (JVM and native mode):

Cluster Role

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  namespace: "${NAMESPACE}"
  name: endpoints-reader
rules:
  - apiGroups: [""] # "" indicates the core API group
    resources: ["endpoints"]
    verbs: ["get", "watch", "list"]

ClusterRoleBinding

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: fabric8-rbac
subjects:
  - kind: ServiceAccount
    # Reference to upper's `metadata.name`
    name: default
    namespace: "${NAMESPACE}"
roleRef:
  kind: ClusterRole
  name: cluster-admin
  apiGroup: rbac.authorization.k8s.io

I think that these requirements should be documented:
DocRef: https://quarkus.io/blog/stork-kubernetes-discovery/

Also would be great to understand why org.bouncycastle:bctls-jdk15on is needed and check if we could do something in order to avoid this extra dependency

Reproducer: quarkus-qe/quarkus-test-suite#572

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@pjgg pjgg added the kind/bug Something isn't working label Mar 21, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 21, 2022

/cc @geoand, @iocanel, @michalszynkiewicz

@michalszynkiewicz
Copy link
Member

CC @aureamunoz

@pjgg
Copy link
Contributor Author

pjgg commented Mar 21, 2022

Regarding native mode is also possible to make it work by adding the following configuration to your application.properties:

quarkus.native.additional-build-args=--allow-incomplete-classpath, --initialize-at-run-time=io.fabric8.kubernetes.client.internal.CertUtils, --enable-url-protocols=https

@iocanel
Copy link
Contributor

iocanel commented Mar 29, 2022

It's not clear to me why we need to setup bouncycastle. Can you please provide some more details?

The rbac configuration is indeed needed (with some minor modifications). However, when the kubernetes-client extension is used along with the kubernetes extension, these are expected to be generated.

I think that the main issue here is that stork-service-discovery-kubernetes is not reusing the kubernetes-client extension.

@aureamunoz
Copy link
Member

aureamunoz commented Mar 29, 2022

Yes, the rbac configuration is needed, I'm working in the documentation about it and will create a PR soon.

Regarding the bouncycastle, the problem is a NoClassDefFoundError:

Error: Class initialization of io.fabric8.kubernetes.client.internal.CertUtils$1 failed. Use the option --initialize-at-run-time=io.fabric8.kubernetes.client.internal.CertUtils$1 to explicitly request delayed initialization of this class.
Original exception that caused the problem: java.lang.NoClassDefFoundError: org/bouncycastle/jce/provider/BouncyCastleProvider
        at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized0(Native Method)
        at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized(Unsafe.java:1042)
        at jdk.unsupported/sun.misc.Unsafe.ensureClassInitialized(Unsafe.java:698)
        at

And this seem to be because GraalVM doesn't have them (the JVM has them).

stork-service-discovery-kubernetes does use the kubernetes-client directly, not via Quarkus because Stork is a framework independent on Quarkus. Why, indeed, these dependencies are not added in the fabric8 Kubernetes Client? We can add them to stork-service-discovery-kubernetes module and it should fix the problem but we think that they should be added from the Kubernetes Client side, where they are actually needed. WDYT @iocanel @manusa ?

@aureamunoz
Copy link
Member

Documentation PR: smallrye/smallrye-stork#278

@manusa
Copy link
Contributor

manusa commented Mar 30, 2022

Bouncy Castle dependencies are optional and should only be necessary in case ECDSA keys are used. I think these might also be required for FIPS mode.

Is this property quarkus.security.security-providers=BCJSSE something you are adding due to this issue (on top of the BC dependencies)?

@michalszynkiewicz
Copy link
Member

michalszynkiewicz commented Apr 7, 2022

A possible solution we discussed for the native problem is to create an conditional dependency on the quarkus-kubernetes-client extension (is this the thing that has all the adjustments to make the client work in native?) that would be "enabled" where the kubernetes service discovery is added to a user's project.
I've never used it but it seems it should work: https://quarkus.io/guides/conditional-extension-dependencies

If the client brings too many things (or we need the quarkus-kubernetes extension which generates the deployment yamls), we could pull out some quarkus-kubernetes-common (hidden) extension.

@geoand
Copy link
Contributor

geoand commented Apr 7, 2022

IIUC, the whole problem is that Stork does not use the Kubernetes Client provided by the Kubernetes extension, correct?

@cescoffier
Copy link
Member

@geoand Yes, stork is a single "extension". We do not have a stork-kubernetes extension that could depend on the Quarkus Kubernetes extension.

Stork itself should not depend on that extension. It's only when you use the kubernetes service discovery that the problem occurs. Having to use an extension for each service discovery / load balancing strategy seems a bit too much. However, I'm pretty sure we would have the issue with consul too. So, we may have to be smart about it...

@geoand
Copy link
Contributor

geoand commented Apr 7, 2022

Thanks for the description of the problem.

Could Stork not have an SPI that basically allows the Kubernetes Client to be set and then in Quarkus when the Kubernetes Client capability is present have the SPI implemented by pulling the Kubernetes Client from CDI?
That would not require a new extension.

Of course, the conditional dependencies could also work (we've used it for various things), but I think the SPI approach would be fine for this use case.

@michalszynkiewicz
Copy link
Member

It could be done via QuarkusStorkInfrastructure: https://github.com/quarkusio/quarkus/blob/main/extensions/smallrye-stork/runtime/src/main/java/io/quarkus/stork/QuarkusStorkInfrastructure.java

Right now it's only used to reuse the Quarkus Vertx instance but it's easy to change the Kube service discovery to use it for kube client as well.

How does it help? Would you want the user to add the kubernetes client extension when they add the kube service discovery?

@geoand
Copy link
Contributor

geoand commented Apr 7, 2022

Would you want the user to add the kubernetes client extension when they add the kube service discovery?

Pretty much, yeah. Similarly to how we expect users to add quarkus-kotlin if they plan to do Kotlin.
We can also easily warn about the missing extension at build time.

@michalszynkiewicz
Copy link
Member

We can require adding kube client without sharing the client between the discovery and the other places. If someone adds it now, it will all work, right @aureamunoz ?

@cescoffier
Copy link
Member

I just discussed this issue with @aureamunoz and @michalszynkiewicz

The plan of action is the following:

  • in the stroke processor, detect if the kubernetes service discovery is used (looking at the provider class on the classpath).
  • if it's used, detect if the kubernetes client extension is present (capability check)
  • if not -> fail the build and ask the user to add the kubernetes client extension

cescoffier added a commit to cescoffier/quarkus that referenced this issue May 4, 2022
Repository owner moved this from In Progress to Done in Quarkus Roadmap/Planning May 5, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 5, 2022
@gsmet gsmet modified the milestones: 2.10 - main, 2.9.1.Final May 12, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue May 12, 2022
…hout the kubernetes client

Fix quarkusio#24444

(cherry picked from commit 4e71579)
@edeandrea
Copy link
Contributor

Hey @cescoffier I just filed #25688. This creates a problem when using stork with the various kubernetes extensions if you have created any of your own kubernetes files in src/main/kubernetes. The resulting yamls generated by the combination of the kubernetes-client and the kubernetes extensions aren't deployable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
9 participants