#238: support client.authentication.k8s.io/v1beta1 ExecCredential #512

jglick · 2019-02-20T17:27:53Z

Implementation of #238 allowing ~/.kube/config to use the new generic exec credentials rather than a vendor-specific plugin or hard-coded token.

Planning to do before taking out of “draft” status:

basic implementation (run a command, get a token)
observed working with GKE
KubeConfigTest addition
relativization to basedir
environment variables
more robustness against bad inputs, logging, etc.

Other things which I think could be left for follow-ups depending on demand:

observed working with EKS (I could make a test cluster if necessary)
time-based expiry (see GCP authentication does not support refreshing tokens #290 (comment) or inline comments for why this would require broader refactoring)
401-based expiry (even trickier)
client certificate support
better exception handling in getAccessToken as per #238: support client.authentication.k8s.io/v1beta1 ExecCredential #512 (comment)

…ication.k8s.io/v1beta1.

brendandburns · 2019-02-20T23:09:55Z

util/src/main/java/io/kubernetes/client/util/KubeConfig.java

+    Object exec = currentUser.get("exec");
+    if (exec != null) {
+      // TODO extract to helper method for clarity
+      Map<String, Object> execMap = (Map<String, Object>) exec;


It's probably better to declare an actual Java object that represents the exec field rather than using generic JSON accessors, it makes the code pretty ugly.

(I realized I'm being hypocritical given the above, so feel free to ignore this...)

hypocritical given the above

You mean the authProviderMap code?

After years of bad experiences I have a general preference for plain old Java code over use of fancy reflection-based (de)serialization libraries, but if you have a strong preference for some particular approach (GSON?) I will follow your guidelines.

@jglick - is the item "observed working with EKS (I could make a test cluster if necessary)" still open, and if not, could I help out with that?

Yes it is still open. If you are using EKS, please help test this system.

util/src/main/java/io/kubernetes/client/util/KubeConfig.java

brendandburns · 2019-02-20T23:12:36Z

util/src/main/java/io/kubernetes/client/util/KubeConfig.java

+          if (r == 0) {
+            if (root != null) {
+              // TODO verify .apiVersion and .kind = ExecCredential
+              JsonObject status = root.getAsJsonObject().get("status").getAsJsonObject();


Let's create a real Java object for this response value too.

As above, feel free to skip if you want.

util/src/main/java/io/kubernetes/client/util/KubeConfig.java

brendandburns · 2019-02-20T23:21:35Z

Thanks for starting this! I have a few comments, also this needs unit tests.

https://google.github.io/styleguide/javaguide.html#s4.4-column-limit explicitly exempts > Lines where obeying the column limit is not possible (for example, a long URL in Javadoc but evidently google-java-format does not understand this.

…h test runs here.

util/src/main/java/io/kubernetes/client/util/KubeConfig.java

brendandburns · 2019-03-07T06:15:26Z

util/src/main/java/io/kubernetes/client/util/KubeConfig.java

+      Process proc = pb.start();
+      JsonElement root;
+      try (InputStream is = proc.getInputStream();
+          Reader r = new InputStreamReader(is, StandardCharsets.UTF_8)) {


indentation is weird here, is this really correct?

What looks incorrect? I do not have a choice about indentation—it is whatever mvn fmt:format picks.

ok, the 'R' aligned with the '(' looks weird to me, but if mvn:format says so, I guess it's right :P

Yeah my preference would be to line up the I and the R.

brendandburns · 2019-03-07T06:16:48Z

util/src/main/java/io/kubernetes/client/util/KubeConfig.java

+        return null;
+      }
+      return root;
+    } catch (IOException | InterruptedException x) {


why swallow the exception here? wouldn't you rather throw?

It is not swallowed—it is sent to the log.

As to whether it is appropriate to throw exceptions out of getAccessToken (presumably wrapped in RuntimeException?) as opposed to logging and proceeding with other authentication modes and ultimately returning null (unauthenticated), I have no strong opinion. I was following the example of existing code which proceeds despite errors (Failed to read token file for example).

On the other hand AzureActiveDirectoryAuthenticator.refresh appears to throw an undeclared exception out of getAccessToken. KubeconfigAuthentication.<init> is permitted to throw IOException so perhaps getAccessToken should as well? That would mean reworking some existing code which catches and logs exceptions, and adding IOException to the signature of Authenticator.refresh.

ok, sounds like it's messy, let's take it as a follow-on cleanup exercise...

Noted in PR description.

brendandburns · 2019-03-07T06:17:25Z

Looks pretty good, some minor nits.

brendandburns · 2019-03-07T22:22:17Z

/lgtm
/approve

Thanks!

k8s-ci-robot · 2019-03-07T22:22:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, jglick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [brendandburns]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Draft implementation of kubernetes-client#238: support client.authent…

28b1732

…ication.k8s.io/v1beta1.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 20, 2019

k8s-ci-robot requested review from brendandburns and mbohlool February 20, 2019 17:27

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 20, 2019

brendandburns reviewed Feb 20, 2019

View reviewed changes

util/src/main/java/io/kubernetes/client/util/KubeConfig.java Outdated Show resolved Hide resolved

brendandburns reviewed Feb 20, 2019

View reviewed changes

util/src/main/java/io/kubernetes/client/util/KubeConfig.java Outdated Show resolved Hide resolved

brendandburns reviewed Feb 20, 2019

View reviewed changes

util/src/main/java/io/kubernetes/client/util/KubeConfig.java Outdated Show resolved Hide resolved

jglick added 4 commits March 4, 2019 15:06

Extracted method with multiple return values for clarity.

d177116

Struggling to stay within the 100-column limit.

0bc22e6

https://google.github.io/styleguide/javaguide.html#s4.4-column-limit explicitly exempts > Lines where obeying the column limit is not possible (for example, a long URL in Javadoc but evidently google-java-format does not understand this.

https://issues.apache.org/jira/browse/SUREFIRE-1226 can interfere wit…

0f8c548

…h test runs here.

Initial test of exec credentials.

0c3ca98

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 4, 2019

jglick added 5 commits March 4, 2019 16:12

Handle environment variables.

a4163e2

Honoring basedir for relative command paths.

2d22dce

Also handle client.authentication.k8s.io/v1alpha1.

558864f

Verify .kind & .apiVersion.

b2f9332

Added debug statement to confirm that a token was at last obtained.

93b87f4

jglick marked this pull request as ready for review March 6, 2019 17:07

brendandburns reviewed Mar 7, 2019

View reviewed changes

util/src/main/java/io/kubernetes/client/util/KubeConfig.java Outdated Show resolved Hide resolved

brendandburns reviewed Mar 7, 2019

View reviewed changes

Broader interpretation of “relative command path”.

f843269

jglick force-pushed the ExecCredential branch from 24930b4 to f843269 Compare March 7, 2019 15:11

k8s-ci-robot assigned brendandburns Mar 7, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2019

brendandburns removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2019

k8s-ci-robot merged commit 0abbd01 into kubernetes-client:master Mar 8, 2019

jglick deleted the ExecCredential branch March 8, 2019 19:35

jglick mentioned this pull request Jun 21, 2019

Port exec-based client auth provider to Java client #238

Closed

saskiawagenaar mentioned this pull request Oct 16, 2019

Client library doesn't support AWS IaM authentication. #315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#238: support client.authentication.k8s.io/v1beta1 ExecCredential #512

#238: support client.authentication.k8s.io/v1beta1 ExecCredential #512

jglick commented Feb 20, 2019 •

edited

Loading

brendandburns Feb 20, 2019

brendandburns Feb 20, 2019

jglick Feb 25, 2019

pklewing Jun 27, 2019

jglick Jul 1, 2019

brendandburns Feb 20, 2019

brendandburns Feb 20, 2019

jglick Feb 25, 2019

brendandburns commented Feb 20, 2019

brendandburns Mar 7, 2019

jglick Mar 7, 2019

brendandburns Mar 7, 2019

jglick Mar 8, 2019

brendandburns Mar 7, 2019

jglick Mar 7, 2019

brendandburns Mar 7, 2019

jglick Mar 8, 2019

brendandburns commented Mar 7, 2019

brendandburns commented Mar 7, 2019

k8s-ci-robot commented Mar 7, 2019

#238: support client.authentication.k8s.io/v1beta1 ExecCredential #512

#238: support client.authentication.k8s.io/v1beta1 ExecCredential #512

Conversation

jglick commented Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Mar 7, 2019

brendandburns commented Mar 7, 2019

k8s-ci-robot commented Mar 7, 2019

jglick commented Feb 20, 2019 •

edited

Loading