Skip to content

Commit 33bd094

Browse files
screspodscrespod
authored
Handle configOption failures gracefully (#3195)
* Handle configOption failures gracefully Signed-off-by: Steven Crespo <[email protected]> * f Signed-off-by: Steven Crespo <[email protected]> * f Signed-off-by: Steven Crespo <[email protected]> --------- Signed-off-by: Steven Crespo <[email protected]>
1 parent 8fe7073 commit 33bd094

File tree

6 files changed

+193
-26
lines changed

6 files changed

+193
-26
lines changed

‎api/internal/managers/app/config/manager.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ func NewAppConfigManager(config kotsv1beta1.Config, opts ...AppConfigManagerOpti
113113
apitemplate.WithIsAirgap(manager.isAirgap),
114114
apitemplate.WithPrivateCACertConfigMapName(manager.privateCACertConfigMapName),
115115
apitemplate.WithKubeClient(manager.kcli),
116+
apitemplate.WithLogger(manager.logger),
116117
)
117118
}
118119

‎api/internal/managers/app/release/manager.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ func NewAppReleaseManager(config kotsv1beta1.Config, opts ...AppReleaseManagerOp
114114
template.WithIsAirgap(manager.isAirgap),
115115
template.WithPrivateCACertConfigMapName(manager.privateCACertConfigMapName),
116116
template.WithKubeClient(manager.kcli),
117+
template.WithLogger(manager.logger),
117118
)
118119
}
119120

‎api/pkg/template/config.go‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,11 @@ func (e *Engine) templateConfigItems() (*kotsv1beta1.Config, error) {
4646
for j := range cfg.Spec.Groups[i].Items {
4747
resolved, err := e.resolveConfigItem(cfg.Spec.Groups[i].Items[j].Name)
4848
if err != nil {
49-
return nil, err
49+
e.logger.WithError(err).WithField("item", cfg.Spec.Groups[i].Items[j].Name).Warn("failed to resolve item, using empty values")
50+
cfg.Spec.Groups[i].Items[j].Value = multitype.FromString("")
51+
cfg.Spec.Groups[i].Items[j].Default = multitype.FromString("")
52+
cfg.Spec.Groups[i].Items[j].Filename = ""
53+
continue
5054
}
5155

5256
// 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) {
7882

7983
resolved, err := e.resolveConfigItem(name)
8084
if err != nil {
81-
return "", fmt.Errorf("resolve config item: %w", err)
85+
e.logger.WithError(err).WithField("item", name).Warn("failed to resolve item, returning empty string")
86+
return "", nil
8287
}
8388
return resolved.Effective, nil
8489
}
@@ -88,13 +93,15 @@ func (e *Engine) configOptionData(name string) (string, error) {
8893

8994
resolved, err := e.resolveConfigItem(name)
9095
if err != nil {
91-
return "", fmt.Errorf("resolve config item: %w", err)
96+
e.logger.WithError(err).WithField("item", name).Warn("failed to resolve item, returning empty string")
97+
return "", nil
9298
}
9399

94100
// Base64 decode for file content
95101
decoded, err := base64.StdEncoding.DecodeString(resolved.Effective)
96102
if err != nil {
97-
return "", fmt.Errorf("decode base64 value: %w", err)
103+
e.logger.WithError(err).WithField("item", name).Warn("failed to decode base64 for item, returning empty string")
104+
return "", nil
98105
}
99106
return string(decoded), nil
100107
}
@@ -104,7 +111,8 @@ func (e *Engine) configOptionEquals(name, expected string) (bool, error) {
104111

105112
resolved, err := e.resolveConfigItem(name)
106113
if err != nil {
107-
return false, fmt.Errorf("resolve config item: %w", err)
114+
e.logger.WithError(err).WithField("item", name).WithField("expected", expected).Warn("failed to resolve item, returning false")
115+
return false, nil
108116
}
109117
return resolved.Effective == expected, nil
110118
}
@@ -114,8 +122,9 @@ func (e *Engine) configOptionNotEquals(name, expected string) (bool, error) {
114122

115123
resolved, err := e.resolveConfigItem(name)
116124
if err != nil {
117-
// NOTE: this is parity from KOTS but I would expect this to return true
118-
return false, fmt.Errorf("resolve config item: %w", err)
125+
// NOTE: logically one might expect this to return true, but this matches KOTS behavior
126+
e.logger.WithError(err).WithField("item", name).WithField("expected", expected).Warn("failed to resolve item, returning false")
127+
return false, nil
119128
}
120129
return resolved.Effective != expected, nil
121130
}
@@ -125,7 +134,8 @@ func (e *Engine) configOptionFilename(name string) (string, error) {
125134

126135
resolved, err := e.resolveConfigItem(name)
127136
if err != nil {
128-
return "", fmt.Errorf("resolve config item: %w", err)
137+
e.logger.WithError(err).WithField("item", name).Warn("failed to resolve item, returning empty string")
138+
return "", nil
129139
}
130140

131141
// Only return user filename, not config filename for KOTS parity

‎api/pkg/template/config_test.go‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,59 @@ func TestEngine_templateConfigItems(t *testing.T) {
265265
},
266266
},
267267
},
268+
{
269+
name: "graceful failure on template error - returns empty values",
270+
config: &kotsv1beta1.Config{
271+
Spec: kotsv1beta1.ConfigSpec{
272+
Groups: []kotsv1beta1.ConfigGroup{
273+
{
274+
Name: "test",
275+
Items: []kotsv1beta1.ConfigItem{
276+
{
277+
Name: "valid_item",
278+
Value: multitype.FromString("valid_value"),
279+
},
280+
{
281+
Name: "invalid_template",
282+
Value: multitype.FromString("repl{{ NonExistentFunc }}"),
283+
},
284+
{
285+
Name: "another_valid",
286+
Value: multitype.FromString("another_value"),
287+
},
288+
},
289+
},
290+
},
291+
},
292+
},
293+
expected: &kotsv1beta1.Config{
294+
Spec: kotsv1beta1.ConfigSpec{
295+
Groups: []kotsv1beta1.ConfigGroup{
296+
{
297+
Name: "test",
298+
Items: []kotsv1beta1.ConfigItem{
299+
{
300+
Name: "valid_item",
301+
Value: multitype.FromString("valid_value"),
302+
Default: multitype.FromString(""),
303+
},
304+
{
305+
Name: "invalid_template",
306+
Value: multitype.FromString(""),
307+
Default: multitype.FromString(""),
308+
Filename: "",
309+
},
310+
{
311+
Name: "another_valid",
312+
Value: multitype.FromString("another_value"),
313+
Default: multitype.FromString(""),
314+
},
315+
},
316+
},
317+
},
318+
},
319+
},
320+
},
268321
}
269322

270323
for _, tt := range tests {
@@ -544,3 +597,88 @@ func TestEngine_ShouldInvalidateItem(t *testing.T) {
544597
engine.depsTree = map[string][]string{}
545598
assert.False(t, engine.shouldInvalidateItem("item1"), "should not invalidate item1 as it doesn't exist in either config values")
546599
}
600+
601+
// TestEngine_ConfigOption_HandlesFailuresGracefully tests that ConfigOption functions return empty
602+
// strings/false instead of propagating errors, allowing templates to complete successfully
603+
func TestEngine_ConfigOption_HandlesFailuresGracefully(t *testing.T) {
604+
tests := []struct {
605+
name string
606+
configItem kotsv1beta1.ConfigItem
607+
template string
608+
expectResult string
609+
}{
610+
{
611+
name: "template error in value - ConfigOption returns empty string",
612+
configItem: kotsv1beta1.ConfigItem{
613+
Name: "bad_template",
614+
Value: multitype.FromString("repl{{ NonExistentFunc }}"),
615+
},
616+
template: "value:repl{{ ConfigOption \"bad_template\" }}",
617+
expectResult: "value:",
618+
},
619+
{
620+
name: "invalid base64 - ConfigOptionData returns empty string",
621+
configItem: kotsv1beta1.ConfigItem{
622+
Name: "invalid_base64",
623+
Value: multitype.FromString("not-valid-base64!@#$"),
624+
},
625+
template: "data:repl{{ ConfigOptionData \"invalid_base64\" }}",
626+
expectResult: "data:",
627+
},
628+
{
629+
name: "nonexistent item in middle of template",
630+
configItem: kotsv1beta1.ConfigItem{
631+
Name: "existing",
632+
Value: multitype.FromString("exists"),
633+
},
634+
template: "prefix-repl{{ ConfigOption \"nonexistent\" }}-suffix",
635+
expectResult: "prefix--suffix",
636+
},
637+
{
638+
name: "ConfigOptionEquals with nonexistent returns false",
639+
configItem: kotsv1beta1.ConfigItem{
640+
Name: "item",
641+
Value: multitype.FromString("value"),
642+
},
643+
template: "repl{{ if ConfigOptionEquals \"nonexistent\" \"test\" }}yes repl{{ else }}no repl{{ end }}",
644+
expectResult: "no ",
645+
},
646+
{
647+
name: "ConfigOptionNotEquals with nonexistent returns false",
648+
configItem: kotsv1beta1.ConfigItem{
649+
Name: "item",
650+
Value: multitype.FromString("value"),
651+
},
652+
template: "repl{{ if ConfigOptionNotEquals \"nonexistent\" \"test\" }}yes repl{{ else }}no repl{{ end }}",
653+
expectResult: "no ",
654+
},
655+
{
656+
name: "multiline YAML with failed ConfigOption",
657+
configItem: kotsv1beta1.ConfigItem{
658+
Name: "existing",
659+
Value: multitype.FromString("value1"),
660+
},
661+
template: "line1: repl{{ ConfigOption \"existing\" }}\nline2: repl{{ ConfigOption \"nonexistent\" }}\nline3: value3",
662+
expectResult: "line1: value1\nline2: \nline3: value3",
663+
},
664+
}
665+
666+
for _, tt := range tests {
667+
t.Run(tt.name, func(t *testing.T) {
668+
config := &kotsv1beta1.Config{
669+
Spec: kotsv1beta1.ConfigSpec{
670+
Groups: []kotsv1beta1.ConfigGroup{
671+
{
672+
Name: "test",
673+
Items: []kotsv1beta1.ConfigItem{tt.configItem},
674+
},
675+
},
676+
},
677+
}
678+
engine := NewEngine(config)
679+
result, err := engine.processTemplate(tt.template)
680+
require.NoError(t, err, "template execution should not fail")
681+
assert.Equal(t, tt.expectResult, result)
682+
})
683+
}
684+
}

‎api/pkg/template/engine.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,12 @@ import (
66
"text/template"
77

88
"github.com/Masterminds/sprig/v3"
9+
"github.com/replicatedhq/embedded-cluster/api/pkg/logger"
910
"github.com/replicatedhq/embedded-cluster/api/types"
1011
ecv1beta1 "github.com/replicatedhq/embedded-cluster/kinds/apis/v1beta1"
1112
"github.com/replicatedhq/embedded-cluster/pkg/release"
1213
kotsv1beta1 "github.com/replicatedhq/kotskinds/apis/kots/v1beta1"
14+
"github.com/sirupsen/logrus"
1315
"sigs.k8s.io/controller-runtime/pkg/client"
1416
)
1517

@@ -31,6 +33,7 @@ type Engine struct {
3133
privateCACertConfigMapName string // ConfigMap name for private CA certificates, empty string if not available
3234
isAirgapInstallation bool // Whether the installation is an airgap installation
3335
kubeClient client.Client
36+
logger logrus.FieldLogger
3437

3538
// ExecOptions
3639
proxySpec *ecv1beta1.ProxySpec // Proxy spec for the proxy template functions, if applicable
@@ -85,6 +88,12 @@ func WithKubeClient(kcli client.Client) EngineOption {
8588
}
8689
}
8790

91+
func WithLogger(logger logrus.FieldLogger) EngineOption {
92+
return func(e *Engine) {
93+
e.logger = logger
94+
}
95+
}
96+
8897
func NewEngine(config *kotsv1beta1.Config, opts ...EngineOption) *Engine {
8998
engine := &Engine{
9099
mode: ModeGeneric, // default to generic mode
@@ -101,6 +110,10 @@ func NewEngine(config *kotsv1beta1.Config, opts ...EngineOption) *Engine {
101110
opt(engine)
102111
}
103112

113+
if engine.logger == nil {
114+
engine.logger = logger.NewDiscardLogger()
115+
}
116+
104117
// Initialize funcMap once
105118
engine.funcMap = sprig.TxtFuncMap()
106119
maps.Copy(engine.funcMap, engine.getFuncMap())

‎api/pkg/template/execute_test.go‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -239,12 +239,12 @@ func TestEngine_ConfigOptionEquals(t *testing.T) {
239239
require.NoError(t, err)
240240
assert.Equal(t, "Snapshot Backup", result)
241241

242-
// Test with an unknown item - an error is returned and false is returned
242+
// Test with an unknown item - returns false with no error (template execution continues)
243243
err = engine.Parse("{{repl if ConfigOptionEquals \"notfound\" \"filesystem\" }}Filesystem Storage{{repl else }}Other Storage{{repl end }}")
244244
require.NoError(t, err)
245245
result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
246-
require.Error(t, err)
247-
assert.Equal(t, "", result)
246+
require.NoError(t, err)
247+
assert.Equal(t, "Other Storage", result)
248248
}
249249

250250
func TestEngine_ConfigOptionNotEquals(t *testing.T) {
@@ -300,12 +300,12 @@ func TestEngine_ConfigOptionNotEquals(t *testing.T) {
300300
require.NoError(t, err)
301301
assert.Equal(t, "Other Backup", result)
302302

303-
// Test with an unknown item - an error is returned and false is returned
303+
// Test with an unknown item - returns false with no error (template execution continues)
304304
err = engine.Parse("{{repl if ConfigOptionNotEquals \"notfound\" \"filesystem\" }}Filesystem Storage{{repl else }}Other Storage{{repl end }}")
305305
require.NoError(t, err)
306306
result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
307-
require.Error(t, err)
308-
assert.Equal(t, "", result)
307+
require.NoError(t, err)
308+
assert.Equal(t, "Other Storage", result)
309309
}
310310

311311
func TestEngine_ConfigOptionData(t *testing.T) {
@@ -419,11 +419,11 @@ func TestEngine_ConfigOptionFilename(t *testing.T) {
419419
require.NoError(t, err)
420420
assert.Equal(t, "", result)
421421

422-
// Test with an unknown item - an error is returned and empty string is returned
422+
// Test with an unknown item - returns empty string with no error (template execution continues)
423423
err = engine.Parse("{{repl ConfigOptionFilename \"notfound\" }}")
424424
require.NoError(t, err)
425425
result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
426-
require.Error(t, err)
426+
require.NoError(t, err)
427427
assert.Equal(t, "", result)
428428
}
429429

@@ -495,9 +495,10 @@ func TestEngine_CircularDependency(t *testing.T) {
495495

496496
err := engine.Parse("{{repl ConfigOption \"item_a\" }}")
497497
require.NoError(t, err)
498-
_, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
499-
require.Error(t, err)
500-
assert.Contains(t, err.Error(), "circular dependency detected for item_a")
498+
result, err := engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
499+
// Circular dependencies return empty string with no error (template execution continues)
500+
require.NoError(t, err)
501+
assert.Equal(t, "", result)
501502
}
502503

503504
func TestEngine_DeepDependencyChain(t *testing.T) {
@@ -764,9 +765,10 @@ func TestEngine_UnknownConfigItem(t *testing.T) {
764765

765766
err := engine.Parse("{{repl ConfigOption \"nonexistent\" }}")
766767
require.NoError(t, err)
767-
_, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
768-
require.Error(t, err)
769-
assert.Contains(t, err.Error(), "config item nonexistent not found")
768+
result, err := engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
769+
// Unknown config items return empty string with no error (template execution continues)
770+
require.NoError(t, err)
771+
assert.Equal(t, "", result)
770772
}
771773

772774
func TestEngine_DependencyTreeAndCaching(t *testing.T) {
@@ -1361,10 +1363,12 @@ func TestEngine_ConfigMode_CircularDependency(t *testing.T) {
13611363

13621364
engine := NewEngine(config, WithMode(ModeConfig))
13631365

1364-
// Should detect circular dependency and return error
1365-
_, err := engine.Execute(nil)
1366-
require.Error(t, err)
1367-
assert.Contains(t, err.Error(), "circular dependency detected")
1366+
// Circular dependencies in config are gracefully handled by returning empty strings
1367+
result, err := engine.Execute(nil)
1368+
require.NoError(t, err)
1369+
// Both items should have empty values since they depend on each other
1370+
assert.Contains(t, result, "item_a")
1371+
assert.Contains(t, result, "item_b")
13681372
}
13691373

13701374
func TestEngine_ConfigMode_ComplexDependencyChain(t *testing.T) {

0 commit comments

Comments
 (0)