fix: properly support overriding from environment variables (#486)

Currently uses a workaround a variable expansion issue.

Fixes #324

.github/workflows/build-for-quarkus-version.yml

@@ -44,9 +44,7 @@ jobs:
         run: mvn -B formatter:validate install --file pom.xml
 
       - name: Build with Maven (Native)
-        #        todo: currently need to pass env variables as args but should find a better way
-        #        see also: https://github.com/quarkusio/quarkus/pull/28692
-        run: mvn -B formatter:validate install -Dnative --file pom.xml -Dquarkus.test.arg-line="-DNAMESPACE_FROM_ENV=fromEnvVarNS -DQUARKUS_OPERATOR_SDK_CONTROLLERS_EMPTY_NAMESPACES=fromEnv1,fromEnv2 -DVARIABLE_NS_ENV=variableNSFromEnv -DQUARKUS_OPERATOR_SDK_CONTROLLERS_KEYCLOAKCONTROLLER_NAMESPACES=keycloak-ns"
+        run: mvn -B formatter:validate install -Dnative --file pom.xml
 
       - name: Kubernetes KinD Cluster
         uses: container-tools/kind-action@v1

common/src/main/java/io/quarkiverse/operatorsdk/common/RuntimeConfigurationUtils.java

@@ -13,16 +13,60 @@
 import io.smallrye.config.ConfigValue;
 import io.smallrye.config.Converters;
 import io.smallrye.config.ExpressionConfigSourceInterceptor;
+import io.smallrye.config.Expressions;
+import io.smallrye.config.common.utils.StringUtil;
 
 public class RuntimeConfigurationUtils {
-
     private static final ExpressionConfigSourceInterceptor RESOLVER = new ExpressionConfigSourceInterceptor();
     private final static Converter<HashSet<String>> converter = Converters.newCollectionConverter(
             Converters.getImplicitConverter(String.class), HashSet::new);
     private final static Config config = ConfigProvider.getConfig();
+    private static final String QUARKUS_OPERATOR_SDK_CONTROLLERS = "quarkus.operator-sdk.controllers.";
+    private static final String NAMESPACES = ".namespaces";
+
+    public static Set<String> namespacesFromConfigurationFor(String controllerName) {
+        final var propName = namespacePropertyKey(controllerName);
+
+        // todo: might need to check several alternatives here :(
+        // first check system properties
+        final var envOrSysVarName = StringUtil.replaceNonAlphanumericByUnderscores(propName.toUpperCase());
+        var namespaces = System.getProperty(envOrSysVarName);
+        if (namespaces != null && !namespaces.isBlank()) {
+            return toSet(namespaces);
+        }
+
+        // then check env variables
+        namespaces = System.getenv(envOrSysVarName);
+        if (namespaces != null && !namespaces.isBlank()) {
+            return toSet(namespaces);
+        }
+
+        // finally check if we have a property for the namespaces
+        var configValue = config.getConfigValue(propName);
+        namespaces = Expressions.withoutExpansion(configValue::getRawValue);
+        if (namespaces != null && !namespaces.isBlank()) {
+            return toSet(namespaces);
+        }
+
+        return null;
+    }
+
+    private static Set<String> toSet(String namespaces) {
+        return converter.convert(namespaces).stream().map(String::trim).collect(Collectors.toSet());
+    }
 
-    public static Set<String> stringPropValueAsSet(String propValue) {
-        return converter.convert(propValue).stream().map(String::trim).collect(Collectors.toSet());
+    // todo: remove, use {@link #expandedValueFrom} when it actually works
+    public static String expandedValueFrom2(String unexpanded) {
+        if (unexpanded.startsWith("${")) {
+            final var substring = unexpanded.substring(2, unexpanded.length() - 1);
+            var expanded = System.getProperty(substring);
+            if (expanded != null) {
+                return expanded;
+            }
+            return System.getenv(substring);
+        } else {
+            return unexpanded;
+        }
     }
 
     public static String expandedValueFrom(String unexpandedValue) {
@@ -53,4 +97,8 @@ public Iterator<ConfigValue> iterateValues() {
         final var value = RESOLVER.getValue(context, unexpandedValue);
         return value.getValue() == null ? unexpandedValue : value.getValue();
     }
+
+    public static String namespacePropertyKey(String controllerName) {
+        return QUARKUS_OPERATOR_SDK_CONTROLLERS + controllerName + NAMESPACES;
+    }
 }

core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/BuildTimeHybridControllerConfiguration.java

@@ -4,7 +4,6 @@
 import java.util.HashSet;
 import java.util.Set;
 
-import org.eclipse.microprofile.config.ConfigProvider;
 import org.jboss.jandex.AnnotationInstance;
 import org.jboss.jandex.AnnotationValue;
 
@@ -13,7 +12,6 @@
 import io.quarkiverse.operatorsdk.common.RuntimeConfigurationUtils;
 import io.quarkiverse.operatorsdk.runtime.BuildTimeControllerConfiguration;
 import io.quarkiverse.operatorsdk.runtime.BuildTimeOperatorConfiguration;
-import io.smallrye.config.Expressions;
 
 class BuildTimeHybridControllerConfiguration {
 
@@ -39,31 +37,11 @@ boolean generationAware() {
     }
 
     Set<String> namespaces(String controllerName) {
-        // first check if we have a property for the namespaces, retrieving it without expanding it
-        final var config = ConfigProvider.getConfig();
-        var withoutExpansion = Expressions.withoutExpansion(
-                () -> config.getConfigValue("quarkus.operator-sdk.controllers."
-                        + controllerName + ".namespaces").getRawValue());
-
-        // check if the controller name is doubly quoted
-        if (withoutExpansion == null) {
-            withoutExpansion = Expressions.withoutExpansion(
-                    () -> config.getConfigValue("quarkus.operator-sdk.controllers.\""
-                            + controllerName + "\".namespaces").getRawValue());
-        }
-
-        // check if the controller name is simply quoted
-        if (withoutExpansion == null) {
-            withoutExpansion = Expressions.withoutExpansion(
-                    () -> config.getConfigValue("quarkus.operator-sdk.controllers.'"
-                            + controllerName + "'.namespaces").getRawValue());
+        final var namespaces = RuntimeConfigurationUtils.namespacesFromConfigurationFor(controllerName);
+        if (namespaces != null && !namespaces.isEmpty()) {
+            return namespaces;
         }
 
-        if (withoutExpansion != null) {
-            // if we have a property, use it and convert it to a set of namespaces,
-            // potentially with unexpanded variable names as namespace names
-            return RuntimeConfigurationUtils.stringPropValueAsSet(withoutExpansion);
-        }
         return ConfigurationUtils.annotationValueOrDefault(controllerAnnotation,
                 "namespaces",
                 v -> new HashSet<>(Arrays.asList(v.asStringArray())),

core/runtime/src/main/java/io/quarkiverse/operatorsdk/runtime/ConfigurationServiceRecorder.java

@@ -1,5 +1,6 @@
 package io.quarkiverse.operatorsdk.runtime;
 
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.Supplier;
@@ -19,7 +20,6 @@
 import io.quarkus.jackson.ObjectMapperCustomizer;
 import io.quarkus.runtime.LaunchMode;
 import io.quarkus.runtime.annotations.Recorder;
-import io.smallrye.config.common.utils.StringUtil;
 
 @Recorder
 public class ConfigurationServiceRecorder {
@@ -40,7 +40,6 @@ public Supplier<QuarkusConfigurationService> configurationServiceSupplier(Versio
             // then override with controller-specific configuration if present
             if (extConfig != null) {
                 extConfig.finalizer.ifPresent(c::setFinalizer);
-                extConfig.namespaces.ifPresent(c::setNamespaces);
                 extConfig.selector.ifPresent(c::setLabelSelector);
                 c.setRetryConfiguration(RetryConfigurationResolver.resolve(extConfig.retry));
             }
@@ -54,26 +53,27 @@ public Supplier<QuarkusConfigurationService> configurationServiceSupplier(Versio
                 c.setRetryConfiguration(null);
             }
 
+            // replace already set namespaces if there is a configuration property overriding the value
+            final var namespaces = RuntimeConfigurationUtils.namespacesFromConfigurationFor(name);
+            if (namespaces != null && !Constants.DEFAULT_NAMESPACES_SET.equals(namespaces)
+                    && !c.getNamespaces().equals(namespaces)) {
+                c.setNamespaces(namespaces);
+            }
+
+            // check for potential expansion need:
+            // this happens when we have namespace configuration from annotation
             // check if we need to expand variable names from namespaces
             if (c.isNamespaceExpansionRequired()) {
-                final var expandedNS = ((QuarkusControllerConfiguration<?>) c).getNamespaces().stream()
-                        .map(RuntimeConfigurationUtils::expandedValueFrom).collect(Collectors.toSet());
+                final var expandedNS = ((QuarkusControllerConfiguration<?>) c).getNamespaces()
+                        .stream()
+                        .map(RuntimeConfigurationUtils::expandedValueFrom2) // todo: fix
+                        .collect(Collectors.toSet());
                 c.setNamespaces(expandedNS);
             }
 
-            // replace already set namespaces if there is an env variable overriding the value
-            final var propName = "quarkus.operator-sdk.controllers." + name + ".namespaces";
-            final var envVarName = StringUtil.replaceNonAlphanumericByUnderscores(
-                    propName.toUpperCase());
-            final var nsFromEnv = System.getProperty(envVarName);
-            if (nsFromEnv != null) {
-                final var namespaces = RuntimeConfigurationUtils.stringPropValueAsSet(nsFromEnv);
-                c.setNamespaces(namespaces);
-            }
-
             // if despite all of this, we still haven't set the namespaces, use the operator-level default if it exists
             if (Constants.DEFAULT_NAMESPACES_SET.equals(c.getNamespaces())) {
-                runTimeConfiguration.namespaces.ifPresent(c::setNamespaces);
+                runTimeConfiguration.namespaces.ifPresent(ns -> c.setNamespaces(new HashSet<>(ns)));
             }
         });
 

integration-tests/pom.xml

@@ -75,6 +75,19 @@
           </execution>
         </executions>
       </plugin>
+      <plugin>
+        <artifactId>maven-surefire-plugin</artifactId>
+        <configuration>
+          <environmentVariables>
+            <NAMESPACE_FROM_ENV>fromEnvVarNS</NAMESPACE_FROM_ENV>
+            <QUARKUS_OPERATOR_SDK_CONTROLLERS_EMPTY_NAMESPACES>fromEnv1, fromEnv2
+            </QUARKUS_OPERATOR_SDK_CONTROLLERS_EMPTY_NAMESPACES>
+            <VARIABLE_NS_ENV>variableNSFromEnv</VARIABLE_NS_ENV>
+            <QUARKUS_OPERATOR_SDK_CONTROLLERS_KEYCLOAKCONTROLLER_NAMESPACES>keycloak-ns
+            </QUARKUS_OPERATOR_SDK_CONTROLLERS_KEYCLOAKCONTROLLER_NAMESPACES>
+          </environmentVariables>
+        </configuration>
+      </plugin>
     </plugins>
   </build>
   <profiles>
@@ -96,6 +109,14 @@
                 </java.util.logging.manager>
                 <maven.home>${maven.home}</maven.home>
               </systemPropertyVariables>
+              <environmentVariables>
+                <NAMESPACE_FROM_ENV>fromEnvVarNS</NAMESPACE_FROM_ENV>
+                <QUARKUS_OPERATOR_SDK_CONTROLLERS_EMPTY_NAMESPACES>fromEnv1, fromEnv2
+                </QUARKUS_OPERATOR_SDK_CONTROLLERS_EMPTY_NAMESPACES>
+                <VARIABLE_NS_ENV>variableNSFromEnv</VARIABLE_NS_ENV>
+                <QUARKUS_OPERATOR_SDK_CONTROLLERS_KEYCLOAKCONTROLLER_NAMESPACES>keycloak-ns
+                </QUARKUS_OPERATOR_SDK_CONTROLLERS_KEYCLOAKCONTROLLER_NAMESPACES>
+              </environmentVariables>
             </configuration>
           </plugin>
           <plugin>
@@ -115,6 +136,14 @@
                     </java.util.logging.manager>
                     <maven.home>${maven.home}</maven.home>
                   </systemPropertyVariables>
+                  <environmentVariables>
+                    <NAMESPACE_FROM_ENV>fromEnvVarNS</NAMESPACE_FROM_ENV>
+                    <QUARKUS_OPERATOR_SDK_CONTROLLERS_EMPTY_NAMESPACES>fromEnv1, fromEnv2
+                    </QUARKUS_OPERATOR_SDK_CONTROLLERS_EMPTY_NAMESPACES>
+                    <VARIABLE_NS_ENV>variableNSFromEnv</VARIABLE_NS_ENV>
+                    <QUARKUS_OPERATOR_SDK_CONTROLLERS_KEYCLOAKCONTROLLER_NAMESPACES>keycloak-ns
+                    </QUARKUS_OPERATOR_SDK_CONTROLLERS_KEYCLOAKCONTROLLER_NAMESPACES>
+                  </environmentVariables>
                 </configuration>
               </execution>
             </executions>

integration-tests/src/main/java/io/quarkiverse/operatorsdk/it/NameWithSpaceReconciler.java

@@ -0,0 +1,19 @@
+package io.quarkiverse.operatorsdk.it;
+
+import io.fabric8.kubernetes.api.model.ServiceAccount;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
+import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
+import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
+
+@ControllerConfiguration(name = NameWithSpaceReconciler.NAME)
+public class NameWithSpaceReconciler implements Reconciler<ServiceAccount> {
+
+    public static final String NAME = "name with space";
+
+    @Override
+    public UpdateControl<ServiceAccount> reconcile(ServiceAccount serviceAccount,
+            Context<ServiceAccount> context) throws Exception {
+        return null;
+    }
+}

integration-tests/src/main/resources/application.properties

@@ -8,6 +8,7 @@ quarkus.operator-sdk.controllers.annotation.retry.interval.initial=20000
 quarkus.operator-sdk.controllers.annotation.selector=environment=production,tier!=frontend
 quarkus.operator-sdk.controllers.ApplicationScoped.namespaces=default
 quarkus.operator-sdk.controllers.variablens.namespaces=${VARIABLE_NS_ENV}
+quarkus.operator-sdk.controllers.name\ with\ space.namespaces=name-with-space
 quarkus.operator-sdk.concurrent-reconciliation-threads=10
 quarkus.operator-sdk.termination-timeout-seconds=20
 quarkus.operator-sdk.crd.validate=false

integration-tests/src/test/java/io/quarkiverse/operatorsdk/it/CustomKubernetesServerTestResource.java

@@ -1,10 +1,5 @@
 package io.quarkiverse.operatorsdk.it;
 
-import static io.quarkiverse.operatorsdk.it.EmptyReconciler.FROM_ENV_NS1;
-import static io.quarkiverse.operatorsdk.it.EmptyReconciler.FROM_ENV_NS2;
-import static io.quarkiverse.operatorsdk.it.NamespaceFromEnvReconciler.FROM_ENV_VAR_NS;
-
-import io.javaoperatorsdk.operator.ReconcilerUtils;
 import io.quarkus.test.kubernetes.client.KubernetesServerTestResource;
 
 public class CustomKubernetesServerTestResource extends KubernetesServerTestResource {
@@ -13,14 +8,6 @@ public class CustomKubernetesServerTestResource extends KubernetesServerTestReso
     protected void configureServer() {
         super.configureServer();
 
-        // hack: set system properties here so that they are set before the app is started
-        System.setProperty(NamespaceFromEnvReconciler.ENV_VAR_NAME, FROM_ENV_VAR_NS);
-        System.setProperty("QUARKUS_OPERATOR_SDK_CONTROLLERS_" + EmptyReconciler.NAME.toUpperCase()
-                + "_NAMESPACES", FROM_ENV_NS1 + ",   " + FROM_ENV_NS2);
-        System.setProperty(VariableNSReconciler.ENV_VAR_NAME, VariableNSReconciler.EXPECTED_NS_VALUE);
-        System.setProperty("QUARKUS_OPERATOR_SDK_CONTROLLERS_" + ReconcilerUtils.getDefaultNameFor(
-                KeycloakController.class).toUpperCase() + "_NAMESPACES", KeycloakController.FROM_ENV);
-
         server.expect().get().withPath("/version")
                 .andReturn(200, "{\"major\": \"13\", \"minor\": \"37\"}").always();
     }

integration-tests/src/test/java/io/quarkiverse/operatorsdk/it/OperatorSDKResourceTest.java

@@ -17,13 +17,26 @@
 import io.quarkus.test.junit.DisabledOnIntegrationTest;
 import io.quarkus.test.junit.QuarkusTest;
 
+/**
+ * This test will only pass in IDEs if you set your runner to set env properties as follow:
+ *
+ * <ul>
+ * <li>{@link NamespaceFromEnvReconciler#ENV_VAR_NAME} = {@link NamespaceFromEnvReconciler#FROM_ENV_VAR_NS}</li>
+ * <li>QUARKUS_OPERATOR_SDK_CONTROLLERS_{@link EmptyReconciler#NAME}.toUpperCase()_NAMESPACES =
+ * {@link EmptyReconciler#FROM_ENV_NS1} + ", " + {@link EmptyReconciler#FROM_ENV_NS2}</li>
+ * <li>{@link VariableNSReconciler#ENV_VAR_NAME} = {@link VariableNSReconciler#EXPECTED_NS_VALUE}</li>
+ * <li>QUARKUS_OPERATOR_SDK_CONTROLLERS_ReconcilerUtils.getDefaultNameFor(KeycloakController.class).toUpperCase()_NAMESPACES =
+ * {@link KeycloakController#FROM_ENV}</li>
+ * </ul>
+ *
+ * See also {@code maven-surefire-plugin} configuration where these same environment variables are set
+ */
 @QuarkusTest
 @QuarkusTestResource(CustomKubernetesServerTestResource.class)
 class OperatorSDKResourceTest {
 
     @BeforeAll
     static void setup() {
-
     }
 
     @Test
@@ -75,8 +88,9 @@ void allControllersShouldHaveAssociatedConfiguration() {
                 ReconcilerUtils.getDefaultNameFor(SecretReconciler.class),
                 ReconcilerUtils.getDefaultNameFor(GatewayReconciler.class),
                 DependentDefiningReconciler.NAME, NamespaceFromEnvReconciler.NAME,
-                EmptyReconciler.NAME, VariableNSReconciler.NAME, ReconcilerUtils.getDefaultNameFor(
-                        KeycloakController.class)));
+                EmptyReconciler.NAME, VariableNSReconciler.NAME,
+                ReconcilerUtils.getDefaultNameFor(KeycloakController.class),
+                NameWithSpaceReconciler.NAME));
     }
 
     @Test
@@ -121,6 +135,13 @@ void applicationPropertiesShouldOverrideDefaultAndAnnotation() {
                 .then()
                 .statusCode(200)
                 .body("namespaces", hasItem("default"));
+
+        given()
+                .when()
+                .get("/operator/" + NameWithSpaceReconciler.NAME + "/config")
+                .then()
+                .statusCode(200)
+                .body("namespaces", hasItem("name-with-space"));
     }
 
     @Test
@@ -145,6 +166,8 @@ void dependentAnnotationsShouldAppearInConfiguration() {
 
     @Test
     void shouldExpandVariablesInNamespacesConfigurationFromAnnotation() {
+        assertThat(System.getenv(NamespaceFromEnvReconciler.ENV_VAR_NAME),
+                is(NamespaceFromEnvReconciler.FROM_ENV_VAR_NS));
         given()
                 .when()
                 .get("/operator/" + NamespaceFromEnvReconciler.NAME + "/config")
@@ -157,6 +180,7 @@ void shouldExpandVariablesInNamespacesConfigurationFromAnnotation() {
 
     @Test
     void shouldExpandVariablesInNamespacesConfigurationFromProperties() {
+        assertThat(System.getenv(VariableNSReconciler.ENV_VAR_NAME), is(VariableNSReconciler.EXPECTED_NS_VALUE));
         given()
                 .when()
                 .get("/operator/" + VariableNSReconciler.NAME + "/config")