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.

Sign up for GitHub

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

Jump to bottom

Created kubernetes client from openapi spec #762

Merged
merged 16 commits into from
Oct 17, 2024
Merged

Created kubernetes client from openapi spec #762

merged 16 commits into from
Oct 17, 2024
+3,028 −2

Conversation

msupic
Copy link
Contributor

@msupic msupic commented Oct 8, 2024

No description provided.

msupic
msupic commented Oct 8, 2024
View reviewed changes
...penapi/src/main/java/io/micronaut/kubernetes/client/openapi/KubernetesHttpClientFactory.java
protected DefaultHttpClient getKubernetesHttpClient() {
URI uri = URI.create(kubeConfig.getCluster().server());

return new DefaultHttpClient(LoadBalancer.fixed(uri),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will replace this with http client builder once micronaut-core 4.7.0 gets released

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this code now use HttpClientRegistry.getClient? Maybe we need to enhance that API to avoid references to these internal APIs? @yawkat WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HttpClientRegistry.getClient can't be used since we need a custom implementation of NettyClientSslBuilder (which is KubernetesClientSslBuilder). Once micronaut-core 4.7.0 gets released entire constructor will be replaced with just a few lines where we set KubernetesClientSslBuilder

@yawkat
Copy link
Member

yawkat commented Oct 8, 2024

i didnt look too much at the k8s specific parts since i am not familiar with it

graemerocher
graemerocher requested changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@graemerocher graemerocher left a comment

Choose a reason for hiding this comment

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

Looks like a good start, needs documentation

kubernetes-client-openapi/build.gradle Outdated Show resolved Hide resolved
kubernetes-client-openapi/build.gradle
testImplementation(mn.micronaut.http.server.netty)
}

tasks.register("prepareOpenapiSpec") {
Copy link
Contributor

Choose a reason for hiding this comment

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

@melix can you review this Gradle logic?

...penapi/src/main/java/io/micronaut/kubernetes/client/openapi/KubernetesHttpClientFactory.java Outdated Show resolved Hide resolved
...penapi/src/main/java/io/micronaut/kubernetes/client/openapi/KubernetesHttpClientFactory.java
protected DefaultHttpClient getKubernetesHttpClient() {
URI uri = URI.create(kubeConfig.getCluster().server());

return new DefaultHttpClient(LoadBalancer.fixed(uri),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this code now use HttpClientRegistry.getClient? Maybe we need to enhance that API to avoid references to these internal APIs? @yawkat WDYT?

...penapi/src/main/java/io/micronaut/kubernetes/client/openapi/KubernetesHttpClientFactory.java
registry.add(MediaType.APPLICATION_JSON_TYPE, new NettyCharSequenceBodyWriter());
registry.add(MediaType.APPLICATION_JSON_STREAM_TYPE, new NettyJsonStreamHandler<>(mapper));
return registry;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

would like to see how we can avoid this logic and recreating all these objects? //cc @yawkat

Copy link
Contributor Author

@msupic msupic Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

All those will be removed once micronaut-core 4.7.0 gets released since that version contains http client builder implementation

...api/src/main/java/io/micronaut/kubernetes/client/openapi/ssl/KubernetesClientSslBuilder.java Outdated Show resolved Hide resolved
...api/src/main/java/io/micronaut/kubernetes/client/openapi/ssl/KubernetesClientSslBuilder.java Outdated Show resolved Hide resolved
...api/src/main/java/io/micronaut/kubernetes/client/openapi/ssl/KubernetesClientSslBuilder.java Show resolved Hide resolved
...api/src/main/java/io/micronaut/kubernetes/client/openapi/ssl/KubernetesPrivateKeyLoader.java Outdated Show resolved Hide resolved
...penapi/src/test/groovy/io/micronaut/kubernetes/client/openapi/KubernetesTokenAuthSpec.groovy Outdated
null,
null,
null,
null)
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if there is a way to get the OpenAPI generator to generate overloaded variants that pass null to avoid having to write code like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andriy-dmytruk - do you maybe know answer to Graeme's above question?

Copy link

Choose a reason for hiding this comment

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

This is the only method signature that we generate. So far we just assume that calling a method is the simplest approach.

I guess we could generate another method that doesn't have any optional parameters. But if someone needs to set one optional parameter, they would need to use the full method and set all the others to null.

The OCI SDK client generates a separate class for each request that has a separate builder that allows ignoring all or some properties for the request. We could have an option to choose what to generate or even generate both. IMO that just complicates everything even more.

Copy link
Contributor

Choose a reason for hiding this comment

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

we would need to generate 1 overloaded method per optional parameter and pass null. So in this case 11 methods would be generated. These could be default methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note I don't think this should block this PR but we should look to improve later

@msupic
Copy link
Contributor Author

msupic commented Oct 10, 2024

Looks like a good start, needs documentation

Added docs. @graemerocher - please review

@msupic
Added kube config loader classes so loading of kube config can be cus…
6317e24 
…tomized, for example if kube config needs to be loaded from cloud service
graemerocher
graemerocher requested changes Oct 11, 2024
View reviewed changes
...pi/src/main/java/io/micronaut/kubernetes/client/openapi/config/AbstractKubeConfigLoader.java Outdated Show resolved Hide resolved
...pi/src/main/java/io/micronaut/kubernetes/client/openapi/config/AbstractKubeConfigLoader.java Show resolved Hide resolved
...api/src/main/java/io/micronaut/kubernetes/client/openapi/config/DefaultKubeConfigLoader.java Show resolved Hide resolved
...api/src/main/java/io/micronaut/kubernetes/client/openapi/config/DefaultKubeConfigLoader.java Show resolved Hide resolved
...s-client-openapi/src/main/java/io/micronaut/kubernetes/client/openapi/config/KubeConfig.java Outdated Show resolved Hide resolved
...nt-openapi/src/main/java/io/micronaut/kubernetes/client/openapi/config/KubeConfigLoader.java Show resolved Hide resolved
...main/java/io/micronaut/kubernetes/client/openapi/credential/ExecCommandCredentialLoader.java Outdated Show resolved Hide resolved
...penapi/src/main/java/io/micronaut/kubernetes/client/openapi/ssl/DefaultPrivateKeyLoader.java Show resolved Hide resolved
@msupic
Copy link
Contributor Author

msupic commented Oct 15, 2024

@graemerocher - I also implemented the service account authentication in this PR: 124d9ac
Since the service account authentication required refactoring of the existing classes I thought it was better to have that in this PR instead of creating a new PR later and having breaking changes.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 16, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
67.1% Coverage on New Code (required ≥ 70%)
2 New Bugs (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@msupic msupic merged commit 1c99ede into 6.2.x Oct 17, 2024
20 of 21 checks passed
@msupic msupic deleted the kubernetes-client-openapi branch October 17, 2024 07:17
@graemerocher graemerocher added the type: enhancement New feature or request label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yawkat yawkat yawkat approved these changes

@graemerocher graemerocher graemerocher approved these changes

@andriy-dmytruk andriy-dmytruk andriy-dmytruk approved these changes

Assignees
No one assigned
Labels
type: enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@msupic @yawkat @graemerocher @andriy-dmytruk