diff --git a/api/internal/managers/app/config/manager.go b/api/internal/managers/app/config/manager.go index 7a6d981dc4..5854fb1fbd 100644 --- a/api/internal/managers/app/config/manager.go +++ b/api/internal/managers/app/config/manager.go @@ -113,6 +113,7 @@ func NewAppConfigManager(config kotsv1beta1.Config, opts ...AppConfigManagerOpti apitemplate.WithIsAirgap(manager.isAirgap), apitemplate.WithPrivateCACertConfigMapName(manager.privateCACertConfigMapName), apitemplate.WithKubeClient(manager.kcli), + apitemplate.WithLogger(manager.logger), ) } diff --git a/api/internal/managers/app/release/manager.go b/api/internal/managers/app/release/manager.go index 7c661c8ff6..8622f862ac 100644 --- a/api/internal/managers/app/release/manager.go +++ b/api/internal/managers/app/release/manager.go @@ -114,6 +114,7 @@ func NewAppReleaseManager(config kotsv1beta1.Config, opts ...AppReleaseManagerOp template.WithIsAirgap(manager.isAirgap), template.WithPrivateCACertConfigMapName(manager.privateCACertConfigMapName), template.WithKubeClient(manager.kcli), + template.WithLogger(manager.logger), ) } diff --git a/api/pkg/template/config.go b/api/pkg/template/config.go index 46e2353e98..daca00b643 100644 --- a/api/pkg/template/config.go +++ b/api/pkg/template/config.go @@ -46,7 +46,11 @@ func (e *Engine) templateConfigItems() (*kotsv1beta1.Config, error) { for j := range cfg.Spec.Groups[i].Items { resolved, err := e.resolveConfigItem(cfg.Spec.Groups[i].Items[j].Name) if err != nil { - return nil, err + e.logger.WithError(err).WithField("item", cfg.Spec.Groups[i].Items[j].Name).Warn("failed to resolve item, using empty values") + cfg.Spec.Groups[i].Items[j].Value = multitype.FromString("") + cfg.Spec.Groups[i].Items[j].Default = multitype.FromString("") + cfg.Spec.Groups[i].Items[j].Filename = "" + continue } // Apply user value if it exists, otherwise use the templated config value (but not the default) @@ -78,7 +82,8 @@ func (e *Engine) configOption(name string) (string, error) { resolved, err := e.resolveConfigItem(name) if err != nil { - return "", fmt.Errorf("resolve config item: %w", err) + e.logger.WithError(err).WithField("item", name).Warn("failed to resolve item, returning empty string") + return "", nil } return resolved.Effective, nil } @@ -88,13 +93,15 @@ func (e *Engine) configOptionData(name string) (string, error) { resolved, err := e.resolveConfigItem(name) if err != nil { - return "", fmt.Errorf("resolve config item: %w", err) + e.logger.WithError(err).WithField("item", name).Warn("failed to resolve item, returning empty string") + return "", nil } // Base64 decode for file content decoded, err := base64.StdEncoding.DecodeString(resolved.Effective) if err != nil { - return "", fmt.Errorf("decode base64 value: %w", err) + e.logger.WithError(err).WithField("item", name).Warn("failed to decode base64 for item, returning empty string") + return "", nil } return string(decoded), nil } @@ -104,7 +111,8 @@ func (e *Engine) configOptionEquals(name, expected string) (bool, error) { resolved, err := e.resolveConfigItem(name) if err != nil { - return false, fmt.Errorf("resolve config item: %w", err) + e.logger.WithError(err).WithField("item", name).WithField("expected", expected).Warn("failed to resolve item, returning false") + return false, nil } return resolved.Effective == expected, nil } @@ -114,8 +122,9 @@ func (e *Engine) configOptionNotEquals(name, expected string) (bool, error) { resolved, err := e.resolveConfigItem(name) if err != nil { - // NOTE: this is parity from KOTS but I would expect this to return true - return false, fmt.Errorf("resolve config item: %w", err) + // NOTE: logically one might expect this to return true, but this matches KOTS behavior + e.logger.WithError(err).WithField("item", name).WithField("expected", expected).Warn("failed to resolve item, returning false") + return false, nil } return resolved.Effective != expected, nil } @@ -125,7 +134,8 @@ func (e *Engine) configOptionFilename(name string) (string, error) { resolved, err := e.resolveConfigItem(name) if err != nil { - return "", fmt.Errorf("resolve config item: %w", err) + e.logger.WithError(err).WithField("item", name).Warn("failed to resolve item, returning empty string") + return "", nil } // Only return user filename, not config filename for KOTS parity diff --git a/api/pkg/template/config_test.go b/api/pkg/template/config_test.go index 7ec4b2f023..306903a899 100644 --- a/api/pkg/template/config_test.go +++ b/api/pkg/template/config_test.go @@ -265,6 +265,59 @@ func TestEngine_templateConfigItems(t *testing.T) { }, }, }, + { + name: "graceful failure on template error - returns empty values", + config: &kotsv1beta1.Config{ + Spec: kotsv1beta1.ConfigSpec{ + Groups: []kotsv1beta1.ConfigGroup{ + { + Name: "test", + Items: []kotsv1beta1.ConfigItem{ + { + Name: "valid_item", + Value: multitype.FromString("valid_value"), + }, + { + Name: "invalid_template", + Value: multitype.FromString("repl{{ NonExistentFunc }}"), + }, + { + Name: "another_valid", + Value: multitype.FromString("another_value"), + }, + }, + }, + }, + }, + }, + expected: &kotsv1beta1.Config{ + Spec: kotsv1beta1.ConfigSpec{ + Groups: []kotsv1beta1.ConfigGroup{ + { + Name: "test", + Items: []kotsv1beta1.ConfigItem{ + { + Name: "valid_item", + Value: multitype.FromString("valid_value"), + Default: multitype.FromString(""), + }, + { + Name: "invalid_template", + Value: multitype.FromString(""), + Default: multitype.FromString(""), + Filename: "", + }, + { + Name: "another_valid", + Value: multitype.FromString("another_value"), + Default: multitype.FromString(""), + }, + }, + }, + }, + }, + }, + }, } for _, tt := range tests { @@ -544,3 +597,88 @@ func TestEngine_ShouldInvalidateItem(t *testing.T) { engine.depsTree = map[string][]string{} assert.False(t, engine.shouldInvalidateItem("item1"), "should not invalidate item1 as it doesn't exist in either config values") } + +// TestEngine_ConfigOption_HandlesFailuresGracefully tests that ConfigOption functions return empty +// strings/false instead of propagating errors, allowing templates to complete successfully +func TestEngine_ConfigOption_HandlesFailuresGracefully(t *testing.T) { + tests := []struct { + name string + configItem kotsv1beta1.ConfigItem + template string + expectResult string + }{ + { + name: "template error in value - ConfigOption returns empty string", + configItem: kotsv1beta1.ConfigItem{ + Name: "bad_template", + Value: multitype.FromString("repl{{ NonExistentFunc }}"), + }, + template: "value:repl{{ ConfigOption \"bad_template\" }}", + expectResult: "value:", + }, + { + name: "invalid base64 - ConfigOptionData returns empty string", + configItem: kotsv1beta1.ConfigItem{ + Name: "invalid_base64", + Value: multitype.FromString("not-valid-base64!@#$"), + }, + template: "data:repl{{ ConfigOptionData \"invalid_base64\" }}", + expectResult: "data:", + }, + { + name: "nonexistent item in middle of template", + configItem: kotsv1beta1.ConfigItem{ + Name: "existing", + Value: multitype.FromString("exists"), + }, + template: "prefix-repl{{ ConfigOption \"nonexistent\" }}-suffix", + expectResult: "prefix--suffix", + }, + { + name: "ConfigOptionEquals with nonexistent returns false", + configItem: kotsv1beta1.ConfigItem{ + Name: "item", + Value: multitype.FromString("value"), + }, + template: "repl{{ if ConfigOptionEquals \"nonexistent\" \"test\" }}yes repl{{ else }}no repl{{ end }}", + expectResult: "no ", + }, + { + name: "ConfigOptionNotEquals with nonexistent returns false", + configItem: kotsv1beta1.ConfigItem{ + Name: "item", + Value: multitype.FromString("value"), + }, + template: "repl{{ if ConfigOptionNotEquals \"nonexistent\" \"test\" }}yes repl{{ else }}no repl{{ end }}", + expectResult: "no ", + }, + { + name: "multiline YAML with failed ConfigOption", + configItem: kotsv1beta1.ConfigItem{ + Name: "existing", + Value: multitype.FromString("value1"), + }, + template: "line1: repl{{ ConfigOption \"existing\" }}\nline2: repl{{ ConfigOption \"nonexistent\" }}\nline3: value3", + expectResult: "line1: value1\nline2: \nline3: value3", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := &kotsv1beta1.Config{ + Spec: kotsv1beta1.ConfigSpec{ + Groups: []kotsv1beta1.ConfigGroup{ + { + Name: "test", + Items: []kotsv1beta1.ConfigItem{tt.configItem}, + }, + }, + }, + } + engine := NewEngine(config) + result, err := engine.processTemplate(tt.template) + require.NoError(t, err, "template execution should not fail") + assert.Equal(t, tt.expectResult, result) + }) + } +} diff --git a/api/pkg/template/engine.go b/api/pkg/template/engine.go index a998e00380..f1edbc7a88 100644 --- a/api/pkg/template/engine.go +++ b/api/pkg/template/engine.go @@ -6,10 +6,12 @@ import ( "text/template" "github.com/Masterminds/sprig/v3" + "github.com/replicatedhq/embedded-cluster/api/pkg/logger" "github.com/replicatedhq/embedded-cluster/api/types" ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1" "github.com/replicatedhq/embedded-cluster/pkg/release" kotsv1beta1 "github.com/replicatedhq/kotskinds/apis/kots/v1beta1" + "github.com/sirupsen/logrus" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -31,6 +33,7 @@ type Engine struct { privateCACertConfigMapName string // ConfigMap name for private CA certificates, empty string if not available isAirgapInstallation bool // Whether the installation is an airgap installation kubeClient client.Client + logger logrus.FieldLogger // ExecOptions proxySpec *ecv1beta1.ProxySpec // Proxy spec for the proxy template functions, if applicable @@ -85,6 +88,12 @@ func WithKubeClient(kcli client.Client) EngineOption { } } +func WithLogger(logger logrus.FieldLogger) EngineOption { + return func(e *Engine) { + e.logger = logger + } +} + func NewEngine(config *kotsv1beta1.Config, opts ...EngineOption) *Engine { engine := &Engine{ mode: ModeGeneric, // default to generic mode @@ -101,6 +110,10 @@ func NewEngine(config *kotsv1beta1.Config, opts ...EngineOption) *Engine { opt(engine) } + if engine.logger == nil { + engine.logger = logger.NewDiscardLogger() + } + // Initialize funcMap once engine.funcMap = sprig.TxtFuncMap() maps.Copy(engine.funcMap, engine.getFuncMap()) diff --git a/api/pkg/template/execute_test.go b/api/pkg/template/execute_test.go index 50e0b7a57f..ce2db6f689 100644 --- a/api/pkg/template/execute_test.go +++ b/api/pkg/template/execute_test.go @@ -239,12 +239,12 @@ func TestEngine_ConfigOptionEquals(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Snapshot Backup", result) - // Test with an unknown item - an error is returned and false is returned + // Test with an unknown item - returns false with no error (template execution continues) err = engine.Parse("{{repl if ConfigOptionEquals \"notfound\" \"filesystem\" }}Filesystem Storage{{repl else }}Other Storage{{repl end }}") require.NoError(t, err) result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{})) - require.Error(t, err) - assert.Equal(t, "", result) + require.NoError(t, err) + assert.Equal(t, "Other Storage", result) } func TestEngine_ConfigOptionNotEquals(t *testing.T) { @@ -300,12 +300,12 @@ func TestEngine_ConfigOptionNotEquals(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Other Backup", result) - // Test with an unknown item - an error is returned and false is returned + // Test with an unknown item - returns false with no error (template execution continues) err = engine.Parse("{{repl if ConfigOptionNotEquals \"notfound\" \"filesystem\" }}Filesystem Storage{{repl else }}Other Storage{{repl end }}") require.NoError(t, err) result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{})) - require.Error(t, err) - assert.Equal(t, "", result) + require.NoError(t, err) + assert.Equal(t, "Other Storage", result) } func TestEngine_ConfigOptionData(t *testing.T) { @@ -419,11 +419,11 @@ func TestEngine_ConfigOptionFilename(t *testing.T) { require.NoError(t, err) assert.Equal(t, "", result) - // Test with an unknown item - an error is returned and empty string is returned + // Test with an unknown item - returns empty string with no error (template execution continues) err = engine.Parse("{{repl ConfigOptionFilename \"notfound\" }}") require.NoError(t, err) result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{})) - require.Error(t, err) + require.NoError(t, err) assert.Equal(t, "", result) } @@ -495,9 +495,10 @@ func TestEngine_CircularDependency(t *testing.T) { err := engine.Parse("{{repl ConfigOption \"item_a\" }}") require.NoError(t, err) - _, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{})) - require.Error(t, err) - assert.Contains(t, err.Error(), "circular dependency detected for item_a") + result, err := engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{})) + // Circular dependencies return empty string with no error (template execution continues) + require.NoError(t, err) + assert.Equal(t, "", result) } func TestEngine_DeepDependencyChain(t *testing.T) { @@ -764,9 +765,10 @@ func TestEngine_UnknownConfigItem(t *testing.T) { err := engine.Parse("{{repl ConfigOption \"nonexistent\" }}") require.NoError(t, err) - _, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{})) - require.Error(t, err) - assert.Contains(t, err.Error(), "config item nonexistent not found") + result, err := engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{})) + // Unknown config items return empty string with no error (template execution continues) + require.NoError(t, err) + assert.Equal(t, "", result) } func TestEngine_DependencyTreeAndCaching(t *testing.T) { @@ -1361,10 +1363,12 @@ func TestEngine_ConfigMode_CircularDependency(t *testing.T) { engine := NewEngine(config, WithMode(ModeConfig)) - // Should detect circular dependency and return error - _, err := engine.Execute(nil) - require.Error(t, err) - assert.Contains(t, err.Error(), "circular dependency detected") + // Circular dependencies in config are gracefully handled by returning empty strings + result, err := engine.Execute(nil) + require.NoError(t, err) + // Both items should have empty values since they depend on each other + assert.Contains(t, result, "item_a") + assert.Contains(t, result, "item_b") } func TestEngine_ConfigMode_ComplexDependencyChain(t *testing.T) {