chore(bors): merge pull request #391

391: fix: fix upgrade r=niladrih a=niladrih

Change:
- Use yq's strenv operator to use special characters with yq when updating the upgrade values.yaml.
- Fix check which forbids upgrade from a stable release to unstable version.
- Prefers the parameters of an existing default StorageClass over the chart defaults.

Co-authored-by: Niladri Halder <[email protected]>

chart/templates/_helpers.tpl

@@ -205,3 +205,30 @@ Usage:
         {{- end -}}
     {{- end -}}
 {{- end -}}
+
+{{/*
+    Generate the default StorageClass parameters.
+    This is required because StorageClass parameters cannot be patched after creation.
+    If the StorageClass already exists, the default StorageClass carries the parameters and values
+    of that StorageClass. Else, it carries the default parameters and values.
+*/}}
+{{- define "storageClass.parameters" -}}
+    {{- $scName := index . 0 -}}
+    {{- $valuesParams := index . 1 -}}
+
+    {{/* Check to see if a default StorageClass already exists */}}
+    {{- $sc := lookup "storage.k8s.io/v1" "StorageClass" "" $scName -}}
+
+    {{- if $sc -}}
+        {{/* Existing defaults */}}
+        {{ range $param, $val := $sc.parameters }}
+{{ $param | quote }}: {{ $val | quote }}
+        {{- end -}}
+
+    {{- else -}}
+        {{/* Current defaults */}}
+        {{ range $param, $val := $valuesParams }}
+{{ $param | quote }}: {{ $val | quote }}
+        {{- end -}}
+    {{- end -}}
+{{- end -}}

chart/templates/storageclass.yaml

@@ -1,14 +1,21 @@
 {{ if .Values.storageClass.enabled }}
+{{- $scName := (printf "%s-%s" .Release.Name .Values.storageClass.nameSuffix | trunc 63) }}
 kind: StorageClass
 apiVersion: storage.k8s.io/v1
 metadata:
-  name: {{ .Release.Name }}-{{ .Values.storageClass.nameSuffix }}
+  name: {{ $scName }}
   {{- if .Values.storageClass.default }}
   annotations:
     storageclass.kubernetes.io/is-default-class: "true"
   {{- end }}
 parameters:
-  repl: {{ .Values.storageClass.parameters.repl | toString | quote }}
-  protocol: 'nvmf'
+{{/*
+  Set StorageClass parameters by adding to the values.yaml 'storageClass.parameters' map.
+  Don't add the parameters to this template directly.
+  This is done so that during an upgrade, an existing default StorageClass's config can
+  be given preference over this chart's defaults.
+*/}}
+{{ $valuesParams := .Values.storageClass.parameters }}
+{{ (include "storageClass.parameters" (list $scName $valuesParams)) | indent 2 }}
 provisioner: io.openebs.csi-mayastor
-{{ end }}
+{{ end }}

chart/values.yaml

@@ -728,6 +728,7 @@ storageClass:
   nameSuffix: single-replica
   default: false
   parameters:
+    protocol: nvmf
     repl: 1
 
 localpv-provisioner:

k8s/upgrade/src/bin/upgrade-job/helm/yaml/yq.rs

@@ -173,6 +173,9 @@ impl YqV4 {
     where
         V: Display + Sized,
     {
+        // Command for use during yq file update
+        let mut command = self.command();
+
         // Assigning value based on if it needs quotes around it or not.
         // Strings require quotes.
         let value = match format!("{value}").as_str() {
@@ -188,8 +191,10 @@ impl YqV4 {
                     break 'other something_else.to_string();
                 }
 
-                // Quotes for a string.
-                format!(r#""{something_else}""#)
+                // Preserve special characters for a string.
+                // Ref: https://mikefarah.gitbook.io/yq/usage/tips-and-tricks#special-characters-in-strings
+                command.env("VAL", something_else);
+                "strenv(VAL)".to_string()
             }
         };
 
@@ -198,14 +203,13 @@ impl YqV4 {
             format!(r#"{} = {value}"#, key.as_str()),
             filepath.to_string_lossy()
         ];
-        let yq_set_output =
-            self.command()
-                .args(yq_set_args.clone())
-                .output()
-                .context(YqCommandExec {
-                    command: self.command_as_str().to_string(),
-                    args: yq_set_args.clone(),
-                })?;
+        let yq_set_output = command
+            .args(yq_set_args.clone())
+            .output()
+            .context(YqCommandExec {
+                command: self.command_as_str().to_string(),
+                args: yq_set_args.clone(),
+            })?;
 
         ensure!(
             yq_set_output.status.success(),

k8s/upgrade/src/plugin/preflight_validations.rs

@@ -186,6 +186,12 @@ impl TryFrom<&[u8]> for UnsupportedVersions {
     }
 }
 
+/// Strips the prefix 'v' from a semver-like literal, e.g.: v1.2.3 -> 1.2.3.
+/// The Version crate doesn't work with the 'v' prefix.
+pub(crate) fn strip_v_prefix(version: &str) -> &str {
+    version.strip_prefix('v').unwrap_or(version)
+}
+
 pub(crate) async fn upgrade_path_validation(
     namespace: &str,
     allow_unstable: bool,
@@ -224,7 +230,8 @@ pub(crate) async fn upgrade_path_validation(
     let mut self_version: Option<Version> = None;
     if let Some(tag) = self_version_info.version_tag {
         if !tag.is_empty() {
-            if let Ok(sv) = Version::parse(tag.as_str()) {
+            let tag = strip_v_prefix(tag.as_str());
+            if let Ok(sv) = Version::parse(tag) {
                 self_version = Some(sv);
             }
         }
@@ -234,7 +241,7 @@ pub(crate) async fn upgrade_path_validation(
     if !allow_unstable {
         let mut self_is_stable: bool = false;
         if let Some(ref version) = self_version {
-            if !version.pre.is_empty() {
+            if version.pre.is_empty() {
                 self_is_stable = true;
             }
         }