Ignore replicaCount when autoscale is enabled

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,19 +165,58 @@ func checkDeprecatedSettings(iop *v1alpha1.IstioOperatorSpec) (util.Errors, stri
 			}
 		}
 	}
-	return errs, strings.Join(messages, "\n")
+	return errs, messages
 }
 
+type FeatureValidator func(*valuesv1alpha1.Values, *v1alpha1.IstioOperatorSpec) (util.Errors, []string)
+
 // 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) (errs util.Errors, warnings []string) {
+	validators := []FeatureValidator{
+		CheckServicePorts,
+		CheckAutoScaleAndReplicaCount,
+	}
+
+	for _, validator := range validators {
+		newErrs, newWarnings := validator(values, spec)
+		errs = util.AppendErrs(errs, newErrs)
+		warnings = append(warnings, newWarnings...)
+	}
+
+	return
+}
+
+// CheckAutoScaleAndReplicaCount warns when autoscaleEnabled is true and k8s replicaCount is set.
+func CheckAutoScaleAndReplicaCount(values *valuesv1alpha1.Values, spec *v1alpha1.IstioOperatorSpec) (errs util.Errors, warnings []string) {
+	if values.GetPilot().GetAutoscaleEnabled().GetValue() && spec.GetComponents().GetPilot().GetK8S().GetReplicaCount() != 0 {
+		warnings = append(warnings,
+			"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 {
+				warnings = append(warnings, 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
 }
 
 // CheckServicePorts validates Service ports. Specifically, this currently
 // asserts that all ports will bind to a port number greater than 1024 when not
 // running as root.
-func CheckServicePorts(values *valuesv1alpha1.Values, spec *v1alpha1.IstioOperatorSpec) util.Errors {
-	var errs util.Errors
+func CheckServicePorts(values *valuesv1alpha1.Values, spec *v1alpha1.IstioOperatorSpec) (errs util.Errors, warnings []string) {
 	if !values.GetGateways().GetIstioIngressgateway().GetRunAsRoot().GetValue() {
 		errs = util.AppendErrs(errs, validateGateways(spec.GetComponents().GetIngressGateways(), "istio-ingressgateway"))
 	}
@@ -203,7 +247,7 @@ func CheckServicePorts(values *valuesv1alpha1.Values, spec *v1alpha1.IstioOperat
 			errs = util.AppendErr(errs, fmt.Errorf("port %v is invalid: targetPort is set to %v, which requires root. Set targetPort to be greater than 1024 or configure values.gateways.istio-ingressgateway.runAsRoot=true", portnum, tp))
 		}
 	}
-	return errs
+	return
 }
 
 func validateGateways(gw []*v1alpha1.GatewaySpec, name string) util.Errors {

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,110 @@
+// Copyright Istio Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package translate
+
+import (
+	"fmt"
+	"testing"
+
+	"istio.io/api/operator/v1alpha1"
+	"istio.io/istio/operator/pkg/name"
+	"istio.io/istio/operator/pkg/util"
+	"istio.io/istio/pkg/test/util/assert"
+)
+
+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 = &v1alpha1.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)
+		})
+	}
+}

releasenotes/notes/36928.yaml

@@ -0,0 +1,8 @@
+apiVersion: release-notes/v2
+kind: bug-fix
+area: installation
+
+releaseNotes:
+- |
+  **Fixed** an issue that was preventing the operator from updating deployments when `.autoscaleEnabled` is `true` and `.k8s.replicaCount` is nonzero.
+  When both autoscale is enabled and replicaCount is nonzero, warning messages will be generated during validation.