From 83b2f437fb40c3c2d1f64e7cc67bf80770355226 Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Tue, 25 Oct 2022 09:00:50 -0400 Subject: [PATCH] fix #4159: correcting how the config refreshes --- CHANGELOG.md | 1 + .../io/fabric8/kubernetes/client/Config.java | 64 +++++++++++++---- .../client/utils/TokenRefreshInterceptor.java | 3 +- .../fabric8/kubernetes/client/ConfigTest.java | 23 +++++- .../utils/TokenRefreshInterceptorTest.java | 71 ++++++++----------- 5 files changed, 105 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d951050132..16caeb82ca0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### 6.3-SNAPSHOT #### Bugs +* Fix #4159: ensure the token refresh obeys how the Config was created #### Improvements diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java index 7500c2c6900..5a0609af9b0 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java @@ -230,7 +230,7 @@ public class Config { */ private Map customHeaders = null; - private Boolean autoConfigure = Boolean.FALSE; + private boolean autoConfigure; private File file; @@ -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); } } @@ -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) { @@ -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)) { @@ -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() { @@ -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; @@ -1365,7 +1393,7 @@ public void setCustomHeaders(Map customHeaders) { this.customHeaders = customHeaders; } - public Boolean getAutoConfigure() { + public boolean getAutoConfigure() { return autoConfigure; } @@ -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; + } + } diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java index f4e4347059d..107752c415c 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java @@ -72,8 +72,7 @@ public CompletableFuture afterFailure(BasicBuilder headerBuilder, HttpR } private CompletableFuture 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 newAccessToken = extractNewAccessTokenFrom(newestConfig); return newAccessToken.thenApply(token -> overrideNewAccessTokenToConfig(token, headerBuilder, config)); diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java index de1a257f496..76bfdf76954 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java @@ -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 { @@ -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"); @@ -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 diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptorTest.java index cf95ec10bf4..afe0d51c256 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptorTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptorTest.java @@ -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; @@ -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 @@ -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 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 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