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

reload service account token after expiry #967

Merged
merged 1 commit into from
Jun 5, 2020
Merged

reload service account token after expiry #967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/src/main/java/io/kubernetes/client/examples/InClusterClientExample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ public static void main(String[] args) throws IOException, ApiException {
// 4. master endpoints(ip, port) from pre-set environment variables
ApiClient client = ClientBuilder.cluster().build();

// if you prefer not to refresh service account token, please use:
// ApiClient client = ClientBuilder.oldCluster().build();

// set the global default api-client to the in-cluster one from above
Configuration.setDefaultApiClient(client);

Expand Down
25 changes: 23 additions & 2 deletions util/src/main/java/io/kubernetes/client/util/ClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import io.kubernetes.client.util.credentials.AccessTokenAuthentication;
import io.kubernetes.client.util.credentials.Authentication;
import io.kubernetes.client.util.credentials.KubeconfigAuthentication;
import io.kubernetes.client.util.credentials.TokenFileAuthentication;
import java.io.BufferedReader;
import java.io.ByteArrayInputStream;
import java.io.File;
Expand Down Expand Up @@ -197,12 +198,12 @@ private static File findConfigInHomeDir() {
}

/**
* Creates a builder which is pre-configured from the cluster configuration.
* [DEPRECATED] Creates a builder which is pre-configured from the cluster configuration.
*
* @return <tt>ClientBuilder</tt> configured from the cluster configuration.
* @throws IOException if the Service Account Token Path or CA Path is not readable.
*/
public static ClientBuilder cluster() throws IOException {
public static ClientBuilder oldCluster() throws IOException {
final ClientBuilder builder = new ClientBuilder();

final String host = System.getenv(ENV_SERVICE_HOST);
Expand All @@ -218,6 +219,26 @@ public static ClientBuilder cluster() throws IOException {
return builder;
}

/**
* Creates a builder which is pre-configured from the cluster configuration.
*
* @return <tt>ClientBuilder</tt> configured from the cluster configuration where service account
* token will be reloaded.
* @throws IOException if the Service Account Token Path or CA Path is not readable.
*/
public static ClientBuilder cluster() throws IOException {
final ClientBuilder builder = new ClientBuilder();

final String host = System.getenv(ENV_SERVICE_HOST);
final String port = System.getenv(ENV_SERVICE_PORT);
builder.setBasePath(host, port);

builder.setCertificateAuthority(Files.readAllBytes(Paths.get(SERVICEACCOUNT_CA_PATH)));
builder.setAuthentication(new TokenFileAuthentication(SERVICEACCOUNT_TOKEN_PATH));
Copy link
Member

@yue9944882 yue9944882 Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

we need to keep backward compatibility and users are expected to opt-in to the change. wdyt add a constructor w/ parameter reloadInterval and factor the original cluster() to adopt an infinite (or zero) interval?

NOTE: even tho minute-ish reloading SA is always recommended and users are even supposed to be not aware of the change, i suppose it will take a release or two for users to get onboard to the SA rotation..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cluster() with reloading should be the default so that users dont have to do anything to migrate and add a oldCluster as the fail safe.

update the InClusterClusterExample to something like:

// new style
ApiClient client = ClientBuilder.cluster().build();

// old style
ApiClient client = ClientBuilder.oldCluster().build();

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

cluster() with reloading should be the default so that users dont have to do anything to migrate and add a oldCluster as the fail safe.

yea we should also reflect how to opt-in SA reloading in the example. note that there's two aspects of change the pull is involving:

  1. introducing capability for periodical SA reloading
  2. moving default behavior the in-cluster client to 1-min reloading (preserving the old-style method or not)

the first is good while i'm leaning on doing the second in another release. i will defer to @brendandburns for thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just do the whole thing. We haven't cut a 9.0.0 release yet so I think it's ok to make this change in the 9.0.0 release.


return builder;
}

protected ClientBuilder setBasePath(String host, String port) {
try {
Integer iPort = Integer.valueOf(port);
Expand Down
62 changes: 62 additions & 0 deletions util/src/main/java/io/kubernetes/client/util/credentials/TokenFileAuthentication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package io.kubernetes.client.util.credentials;

import io.kubernetes.client.openapi.ApiClient;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.time.Instant;
import okhttp3.Interceptor;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;

// TODO: prefer OpenAPI backed Auentication once it is available. see details in
// https://github.com/OpenAPITools/openapi-generator/pull/6036. currently, the
// workaround is to hijack the http request.
public class TokenFileAuthentication implements Authentication, Interceptor {
Copy link
Member

Choose a reason for hiding this comment

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

i presume using Interceptor to hijack the header is a bit overkilling.. the previous approach by AccessTokenAuthentication is setting tokens by ((ApiKeyAuth) auth).setApiKey(apiKey); in the provide call, wdyt if we do the token-reloading similarly?

Copy link
Contributor Author

@zshihang zshihang Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

the current openapi doesn't have reloading logic in ApiKeyAuth which will only be set once in the initialization of in-cluster client. i couldn't figure out a way to achieve what you described without making changes to openapi.

we need something similar to this change OpenAPITools/openapi-generator#6036 (it might take much longer time to merge) to implement reloading.

Copy link
Member

Choose a reason for hiding this comment

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

the current openapi doesn't have reloading logic in ApiKeyAuth which will only be set once in the initialization of in-cluster client.

convinced, can you leave a TODO comment to reference OpenAPITools/openapi-generator#6036 so that we can converge in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, done

private String file;
private String token;
private Instant expiry;

public TokenFileAuthentication(String file) {
this.expiry = Instant.MIN;
this.file = file;
}

private String getToken() {
if (Instant.now().isAfter(this.expiry)) {
try {
this.token =
new String(Files.readAllBytes(Paths.get(this.file)), Charset.defaultCharset()).trim();
expiry = Instant.now().plusSeconds(60);
} catch (IOException ie) {
throw new RuntimeException("Cannot read file: " + this.file);
}
}

return this.token;
}

public void setExpiry(Instant expiry) {
this.expiry = expiry;
}

public void setFile(String file) {
this.file = file;
}

@Override
public void provide(ApiClient client) {
OkHttpClient withInterceptor = client.getHttpClient().newBuilder().addInterceptor(this).build();
Copy link
Member

Choose a reason for hiding this comment

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

the # of interceptors seems to be growing unboundly upon every provide call, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the assumption is made that the provide function is called only once for each in-cluster client. i am not sure when a user will call this twice on the same client instance.

Copy link
Member

Choose a reason for hiding this comment

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

the assumption is made that the provide function is called only once for each in-cluster client.

yea, that's true

client.setHttpClient(withInterceptor);
}

@Override
public Response intercept(Interceptor.Chain chain) throws IOException {
Request request = chain.request();
Request newRequest;
newRequest = request.newBuilder().header("Authorization", "Bearer " + getToken()).build();
return chain.proceed(newRequest);
}
}
63 changes: 63 additions & 0 deletions util/src/test/java/io/kubernetes/client/util/credentials/TokenFileAuthenticationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package io.kubernetes.client.util.credentials;

import static com.github.tomakehurst.wiremock.client.WireMock.*;

import com.github.tomakehurst.wiremock.client.WireMock;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import com.google.common.io.Resources;
import io.kubernetes.client.openapi.ApiClient;
import io.kubernetes.client.openapi.ApiException;
import io.kubernetes.client.openapi.Configuration;
import io.kubernetes.client.openapi.apis.CoreV1Api;
import java.io.IOException;
import java.time.Instant;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;

public class TokenFileAuthenticationTest {
private static final String SERVICEACCOUNT_TOKEN1_PATH =
Resources.getResource("token1").getPath();
private static final String SERVICEACCOUNT_TOKEN2_PATH =
Resources.getResource("token2").getPath();
private static final int PORT = 8089;
private TokenFileAuthentication auth;

@Rule public WireMockRule wireMockRule = new WireMockRule(PORT);

@Before
public void setup() throws IOException {
final ApiClient client = new ApiClient();
client.setBasePath("http://localhost:" + PORT);
this.auth = new TokenFileAuthentication(SERVICEACCOUNT_TOKEN1_PATH);
this.auth.provide(client);
Configuration.setDefaultApiClient(client);
}

@Test
public void testTokenProvided() throws IOException, ApiException {
stubFor(
get(urlPathEqualTo("/api/v1/pods")).willReturn(okForContentType("application/json", "{}")));
CoreV1Api api = new CoreV1Api();

api.listPodForAllNamespaces(null, null, null, null, null, null, null, null, null);
WireMock.verify(
1,
getRequestedFor(urlPathEqualTo("/api/v1/pods"))
.withHeader("Authorization", equalTo("Bearer token1")));

this.auth.setFile(SERVICEACCOUNT_TOKEN2_PATH);
api.listPodForAllNamespaces(null, null, null, null, null, null, null, null, null);
WireMock.verify(
2,
getRequestedFor(urlPathEqualTo("/api/v1/pods"))
.withHeader("Authorization", equalTo("Bearer token1")));

this.auth.setExpiry(Instant.now().minusSeconds(1));
api.listPodForAllNamespaces(null, null, null, null, null, null, null, null, null);
WireMock.verify(
1,
getRequestedFor(urlPathEqualTo("/api/v1/pods"))
.withHeader("Authorization", equalTo("Bearer token2")));
}
}
1 change: 1 addition & 0 deletions util/src/test/resources/token1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
token1
1 change: 1 addition & 0 deletions util/src/test/resources/token2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
token2