fix(parameters): reject immutable parameter updates (#10204)

Author: weicao

pkg/controller/configuration/patch_merger.go

@@ -60,6 +60,14 @@ func mergeUpdatedParams(base map[string]string,
 	// merge updated files into configmap
 	if len(updatedFiles) != 0 {
 		updatedConfig = core.MergeUpdatedConfig(base, updatedFiles)
+		// Reject content-level updates that would alter any immutable parameter.
+		// MergeUpdatedConfig substitutes whole file contents key-by-key, so
+		// without this guard a user submitting raw file content could bypass
+		// the parameter-level immutable check performed below by
+		// MergeAndValidateConfigs/filterImmutableParameters.
+		if err := intctrlutil.ValidateImmutableContentChanges(base, updatedConfig, updatedFiles, paramsDefs, configDescs); err != nil {
+			return nil, err
+		}
 	}
 	if len(configDescs) == 0 {
 		return updatedConfig, nil

pkg/controller/configuration/patch_merger_test.go

@@ -0,0 +1,264 @@
+/*
+Copyright (C) 2022-2025 ApeCloud Co., Ltd
+
+This file is part of KubeBlocks project
+
+This program is free software: you can redistribute it and/or modify
+it under the terms of the GNU Affero General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU Affero General Public License for more details.
+
+You should have received a copy of the GNU Affero General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+package configuration
+
+import (
+	"strings"
+	"testing"
+
+	parametersv1alpha1 "github.com/apecloud/kubeblocks/apis/parameters/v1alpha1"
+)
+
+// The following tests cover the content-path immutable guard added in
+// intctrlutil.ValidateImmutableContentChanges (release-1.0 counterpart of
+// the main-branch PR; see pkg/parameters/patch_merger_test.go on main for
+// the same coverage matrix).
+//
+// Coverage matrix:
+//   - content modifies an immutable parameter   -> reject
+//   - content adds an immutable parameter       -> reject
+//   - content removes an immutable parameter    -> reject
+//   - content only reorders / re-formats        -> accept
+//   - content has mutable change plus format
+//     noise but immutable unchanged             -> accept
+//   - file has no immutable candidate, missing
+//     FileFormatConfig                          -> accept
+//   - file has immutable candidate but missing
+//     FileFormatConfig                          -> reject (fail-safe)
+
+func strPtr(v string) *string {
+	return &v
+}
+
+func iniConfigDescsForMyCnf() []parametersv1alpha1.ComponentConfigDescription {
+	return []parametersv1alpha1.ComponentConfigDescription{{
+		Name:         "my.cnf",
+		TemplateName: "mysql-config",
+		FileFormatConfig: &parametersv1alpha1.FileFormatConfig{
+			Format: parametersv1alpha1.Ini,
+			FormatterAction: parametersv1alpha1.FormatterAction{
+				IniConfig: &parametersv1alpha1.IniConfig{SectionName: "mysqld"},
+			},
+		},
+	}}
+}
+
+func paramsDefsWithImmutable(immutable []string) []*parametersv1alpha1.ParametersDefinition {
+	return []*parametersv1alpha1.ParametersDefinition{{
+		Spec: parametersv1alpha1.ParametersDefinitionSpec{
+			FileName:            "my.cnf",
+			ImmutableParameters: immutable,
+		},
+	}}
+}
+
+func TestDoMergeRejectsContentModifyingImmutableParameter(t *testing.T) {
+	base := map[string]string{
+		"my.cnf": "[mysqld]\ngtid_mode=OFF\nmax_connections=1000\n",
+	}
+	patch := map[string]parametersv1alpha1.ParametersInFile{
+		"my.cnf": {
+			Content: strPtr("[mysqld]\ngtid_mode=ON\nmax_connections=1000\n"),
+		},
+	}
+	configDescs := iniConfigDescsForMyCnf()
+	paramsDefs := paramsDefsWithImmutable([]string{"gtid_mode"})
+
+	_, err := DoMerge(base, patch, paramsDefs, configDescs)
+	if err == nil {
+		t.Fatalf("expected immutable parameter modification via content to be rejected, got nil error")
+	}
+	if !strings.Contains(err.Error(), "gtid_mode") {
+		t.Fatalf("expected error to mention immutable parameter gtid_mode, got %q", err.Error())
+	}
+}
+
+func TestDoMergeRejectsContentAddingImmutableParameter(t *testing.T) {
+	base := map[string]string{
+		"my.cnf": "[mysqld]\nmax_connections=1000\n",
+	}
+	patch := map[string]parametersv1alpha1.ParametersInFile{
+		"my.cnf": {
+			Content: strPtr("[mysqld]\nmax_connections=1000\ngtid_mode=ON\n"),
+		},
+	}
+	configDescs := iniConfigDescsForMyCnf()
+	paramsDefs := paramsDefsWithImmutable([]string{"gtid_mode"})
+
+	_, err := DoMerge(base, patch, paramsDefs, configDescs)
+	if err == nil {
+		t.Fatalf("expected immutable parameter addition via content to be rejected, got nil error")
+	}
+	if !strings.Contains(err.Error(), "gtid_mode") {
+		t.Fatalf("expected error to mention immutable parameter gtid_mode, got %q", err.Error())
+	}
+}
+
+func TestDoMergeRejectsContentRemovingImmutableParameter(t *testing.T) {
+	base := map[string]string{
+		"my.cnf": "[mysqld]\ngtid_mode=OFF\nmax_connections=1000\n",
+	}
+	patch := map[string]parametersv1alpha1.ParametersInFile{
+		"my.cnf": {
+			Content: strPtr("[mysqld]\nmax_connections=1000\n"),
+		},
+	}
+	configDescs := iniConfigDescsForMyCnf()
+	paramsDefs := paramsDefsWithImmutable([]string{"gtid_mode"})
+
+	_, err := DoMerge(base, patch, paramsDefs, configDescs)
+	if err == nil {
+		t.Fatalf("expected immutable parameter removal via content to be rejected, got nil error")
+	}
+	if !strings.Contains(err.Error(), "gtid_mode") {
+		t.Fatalf("expected error to mention immutable parameter gtid_mode, got %q", err.Error())
+	}
+}
+
+func TestDoMergeAcceptsContentWithOnlyFormatChange(t *testing.T) {
+	base := map[string]string{
+		"my.cnf": "[mysqld]\ngtid_mode=OFF\nmax_connections=1000\n",
+	}
+	// Same semantic content, just reordered and with an added blank line.
+	// Must not be flagged as an immutable change because the parsed parameter
+	// map for gtid_mode is unchanged.
+	patch := map[string]parametersv1alpha1.ParametersInFile{
+		"my.cnf": {
+			Content: strPtr("[mysqld]\n\nmax_connections=1000\ngtid_mode=OFF\n"),
+		},
+	}
+	configDescs := iniConfigDescsForMyCnf()
+	paramsDefs := paramsDefsWithImmutable([]string{"gtid_mode"})
+
+	if _, err := DoMerge(base, patch, paramsDefs, configDescs); err != nil {
+		t.Fatalf("expected format-only content change to be accepted, got error %v", err)
+	}
+}
+
+func TestDoMergeAcceptsContentWithMutableChangeAndFormatNoiseWhileImmutableUnchanged(t *testing.T) {
+	// Realistic MySQL-style fixture covering the joint case the guard must
+	// accept:
+	//   1) format noise — comment rewrites, extra blank lines, `key=value`
+	//      vs `key = value` spacing — must NOT trip the immutable check;
+	//   2) a mutable parameter (max_connections) changes value — that is
+	//      explicitly allowed because it is not in ImmutableParameters;
+	//   3) the immutable parameter (gtid_mode) keeps the same parsed value
+	//      across base and patch, so the guard must let the merge through.
+	//
+	// Together these confirm the guard only triggers on immutable-parameter
+	// semantic delta and ignores everything else.
+	base := map[string]string{
+		"my.cnf": "" +
+			"# header comment\n" +
+			"[mysqld]\n" +
+			"gtid_mode=OFF\n" +
+			"max_connections=1000\n" +
+			"# inline comment\n" +
+			"slow_query_log=ON\n",
+	}
+	patch := map[string]parametersv1alpha1.ParametersInFile{
+		"my.cnf": {
+			Content: strPtr("" +
+				"# rewritten header\n" +
+				"\n" +
+				"[mysqld]\n" +
+				"\n" +
+				"# reordered block\n" +
+				"slow_query_log = ON\n" +
+				"max_connections = 2000\n" +
+				"gtid_mode = OFF\n"),
+		},
+	}
+	configDescs := iniConfigDescsForMyCnf()
+	paramsDefs := paramsDefsWithImmutable([]string{"gtid_mode"})
+
+	if _, err := DoMerge(base, patch, paramsDefs, configDescs); err != nil {
+		t.Fatalf("expected realistic format/whitespace/comment noise without immutable-parameter change to be accepted, got error %v", err)
+	}
+}
+
+func TestDoMergeAcceptsContentOnFileWithoutImmutableCandidate(t *testing.T) {
+	// File has no ParametersDefinition entry, and the content payload uses an
+	// arbitrary format with no FileFormatConfig registered. The guard must
+	// stay out of the way: there is no immutable contract to protect on this
+	// file, so a missing parser must not block a benign content update.
+	base := map[string]string{
+		"custom.conf": "key1=value1\n",
+	}
+	patch := map[string]parametersv1alpha1.ParametersInFile{
+		"custom.conf": {
+			Content: strPtr("key1=value1\nkey2=value2\n"),
+		},
+	}
+
+	if _, err := DoMerge(base, patch, nil, nil); err != nil {
+		t.Fatalf("expected content update on file without immutable candidate to be accepted even without parser, got error %v", err)
+	}
+}
+
+func TestDoMergeRejectsContentRemovingEntireSectionContainingImmutableParameter(t *testing.T) {
+	// Regression for the nil-deref panic introduced by core.FromConfigObject
+	// returning nil when an INI sub-section is absent. The content payload
+	// drops the entire [mysqld] section that contains the immutable
+	// parameter gtid_mode; the guard must reject the change (immutable
+	// removed) instead of panicking on GetAllParameters of a nil
+	// ConfigObject.
+	base := map[string]string{
+		"my.cnf": "[mysqld]\ngtid_mode=OFF\nmax_connections=1000\n",
+	}
+	patch := map[string]parametersv1alpha1.ParametersInFile{
+		"my.cnf": {
+			Content: strPtr("[other]\nfoo=bar\n"),
+		},
+	}
+	configDescs := iniConfigDescsForMyCnf()
+	paramsDefs := paramsDefsWithImmutable([]string{"gtid_mode"})
+
+	_, err := DoMerge(base, patch, paramsDefs, configDescs)
+	if err == nil {
+		t.Fatalf("expected immutable parameter removal via whole-section deletion to be rejected, got nil error")
+	}
+	if !strings.Contains(err.Error(), "gtid_mode") {
+		t.Fatalf("expected error to mention immutable parameter gtid_mode, got %q", err.Error())
+	}
+}
+
+func TestDoMergeRejectsContentWhenImmutableDeclaredButFormatMissing(t *testing.T) {
+	// File has an immutable parameter declared but no FileFormatConfig is
+	// available. Fail-safe: reject rather than silently allow because the
+	// guard cannot verify whether the content change touched gtid_mode.
+	base := map[string]string{
+		"my.cnf": "[mysqld]\ngtid_mode=OFF\n",
+	}
+	patch := map[string]parametersv1alpha1.ParametersInFile{
+		"my.cnf": {
+			Content: strPtr("[mysqld]\ngtid_mode=OFF\nmax_connections=1000\n"),
+		},
+	}
+	paramsDefs := paramsDefsWithImmutable([]string{"gtid_mode"})
+
+	_, err := DoMerge(base, patch, paramsDefs, nil)
+	if err == nil {
+		t.Fatalf("expected fail-safe rejection when immutable parameter declared but file format config is missing, got nil error")
+	}
+	if !strings.Contains(err.Error(), "missing file format config") {
+		t.Fatalf("expected error to mention missing file format config, got %q", err.Error())
+	}
+}

pkg/controllerutil/config_util.go

@@ -78,7 +78,10 @@ func MergeAndValidateConfigs(baseConfigs map[string]string,
 
 	// merge param to config file
 	for _, params := range updatedParams {
-		validUpdatedParameters := filterImmutableParameters(params.UpdatedParams, params.Key, paramsDefs)
+		validUpdatedParameters, err := filterImmutableParameters(params.UpdatedParams, params.Key, paramsDefs)
+		if err != nil {
+			return nil, err
+		}
 		if len(validUpdatedParameters) == 0 {
 			continue
 		}
@@ -245,20 +248,98 @@ func resolveParametersDef(paramsDefs []*parametersv1alpha1.ParametersDefinition,
 	return nil
 }
 
-func filterImmutableParameters(parameters map[string]any, fileName string, paramsDefs []*parametersv1alpha1.ParametersDefinition) map[string]any {
+func filterImmutableParameters(parameters map[string]any, fileName string, paramsDefs []*parametersv1alpha1.ParametersDefinition) (map[string]any, error) {
 	paramsDef := resolveParametersDef(paramsDefs, fileName)
 	if paramsDef == nil || len(paramsDef.Spec.ImmutableParameters) == 0 {
-		return parameters
+		return parameters, nil
 	}
 
 	immutableParams := paramsDef.Spec.ImmutableParameters
 	validParameters := make(map[string]any, len(parameters))
 	for key, val := range parameters {
-		if !slices.Contains(immutableParams, key) {
-			validParameters[key] = val
+		if slices.Contains(immutableParams, key) {
+			return nil, fmt.Errorf("immutable parameter %s cannot be modified", key)
+		}
+		validParameters[key] = val
+	}
+	return validParameters, nil
+}
+
+// ValidateImmutableContentChanges rejects raw-content updates that would
+// modify (add, remove, or change) any immutable parameter declared on the
+// file's ParametersDefinition.
+//
+// Scope: only files present in updatedFiles are inspected. Files outside
+// that set are untouched by this call.
+//
+// Skip rule: if a file has no ParametersDefinition match, or its
+// ParametersDefinition declares no immutable parameters, the file is
+// skipped entirely. This preserves backward-compatible behavior for content
+// updates on files that have no immutable-parameter contract.
+//
+// Diff is computed on parsed parameter maps via the file's FileFormatConfig,
+// not on raw text. Pure format changes (whitespace, comment order, blank
+// lines) therefore do not trip the check; only a semantic parameter delta
+// will.
+//
+// Fail-safe: when a file *does* have immutable-parameter candidates but the
+// parser cannot be applied — missing FileFormatConfig in configDescs, or
+// parse error on base/merged content — the operation is rejected rather
+// than silently allowed. Otherwise an unknown-format file would let
+// immutable parameter changes leak through this guard.
+func ValidateImmutableContentChanges(
+	base map[string]string,
+	merged map[string]string,
+	updatedFiles map[string]string,
+	paramsDefs []*parametersv1alpha1.ParametersDefinition,
+	configDescs []parametersv1alpha1.ComponentConfigDescription,
+) error {
+	for fileName := range updatedFiles {
+		paramsDef := resolveParametersDef(paramsDefs, fileName)
+		if paramsDef == nil || len(paramsDef.Spec.ImmutableParameters) == 0 {
+			continue
+		}
+		formatConfig := core.ResolveConfigFormat(configDescs, fileName)
+		if formatConfig == nil {
+			return fmt.Errorf("cannot validate immutable parameters for file %q: missing file format config", fileName)
+		}
+		baseParams, err := parseFileParameters(fileName, base[fileName], formatConfig)
+		if err != nil {
+			return fmt.Errorf("failed to parse base content of file %q for immutable validation: %w", fileName, err)
 		}
+		mergedParams, err := parseFileParameters(fileName, merged[fileName], formatConfig)
+		if err != nil {
+			return fmt.Errorf("failed to parse merged content of file %q for immutable validation: %w", fileName, err)
+		}
+		for _, immutableKey := range paramsDef.Spec.ImmutableParameters {
+			baseVal, baseOK := baseParams[immutableKey]
+			mergedVal, mergedOK := mergedParams[immutableKey]
+			if baseOK != mergedOK {
+				return fmt.Errorf("immutable parameter %s cannot be modified (file %q)", immutableKey, fileName)
+			}
+			if baseOK && !reflect.DeepEqual(baseVal, mergedVal) {
+				return fmt.Errorf("immutable parameter %s cannot be modified (file %q)", immutableKey, fileName)
+			}
+		}
+	}
+	return nil
+}
+
+func parseFileParameters(name, content string, formatConfig *parametersv1alpha1.FileFormatConfig) (map[string]interface{}, error) {
+	configObject, err := core.FromConfigObject(name, content, formatConfig)
+	if err != nil {
+		return nil, err
+	}
+	if configObject == nil {
+		// core.FromConfigObject returns nil when the requested sub-section is
+		// absent (e.g., the INI [section] header is missing or has been
+		// removed in the content payload). Treat as an empty parameter map
+		// so the caller's diff sees this as "all keys in that section
+		// removed" and rejects immutable removals, instead of nil-deref
+		// panicking on GetAllParameters below.
+		return map[string]interface{}{}, nil
 	}
-	return validParameters
+	return configObject.GetAllParameters(), nil
 }
 
 func ResolveCmpdParametersDefs(ctx context.Context, reader client.Reader, cmpd *appsv1.ComponentDefinition) (*parametersv1alpha1.ParamConfigRenderer, []*parametersv1alpha1.ParametersDefinition, error) {