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

[JENKINS-49817] Support kubeconfig from secretFile credentials #294

Merged
merged 3 commits into from
Mar 20, 2018
Merged

[JENKINS-49817] Support kubeconfig from secretFile credentials #294

merged 3 commits into from
Mar 20, 2018

Conversation

colemickens
Copy link
Contributor

@colemickens colemickens commented Mar 13, 2018 •
edited
Loading

Note: this is a WIP because it is blocked by this PR on the fabric8 kubernetes-client: config: support loading from kubeconfig string.

* Updates to the 3.x version of the kubernetes-client package

  • Supports kubeconfig files (secretFile credentails) as cluster credentials

I have (minimally) tested it, however. Sending it now so people can take a look. I'll update the title when it's no longer blocked.

This is my preferred way of handling all credentials for kubernetes clusters, whenever the credentials are "portable" and sealed. For example, when producing limiting service accounts in remote namespaces and then generating kubeconfigs for them.

Fixes: JENKINS-49817

@colemickens
Copy link
Contributor Author

The code was merged in kubernetes-client. Just need to wait for a release and then I'll update this PR.

@carlossg
Copy link
Contributor

so this depends on #271

@colemickens
Copy link
Contributor Author

colemickens commented Mar 13, 2018 •
edited
Loading

Great! I hadn't seen that you'd sent a PR for that already. I had patched up any other API breakages myself in this PR as well.

Is there anything I can do to help with #271 in this case? Never mind, I see you're waiting on a release as well.

@colemickens colemickens force-pushed the kubeconfig-from-secretfile branch 3 times, most recently from da07e9a to acc68a8 Compare March 16, 2018 18:26
@colemickens
Copy link
Contributor Author

Hello @carlossg. The fabric8 folks have released 3.1.10. I have updated this PR to consume this package.

mvn clean install is working, all tests are passing.

@colemickens colemickens changed the title WIP: Bump to 3.x of kubernetes-client & support kubeconfig from secretFile creds Bump kubernetes-client to v3.1.10; Support kubeconfig from secretFile creds Mar 16, 2018
@colemickens
Copy link
Contributor Author

(Updating PR title and description, it's no longer a WIP.)

@carlossg
Copy link
Contributor

carlossg commented Mar 16, 2018 •
edited
Loading

you would need to also add the option to

And add some tests

@carlossg carlossg changed the title Bump kubernetes-client to v3.1.10; Support kubeconfig from secretFile creds Support kubeconfig from secretFile credentials Mar 16, 2018
@colemickens
Copy link
Contributor Author

colemickens commented Mar 17, 2018 •
edited
Loading

The change to doFillCredentialsIdItems is already in the commit though? Or else I don't know what else is needed? (it looks like there's two. I had added it to KubernetesCloud already. I've added it in KubectlBuildWrapper as well, now that kubeconfig works there too.)

What is KubectlBuildWrapper? How do I sanity test a change to it?

I'll work on tests.

@colemickens
Copy link
Contributor Author

colemickens commented Mar 17, 2018 •
edited
Loading

Actually, I see what KubectlBuildWrapper is, it already works with kubeconfig so this is straightforward.

Any pointers on what test(s) you are looking for? The net change boils down to calling a single method from the client package and I don't see any existing auth-related tests.

@colemickens colemickens force-pushed the kubeconfig-from-secretfile branch 2 times, most recently from e467be5 to 26ee3e2 Compare March 17, 2018 09:20
@colemickens
Copy link
Contributor Author

With the latest commit, I have tested a declarative pipeline and a freestyle script with the kubectl wrapper. Both are working as expected.

carlossg
carlossg reviewed Mar 19, 2018
View reviewed changes
src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesFactoryAdapter.java Outdated
String kubeconfigContents = reader.lines().collect(Collectors.joining("\n"));
Config config = Config.fromKubeconfig(kubeconfigContents);
builder = new ConfigBuilder(config);
reader.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

the block should be in a try/catch/finally

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubectlBuildWrapper.java Outdated
BufferedReader reader = new BufferedReader(new InputStreamReader(configStream, StandardCharsets.UTF_8));
String kubeconfigContents = reader.lines().collect(Collectors.joining("\n"));
configFile.write(kubeconfigContents, null);
reader.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

the block should be in a try/catch/finally

@colemickens colemickens force-pushed the kubeconfig-from-secretfile branch from 1b7f3f0 to a565ff6 Compare March 20, 2018 08:05
@colemickens
Copy link
Contributor Author

I fixed up both of those spots.

@carlossg carlossg changed the title Support kubeconfig from secretFile credentials [JENKINS-49817] Support kubeconfig from secretFile credentials Mar 20, 2018
@carlossg
Use try with resources
4ab127c 
and reorder Config creation flow
@carlossg
Copy link
Contributor

I have switched to try with resources and simplify a bit the code.

@colemickens
Copy link
Contributor Author

Thanks @carlossg, I like the commit you made.

@carlossg carlossg merged commit d76e72d into jenkinsci:master Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlossg carlossg carlossg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@colemickens @carlossg