Skip to content

Commit 2250a47

Browse files
screspodcrdant
screspod
authored and
crdant
committed
Handle configOption failures gracefully (#3195)
* Handle configOption failures gracefully Signed-off-by: Steven Crespo <screswk@gmail.com> * f Signed-off-by: Steven Crespo <screswk@gmail.com> * f Signed-off-by: Steven Crespo <screswk@gmail.com> --------- Signed-off-by: Steven Crespo <screswk@gmail.com>
1 parent e238e1c commit 2250a47

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
@@ -114,6 +114,7 @@ func NewAppConfigManager(config kotsv1beta1.Config, opts ...AppConfigManagerOpti
114114
apitemplate.WithIsAirgap(manager.isAirgap),
115115
apitemplate.WithPrivateCACertConfigMapName(manager.privateCACertConfigMapName),
116116
apitemplate.WithKubeClient(manager.kcli),
117+
apitemplate.WithLogger(manager.logger),
117118
)
118119
}
119120

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

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

‎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,11 +6,13 @@ 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"
1314
"github.com/replicatedhq/kotskinds/pkg/licensewrapper"
15+
"github.com/sirupsen/logrus"
1416
"sigs.k8s.io/controller-runtime/pkg/client"
1517
)
1618

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

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

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

114+
if engine.logger == nil {
115+
engine.logger = logger.NewDiscardLogger()
116+
}
117+
105118
// Initialize funcMap once
106119
engine.funcMap = sprig.TxtFuncMap()
107120
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
@@ -247,12 +247,12 @@ func TestEngine_ConfigOptionEquals(t *testing.T) {
247247
require.NoError(t, err)
248248
assert.Equal(t, "Snapshot Backup", result)
249249

250-
// Test with an unknown item - an error is returned and false is returned
250+
// Test with an unknown item - returns false with no error (template execution continues)
251251
err = engine.Parse("{{repl if ConfigOptionEquals \"notfound\" \"filesystem\" }}Filesystem Storage{{repl else }}Other Storage{{repl end }}")
252252
require.NoError(t, err)
253253
result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
254-
require.Error(t, err)
255-
assert.Equal(t, "", result)
254+
require.NoError(t, err)
255+
assert.Equal(t, "Other Storage", result)
256256
}
257257

258258
func TestEngine_ConfigOptionNotEquals(t *testing.T) {
@@ -308,12 +308,12 @@ func TestEngine_ConfigOptionNotEquals(t *testing.T) {
308308
require.NoError(t, err)
309309
assert.Equal(t, "Other Backup", result)
310310

311-
// Test with an unknown item - an error is returned and false is returned
311+
// Test with an unknown item - returns false with no error (template execution continues)
312312
err = engine.Parse("{{repl if ConfigOptionNotEquals \"notfound\" \"filesystem\" }}Filesystem Storage{{repl else }}Other Storage{{repl end }}")
313313
require.NoError(t, err)
314314
result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
315-
require.Error(t, err)
316-
assert.Equal(t, "", result)
315+
require.NoError(t, err)
316+
assert.Equal(t, "Other Storage", result)
317317
}
318318

319319
func TestEngine_ConfigOptionData(t *testing.T) {
@@ -427,11 +427,11 @@ func TestEngine_ConfigOptionFilename(t *testing.T) {
427427
require.NoError(t, err)
428428
assert.Equal(t, "", result)
429429

430-
// Test with an unknown item - an error is returned and empty string is returned
430+
// Test with an unknown item - returns empty string with no error (template execution continues)
431431
err = engine.Parse("{{repl ConfigOptionFilename \"notfound\" }}")
432432
require.NoError(t, err)
433433
result, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
434-
require.Error(t, err)
434+
require.NoError(t, err)
435435
assert.Equal(t, "", result)
436436
}
437437

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

504504
err := engine.Parse("{{repl ConfigOption \"item_a\" }}")
505505
require.NoError(t, err)
506-
_, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
507-
require.Error(t, err)
508-
assert.Contains(t, err.Error(), "circular dependency detected for item_a")
506+
result, err := engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
507+
// Circular dependencies return empty string with no error (template execution continues)
508+
require.NoError(t, err)
509+
assert.Equal(t, "", result)
509510
}
510511

511512
func TestEngine_DeepDependencyChain(t *testing.T) {
@@ -772,9 +773,10 @@ func TestEngine_UnknownConfigItem(t *testing.T) {
772773

773774
err := engine.Parse("{{repl ConfigOption \"nonexistent\" }}")
774775
require.NoError(t, err)
775-
_, err = engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
776-
require.Error(t, err)
777-
assert.Contains(t, err.Error(), "config item nonexistent not found")
776+
result, err := engine.Execute(nil, WithProxySpec(&ecv1beta1.ProxySpec{}))
777+
// Unknown config items return empty string with no error (template execution continues)
778+
require.NoError(t, err)
779+
assert.Equal(t, "", result)
778780
}
779781

780782
func TestEngine_DependencyTreeAndCaching(t *testing.T) {
@@ -1369,10 +1371,12 @@ func TestEngine_ConfigMode_CircularDependency(t *testing.T) {
13691371

13701372
engine := NewEngine(config, WithMode(ModeConfig))
13711373

1372-
// Should detect circular dependency and return error
1373-
_, err := engine.Execute(nil)
1374-
require.Error(t, err)
1375-
assert.Contains(t, err.Error(), "circular dependency detected")
1374+
// Circular dependencies in config are gracefully handled by returning empty strings
1375+
result, err := engine.Execute(nil)
1376+
require.NoError(t, err)
1377+
// Both items should have empty values since they depend on each other
1378+
assert.Contains(t, result, "item_a")
1379+
assert.Contains(t, result, "item_b")
13761380
}
13771381

13781382
func TestEngine_ConfigMode_ComplexDependencyChain(t *testing.T) {

0 commit comments

Comments
 (0)