istio config validation + helm chart fix (#2722)

* disallowing vcluster.yaml combination with istio integration that can lead to undefined behavior
* create cluster role when istio integration is enabled
---------

Signed-off-by: Paweł Bojanowski <pawel.bojanowski@loft.sh>

Author: Paweł Bojanowski

chart/templates/_rbac.tpl

@@ -43,7 +43,9 @@
     (and .Values.integrations.metricsServer.enabled .Values.integrations.metricsServer.nodes)
     .Values.experimental.multiNamespaceMode.enabled
 	.Values.sync.fromHost.configMaps.enabled
-    .Values.sync.fromHost.secrets.enabled -}}
+    .Values.sync.fromHost.secrets.enabled
+     .Values.integrations.istio.enabled
+     -}}
 {{- true -}}
 {{- end -}}
 {{- end -}}

chart/tests/clusterrole_test.yaml

@@ -683,3 +683,23 @@ tests:
             resourceNames: [ "my-secret", "my-secret-2" ]
             resources: [ "secrets" ]
             verbs: [ "get" ]
+  - it: istio enabled
+    set:
+      integrations:
+        istio:
+          enabled: true
+    release:
+      name: my-release
+      namespace: my-namespace
+    asserts:
+      - hasDocuments:
+          count: 1
+      - lengthEqual:
+          path: rules
+          count: 1
+      - contains:
+          path: rules
+          content:
+            apiGroups: [ "apiextensions.k8s.io" ]
+            resources: [ "customresourcedefinitions" ]
+            verbs: [ "get", "list", "watch" ]

pkg/config/validation.go

@@ -65,6 +65,11 @@ func ValidateConfigAndSetDefaults(vConfig *VirtualClusterConfig) error {
 		return fmt.Errorf("embedded database is not supported with multiple replicas")
 	}
 
+	// disallow listing istio CRDs in sync.toHost.customResources if istio integration is enabled
+	if err := validateIstioEnabled(vConfig.Sync.ToHost.CustomResources, vConfig.Integrations.Istio); err != nil {
+		return err
+	}
+
 	// check if custom resources have correct scope
 	for key, customResource := range vConfig.Sync.ToHost.CustomResources {
 		if customResource.Scope != "" && customResource.Scope != config.ScopeNamespaced {
@@ -723,6 +728,33 @@ func validateExportKubeConfig(exportKubeConfig config.ExportKubeConfig) error {
 	return nil
 }
 
+func validateIstioEnabled(toHostCustomResources map[string]config.SyncToHostCustomResource, istioIntegration config.Istio) error {
+	if !istioIntegration.Enabled {
+		return nil
+	}
+	for crdName, crdConfig := range toHostCustomResources {
+		if crdName == "destinationrules.networking.istio.io" && crdConfig.Enabled && istioIntegration.Sync.ToHost.DestinationRules.Enabled {
+			return errors.New("" +
+				"istio integration is enabled but istio custom resource (destinationrules.networking.istio.io) is also set in the sync.toHost.customResources. " +
+				"This is not supported, please remove the entry from sync.toHost.customResources",
+			)
+		}
+		if crdName == "gateways.networking.istio.io" && crdConfig.Enabled && istioIntegration.Sync.ToHost.Gateways.Enabled {
+			return errors.New("" +
+				"istio integration is enabled but istio custom resource (gateways.networking.istio.io) is also set in the sync.toHost.customResources. " +
+				"This is not supported, please remove the entry from sync.toHost.customResources",
+			)
+		}
+		if crdName == "virtualservices.networking.istio.io" && crdConfig.Enabled && istioIntegration.Sync.ToHost.VirtualServices.Enabled {
+			return errors.New("" +
+				"istio integration is enabled but istio custom resource (virtualservices.networking.istio.io) is also set in the sync.toHost.customResources. " +
+				"This is not supported, please remove the entry from sync.toHost.customResources",
+			)
+		}
+	}
+	return nil
+}
+
 var ProValidateConfig = func(_ *VirtualClusterConfig) error {
 	return nil
 }

pkg/config/validation_test.go

@@ -411,24 +411,6 @@ func TestValidateFromHostSyncMappings(t *testing.T) {
 }
 
 func TestValidateFromHostSyncCustomResources(t *testing.T) {
-	noErrExpected := func(t *testing.T, err error) {
-		t.Helper()
-		if err != nil {
-			t.Fatalf("unexpected error: %v", err)
-		}
-	}
-	expectErr := func(errMsg string) func(t *testing.T, err error) {
-		return func(t *testing.T, err error) {
-			t.Helper()
-			if err == nil {
-				t.Fatal("expected error, got nil")
-			}
-			if err.Error() != errMsg {
-				t.Fatalf("unexpected error: %v", err)
-			}
-		}
-	}
-
 	cases := []struct {
 		name                        string
 		customResourcesFromHostSync map[string]config.SyncFromHostCustomResource
@@ -757,3 +739,128 @@ func TestValidateAllSyncPatches(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateToHostSyncAndIstioIntegration(t *testing.T) {
+	istioEnabled := config.Istio{
+		EnableSwitch: config.EnableSwitch{Enabled: true},
+		Sync: config.IstioSync{ToHost: config.IstioSyncToHost{
+			DestinationRules: config.EnableSwitch{Enabled: true},
+			Gateways:         config.EnableSwitch{Enabled: true},
+			VirtualServices:  config.EnableSwitch{Enabled: true},
+		}},
+	}
+	istioDisabled := config.Istio{
+		EnableSwitch: config.EnableSwitch{Enabled: false},
+	}
+
+	cases := []struct {
+		name                      string
+		customResourcesToHostSync map[string]config.SyncToHostCustomResource
+		istioIntegration          config.Istio
+		checkErr                  func(t *testing.T, err error)
+	}{
+		{
+			name: "valid config",
+			customResourcesToHostSync: map[string]config.SyncToHostCustomResource{
+				"clusterissuers.cert-manager.io": {
+					Enabled: true,
+					Scope:   config.ScopeCluster,
+				},
+			},
+			istioIntegration: istioEnabled,
+			checkErr:         noErrExpected,
+		},
+		{
+			name: "destination rule listed in sync.toHost.customResources",
+			customResourcesToHostSync: map[string]config.SyncToHostCustomResource{
+				"destinationrules.networking.istio.io": {
+					Enabled: true,
+					Scope:   config.ScopeNamespaced,
+				},
+			},
+			istioIntegration: istioEnabled,
+			checkErr:         expectErr("istio integration is enabled but istio custom resource (destinationrules.networking.istio.io) is also set in the sync.toHost.customResources. This is not supported, please remove the entry from sync.toHost.customResources"),
+		},
+		{
+			name: "destination rule listed in sync.toHost.customResources but istio disabled",
+			customResourcesToHostSync: map[string]config.SyncToHostCustomResource{
+				"destinationrules.networking.istio.io": {
+					Enabled: true,
+					Scope:   config.ScopeNamespaced,
+				},
+			},
+			istioIntegration: istioDisabled,
+			checkErr:         noErrExpected,
+		},
+		{
+			name: "gateways listed in sync.toHost.customResources",
+			customResourcesToHostSync: map[string]config.SyncToHostCustomResource{
+				"gateways.networking.istio.io": {
+					Enabled: true,
+					Scope:   config.ScopeNamespaced,
+				},
+			},
+			istioIntegration: istioEnabled,
+			checkErr:         expectErr("istio integration is enabled but istio custom resource (gateways.networking.istio.io) is also set in the sync.toHost.customResources. This is not supported, please remove the entry from sync.toHost.customResources"),
+		},
+		{
+			name: "gateways listed in sync.toHost.customResources but istio disabled",
+			customResourcesToHostSync: map[string]config.SyncToHostCustomResource{
+				"gateways.networking.istio.io": {
+					Enabled: true,
+					Scope:   config.ScopeNamespaced,
+				},
+			},
+			istioIntegration: istioDisabled,
+			checkErr:         noErrExpected,
+		},
+		{
+			name: "virtual services listed in sync.toHost.customResources",
+			customResourcesToHostSync: map[string]config.SyncToHostCustomResource{
+				"virtualservices.networking.istio.io": {
+					Enabled: true,
+					Scope:   config.ScopeNamespaced,
+				},
+			},
+			istioIntegration: istioEnabled,
+			checkErr:         expectErr("istio integration is enabled but istio custom resource (virtualservices.networking.istio.io) is also set in the sync.toHost.customResources. This is not supported, please remove the entry from sync.toHost.customResources"),
+		},
+		{
+			name: "virtual services listed in sync.toHost.customResources but istio disabled",
+			customResourcesToHostSync: map[string]config.SyncToHostCustomResource{
+				"virtualservices.networking.istio.io": {
+					Enabled: true,
+					Scope:   config.ScopeNamespaced,
+				},
+			},
+			istioIntegration: istioDisabled,
+			checkErr:         noErrExpected,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateIstioEnabled(tc.customResourcesToHostSync, tc.istioIntegration)
+			tc.checkErr(t, err)
+		})
+	}
+}
+
+func expectErr(errMsg string) func(t *testing.T, err error) {
+	return func(t *testing.T, err error) {
+		t.Helper()
+		if err == nil {
+			t.Fatal("expected error, got nil")
+		}
+		if err.Error() != errMsg {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	}
+}
+
+func noErrExpected(t *testing.T, err error) {
+	t.Helper()
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}