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

fix #4519: correcting how the config refreshes #4528

Merged
merged 2 commits into from Nov 8, 2022
Merged

fix #4519: correcting how the config refreshes #4528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31efb15
fix #4519: correcting how the config refreshes
shawkins Oct 25, 2022
7964f14
test: remove unneeded mocks for #4442 tests
manusa Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
### 6.3-SNAPSHOT

#### Bugs
* Fix #4159: ensure the token refresh obeys how the Config was created
* Fix #4491: added a more explicit shutdown exception for okhttp
* Fix #4534: Java Generator CLI default handling of skipGeneratedAnnotations
* Fix #4535: The shell command string will now have single quotes sanitized
Expand Down
64 changes: 50 additions & 14 deletions kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java
View file
Open in desktop
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;
}
Comment on lines +1463 to +1469
Copy link
Member

Choose a reason for hiding this comment

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

Why are the setters needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that they will get set when we clone via the builder. We create modified configs in special circumstances, in particular with the withRequestConfig calls.

Copy link
Member

Choose a reason for hiding this comment

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

From the generated io.fabric8.kubernetes.client.ConfigBuilder:

  public Config build() {
    Config buildable = new Config(fluent.getMasterUrl(),fluent.getApiVersion(),fluent.getNamespace(),fluent.isTrustCerts(),fluent.isDisableHostnameVerification(),fluent.getCaCertFile(),fluent.getCaCertData(),fluent.getClientCertFile(),fluent.getClientCertData(),fluent.getClientKeyFile(),fluent.getClientKeyData(),fluent.getClientKeyAlgo(),fluent.getClientKeyPassphrase(),fluent.getUsername(),fluent.getPassword(),fluent.getOauthToken(),fluent.getWatchReconnectInterval(),fluent.getWatchReconnectLimit(),fluent.getConnectionTimeout(),fluent.getRequestTimeout(),fluent.getRollingTimeout(),fluent.getScaleTimeout(),fluent.getLoggingInterval(),fluent.getMaxConcurrentRequests(),fluent.getMaxConcurrentRequestsPerHost(),fluent.isHttp2Disable(),fluent.getHttpProxy(),fluent.getHttpsProxy(),fluent.getNoProxy(),fluent.getErrorMessages(),fluent.getUserAgent(),fluent.getTlsVersions(),fluent.getWebsocketTimeout(),fluent.getWebsocketPingInterval(),fluent.getProxyUsername(),fluent.getProxyPassword(),fluent.getTrustStoreFile(),fluent.getTrustStorePassphrase(),fluent.getKeyStoreFile(),fluent.getKeyStorePassphrase(),fluent.getImpersonateUsername(),fluent.getImpersonateGroups(),fluent.getImpersonateExtras(),fluent.getOauthTokenProvider(),fluent.getCustomHeaders(),fluent.getRequestRetryBackoffLimit(),fluent.getRequestRetryBackoffInterval(),fluent.getUploadConnectionTimeout(),fluent.getUploadRequestTimeout());
    buildable.setDefaultNamespace(fluent.isDefaultNamespace());
    buildable.setAuthProvider(fluent.getAuthProvider());
    buildable.setContexts(fluent.getContexts());
    buildable.setCurrentContext(fluent.getCurrentContext());
    buildable.setAutoConfigure(fluent.isAutoConfigure());
    buildable.setFile(fluent.getFile());
    return buildable;
  }


}
3 changes: 1 addition & 2 deletions ...-client-api/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java
View file
Open in desktop
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
Open in desktop
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
74 changes: 34 additions & 40 deletions ...ent-api/src/test/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptorTest.java
View file
Open in desktop
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,8 @@
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.spy;
import static org.mockito.Mockito.when;

/**
* 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 +98,43 @@ 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 originalConfig = spy(new ConfigBuilder(Config.empty())
.withOauthToken("existing-token")
.build());
final Config autoConfig = new ConfigBuilder(Config.empty())
.withOauthToken("") // empty token
.build();
when(originalConfig.refresh()).thenReturn(autoConfig);
final TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(
originalConfig, null, Instant.now().minusSeconds(61));
// When
final boolean result = tokenRefreshInterceptor
.afterFailure(new StandardHttpRequest.Builder(), new TestHttpResponse<>().withCode(401)).get();
// Then
assertThat(result).isFalse();
assertThat(originalConfig).hasFieldOrPropertyWithValue("oauthToken", "existing-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 originalConfig = spy(new ConfigBuilder(Config.empty())
.withOauthToken("existing-token")
.build());
final Config autoConfig = new ConfigBuilder(Config.empty())
.withOauthToken("new-token")
.build();
when(originalConfig.refresh()).thenReturn(autoConfig);
final TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(
originalConfig, null, Instant.now().minusSeconds(61));
// When
final boolean result = tokenRefreshInterceptor
.afterFailure(new StandardHttpRequest.Builder(), new TestHttpResponse<>().withCode(401)).get();
// Then
assertThat(result).isTrue();
assertThat(originalConfig).hasFieldOrPropertyWithValue("oauthToken", "new-token");
}

@Test
Expand Down