Skip to content

Commit

Permalink
fix fabric8io#4159: correcting how the config refreshes
Browse files Browse the repository at this point in the history
  • Loading branch information
@shawkins
shawkins committed Oct 25, 2022
1 parent 7a45516 commit 83b2f43
Show file tree
Hide file tree
Showing 5 changed files with 105 additions and 57 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### 6.3-SNAPSHOT

#### Bugs
* Fix #4159: ensure the token refresh obeys how the Config was created

#### Improvements

Expand Down
64 changes: 50 additions & 14 deletions kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ public class Config {
*/
private Map<String, String> customHeaders = null;

private Boolean autoConfigure = Boolean.FALSE;
private boolean autoConfigure;

private File file;

Expand All @@ -242,12 +242,15 @@ public class Config {
*/
@Deprecated
public Config() {
this(!Utils.getSystemPropertyOrEnvVar(KUBERNETES_DISABLE_AUTO_CONFIG_SYSTEM_PROPERTY, false));
this(!disableAutoConfig());
}

private Config(Boolean autoConfigure) {
if (Boolean.TRUE.equals(autoConfigure)) {
this.autoConfigure = Boolean.TRUE;
private static boolean disableAutoConfig() {
return Utils.getSystemPropertyOrEnvVar(KUBERNETES_DISABLE_AUTO_CONFIG_SYSTEM_PROPERTY, false);
}

private Config(boolean autoConfigure) {
if (autoConfigure) {
autoConfigure(this, null);
}
}
Expand Down Expand Up @@ -287,12 +290,16 @@ private static Config autoConfigure(Config config, String context) {
tryServiceAccount(config);
tryNamespaceFromPath(config);
}
postAutoConfigure(config);
config.autoConfigure = true;
return config;
}

private static void postAutoConfigure(Config config) {
configFromSysPropsOrEnvVars(config);

config.masterUrl = ensureHttps(config.masterUrl, config);
config.masterUrl = ensureEndsWithSlash(config.masterUrl);

return config;
}

private static String ensureEndsWithSlash(String masterUrl) {
Expand Down Expand Up @@ -590,13 +597,35 @@ public static Config fromKubeconfig(String kubeconfigContents) {
// Note: kubeconfigPath is optional (see note on loadFromKubeConfig)
public static Config fromKubeconfig(String context, String kubeconfigContents, String kubeconfigPath) {
// we allow passing context along here, since downstream accepts it
Config config = new Config();
if (kubeconfigPath != null)
Config config = new Config(false);
if (kubeconfigPath != null) {
config.file = new File(kubeconfigPath);
loadFromKubeconfig(config, context, kubeconfigContents);
}
if (!loadFromKubeconfig(config, context, kubeconfigContents)) {
throw new KubernetesClientException("Could not create Config from kubeconfig");
}
if (!disableAutoConfig()) {
postAutoConfigure(config);
}
return config;
}

public Config refresh() {
final String currentContextName = this.getCurrentContext() != null ? this.getCurrentContext().getName() : null;
if (this.autoConfigure) {
return Config.autoConfigure(currentContextName);
}
if (this.file != null) {
String kubeconfigContents = getKubeconfigContents(this.file);
if (kubeconfigContents == null) {
return this; // getKubeconfigContents will have logged an exception
}
return Config.fromKubeconfig(currentContextName, kubeconfigContents, this.file.getPath());
}
// nothing to refresh - the kubeconfig was directly supplied
return this;
}

private static boolean tryKubeConfig(Config config, String context) {
LOGGER.debug("Trying to configure client from Kubernetes config...");
if (!Utils.getSystemPropertyOrEnvVar(KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, true)) {
Expand All @@ -613,8 +642,7 @@ private static boolean tryKubeConfig(Config config, String context) {
return false;
}
config.file = new File(kubeConfigFile.getPath());
loadFromKubeconfig(config, context, kubeconfigContents);
return true;
return loadFromKubeconfig(config, context, kubeconfigContents);
}

public static String getKubeconfigFilename() {
Expand Down Expand Up @@ -715,7 +743,7 @@ private static boolean loadFromKubeconfig(Config config, String context, String
return true;
}
} catch (Exception e) {
LOGGER.error("Failed to parse the kubeconfig.", e);
throw KubernetesClientException.launderThrowable("Failed to parse the kubeconfig.", e);
}

return false;
Expand Down Expand Up @@ -1365,7 +1393,7 @@ public void setCustomHeaders(Map<String, String> customHeaders) {
this.customHeaders = customHeaders;
}

public Boolean getAutoConfigure() {
public boolean getAutoConfigure() {
return autoConfigure;
}

Expand Down Expand Up @@ -1432,4 +1460,12 @@ public void setAdditionalProperty(String name, Object value) {
this.additionalProperties.put(name, value);
}

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

public void setAutoConfigure(boolean autoConfigure) {
this.autoConfigure = autoConfigure;
}

}
3 changes: 1 addition & 2 deletions ...-client-api/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ public CompletableFuture<Boolean> afterFailure(BasicBuilder headerBuilder, HttpR
}

private CompletableFuture<Boolean> refreshToken(BasicBuilder headerBuilder) {
final String currentContextName = config.getCurrentContext() != null ? config.getCurrentContext().getName() : null;
final Config newestConfig = Config.autoConfigure(currentContextName);
Config newestConfig = config.refresh();
final CompletableFuture<String> newAccessToken = extractNewAccessTokenFrom(newestConfig);

return newAccessToken.thenApply(token -> overrideNewAccessTokenToConfig(token, headerBuilder, config));
Expand Down
23 changes: 22 additions & 1 deletion kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,13 @@
import java.util.List;
import java.util.Map;

import static org.junit.Assert.assertNotSame;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class ConfigTest {
Expand Down Expand Up @@ -242,6 +244,21 @@ void testMasterUrlWithServiceAccount() {
assertEquals(null, config.getFile());
}

@Test
void testAutoConfig() {
System.setProperty(Config.KUBERNETES_KUBECONFIG_FILE, "/dev/null");
Config config = Config.autoConfigure(null);
assertNotNull(config);
assertNull(config.getFile());
assertTrue(config.getAutoConfigure());

// ensure that refresh creates a new instance
Config refresh = config.refresh();
assertNotSame(config, refresh);
assertNull(refresh.getFile());
assertTrue(refresh.getAutoConfigure());
}

@Test
void testMasterUrlWithServiceAccountIPv6() {
System.setProperty(Config.KUBERNETES_KUBECONFIG_FILE, "/dev/null");
Expand Down Expand Up @@ -328,6 +345,10 @@ void testFromKubeconfigContent() throws IOException {
final String configYAML = String.join("\n", Files.readAllLines(configFile.toPath()));
final Config config = Config.fromKubeconfig(configYAML);
assertEquals("https://172.28.128.4:8443/", config.getMasterUrl());

assertFalse(config.getAutoConfigure());
assertNull(config.getFile());
assertSame(config, config.refresh());
}

@Test
Expand Down
71 changes: 31 additions & 40 deletions ...ent-api/src/test/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import io.fabric8.kubernetes.client.http.TestHttpResponse;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;

import java.io.File;
Expand All @@ -39,9 +38,7 @@
import static io.fabric8.kubernetes.client.Config.KUBERNETES_KUBECONFIG_FILE;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.CALLS_REAL_METHODS;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.never;

/**
* Ignoring for now - the token refresh should be based upon the java 11 client or the provided client library and not okhttp
Expand Down Expand Up @@ -100,47 +97,41 @@ void shouldAutoconfigureAfter1Minute() throws Exception {
@Test
@DisplayName("#4442 token auto refresh should not overwrite existing token when not applicable")
void refreshShouldNotOverwriteExistingToken() throws Exception {
try (MockedStatic<Config> configMock = mockStatic(Config.class, CALLS_REAL_METHODS)) {
// Given
final Config autoConfig = new ConfigBuilder(Config.empty())
.withOauthToken("") // empty token
.build();
configMock.when(() -> Config.autoConfigure(any())).thenReturn(autoConfig);
final Config config = new ConfigBuilder(Config.empty())
.withOauthToken("existing-token")
.build();
final TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(
config, null, Instant.now().minusSeconds(61));
// When
final boolean result = tokenRefreshInterceptor
.afterFailure(new StandardHttpRequest.Builder(), new TestHttpResponse<>().withCode(401)).get();
// Then
assertThat(result).isFalse();
assertThat(config).hasFieldOrPropertyWithValue("oauthToken", "existing-token");
}
// Given
final Config autoConfig = new ConfigBuilder(Config.empty())
.withOauthToken("") // empty token
.build();
final Config config = Mockito.mock(Config.class);
Mockito.when(config.getOauthToken()).thenReturn("existing-token");
Mockito.when(config.refresh()).thenReturn(autoConfig);
final TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(
config, null, Instant.now().minusSeconds(61));
// When
final boolean result = tokenRefreshInterceptor
.afterFailure(new StandardHttpRequest.Builder(), new TestHttpResponse<>().withCode(401)).get();
// Then
assertThat(result).isFalse();
Mockito.verify(config, never()).setOauthToken("new-token");
}

@Test
@DisplayName("#4442 token auto refresh should overwrite existing token when applicable")
void refreshShouldOverwriteExistingToken() throws Exception {
try (MockedStatic<Config> configMock = mockStatic(Config.class, CALLS_REAL_METHODS)) {
// Given
final Config autoConfig = new ConfigBuilder(Config.empty())
.withOauthToken("new-token")
.build();
configMock.when(() -> Config.autoConfigure(any())).thenReturn(autoConfig);
final Config config = new ConfigBuilder(Config.empty())
.withOauthToken("existing-token")
.build();
final TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(
config, null, Instant.now().minusSeconds(61));
// When
final boolean result = tokenRefreshInterceptor
.afterFailure(new StandardHttpRequest.Builder(), new TestHttpResponse<>().withCode(401)).get();
// Then
assertThat(result).isTrue();
assertThat(config).hasFieldOrPropertyWithValue("oauthToken", "new-token");
}
// Given
final Config autoConfig = new ConfigBuilder(Config.empty())
.withOauthToken("new-token")
.build();
final Config config = Mockito.mock(Config.class);
Mockito.when(config.getOauthToken()).thenReturn("existing-token");
Mockito.when(config.refresh()).thenReturn(autoConfig);
final TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(
config, null, Instant.now().minusSeconds(61));
// When
final boolean result = tokenRefreshInterceptor
.afterFailure(new StandardHttpRequest.Builder(), new TestHttpResponse<>().withCode(401)).get();
// Then
assertThat(result).isTrue();
Mockito.verify(config).setOauthToken("new-token");
}

@Test
Expand Down

0 comments on commit 83b2f43

Please sign in to comment.