Unset replicaCount when autoscale is enabled

Signed-off-by: Tianpeng Wang <tpwang@alauda.io>

operator/pkg/apis/istio/v1alpha1/validation/validation.go

@@ -43,20 +43,25 @@ type deprecatedSettings struct {
 // ValidateConfig  calls validation func for every defined element in Values
 func ValidateConfig(failOnMissingValidation bool, iopls *v1alpha1.IstioOperatorSpec) (util.Errors, string) {
 	var validationErrors util.Errors
-	var warningMessage string
+	var warningMessages []string
 	iopvalString := util.ToYAML(iopls.Values)
 	values := &valuesv1alpha1.Values{}
 	if err := util.UnmarshalWithJSONPB(iopvalString, values, true); err != nil {
 		return util.NewErrs(err), ""
 	}
 
 	validationErrors = util.AppendErrs(validationErrors, ValidateSubTypes(reflect.ValueOf(values).Elem(), failOnMissingValidation, values, iopls))
-	validationErrors = util.AppendErrs(validationErrors, validateFeatures(values, iopls))
-	deprecatedErrors, warningMessage := checkDeprecatedSettings(iopls)
+
+	featureErrors, featureWarningMessages := validateFeatures(values, iopls)
+	validationErrors = util.AppendErrs(validationErrors, featureErrors)
+	warningMessages = append(warningMessages, featureWarningMessages...)
+
+	deprecatedErrors, deprecatedWarningMessages := checkDeprecatedSettings(iopls)
 	if deprecatedErrors != nil {
 		validationErrors = util.AppendErr(validationErrors, deprecatedErrors)
 	}
-	return validationErrors, warningMessage
+	warningMessages = append(warningMessages, deprecatedWarningMessages...)
+	return validationErrors, strings.Join(warningMessages, "\n")
 }
 
 // Converts from struct paths to helm paths
@@ -78,7 +83,7 @@ func firstCharsToLower(s string) string {
 		s)
 }
 
-func checkDeprecatedSettings(iop *v1alpha1.IstioOperatorSpec) (util.Errors, string) {
+func checkDeprecatedSettings(iop *v1alpha1.IstioOperatorSpec) (util.Errors, []string) {
 	var errs util.Errors
 	messages := []string{}
 	warningSettings := []deprecatedSettings{
@@ -160,12 +165,43 @@ func checkDeprecatedSettings(iop *v1alpha1.IstioOperatorSpec) (util.Errors, stri
 			}
 		}
 	}
-	return errs, strings.Join(messages, "\n")
+	return errs, messages
 }
 
 // validateFeatures check whether the config sematically make sense. For example, feature X and feature Y can't be enabled together.
-func validateFeatures(values *valuesv1alpha1.Values, spec *v1alpha1.IstioOperatorSpec) util.Errors {
-	return CheckServicePorts(values, spec)
+func validateFeatures(values *valuesv1alpha1.Values, spec *v1alpha1.IstioOperatorSpec) (util.Errors, []string) {
+	errs := CheckServicePorts(values, spec)
+	warningMessages := checkAutoScaleAndReplicaCount(values, spec)
+	return errs, warningMessages
+}
+
+// checkAutoScaleAndReplicaCount warns when autoscaleEnabled is true and k8s replicaCount is set.
+func checkAutoScaleAndReplicaCount(values *valuesv1alpha1.Values, spec *v1alpha1.IstioOperatorSpec) []string {
+	var messages []string
+
+	if values.GetPilot().GetAutoscaleEnabled().GetValue() && spec.GetComponents().GetPilot().GetK8S().GetReplicaCount() != 0 {
+		messages = append(messages,
+			"components.pilot.k8s.replicaCount should not be set when values.pilot.autoscaleEnabled is true")
+	}
+
+	validateGateways := func(gateways []*v1alpha1.GatewaySpec, gwType string) {
+		const format = "components.%sGateways[name=%s].k8s.replicaCount should not be set when values.gateways.istio-%sgateway.autoscaleEnabled is true"
+		for _, gw := range gateways {
+			if gw.GetK8S().GetReplicaCount() != 0 {
+				messages = append(messages, fmt.Sprintf(format, gwType, gw.Name, gwType))
+			}
+		}
+	}
+
+	if values.GetGateways().GetIstioIngressgateway().GetAutoscaleEnabled().GetValue() {
+		validateGateways(spec.GetComponents().GetIngressGateways(), "ingress")
+	}
+
+	if values.GetGateways().GetIstioEgressgateway().GetAutoscaleEnabled().GetValue() {
+		validateGateways(spec.GetComponents().GetEgressGateways(), "egress")
+	}
+
+	return messages
 }
 
 // CheckServicePorts validates Service ports. Specifically, this currently

operator/pkg/apis/istio/v1alpha1/validation/validation_test.go

@@ -18,6 +18,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/gogo/protobuf/types"
@@ -193,6 +194,38 @@ values:
 `,
 			errors: ``,
 		},
+		{
+			name: "replicaCount set when autoscaleEnabled is true",
+			values: `
+values:
+  pilot:
+    autoscaleEnabled: true
+  gateways:
+    istio-ingressgateway:
+      autoscaleEnabled: true
+    istio-egressgateway:
+      autoscaleEnabled: true
+components:
+  pilot:
+    k8s:
+      replicaCount: 2
+  ingressGateways:
+    - name: istio-ingressgateway
+      enabled: true
+      k8s:
+        replicaCount: 2
+  egressGateways:
+    - name: istio-egressgateway
+      enabled: true
+      k8s:
+        replicaCount: 2
+`,
+			warnings: strings.TrimSpace(`
+components.pilot.k8s.replicaCount should not be set when values.pilot.autoscaleEnabled is true
+components.ingressGateways[name=istio-ingressgateway].k8s.replicaCount should not be set when values.gateways.istio-ingressgateway.autoscaleEnabled is true
+components.egressGateways[name=istio-egressgateway].k8s.replicaCount should not be set when values.gateways.istio-egressgateway.autoscaleEnabled is true
+`),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -205,10 +238,10 @@ values:
 			}
 			err, warnings := validation.ValidateConfig(false, iop)
 			if tt.errors != err.String() {
-				t.Fatalf("expected errors: %q got %q", tt.errors, err.String())
+				t.Fatalf("expected errors: \n%q\n got: \n%q\n", tt.errors, err.String())
 			}
 			if tt.warnings != warnings {
-				t.Fatalf("expected warnings: %q got %q", tt.warnings, warnings)
+				t.Fatalf("expected warnings: \n%q\n got \n%q\n", tt.warnings, warnings)
 			}
 		})
 	}

operator/pkg/translate/translate.go

@@ -249,6 +249,19 @@ func (t *Translator) OverlayK8sSettings(yml string, iop *v1alpha1.IstioOperatorS
 			continue
 		}
 
+		// When autoscale is enabled we should not overwrite replica count, consider following scenario:
+		// 0. Set values.pilot.autoscaleEnabled=true, components.pilot.k8s.replicaCount=1
+		// 1. In istio operator it "caches" the generated manifests (with istiod.replicas=1)
+		// 2. HPA autoscales our pilot replicas to 3
+		// 3. Set values.pilot.autoscaleEnabled=false
+		// 4. The generated manifests (with istiod.replicas=1) is same as istio operator "cache",
+		//    the deployment will not get updated unless istio operator is restarted.
+		if inPathParts[len(inPathParts)-1] == "ReplicaCount" {
+			if skipReplicaCountWithAutoscaleEnabled(iop, componentName) {
+				continue
+			}
+		}
+
 		// strategic merge overlay m to the base object oo
 		mergedObj, err := MergeK8sObject(oo, m, path[1:])
 		if err != nil {
@@ -273,6 +286,27 @@ func (t *Translator) OverlayK8sSettings(yml string, iop *v1alpha1.IstioOperatorS
 	return objects.YAMLManifest()
 }
 
+var componentToAutoScaleEnabledPath = map[name.ComponentName]string{
+	name.PilotComponentName:   "Values.pilot.autoscaleEnabled",
+	name.IngressComponentName: "Values.gateways.istio-ingressgateway.autoscaleEnabled",
+	name.EgressComponentName:  "Values.gateways.istio-egressgateway.autoscaleEnabled",
+}
+
+func skipReplicaCountWithAutoscaleEnabled(iop *v1alpha1.IstioOperatorSpec, componentName name.ComponentName) bool {
+	path, ok := componentToAutoScaleEnabledPath[componentName]
+	if !ok {
+		return false
+	}
+
+	enabledVal, found, err := tpath.GetFromStructPath(iop, path)
+	if err != nil || !found {
+		return false
+	}
+
+	enabled, ok := enabledVal.(bool)
+	return ok && enabled
+}
+
 func (t *Translator) fixMergedObjectWithCustomServicePortOverlay(oo *object.K8sObject,
 	msvc *v1alpha1.ServiceSpec, mergedObj *object.K8sObject) (*object.K8sObject, error) {
 	var basePorts []*v1.ServicePort

operator/pkg/translate/translate_test.go

@@ -0,0 +1,98 @@
+package translate
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"istio.io/api/operator/v1alpha1"
+	v1alpha12 "istio.io/api/operator/v1alpha1"
+	"istio.io/istio/operator/pkg/name"
+	"istio.io/istio/operator/pkg/util"
+)
+
+func Test_skipReplicaCountWithAutoscaleEnabled(t *testing.T) {
+	const valuesWithHPAndReplicaCountFormat = `
+values:
+  pilot:
+    autoscaleEnabled: %t
+  gateways:
+    istio-ingressgateway:
+      autoscaleEnabled: %t
+    istio-egressgateway:
+      autoscaleEnabled: %t
+components:
+  pilot:
+    k8s:
+      replicaCount: 2
+  ingressGateways:
+    - name: istio-ingressgateway
+      enabled: true
+      k8s:
+        replicaCount: 2
+  egressGateways:
+    - name: istio-egressgateway
+      enabled: true
+      k8s:
+        replicaCount: 2
+`
+
+	cases := []struct {
+		name       string
+		component  name.ComponentName
+		values     string
+		expectSkip bool
+	}{
+		{
+			name:       "hpa enabled for pilot without replicas",
+			component:  name.PilotComponentName,
+			values:     fmt.Sprintf(valuesWithHPAndReplicaCountFormat, false, false, false),
+			expectSkip: false,
+		},
+		{
+			name:       "hpa enabled for ingressgateway without replica",
+			component:  name.IngressComponentName,
+			values:     fmt.Sprintf(valuesWithHPAndReplicaCountFormat, false, false, false),
+			expectSkip: false,
+		},
+		{
+			name:       "hpa enabled for pilot without replicas",
+			component:  name.EgressComponentName,
+			values:     fmt.Sprintf(valuesWithHPAndReplicaCountFormat, false, false, false),
+			expectSkip: false,
+		},
+		{
+			name:       "hpa enabled for pilot with replicas",
+			component:  name.PilotComponentName,
+			values:     fmt.Sprintf(valuesWithHPAndReplicaCountFormat, true, false, false),
+			expectSkip: true,
+		},
+		{
+			name:       "hpa enabled for ingressgateway with replicass",
+			component:  name.IngressComponentName,
+			values:     fmt.Sprintf(valuesWithHPAndReplicaCountFormat, false, true, false),
+			expectSkip: true,
+		},
+		{
+			name:       "hpa enabled for egressgateway with replicas",
+			component:  name.EgressComponentName,
+			values:     fmt.Sprintf(valuesWithHPAndReplicaCountFormat, true, false, true),
+			expectSkip: true,
+		},
+	}
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			var iop *v1alpha1.IstioOperatorSpec
+			if tt.values != "" {
+				iop = &v1alpha12.IstioOperatorSpec{}
+				if err := util.UnmarshalWithJSONPB(tt.values, iop, true); err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			got := skipReplicaCountWithAutoscaleEnabled(iop, tt.component)
+			assert.Equal(t, tt.expectSkip, got)
+		})
+	}
+}