-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix issue 2008 (3.2.x) #2015
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.
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
fix issue 2008 (3.2.x) #2015
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| /* | ||
| * Copyright 2013-present the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.springframework.cloud.kubernetes.commons.config; | ||
|
|
||
| /** | ||
| * @author wind57 | ||
| */ | ||
| public final class MountSecretPropertySource extends SecretsPropertySource { | ||
|
|
||
| public MountSecretPropertySource(SourceData sourceData) { | ||
| super(sourceData); | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,33 +152,33 @@ protected void putPathConfig(CompositePropertySource composite) { | |
| * @author wind57 | ||
| */ | ||
| private static class SecretsPropertySourceCollector | ||
| implements Collector<Path, List<SecretsPropertySource>, List<SecretsPropertySource>> { | ||
| implements Collector<Path, List<MountSecretPropertySource>, List<MountSecretPropertySource>> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the only change here is : |
||
|
|
||
| @Override | ||
| public Supplier<List<SecretsPropertySource>> supplier() { | ||
| public Supplier<List<MountSecretPropertySource>> supplier() { | ||
| return ArrayList::new; | ||
| } | ||
|
|
||
| @Override | ||
| public BiConsumer<List<SecretsPropertySource>, Path> accumulator() { | ||
| public BiConsumer<List<MountSecretPropertySource>, Path> accumulator() { | ||
| return (list, filePath) -> { | ||
| SecretsPropertySource source = property(filePath); | ||
| MountSecretPropertySource source = property(filePath); | ||
| if (source != null) { | ||
| list.add(source); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public BinaryOperator<List<SecretsPropertySource>> combiner() { | ||
| public BinaryOperator<List<MountSecretPropertySource>> combiner() { | ||
| return (left, right) -> { | ||
| left.addAll(right); | ||
| return left; | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public Function<List<SecretsPropertySource>, List<SecretsPropertySource>> finisher() { | ||
| public Function<List<MountSecretPropertySource>, List<MountSecretPropertySource>> finisher() { | ||
| return Function.identity(); | ||
| } | ||
|
|
||
|
|
@@ -187,15 +187,15 @@ public Set<Characteristics> characteristics() { | |
| return EnumSet.of(Characteristics.UNORDERED, Characteristics.IDENTITY_FINISH); | ||
| } | ||
|
|
||
| private SecretsPropertySource property(Path filePath) { | ||
| private MountSecretPropertySource property(Path filePath) { | ||
|
|
||
| String fileName = filePath.getFileName().toString(); | ||
|
|
||
| try { | ||
| String content = new String(Files.readAllBytes(filePath)).trim(); | ||
| String sourceName = fileName.toLowerCase(Locale.ROOT); | ||
| SourceData sourceData = new SourceData(sourceName, Map.of(fileName, content)); | ||
| return new SecretsPropertySource(sourceData); | ||
| return new MountSecretPropertySource(sourceData); | ||
| } | ||
| catch (IOException e) { | ||
| LOG.warn("Error reading properties file", e); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ | |
| import org.springframework.cloud.bootstrap.config.BootstrapPropertySource; | ||
| import org.springframework.cloud.bootstrap.config.PropertySourceLocator; | ||
| import org.springframework.cloud.kubernetes.commons.config.MountConfigMapPropertySource; | ||
| import org.springframework.cloud.kubernetes.commons.config.SecretsPropertySource; | ||
| import org.springframework.cloud.kubernetes.commons.config.MountSecretPropertySource; | ||
| import org.springframework.core.env.CompositePropertySource; | ||
| import org.springframework.core.env.ConfigurableEnvironment; | ||
| import org.springframework.core.env.MapPropertySource; | ||
|
|
@@ -89,6 +89,7 @@ public static boolean reload(PropertySourceLocator locator, ConfigurableEnvironm | |
| * @deprecated this method will not be public in the next major release. | ||
| */ | ||
| @Deprecated(forRemoval = false) | ||
| @SuppressWarnings("unchecked") | ||
| public static <S extends PropertySource<?>> List<S> findPropertySources(Class<S> sourceClass, | ||
| ConfigurableEnvironment environment) { | ||
| List<S> managedSources = new ArrayList<>(); | ||
|
|
@@ -111,9 +112,9 @@ else if (source instanceof MountConfigMapPropertySource mountConfigMapPropertySo | |
| // we know that the type is correct here | ||
| managedSources.add((S) mountConfigMapPropertySource); | ||
| } | ||
| else if (source instanceof SecretsPropertySource secretsPropertySource) { | ||
| else if (source instanceof MountSecretPropertySource mountSecretPropertySource) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we now check for |
||
| // we know that the type is correct here | ||
| managedSources.add((S) secretsPropertySource); | ||
| managedSources.add((S) mountSecretPropertySource); | ||
| } | ||
| else if (source instanceof BootstrapPropertySource<?> bootstrapPropertySource) { | ||
| PropertySource<?> propertySource = bootstrapPropertySource.getDelegate(); | ||
|
|
@@ -125,9 +126,9 @@ else if (propertySource instanceof MountConfigMapPropertySource mountConfigMapPr | |
| // we know that the type is correct here | ||
| managedSources.add((S) mountConfigMapPropertySource); | ||
| } | ||
| else if (propertySource instanceof SecretsPropertySource secretsPropertySource) { | ||
| else if (propertySource instanceof MountSecretPropertySource mountSecretPropertySource) { | ||
| // we know that the type is correct here | ||
| managedSources.add((S) secretsPropertySource); | ||
| managedSources.add((S) mountSecretPropertySource); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,198 @@ | ||
| /* | ||
| * Copyright 2012-present the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.springframework.cloud.kubernetes.fabric8.config.reload_it; | ||
|
|
||
| import java.nio.charset.StandardCharsets; | ||
| import java.time.Duration; | ||
| import java.util.Base64; | ||
| import java.util.Collections; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import io.fabric8.kubernetes.api.model.ConfigMapBuilder; | ||
| import io.fabric8.kubernetes.api.model.SecretBuilder; | ||
| import io.fabric8.kubernetes.client.Config; | ||
| import io.fabric8.kubernetes.client.KubernetesClient; | ||
| import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient; | ||
| import org.awaitility.Awaitility; | ||
| import org.junit.jupiter.api.BeforeAll; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.boot.test.context.SpringBootTest; | ||
| import org.springframework.boot.test.context.TestConfiguration; | ||
| import org.springframework.boot.test.system.CapturedOutput; | ||
| import org.springframework.boot.test.system.OutputCaptureExtension; | ||
| import org.springframework.cloud.kubernetes.commons.config.reload.ConfigReloadProperties; | ||
| import org.springframework.cloud.kubernetes.commons.config.reload.ConfigurationUpdateStrategy; | ||
| import org.springframework.cloud.kubernetes.commons.config.reload.PollingConfigMapChangeDetector; | ||
| import org.springframework.cloud.kubernetes.fabric8.config.Fabric8ConfigMapPropertySource; | ||
| import org.springframework.cloud.kubernetes.fabric8.config.Fabric8ConfigMapPropertySourceLocator; | ||
| import org.springframework.cloud.kubernetes.fabric8.config.example.App; | ||
| import org.springframework.context.annotation.Bean; | ||
| import org.springframework.context.annotation.Primary; | ||
| import org.springframework.core.env.AbstractEnvironment; | ||
| import org.springframework.core.env.ConfigurableEnvironment; | ||
| import org.springframework.core.env.PropertySource; | ||
| import org.springframework.scheduling.concurrent.ThreadPoolTaskScheduler; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** | ||
| * Proves that | ||
| * <a href="https://github.com/spring-cloud/spring-cloud-kubernetes/issues/2008">this</a> | ||
| * issue is fixed. | ||
| * | ||
| * @author wind57 | ||
| */ | ||
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a test that uses the exact scenario the OP was presenting in the issue and shows that this is now fixed |
||
| properties = { "spring.cloud.bootstrap.enabled=true", "spring.cloud.kubernetes.config.enabled=true", | ||
| "spring.cloud.bootstrap.name=polling-reload-configmap-and-secret", | ||
| "spring.main.cloud-platform=KUBERNETES", "spring.application.name=polling-reload-configmap-and-secret", | ||
| "spring.main.allow-bean-definition-overriding=true", | ||
| "spring.cloud.kubernetes.client.namespace=spring-k8s", | ||
| "logging.level.org.springframework.cloud.kubernetes.commons.config.reload=debug" }, | ||
| classes = { PollingReloadConfigMapAndSecretTest.TestConfig.class, App.class }) | ||
| @EnableKubernetesMockClient(crud = true, https = false) | ||
| @ExtendWith(OutputCaptureExtension.class) | ||
| class PollingReloadConfigMapAndSecretTest { | ||
|
|
||
| private static final String NAMESPACE = "spring-k8s"; | ||
|
|
||
| private static final AtomicBoolean STRATEGY_FOR_SECRET_CALLED = new AtomicBoolean(false); | ||
|
|
||
| private static KubernetesClient mockClient; | ||
|
|
||
| @Autowired | ||
| private ConfigurableEnvironment environment; | ||
|
|
||
| @BeforeAll | ||
| static void beforeAll() { | ||
| // Configure the kubernetes master url to point to the mock server | ||
| System.setProperty(Config.KUBERNETES_MASTER_SYSTEM_PROPERTY, mockClient.getConfiguration().getMasterUrl()); | ||
| System.setProperty(Config.KUBERNETES_TRUST_CERT_SYSTEM_PROPERTY, "true"); | ||
| System.setProperty(Config.KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, "false"); | ||
| System.setProperty(Config.KUBERNETES_AUTH_TRYSERVICEACCOUNT_SYSTEM_PROPERTY, "false"); | ||
| System.setProperty(Config.KUBERNETES_NAMESPACE_SYSTEM_PROPERTY, "test"); | ||
| System.setProperty(Config.KUBERNETES_HTTP2_DISABLE, "true"); | ||
|
|
||
| // namespace: spring-k8s, name: secret-a | ||
| Map<String, String> secretA = Collections.singletonMap("one", | ||
| Base64.getEncoder().encodeToString("a".getBytes(StandardCharsets.UTF_8))); | ||
| createSecret("secret-a", secretA); | ||
|
|
||
| // namespace: spring-k8s, name: secret-b | ||
| Map<String, String> secretB = Collections.singletonMap("two", | ||
| Base64.getEncoder().encodeToString("b".getBytes(StandardCharsets.UTF_8))); | ||
| createSecret("secret-b", secretB); | ||
|
|
||
| // namespace: spring-k8s, name: configmap-a | ||
| Map<String, String> configMapA = Collections.singletonMap("one", "a"); | ||
| createConfigMap("configmap-a", configMapA); | ||
|
|
||
| // namespace: spring-k8s, name: configmap-b | ||
| Map<String, String> configMapB = Collections.singletonMap("two", "b"); | ||
| createConfigMap("configmap-b", configMapB); | ||
|
|
||
| } | ||
|
|
||
| @Test | ||
| void test(CapturedOutput output) { | ||
|
|
||
| Set<String> sources = environment.getPropertySources() | ||
| .stream() | ||
| .map(PropertySource::getName) | ||
| .collect(Collectors.toSet()); | ||
| assertThat(sources).contains("bootstrapProperties-configmap.configmap-b.spring-k8s", | ||
| "bootstrapProperties-configmap.configmap-a.spring-k8s", | ||
| "bootstrapProperties-secret.secret-b.spring-k8s", "bootstrapProperties-secret.secret-a.spring-k8s"); | ||
|
|
||
| // 1. first, wait for a cycle where we see the configmaps as being the same | ||
| Awaitility.await() | ||
| .atMost(Duration.ofSeconds(10)) | ||
| .pollInterval(Duration.ofSeconds(1)) | ||
| .until(() -> output.getOut() | ||
| .contains("Reloadable condition was not satisfied, reload will not be triggered")); | ||
|
|
||
| // 2. then change a configmap, so the cycle seems them as different and triggers a | ||
| // reload | ||
| Map<String, String> configMapA = Collections.singletonMap("one", "aa"); | ||
| replaceConfigMap("configmap-a", configMapA); | ||
|
|
||
| // 3. reload is triggered | ||
| Awaitility.await() | ||
| .atMost(Duration.ofSeconds(10)) | ||
| .pollInterval(Duration.ofSeconds(1)) | ||
| .until(STRATEGY_FOR_SECRET_CALLED::get); | ||
|
|
||
| } | ||
|
|
||
| private static void createSecret(String name, Map<String, String> data) { | ||
| mockClient.secrets() | ||
| .inNamespace(NAMESPACE) | ||
| .resource(new SecretBuilder().withNewMetadata().withName(name).endMetadata().addToData(data).build()) | ||
| .create(); | ||
| } | ||
|
|
||
| private static void createConfigMap(String name, Map<String, String> data) { | ||
| mockClient.configMaps() | ||
| .inNamespace(NAMESPACE) | ||
| .resource(new ConfigMapBuilder().withNewMetadata().withName(name).endMetadata().addToData(data).build()) | ||
| .create(); | ||
| } | ||
|
|
||
| private static void replaceConfigMap(String name, Map<String, String> data) { | ||
| mockClient.configMaps() | ||
| .inNamespace(NAMESPACE) | ||
| .resource(new ConfigMapBuilder().withNewMetadata().withName(name).endMetadata().addToData(data).build()) | ||
| .createOrReplace(); | ||
| } | ||
|
|
||
| @TestConfiguration | ||
| static class TestConfig { | ||
|
|
||
| @Bean | ||
| @Primary | ||
| PollingConfigMapChangeDetector pollingConfigMapChangeDetector(AbstractEnvironment environment, | ||
| ConfigReloadProperties configReloadProperties, ConfigurationUpdateStrategy configurationUpdateStrategy, | ||
| Fabric8ConfigMapPropertySourceLocator fabric8ConfigMapPropertySourceLocator) { | ||
| ThreadPoolTaskScheduler scheduler = new ThreadPoolTaskScheduler(); | ||
| scheduler.initialize(); | ||
| return new PollingConfigMapChangeDetector(environment, configReloadProperties, configurationUpdateStrategy, | ||
| Fabric8ConfigMapPropertySource.class, fabric8ConfigMapPropertySourceLocator, scheduler); | ||
| } | ||
|
|
||
| @Bean | ||
| @Primary | ||
| ConfigReloadProperties configReloadProperties() { | ||
| return new ConfigReloadProperties(true, true, true, ConfigReloadProperties.ReloadStrategy.REFRESH, | ||
| ConfigReloadProperties.ReloadDetectionMode.POLLING, Duration.ofMillis(200), Set.of(NAMESPACE), | ||
| false, Duration.ofSeconds(2)); | ||
| } | ||
|
|
||
| @Bean | ||
| @Primary | ||
| ConfigurationUpdateStrategy secretConfigurationUpdateStrategy() { | ||
| return new ConfigurationUpdateStrategy("to-console", () -> STRATEGY_FOR_SECRET_CALLED.set(true)); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| spring: | ||
| application: | ||
| name: polling-reload-configmap-and-secret | ||
| cloud: | ||
| kubernetes: | ||
| reload: | ||
| enabled: true | ||
| monitoring-config-maps: true | ||
| monitoring-secrets: true | ||
| mode: polling | ||
| config: | ||
| namespace: spring-k8s | ||
| sources: | ||
| - name: configmap-a | ||
| - name: configmap-b | ||
| enable-api: true | ||
| include-profile-specific-sources: false | ||
| secrets: | ||
| namespace: spring-k8s | ||
| sources: | ||
| - name: secret-a | ||
| - name: secret-b | ||
| enable-api: true | ||
| include-profile-specific-sources: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was long due for us to have this. We have
MountConfigMapPropertySource, but not one for secret. This has caused various issues over time and confusion. Introducing it should make things far easier imo