aws: fail on ListCertificatesPages error (#725)

* aws: refactor ACM fake client to support returning errors Signed-off-by: Alexander Yastrebov <[email protected]> * aws: fail on ListCertificatesPages error `getACMCertificateSummaries` did not check `ListCertificatesPages` error and returned empty `CertificateSummary` slice and no error via `filterCertificatesByTag` if `filterTag` is configured. Empty `CertificateSummary` results in empty `existingStackCertificateARNs` for each `loadBalancer` model which triggered stack update: ``` level=info msg="Updating \"internet-facing\" stack for 0 certificates / 0 ingresses" ``` and then stack deletion on the next cycle: ``` level=info msg="Deleted orphaned stack \"kube-ingress-aws-controller-aws-XXX\"" ``` due to empty certificate tags after previous update. Follow up on #658 Signed-off-by: Alexander Yastrebov <[email protected]> --------- Signed-off-by: Alexander Yastrebov <[email protected]>
zalando-incubator · Oct 16, 2024 · bad6e75 · bad6e75
1 parent 034d901
commit bad6e75
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 35 deletions.
diff --git a/aws/acm.go b/aws/acm.go
@@ -44,16 +44,18 @@ func getACMCertificateSummaries(api acmiface.ACMAPI, filterTag string) ([]*acm.C
 	}
 	acmSummaries := make([]*acm.CertificateSummary, 0)
 
-	err := api.ListCertificatesPages(params, func(page *acm.ListCertificatesOutput, lastPage bool) bool {
+	if err := api.ListCertificatesPages(params, func(page *acm.ListCertificatesOutput, lastPage bool) bool {
 		acmSummaries = append(acmSummaries, page.CertificateSummaryList...)
 		return true
-	})
+	}); err != nil {
+		return nil, err
+	}
 
 	if tag := strings.Split(filterTag, "="); filterTag != "=" && len(tag) == 2 {
 		return filterCertificatesByTag(api, acmSummaries, tag[0], tag[1])
 	}
 
-	return acmSummaries, err
+	return acmSummaries, nil
 }
 
 func filterCertificatesByTag(api acmiface.ACMAPI, allSummaries []*acm.CertificateSummary, key, value string) ([]*acm.CertificateSummary, error) {

diff --git a/aws/acm_test.go b/aws/acm_test.go
@@ -1,6 +1,7 @@
 package aws
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -14,7 +15,7 @@ type acmExpect struct {
 	ARN         string
 	DomainNames []string
 	Chain       int
-	Error       error
+	Error       string
 	EmptyList   bool
 }
 
@@ -45,11 +46,11 @@ func TestACM(t *testing.T) {
 						CertificateChain: aws.String(chain),
 					},
 				},
+				nil,
 			),
 			expect: acmExpect{
 				ARN:         "foobar",
 				DomainNames: []string{"foobar.de"},
-				Error:       nil,
 			},
 		},
 		{
@@ -68,16 +69,16 @@ func TestACM(t *testing.T) {
 						Certificate: aws.String(cert),
 					},
 				},
+				nil,
 			),
 			expect: acmExpect{
 				ARN:         "foobar",
 				DomainNames: []string{"foobar.de"},
-				Error:       nil,
 			},
 		},
 		{
 			msg: "Found one ACM Cert with correct filter tag",
-			api: fake.NewACMClientWithTags(
+			api: fake.NewACMClient(
 				acm.ListCertificatesOutput{
 					CertificateSummaryList: []*acm.CertificateSummary{
 						{
@@ -111,12 +112,11 @@ func TestACM(t *testing.T) {
 			expect: acmExpect{
 				ARN:         "foobar",
 				DomainNames: []string{"foobar.de"},
-				Error:       nil,
 			},
 		},
 		{
 			msg: "ACM Cert with incorrect filter tag should not be found",
-			api: fake.NewACMClientWithTags(
+			api: fake.NewACMClient(
 				acm.ListCertificatesOutput{
 					CertificateSummaryList: []*acm.CertificateSummary{
 						{
@@ -141,23 +141,34 @@ func TestACM(t *testing.T) {
 				EmptyList:   true,
 				ARN:         "foobar",
 				DomainNames: []string{"foobar.de"},
-				Error:       nil,
+			},
+		},
+		{
+			msg: "Fail on ListCertificatesPages error",
+			api: fake.NewACMClient(
+				acm.ListCertificatesOutput{}, nil, nil,
+			).WithListCertificatesPages(func(input *acm.ListCertificatesInput, fn func(p *acm.ListCertificatesOutput, lastPage bool) (shouldContinue bool)) error {
+				return fmt.Errorf("ListCertificatesPages error")
+			}),
+			filterTag: "production=true",
+			expect: acmExpect{
+				Error: "ListCertificatesPages error",
 			},
 		},
 	} {
 		t.Run(ti.msg, func(t *testing.T) {
 			provider := newACMCertProvider(ti.api, ti.filterTag)
 			list, err := provider.GetCertificates()
 
-			if ti.expect.Error != nil {
-				require.Equal(t, ti.expect.Error, err)
+			if ti.expect.Error != "" {
+				require.EqualError(t, err, ti.expect.Error)
+				return
 			} else {
 				require.NoError(t, err)
 			}
 
 			if ti.expect.EmptyList {
 				require.Equal(t, 0, len(list))
-
 			} else {
 				require.Equal(t, 1, len(list))
 

diff --git a/aws/fake/acm.go b/aws/fake/acm.go
@@ -9,47 +9,45 @@ import (
 
 type ACMClient struct {
 	acmiface.ACMAPI
-	output acm.ListCertificatesOutput
-	cert   map[string]*acm.GetCertificateOutput
-	tags   map[string]*acm.ListTagsForCertificateOutput
-}
+	cert map[string]*acm.GetCertificateOutput
+	tags map[string]*acm.ListTagsForCertificateOutput
 
-func (m ACMClient) ListCertificates(in *acm.ListCertificatesInput) (*acm.ListCertificatesOutput, error) {
-	return &m.output, nil
+	listCertificatesPages func(input *acm.ListCertificatesInput, fn func(p *acm.ListCertificatesOutput, lastPage bool) (shouldContinue bool)) error
 }
 
-func (m ACMClient) ListCertificatesPages(input *acm.ListCertificatesInput, fn func(p *acm.ListCertificatesOutput, lastPage bool) (shouldContinue bool)) error {
-	fn(&m.output, true)
-	return nil
+func (m *ACMClient) ListCertificatesPages(input *acm.ListCertificatesInput, fn func(p *acm.ListCertificatesOutput, lastPage bool) (shouldContinue bool)) error {
+	return m.listCertificatesPages(input, fn)
 }
 
-func (m ACMClient) GetCertificate(input *acm.GetCertificateInput) (*acm.GetCertificateOutput, error) {
+func (m *ACMClient) GetCertificate(input *acm.GetCertificateInput) (*acm.GetCertificateOutput, error) {
 	return m.cert[*input.CertificateArn], nil
 }
 
-func (m ACMClient) ListTagsForCertificate(in *acm.ListTagsForCertificateInput) (*acm.ListTagsForCertificateOutput, error) {
+func (m *ACMClient) ListTagsForCertificate(in *acm.ListTagsForCertificateInput) (*acm.ListTagsForCertificateOutput, error) {
 	if in.CertificateArn == nil {
 		return nil, fmt.Errorf("expected a valid CertificateArn, got: nil")
 	}
 	arn := *in.CertificateArn
 	return m.tags[arn], nil
 }
 
-func NewACMClient(output acm.ListCertificatesOutput, cert map[string]*acm.GetCertificateOutput) ACMClient {
-	return ACMClient{
-		output: output,
-		cert:   cert,
-	}
+func (m *ACMClient) WithListCertificatesPages(f func(input *acm.ListCertificatesInput, fn func(p *acm.ListCertificatesOutput, lastPage bool) (shouldContinue bool)) error) *ACMClient {
+	m.listCertificatesPages = f
+	return m
 }
 
-func NewACMClientWithTags(
+func NewACMClient(
 	output acm.ListCertificatesOutput,
 	cert map[string]*acm.GetCertificateOutput,
 	tags map[string]*acm.ListTagsForCertificateOutput,
-) ACMClient {
-	return ACMClient{
-		output: output,
-		cert:   cert,
-		tags:   tags,
+) *ACMClient {
+	c := &ACMClient{
+		cert: cert,
+		tags: tags,
 	}
+	c.WithListCertificatesPages(func(input *acm.ListCertificatesInput, fn func(p *acm.ListCertificatesOutput, lastPage bool) (shouldContinue bool)) error {
+		fn(&output, true)
+		return nil
+	})
+	return c
 }