OWLS-76832 (Resolves #1232) - Error using variable in pod template if…

… value has underscore (#1286)

* convert substituted variables to DNS1123 if used as names

* only convert variables for selected, customizable list of fields

* checkstyle fixes

* add dns1123Fields to helm chart to override default list of field names that requires dns-1123 values

* document dns1123fields

* dns1123Names values no longer case sensitive

* minor cleanup

* doc update base on feedback

docs-source/content/userguide/managing-operators/using-the-operator/using-helm.md

@@ -164,6 +164,27 @@ These examples show two valid YAML syntax options for arrays.
 You must include the `default` namespace in the list if you want the operator to monitor both the `default` namespace and some other namespaces.
 {{% /notice %}}
 
+##### `dns1123Fields`
+
+When the operator processes variable references in the domain resource, such as `$(SERVER_NAME)`,
+it will convert them into DNS-1123 legal values if the variables are referenced
+in fields that require their values to be conforming to DNS-1123. A default list of such field 
+names can be found inside the class `Legalnames`
+in the `oracle.kubernetes.operator.helpers package`.
+For example, a value `$(SERVER_NAME)-volume` is specified in a field in a domain resource that 
+requires DNS-1123. Suppose the name of the server is `managed_server1` which contains an illegal 
+underscore character. Upon variable substitution, the operator will replace it with a hyphen, and 
+the resulting value will become `managed-server1-volume`.
+
+A comma separated list of field names can be supplied in `dns1123Fields` to customize the list of 
+field names, or it can be left commented out to use the default list of field names.
+
+This property is optional.
+
+Example:
+```
+dns1123Fields: "name, claimName, volumeName"
+```
 #### Elastic Stack integration
 
 ##### `elkIntegrationEnabled`

kubernetes/charts/weblogic-operator/templates/_operator-cm.tpl

@@ -14,6 +14,9 @@ data:
   {{- end }}
   serviceaccount: {{ .serviceAccount | quote }}
   targetNamespaces: {{ .domainNamespaces | uniq | sortAlpha | join "," | quote }}
+  {{- if .dns1123Fields }}
+  dns1123Fields: {{ .dns1123Fields | quote }}
+  {{- end }}
 kind: "ConfigMap"
 metadata:
   labels:

kubernetes/charts/weblogic-operator/values.yaml

@@ -101,6 +101,15 @@ elasticSearchHost: "elasticsearch.default.svc.cluster.local"
 # This parameter is ignored if 'elkIntegrationEnabled' is false.
 elasticSearchPort: 9200
 
+# dns1123Fields overrides the default list of field names that the operator
+# converts to DNS-1123 legal values when replacing variable references in the 
+# domain resource. The default list can be found inside the class Legalnames 
+# in the oracle.kubernetes.operator.helpers package.
+# Supply a comma seperated list of field names to customize the list of fields
+# such as "name, claimName, volumeName", or leave it commented out to use 
+# the default list of field names.
+# dns1123Fields: ""
+
 # Istio service mesh support is experimental.
 # istioEnabled specifies whether or not the domain is deployed under an Istio service mesh.
-istioEnabled: false
+istioEnabled: false

operator/src/main/java/oracle/kubernetes/operator/helpers/LegalNames.java

@@ -3,6 +3,13 @@
 
 package oracle.kubernetes.operator.helpers;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Optional;
+import java.util.StringTokenizer;
+
+import oracle.kubernetes.operator.TuningParameters;
+
 /** A class to create DNS-1123 legal names for Kubernetes objects. */
 public class LegalNames {
 
@@ -11,6 +18,34 @@ public class LegalNames {
   private static final String DOMAIN_INTROSPECTOR_JOB_PATTERN = "%s-introspect-domain-job";
   private static final String EXTERNAL_SERVICE_PATTERN = "%s-%s-external";
 
+  public static final String DNS_1123_FIELDS_PARAM = "dns1123Fields";
+
+  // Fields that requires values to be DNS1123 legal
+  private static final String[] DEFAULT_DNS1123_FIELDS = {
+      "ClaimName",          // V1PersistentVolumeClaimVolumeSource
+      "ClusterName",        // V1ObjectMetaData
+      "ContainerName",      // V1ResourceFieldSelector
+      "ExternalName",       // V1ServiceSpec
+      "GenerateName",       // V1ObjectMetaData
+      "MetricName",         // V2beta1PodsMetricSource, etc
+      "Name",
+      "NodeName",           // V1PodSpec, etc
+      // "NominatedNodeName",  // V1PodStatus - excluded since it is not used within V1PodSpec
+      "PersistentVolumeName",// V1VolumeAttachmentSource, etc
+      "PriorityClassName",  // V1PodSpec
+      "RuntimeClassName",   // V1PodSpec
+      "SchedulerName",      // V1PodSpec
+      "ScopeName",          // V1ScopedResourceSelectorRequirement
+      "SecretName",         // V1SecretVolumeSource, etc
+      "ServiceAccountName", // V1PodSpec
+      "ServiceName",        // NetworkingV1beta1IngressBackend, etc
+      "SingularName",       // V1APIResource
+      "StorageClassName",   // V1PersistentVolumeSpec, V1PersistentVolumeClaimSpec
+      "VolumeName"         // V1PersistentVolumeClaimSpec, etc
+  };
+
+  static String[] dns1123Fields;
+
   public static String toServerServiceName(String domainUid, String serverName) {
     return toServerName(domainUid, serverName);
   }
@@ -48,4 +83,57 @@ public static String toExternalServiceName(String domainUid, String serverName)
   public static String toDns1123LegalName(String value) {
     return value.toLowerCase().replace('_', '-');
   }
+
+  /**
+   * Returns a list of field names of fields that needs to be in DNS-1123 format from the
+   * "dns1123Fields" tuning parameter, if it is configured with a comma delimited values
+   * containing field names.
+   *
+   * @return String array containing a list of fields that are required to be * in DNS-1123 format,
+   *         or null if "dns1123Fields" tuning parameter is not configured.
+   */
+  private static String[] getConfiguredDns1123Fields() {
+    String configuredValue = TuningParameters.getInstance().get(DNS_1123_FIELDS_PARAM);
+
+    if (configuredValue == null) {
+      return null;
+    }
+
+    Collection<String> fields = new ArrayList<>();
+
+    if (configuredValue != null) {
+      StringTokenizer st = new StringTokenizer(configuredValue, ",");
+      while (st.hasMoreTokens()) {
+        fields.add(st.nextToken().trim());
+      }
+    }
+    String[] fieldsArray = new String[fields.size()];
+    return fields.toArray(fieldsArray);
+  }
+
+
+  private static String[] getDns1123Fields() {
+    if (dns1123Fields == null) {
+      dns1123Fields = Optional.ofNullable(getConfiguredDns1123Fields()).orElse(DEFAULT_DNS1123_FIELDS);
+    }
+    return dns1123Fields;
+  }
+
+  /**
+   * Returns true if the value in the field is required to be DNS-1123 legal.
+   *
+   * @param fieldName Name of the field to be checked
+   * @return true if the value needs to be DNS1123 legal, false otherwise
+   */
+  public static boolean isDNS1123Required(String fieldName) {
+    if (fieldName != null) {
+      for (String dns1123Field: getDns1123Fields()) {
+        if (dns1123Field.equalsIgnoreCase(fieldName)) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
 }

operator/src/main/java/oracle/kubernetes/operator/helpers/PodHelper.java

@@ -160,8 +160,9 @@ static List<V1EnvVar> createCopy(List<V1EnvVar> envVars) {
     ArrayList<V1EnvVar> copy = new ArrayList<>();
     if (envVars != null) {
       for (V1EnvVar envVar : envVars) {
-        // note that a deep copy of valueFrom is not needed here as, unlike with value, we are
-        // not doing any modifications or macro substitutions on the valueFrom fields
+        // note that a deep copy of valueFrom is not needed here as, unlike with value, the
+        // new V1EnvVarFrom objects would be created by the doDeepSubstitutions() method in
+        // StepContextBase class.
         copy.add(new V1EnvVar()
             .name(envVar.getName())
             .value(envVar.getValue())

operator/src/main/java/oracle/kubernetes/operator/helpers/StepContextBase.java

@@ -94,9 +94,7 @@ final List<V1EnvVar> getEnvironmentVariables(TuningParameters tuningParameters)
         vars, "USER_MEM_ARGS", "-XX:+UseContainerSupport -Djava.security.egd=file:/dev/./urandom");
 
     hideAdminUserCredentials(vars);
-    doSubstitution(varsToSubVariables(vars), vars);
-
-    return vars;
+    return doDeepSubstitution(varsToSubVariables(vars), vars);
   }
 
   protected Map<String, String> varsToSubVariables(List<V1EnvVar> vars) {
@@ -110,15 +108,13 @@ protected Map<String, String> varsToSubVariables(List<V1EnvVar> vars) {
     return substitutionVariables;
   }
 
-  protected void doSubstitution(final Map<String, String> substitutionVariables, List<V1EnvVar> vars) {
-    for (V1EnvVar var : vars) {
-      var.setValue(translate(substitutionVariables, var.getValue()));
-    }
+  protected <T> T doDeepSubstitution(final Map<String, String> substitutionVariables, T obj) {
+    return doDeepSubstitution(substitutionVariables, obj, false);
   }
 
-  protected <T> T doDeepSubstitution(final Map<String, String> substitutionVariables, T obj) {
+  protected <T> T doDeepSubstitution(final Map<String, String> substitutionVariables, T obj, boolean requiresDNS1123) {
     if (obj instanceof String) {
-      return (T) translate(substitutionVariables, (String) obj);
+      return (T) translate(substitutionVariables, (String) obj, requiresDNS1123);
     } else if (obj instanceof List) {
       List<Object> result = new ArrayList<>();
       for (Object o : (List) obj) {
@@ -144,7 +140,11 @@ protected <T> T doDeepSubstitution(final Map<String, String> substitutionVariabl
           for (Pair<Method, Method> item : typeBeans) {
             item.getRight()
                 .invoke(
-                    subObj, doDeepSubstitution(substitutionVariables, item.getLeft().invoke(obj)));
+                    subObj,
+                    doDeepSubstitution(
+                        substitutionVariables,
+                        item.getLeft().invoke(obj),
+                        isDNS1123Required(item.getLeft())));
           }
           return subObj;
         } catch (NoSuchMethodException
@@ -158,6 +158,12 @@ protected <T> T doDeepSubstitution(final Map<String, String> substitutionVariabl
     return obj;
   }
 
+  boolean isDNS1123Required(Method method) {
+    // value requires to be in DNS1123 if the value is for a name, which is assumed to be
+    // name for a kubernetes object
+    return LegalNames.isDNS1123Required(method.getName().substring(3));
+  }
+
   private static final String MODELS_PACKAGE = V1Pod.class.getPackageName();
   private static final String DOMAIN_MODEL_PACKAGE = Domain.class.getPackageName();
 
@@ -193,10 +199,15 @@ private List<Pair<Method, Method>> typeBeans(Class cls) {
   }
 
   private String translate(final Map<String, String> substitutionVariables, String rawValue) {
+    return translate(substitutionVariables, rawValue, false);
+  }
+
+  private String translate(final Map<String, String> substitutionVariables, String rawValue, boolean requiresDNS1123) {
     String result = rawValue;
     for (Map.Entry<String, String> entry : substitutionVariables.entrySet()) {
       if (result != null && entry.getValue() != null) {
-        result = result.replace(String.format("$(%s)", entry.getKey()), entry.getValue());
+        result = result.replace(String.format("$(%s)", entry.getKey()),
+            requiresDNS1123 ? LegalNames.toDns1123LegalName(entry.getValue()) : entry.getValue());
       }
     }
     return result;

operator/src/main/java/oracle/kubernetes/weblogic/domain/DomainConfigurator.java

@@ -223,6 +223,8 @@ protected DomainSpec getDomainSpec() {
 
   public abstract DomainConfigurator withAdditionalVolume(String name, String path);
 
+  public abstract DomainConfigurator withAdditionalPVClaimVolume(String name, String claimName);
+
   public abstract DomainConfigurator withAdditionalVolumeMount(String name, String path);
 
   public abstract DomainConfigurator withInitContainer(V1Container initContainer);

operator/src/main/java/oracle/kubernetes/weblogic/domain/model/BaseConfiguration.java

@@ -272,6 +272,10 @@ void addAdditionalVolume(String name, String path) {
     serverPod.addAdditionalVolume(name, path);
   }
 
+  void addAdditionalPVClaimVolume(String name, String claimName) {
+    serverPod.addAdditionalPVClaimVolume(name, claimName);
+  }
+
   public List<V1VolumeMount> getAdditionalVolumeMounts() {
     return serverPod.getAdditionalVolumeMounts();
   }

operator/src/main/java/oracle/kubernetes/weblogic/domain/model/DomainCommonConfigurator.java

@@ -92,6 +92,12 @@ public DomainConfigurator withAdditionalVolume(String name, String path) {
     return this;
   }
 
+  @Override
+  public DomainConfigurator withAdditionalPVClaimVolume(String name, String claimName) {
+    getDomainSpec().addAdditionalPVClaimVolume(name, claimName);
+    return this;
+  }
+
   @Override
   public DomainConfigurator withAdditionalVolumeMount(String name, String path) {
     getDomainSpec().addAdditionalVolumeMount(name, path);

operator/src/main/java/oracle/kubernetes/weblogic/domain/model/ServerPod.java

@@ -22,6 +22,7 @@
 import io.kubernetes.client.models.V1HostPathVolumeSource;
 import io.kubernetes.client.models.V1NodeAffinity;
 import io.kubernetes.client.models.V1NodeSelector;
+import io.kubernetes.client.models.V1PersistentVolumeClaimVolumeSource;
 import io.kubernetes.client.models.V1PodAffinity;
 import io.kubernetes.client.models.V1PodAntiAffinity;
 import io.kubernetes.client.models.V1PodReadinessGate;
@@ -608,6 +609,13 @@ private void addAdditionalVolume(V1Volume var) {
     volumes.add(var);
   }
 
+  void addAdditionalPVClaimVolume(String name, String claimName) {
+    addAdditionalVolume(
+        new V1Volume().name(name).persistentVolumeClaim(
+            new V1PersistentVolumeClaimVolumeSource().claimName(claimName))
+    );
+  }
+
   void addAdditionalVolumeMount(String name, String path) {
     addAdditionalVolumeMount(new V1VolumeMount().name(name).mountPath(path));
   }