From 96a0e69c1c0dc806690a0a88f64db2c1dcb3fcad Mon Sep 17 00:00:00 2001 From: Robert Moucha Date: Sat, 6 Jun 2026 19:17:00 +0200 Subject: [PATCH] fix(secret): stop pulp-server Secret churn from nondeterministic settings needsMigrationSetting iterated MigrationSettingsList as a map, whose range order Go randomizes per invocation. With RedirectToObjectStorage=true and HideGuardedDistributions=false (the default), each reconcile produced one of two different settings.py contents depending on which key the runtime visited first: - if HideGuardedDistributions was visited first, the loop hit `if !config { return }` and aborted, dropping REDIRECT_TO_OBJECT_STORAGE entirely; - if RedirectToObjectStorage was visited first, the line was written and HideGuardedDistributions then triggered the early return. The two outputs differed, so ReconcileObject kept rewriting the pulp-server Secret and restarting pulpcore pods on every reconcile. Fix both issues at the source: - change MigrationSettingsList to return []MigrationSetting (a slice of named OperatorField/PulpField pairs) so iteration order is fixed at the data definition rather than reconstructed at each call site; - replace the buggy `return` with `continue` so a single disabled flag no longer skips subsequent settings. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Robert Moucha --- controllers/repo_manager/secret.go | 17 ++++++----------- controllers/utils.go | 28 ++++++++++++++++++---------- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/controllers/repo_manager/secret.go b/controllers/repo_manager/secret.go index 02e10fe77..88e8dfa62 100644 --- a/controllers/repo_manager/secret.go +++ b/controllers/repo_manager/secret.go @@ -620,24 +620,19 @@ func debugLogging(resources controllers.FunctionResources, pulpSettings *string) // needsMigrationSetting defines settings.py with some specific configurations that when changed will // also trigger a migration job func needsMigrationSetting(resources controllers.FunctionResources, pulpSettings *string, customSettings map[string]struct{}) { - for operatorFieldName, pulpFieldName := range controllers.MigrationSettingsList() { - customSettingsFound := false - if _, exists := customSettings[pulpFieldName]; exists { - logMessage := fmt.Sprintf("%v should not be defined in custom_pulp_settings. Use pulp.Spec.%v instead", pulpFieldName, strings.ToLower(pulpFieldName)) + for _, s := range controllers.MigrationSettingsList() { + if _, exists := customSettings[s.PulpField]; exists { + logMessage := fmt.Sprintf("%v should not be defined in custom_pulp_settings. Use pulp.Spec.%v instead", s.PulpField, strings.ToLower(s.PulpField)) controllers.CustomZapLogger().Warn(logMessage) - customSettingsFound = true - } - if customSettingsFound { continue } - config := reflect.ValueOf(resources.Pulp.Spec).FieldByName(operatorFieldName).Bool() + config := reflect.ValueOf(resources.Pulp.Spec).FieldByName(s.OperatorField).Bool() if !config { - return + continue } configCapitalized := cases.Title(language.English, cases.Compact).String(strconv.FormatBool(config)) - *pulpSettings = *pulpSettings + fmt.Sprintf("%v = %v\n", pulpFieldName, configCapitalized) - + *pulpSettings = *pulpSettings + fmt.Sprintf("%v = %v\n", s.PulpField, configCapitalized) } } diff --git a/controllers/utils.go b/controllers/utils.go index 5cdddbb78..7c1c83b09 100644 --- a/controllers/utils.go +++ b/controllers/utils.go @@ -297,13 +297,21 @@ func StorageTypeChanged(pulp *pulpv1.Pulp) bool { return currentStorageType != definedStorageType[0] } -// MigrationSettingsList returns a map[string]string of configurations that should trigger a migration job -// note: in the current implementation, all fields should be boolean. Trying to add a non-bool field +// MigrationSetting pairs a Pulp CR field name (on Spec/Status) with the +// corresponding setting key written to settings.py. +type MigrationSetting struct { + OperatorField string + PulpField string +} + +// MigrationSettingsList returns the configurations that should trigger a migration job, +// in a stable, deterministic order. +// Note: in the current implementation, all fields should be boolean. Trying to add a non-bool field // to the list will fail SettingNeedsMigrationChanged execution. -func MigrationSettingsList() map[string]string { - return map[string]string{ - "RedirectToObjectStorage": "REDIRECT_TO_OBJECT_STORAGE", - "HideGuardedDistributions": "HIDE_GUARDED_DISTRIBUTIONS", +func MigrationSettingsList() []MigrationSetting { + return []MigrationSetting{ + {"HideGuardedDistributions", "HIDE_GUARDED_DISTRIBUTIONS"}, + {"RedirectToObjectStorage", "REDIRECT_TO_OBJECT_STORAGE"}, } } @@ -311,11 +319,11 @@ func MigrationSettingsList() map[string]string { // returns a list with the settings modified func SettingNeedsMigrationChanged(pulp *pulpv1.Pulp) []string { settingsChanged := []string{} - for setting := range MigrationSettingsList() { - currentSpec := reflect.ValueOf(pulp.Spec).FieldByName(setting).Bool() - oldSpec := reflect.ValueOf(pulp.Status).FieldByName(setting).Bool() + for _, s := range MigrationSettingsList() { + currentSpec := reflect.ValueOf(pulp.Spec).FieldByName(s.OperatorField).Bool() + oldSpec := reflect.ValueOf(pulp.Status).FieldByName(s.OperatorField).Bool() if currentSpec != oldSpec { - settingsChanged = append(settingsChanged, setting) + settingsChanged = append(settingsChanged, s.OperatorField) } } return settingsChanged