From 44f3e2e933abeb427216b477f22d952c2fa00ca9 Mon Sep 17 00:00:00 2001 From: MrLuje Date: Tue, 29 Mar 2022 08:17:00 +0200 Subject: [PATCH] fix(application): prevent crash on empty sync_policy/automated block (#156) * fix(automated): prevent crash on empty block * fix(sync_policy): ensure defaults are used only when necessary --- argocd/resource_argocd_application_test.go | 211 +++++++++++++++++++++ argocd/structure_application.go | 107 ++++++----- 2 files changed, 269 insertions(+), 49 deletions(-) diff --git a/argocd/resource_argocd_application_test.go b/argocd/resource_argocd_application_test.go index 80e5af1d..c53e5b3a 100644 --- a/argocd/resource_argocd_application_test.go +++ b/argocd/resource_argocd_application_test.go @@ -217,6 +217,102 @@ ingress: }) } +func TestAccArgoCDApplication_NoSyncPolicyBlock(t *testing.T) { + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: testAccArgoCDApplicationNoSyncPolicy(acctest.RandomWithPrefix("test-acc")), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet( + "argocd_application.simple", + "metadata.0.uid", + ), + resource.TestCheckNoResourceAttr( + "argocd_application.simple", + "spec.0.sync_policy.0.retry.0.backoff.duration", + ), + resource.TestCheckNoResourceAttr( + "argocd_application.simple", + "spec.0.sync_policy.0.automated.prune", + ), + ), + }, + }}) +} + +func TestAccArgoCDApplication_EmptySyncPolicyBlock(t *testing.T) { + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: testAccArgoCDApplicationEmptySyncPolicy(acctest.RandomWithPrefix("test-acc")), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet( + "argocd_application.simple", + "metadata.0.uid", + ), + resource.TestCheckNoResourceAttr( + "argocd_application.simple", + "spec.0.sync_policy.0.retry.0.backoff.duration", + ), + resource.TestCheckNoResourceAttr( + "argocd_application.simple", + "spec.0.sync_policy.0.automated.prune", + ), + ), + }, + }}) +} + +func TestAccArgoCDApplication_NoAutomatedBlock(t *testing.T) { + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: testAccArgoCDApplicationNoAutomated(acctest.RandomWithPrefix("test-acc")), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet( + "argocd_application.simple", + "metadata.0.uid", + ), + resource.TestCheckResourceAttrSet( + "argocd_application.simple", + "spec.0.sync_policy.0.retry.0.backoff.duration", + ), + resource.TestCheckNoResourceAttr( + "argocd_application.simple", + "spec.0.sync_policy.0.automated.prune", + ), + ), + }, + }}) +} + +func TestAccArgoCDApplication_EmptyAutomatedBlock(t *testing.T) { + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + ProviderFactories: testAccProviders, + Steps: []resource.TestStep{ + { + Config: testAccArgoCDApplicationEmptyAutomated(acctest.RandomWithPrefix("test-acc")), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttrSet( + "argocd_application.simple", + "metadata.0.uid", + ), + resource.TestCheckNoResourceAttr( + "argocd_application.simple", + "spec.0.sync_policy.0.automated.prune", + ), + ), + }, + }}) +} + func testAccArgoCDApplicationSimple(name string) string { return fmt.Sprintf(` resource "argocd_application" "simple" { @@ -567,6 +663,121 @@ resource "argocd_application" "ignore_differences_jqpe" { `, name) } +func testAccArgoCDApplicationNoSyncPolicy(name string) string { + return fmt.Sprintf(` +resource "argocd_application" "simple" { + metadata { + name = "%s" + namespace = "argocd" + } + spec { + source { + repo_url = "https://charts.bitnami.com/bitnami" + chart = "redis" + target_revision = "15.3.0" + helm { + release_name = "testing" + } + } + destination { + server = "https://kubernetes.default.svc" + namespace = "default" + } + } +} + `, name) +} + +func testAccArgoCDApplicationEmptySyncPolicy(name string) string { + return fmt.Sprintf(` +resource "argocd_application" "simple" { + metadata { + name = "%s" + namespace = "argocd" + } + spec { + source { + repo_url = "https://charts.bitnami.com/bitnami" + chart = "redis" + target_revision = "15.3.0" + helm { + release_name = "testing" + } + } + sync_policy { + } + destination { + server = "https://kubernetes.default.svc" + namespace = "default" + } + } +} + `, name) +} + +func testAccArgoCDApplicationNoAutomated(name string) string { + return fmt.Sprintf(` +resource "argocd_application" "simple" { + metadata { + name = "%s" + namespace = "argocd" + } + spec { + source { + repo_url = "https://charts.bitnami.com/bitnami" + chart = "redis" + target_revision = "15.3.0" + helm { + release_name = "testing" + } + } + sync_policy { + retry { + limit = "5" + backoff = { + duration = "30s" + max_duration = "2m" + factor = "2" + } + } + } + destination { + server = "https://kubernetes.default.svc" + namespace = "default" + } + } +} + `, name) +} + +func testAccArgoCDApplicationEmptyAutomated(name string) string { + return fmt.Sprintf(` +resource "argocd_application" "simple" { + metadata { + name = "%s" + namespace = "argocd" + } + spec { + source { + repo_url = "https://charts.bitnami.com/bitnami" + chart = "redis" + target_revision = "15.3.0" + helm { + release_name = "testing" + } + } + sync_policy { + automated = {} + } + destination { + server = "https://kubernetes.default.svc" + namespace = "default" + } + } +} + `, name) +} + func testAccSkipFeatureIgnoreDiffJQPathExpressions() (bool, error) { p, _ := testAccProviders["argocd"]() _ = p.Configure(context.Background(), &terraform.ResourceConfig{}) diff --git a/argocd/structure_application.go b/argocd/structure_application.go index 5adcc8bb..3d8d949e 100644 --- a/argocd/structure_application.go +++ b/argocd/structure_application.go @@ -257,78 +257,87 @@ func expandApplicationSyncPolicy(_sp []interface{}) (*application.SyncPolicy, di return nil, nil } sp := _sp[0] + if sp == nil { + return &application.SyncPolicy{}, nil + } var automated = &application.SyncPolicyAutomated{} var syncOptions application.SyncOptions var retry = &application.RetryStrategy{} + var syncPolicy = &application.SyncPolicy{} - if a, ok := sp.(map[string]interface{})["automated"]; ok { - for k, v := range a.(map[string]interface{}) { - if k == "prune" { - automated.Prune = v.(bool) - } - if k == "self_heal" { - automated.SelfHeal = v.(bool) - } - if k == "allow_empty" { - automated.AllowEmpty = v.(bool) + if a, ok := sp.(map[string]interface{})["automated"].(map[string]interface{}); ok { + if len(a) > 0 { + for k, v := range a { + if k == "prune" { + automated.Prune = v.(bool) + } + if k == "self_heal" { + automated.SelfHeal = v.(bool) + } + if k == "allow_empty" { + automated.AllowEmpty = v.(bool) + } } + syncPolicy.Automated = automated } } if v, ok := sp.(map[string]interface{})["sync_options"]; ok { sOpts := v.([]interface{}) - for _, sOpt := range sOpts { - syncOptions = append(syncOptions, sOpt.(string)) + if len(sOpts) > 0 { + for _, sOpt := range sOpts { + syncOptions = append(syncOptions, sOpt.(string)) + } + syncPolicy.SyncOptions = syncOptions } } if _retry, ok := sp.(map[string]interface{})["retry"].([]interface{}); ok { if len(_retry) > 0 { - r := _retry[0] - for k, v := range r.(map[string]interface{}) { - if k == "limit" { - var err error - retry.Limit, err = convertStringToInt64(v.(string)) - if err != nil { - return nil, []diag.Diagnostic{ - { - Severity: diag.Error, - Summary: "Error converting retry limit to integer", - Detail: err.Error(), - }, + r := (_retry[0]).(map[string]interface{}) + if len(r) > 0 { + for k, v := range r { + if k == "limit" { + var err error + retry.Limit, err = convertStringToInt64(v.(string)) + if err != nil { + return nil, []diag.Diagnostic{ + { + Severity: diag.Error, + Summary: "Error converting retry limit to integer", + Detail: err.Error(), + }, + } } } - } - if k == "backoff" { - retry.Backoff = &application.Backoff{} - for kb, vb := range v.(map[string]interface{}) { - if kb == "duration" { - retry.Backoff.Duration = vb.(string) - } - if kb == "max_duration" { - retry.Backoff.MaxDuration = vb.(string) - } - if kb == "factor" { - factor, err := convertStringToInt64Pointer(vb.(string)) - if err != nil { - return nil, []diag.Diagnostic{ - { - Severity: diag.Error, - Summary: "Error converting backoff factor to integer", - Detail: err.Error(), - }, + if k == "backoff" { + retry.Backoff = &application.Backoff{} + for kb, vb := range v.(map[string]interface{}) { + if kb == "duration" { + retry.Backoff.Duration = vb.(string) + } + if kb == "max_duration" { + retry.Backoff.MaxDuration = vb.(string) + } + if kb == "factor" { + factor, err := convertStringToInt64Pointer(vb.(string)) + if err != nil { + return nil, []diag.Diagnostic{ + { + Severity: diag.Error, + Summary: "Error converting backoff factor to integer", + Detail: err.Error(), + }, + } } + retry.Backoff.Factor = factor } - retry.Backoff.Factor = factor } } } + syncPolicy.Retry = retry } } } - return &application.SyncPolicy{ - Automated: automated, - SyncOptions: syncOptions, - Retry: retry, - }, nil + return syncPolicy, nil } func expandApplicationIgnoreDifferences(ids []interface{}) (