-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
K3sContainer: expose kubeConfig for docker network access #5097
K3sContainer: expose kubeConfig for docker network access #5097
Conversation
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Outdated
Show resolved
Hide resolved
Thanks for providing the PR @johnathana. Besides the comment, it looks good to me, but we need someone with more k3s experience (such as @rnorth) to have another look 🙂. |
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Outdated
Show resolved
Hide resolved
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Outdated
Show resolved
Hide resolved
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Outdated
Show resolved
Hide resolved
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Outdated
Show resolved
Hide resolved
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Outdated
Show resolved
Hide resolved
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Outdated
Show resolved
Hide resolved
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Show resolved
Hide resolved
@@ -47,8 +49,8 @@ public K3sContainer(DockerImageName dockerImageName) { | |||
} | |||
|
|||
@SneakyThrows | |||
@Override | |||
protected void containerIsStarted(InspectContainerResponse containerInfo) { | |||
public String getKubeConfigYaml(String networkAlias) { |
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.
To avoid confusion, IMO we should rename this method to something that will tell that the returned config is for internal communication, as otherwise someone may call getKubeConfigYaml("k3s")
and then expect it to work from their machine (in addition to in-network usage)
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.
Yeah, my main thought with this PR is the risk of user confusion. This will be, I think, the first time in a module that we're outputting an address which is specifically not usable by the test host machine itself.
I think we need this method to be named to reflect that this returns a kubeconfig that is for use inside of the docker network.
We should also enhance the documentation here - both Javadocs and the md
docs, so that people can understand when to use this. As @bsideup suggested, including the test into the docs would be a good way to demonstrate it.
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.
@rnorth BTW how do you feel about changing from containerIsStarted
to "get file every time"? IMO getKubeConfigYaml
should return pre-calculated value from containerIsStarted
, while generateInternalKubeConfigYaml
(or whatever the name is) can do it on demand (or maybe from the same file, just by changing the host)
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.
I agree that getKubeConfigYaml
being a side-effect-free simple getter would feel better and be the more intuitive API.
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.
Yeah, I think it would be nicer to keep the fetch of the kubeconfig file in containerIsStarted
. It would make it clearer that the file is effectively immutable rather than something that k3s might be changing over time.
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.
I have updated the code. The documentation part is missing, I will include it on another commit.
modules/k3s/src/test/java/org/testcontainers/k3s/KubectlContainerTest.java
Outdated
Show resolved
Hide resolved
modules/k3s/src/main/java/org/testcontainers/k3s/K3sContainer.java
Outdated
Show resolved
Hide resolved
modules/k3s/src/main/java/org/testcontainers/k3s/K3sContainer.java
Outdated
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.
This is a nice PR - have just suggested a couple of trivial wording tweaks but am otherwise pretty happy with it.
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.
👍
Merged, thanks a lot for your contribution @johnathana 🙌 |
The
getKubeConfigYaml
method has been extended to expose the kubeConfig with a k3s container network alias as server URL. This would enable Kubernetes cluster access from other containers in the docker network.