Merge pull request #1754 from johannesfrey/prevent-distro-store-switc…

…h-vclusterctl-5

Prevent distro store switch vclusterctl

.github/workflows/e2e.yaml

@@ -204,6 +204,7 @@ jobs:
       - name: install sed
         run: |
           sudo apt-get install -y sed
+
       - name: create vcluster with current cli
         run: |
           chmod +x ./vcluster-current

config/config.go

@@ -111,6 +111,33 @@ func (c *Config) Distro() string {
 	return K8SDistro
 }
 
+// ValidateChanges checks for disallowed config changes.
+// Currently only certain backingstore changes are allowed but no distro change.
+func ValidateChanges(oldCfg, newCfg *Config) error {
+	oldDistro, newDistro := oldCfg.Distro(), newCfg.Distro()
+	oldBackingStore, newBackingStore := oldCfg.BackingStoreType(), newCfg.BackingStoreType()
+
+	return ValidateStoreAndDistroChanges(newBackingStore, oldBackingStore, newDistro, oldDistro)
+}
+
+// ValidateStoreAndDistroChanges checks whether migrating from one store to the other is allowed.
+func ValidateStoreAndDistroChanges(currentStoreType, previousStoreType StoreType, currentDistro, previousDistro string) error {
+	if currentDistro != previousDistro {
+		return fmt.Errorf("seems like you were using %s as a distro before and now have switched to %s, please make sure to not switch between vCluster distros", previousDistro, currentDistro)
+	}
+
+	if currentStoreType != previousStoreType {
+		if currentStoreType != StoreTypeEmbeddedEtcd {
+			return fmt.Errorf("seems like you were using %s as a store before and now have switched to %s, please make sure to not switch between vCluster stores", previousStoreType, currentStoreType)
+		}
+		if previousStoreType != StoreTypeExternalEtcd && previousStoreType != StoreTypeEmbeddedDatabase {
+			return fmt.Errorf("seems like you were using %s as a store before and now have switched to %s, please make sure to not switch between vCluster stores", previousStoreType, currentStoreType)
+		}
+	}
+
+	return nil
+}
+
 func (c *Config) IsProFeatureEnabled() bool {
 	if len(c.Networking.ResolveDNS) > 0 {
 		return true

pkg/cli/create_helm.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/ghodss/yaml"
 	"github.com/loft-sh/log"
 	"github.com/loft-sh/log/survey"
 	"github.com/loft-sh/log/terminal"
@@ -146,33 +147,54 @@ func CreateHelm(ctx context.Context, options *CreateOptions, globalFlags *flags.
 		}
 	}
 
-	// TODO Refactor after vCluster 0.19.x resp. the old config format is out of support.
-	// Early abort if a user runs a virtual cluster < v0.20 without providing any values files during an upgrade.
-	// We do this because we don't want to "automagically" convert the old config implicitly, without the user
-	// realizing that the virtual cluster is running with the old config format.
-	if isVClusterDeployed(release) && isLegacyVCluster(release.Chart.Metadata.Version) && len(cmd.Values) == 0 {
-		// If we have a < v0.20 virtual cluster running we have to infer the distro from the current chart name.
-		currentDistro := strings.TrimPrefix(release.Chart.Metadata.Name, "vcluster-")
-		// If we are upgrading a vCluster < v0.20 the old k3s chart is the one without a prefix.
-		if currentDistro == "vcluster" {
-			currentDistro = config.K3SDistro
-		}
-		// A virtual cluster could either be created via vcluster CLI or via helm.
-		// When using vcluster CLI we always have extra values configured.
-		// When using helm without any modifications we don't have any extra values,
-		// so we must take the default values from the release into account.
-		helmCommand := fmt.Sprintf("helm -n %s get values %s -o yaml", cmd.Namespace, vClusterName)
-		if release.Config == nil {
-			helmCommand = fmt.Sprintf("%s -a", helmCommand)
+	currentVClusterConfig := &config.Config{}
+	if isVClusterDeployed(release) {
+		currentValues, err := helmExtraValuesYAML(release)
+		if err != nil {
+			return err
 		}
+		// TODO Delete after vCluster 0.19.x resp. the old config format is out of support.
+		if isLegacyVCluster(release.Chart.Metadata.Version) {
+			// If we have a < v0.20 virtual cluster running we have to infer the distro from the current chart name.
+			currentDistro := strings.TrimPrefix(release.Chart.Metadata.Name, "vcluster-")
+			// If we are upgrading a vCluster < v0.20 the old k3s chart is the one without a prefix.
+			if currentDistro == "vcluster" {
+				currentDistro = config.K3SDistro
+			}
+			// Early abort if a user runs a virtual cluster < v0.20 without providing any values files during an upgrade.
+			// We do this because we don't want to "automagically" convert the old config implicitly, without the user
+			// realizing that the virtual cluster is running with the old config format.
+			if len(cmd.Values) == 0 {
+				helmCommand := fmt.Sprintf("helm -n %s get values %s -o yaml", cmd.Namespace, vClusterName)
+				if currentValues == "" {
+					helmCommand = fmt.Sprintf("%s -a", helmCommand)
+				}
 
-		command := fmt.Sprintf("%s | vcluster convert config --distro %s", helmCommand, currentDistro)
-		return fmt.Errorf("it appears you are using a vCluster configuration using pre-v0.20 formatting. Please run the following to convert the values to the latest format:\n%s", command)
+				command := fmt.Sprintf("%s | vcluster convert config --distro %s", helmCommand, currentDistro)
+				return fmt.Errorf("it appears you are using a vCluster configuration using pre-v0.20 formatting. Please run the following to convert the values to the latest format:\n%s", command)
+			}
 
-		// TODO(johannesfrey): Later we want to save the current values in order to be able to validate them against newly given values below.
-		// If it happens to be a legacy config, we need to convert values here as well to the new format in order to be able validate them against newly given values below.
+			// At this point the user did pass a config file.
+			// We have to convert the current old vcluster config to the new format in order to be able validate it against the passed in vcluster configs below.
+			migratedValues, err := legacyconfig.MigrateLegacyConfig(currentDistro, currentValues)
+			// TODO(johannesfrey): Let's log here all unsupported fields that have been discovered during the migration.
+			if err != nil {
+				// If the user previously used values that are now unsupported create a fresh config for the given distro.
+				migratedValues, err = legacyconfig.MigrateLegacyConfig(currentDistro, "")
+				if err != nil {
+					return err
+				}
+			}
+			if err := currentVClusterConfig.UnmarshalYAMLStrict([]byte(migratedValues)); err != nil {
+				return err
+			}
+		} else {
+			if err := currentVClusterConfig.UnmarshalYAMLStrict([]byte(currentValues)); err != nil {
+				return err
+			}
+		}
+		// TODO end
 	}
-	// TODO end
 
 	// build extra values
 	var newExtraValues []string
@@ -232,6 +254,13 @@ func CreateHelm(ctx context.Context, options *CreateOptions, globalFlags *flags.
 		return err
 	}
 
+	if isVClusterDeployed(release) {
+		// While certain backing store changes are allowed we prohibit changes to another distro.
+		if err := config.ValidateChanges(currentVClusterConfig, vClusterConfig); err != nil {
+			return err
+		}
+	}
+
 	// create platform secret
 	if cmd.Activate {
 		err = cmd.activateVCluster(ctx, vClusterConfig)
@@ -350,6 +379,20 @@ func isLegacyConfig(values []byte) bool {
 
 // TODO end
 
+// helmValuesYAML returns the extraValues from the helm release in yaml format.
+// If the extra values in the chart are nil it returns an empty string.
+func helmExtraValuesYAML(release *helm.Release) (string, error) {
+	if release.Config == nil {
+		return "", nil
+	}
+	extraValues, err := yaml.Marshal(release.Config)
+	if err != nil {
+		return "", err
+	}
+
+	return string(extraValues), nil
+}
+
 func getBase64DecodedString(values string) (string, error) {
 	strDecoded, err := base64.StdEncoding.DecodeString(values)
 	if err != nil {

pkg/setup/config.go

@@ -139,7 +139,7 @@ func CheckUsingHelm(ctx context.Context, client kubernetes.Interface, name, name
 			return false, nil
 		}
 
-		if err := validateChanges(
+		if err := vclusterconfig.ValidateStoreAndDistroChanges(
 			backingStoreType,
 			previousConfig.BackingStoreType(),
 			distro,
@@ -201,7 +201,7 @@ func CheckUsingHelm(ctx context.Context, client kubernetes.Interface, name, name
 		}
 	}
 
-	if err := validateChanges(backingStoreType, previousStoreType, distro, previousDistro); err != nil {
+	if err := vclusterconfig.ValidateStoreAndDistroChanges(backingStoreType, previousStoreType, distro, previousDistro); err != nil {
 		return false, err
 	}
 
@@ -247,15 +247,15 @@ func CheckUsingSecretAnnotation(ctx context.Context, client kubernetes.Interface
 	// Thus we can check if the distro has changed.
 	okCounter := 0
 	if annotatedDistro, ok := secret.Annotations[AnnotationDistro]; ok {
-		if err := validateChanges("", "", distro, annotatedDistro); err != nil {
+		if err := vclusterconfig.ValidateStoreAndDistroChanges("", "", distro, annotatedDistro); err != nil {
 			return false, err
 		}
 
 		okCounter++
 	}
 
 	if annotatedStore, ok := secret.Annotations[AnnotationStore]; ok {
-		if err := validateChanges(backingStoreType, vclusterconfig.StoreType(annotatedStore), "", ""); err != nil {
+		if err := vclusterconfig.ValidateStoreAndDistroChanges(backingStoreType, vclusterconfig.StoreType(annotatedStore), "", ""); err != nil {
 			return false, err
 		}
 
@@ -290,21 +290,3 @@ func updateSecretAnnotations(ctx context.Context, client kubernetes.Interface, n
 		return nil
 	})
 }
-
-// validateChanges checks whether migrating from one store to the other is allowed.
-func validateChanges(currentStoreType, previousStoreType vclusterconfig.StoreType, currentDistro, previousDistro string) error {
-	if currentDistro != previousDistro {
-		return fmt.Errorf("seems like you were using %s as a distro before and now have switched to %s, please make sure to not switch between vCluster distros", previousDistro, currentDistro)
-	}
-
-	if currentStoreType != previousStoreType {
-		if currentStoreType != vclusterconfig.StoreTypeEmbeddedEtcd {
-			return fmt.Errorf("seems like you were using %s as a store before and now have switched to %s, please make sure to not switch between vCluster stores", previousStoreType, currentStoreType)
-		}
-		if previousStoreType != vclusterconfig.StoreTypeExternalEtcd && previousStoreType != vclusterconfig.StoreTypeEmbeddedDatabase {
-			return fmt.Errorf("seems like you were using %s as a store before and now have switched to %s, please make sure to not switch between vCluster stores", previousStoreType, currentStoreType)
-		}
-	}
-
-	return nil
-}