BCH-1290: Implement ACM DataFetcher with paginated multi-region certi…#2
Closed
saltpy-cs wants to merge 1 commit into
Closed
BCH-1290: Implement ACM DataFetcher with paginated multi-region certi…#2saltpy-cs wants to merge 1 commit into
saltpy-cs wants to merge 1 commit into
Conversation
…ficate collection Replaces the stub FetchData() with a full implementation that calls ListCertificates (paginated), DescribeCertificate, and ListTagsForCertificate for every configured region. The ACMClient interface enables unit tests with a mock client; AWS_ENDPOINT_URL is honoured automatically by the SDK for LocalStack compatibility (BCH-1294). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements AWS ACM certificate data retrieval with multi-region support, adding certificate listing and detail fetching through paginated ACM API calls. The implementation integrates into the plugin evaluation pipeline, includes comprehensive test coverage, and updates AWS SDK v2 dependencies. ChangesACM Certificate Data Fetching
Sequence DiagramsequenceDiagram
participant Eval as CompliancePlugin.Eval
participant Fetcher as DataFetcher
participant ACM as ACMClient
Eval->>Fetcher: FetchData(ctx)
loop per configured region
Fetcher->>ACM: ListCertificates(ctx)
loop paginated results with NextToken
ACM-->>Fetcher: certificate summaries
loop per certificate with ARN
Fetcher->>ACM: DescribeCertificate(arn)
ACM-->>Fetcher: certificate details
Fetcher->>ACM: ListTagsForCertificate(arn)
ACM-->>Fetcher: tags
Fetcher->>Fetcher: assemble CertificateContext
end
end
end
Fetcher-->>Eval: []CertificateContext or error
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
59-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout (deadline) to the context used for
FetchDatainCompliancePlugin.Eval.
main.gocreatesctx := context.Background()and passes it todataFetcher.FetchData(ctx); without a deadline on this call path, ACM/AWS SDK requests can block indefinitely. Usecontext.WithTimeout/WithDeadline(config-driven if available) before callingFetchData.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.go` around lines 59 - 67, Replace creating a bare context in CompliancePlugin.Eval with a context that has a deadline: create ctx, cancel := context.WithTimeout(context.Background(), <timeout>) (or use a config-driven duration from l.config), defer cancel() immediately, and pass that ctx to dataFetcher.FetchData(ctx); ensure the timeout value is configurable (use a sensible default if missing) and handle context deadline/cancellation errors returned by FetchData so the function returns a failure status when the fetch is aborted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/data_test.go`:
- Around line 150-160: The pagination mock in the test's listCertificates
function ignores the incoming request, so the test won't catch if FetchData
fails to forward NextToken; update the mock to inspect its input parameter (the
*acm.ListCertificatesInput) and assert that input.NextToken is nil/empty on the
first call and equals "page2" on the second call (use callCount to distinguish
calls), and if the value is not as expected return a descriptive error to fail
the test; reference the listCertificates mock, callCount and the FetchData
behavior to locate where to add these checks.
- Around line 209-210: The test currently asserts clientRegions in a specific
order (clientRegions[0] == "us-east-1" etc.), which is fragile; change the
assertion in the test that checks multi-region client creation to be
order-insensitive by either converting clientRegions into a set/map and
asserting it contains "us-east-1" and "eu-west-1", or by sorting clientRegions
(e.g., sort.Strings(clientRegions)) and then comparing against the expected
slice, and update the t.Errorf message to report the actual slice when the
assertion fails; locate and update the assertion that references clientRegions
in the test (the if statement comparing length and indices) to use one of these
order-insensitive approaches.
- Around line 42-229: Add tests that assert FetchData returns errors when
underlying ACM calls fail by extending the test suite with cases that inject
failures via mockACMClient: create TestFetchData_ListCertificatesError where
mock.listCertificates returns an error and ensure FetchData returns that error;
TestFetchData_DescribeCertificateError where listCertificates returns a
certificate ARN but mock.describeCertificate returns an error and FetchData
propagates it; and TestFetchData_ListTagsForCertificateError where
describeCertificate succeeds but mock.listTagsForCertificate returns an error
and FetchData fails accordingly. Use the existing newTestFetcher/DataFetcher
setup, call FetchData(context.Background()), and assert non-nil error and that
the error message or type matches the injected error to lock down propagation.
In `@internal/data.go`:
- Line 127: After assigning detail := descOut.Certificate, add a nil check for
descOut.Certificate and handle it gracefully (e.g., return an error or wrap with
context) instead of proceeding to access detail.Options,
detail.DomainValidationOptions, detail.InUseBy, detail.DomainName,
detail.Status, detail.NotAfter, or detail.KeyAlgorithm; locate the assignment to
detail (using the symbols descOut and detail) in internal/data.go and if
descOut.Certificate == nil return a descriptive error like "DescribeCertificate
returned nil Certificate" so callers won't hit a nil pointer panic.
---
Outside diff comments:
In `@main.go`:
- Around line 59-67: Replace creating a bare context in CompliancePlugin.Eval
with a context that has a deadline: create ctx, cancel :=
context.WithTimeout(context.Background(), <timeout>) (or use a config-driven
duration from l.config), defer cancel() immediately, and pass that ctx to
dataFetcher.FetchData(ctx); ensure the timeout value is configurable (use a
sensible default if missing) and handle context deadline/cancellation errors
returned by FetchData so the function returns a failure status when the fetch is
aborted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 015123a2-dcad-4a97-a96d-15a9eb1bc366
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modinternal/data.gointernal/data_test.gomain.go
Comment on lines
+42
to
+229
| func TestFetchData_Empty(t *testing.T) { | ||
| mock := &mockACMClient{ | ||
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | ||
| return &acm.ListCertificatesOutput{}, nil | ||
| }, | ||
| } | ||
| f := newTestFetcher([]string{"us-east-1"}, mock) | ||
| certs, err := f.FetchData(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(certs) != 0 { | ||
| t.Errorf("expected 0 certs, got %d", len(certs)) | ||
| } | ||
| } | ||
|
|
||
| func TestFetchData_SingleCertificate(t *testing.T) { | ||
| arn := "arn:aws:acm:us-east-1:123456789012:certificate/abc-123" | ||
| notAfter := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) | ||
|
|
||
| mock := &mockACMClient{ | ||
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | ||
| return &acm.ListCertificatesOutput{ | ||
| CertificateSummaryList: []types.CertificateSummary{ | ||
| {CertificateArn: aws.String(arn)}, | ||
| }, | ||
| }, nil | ||
| }, | ||
| describeCertificate: func(_ context.Context, _ *acm.DescribeCertificateInput, _ ...func(*acm.Options)) (*acm.DescribeCertificateOutput, error) { | ||
| return &acm.DescribeCertificateOutput{ | ||
| Certificate: &types.CertificateDetail{ | ||
| CertificateArn: aws.String(arn), | ||
| DomainName: aws.String("example.com"), | ||
| Status: types.CertificateStatus("ISSUED"), | ||
| NotAfter: ¬After, | ||
| KeyAlgorithm: types.KeyAlgorithm("RSA_2048"), | ||
| Options: &types.CertificateOptions{ | ||
| CertificateTransparencyLoggingPreference: types.CertificateTransparencyLoggingPreference("ENABLED"), | ||
| }, | ||
| DomainValidationOptions: []types.DomainValidation{ | ||
| {DomainName: aws.String("example.com"), ValidationMethod: types.ValidationMethod("DNS")}, | ||
| }, | ||
| InUseBy: []string{"arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/foo/bar"}, | ||
| }, | ||
| }, nil | ||
| }, | ||
| listTagsForCertificate: func(_ context.Context, _ *acm.ListTagsForCertificateInput, _ ...func(*acm.Options)) (*acm.ListTagsForCertificateOutput, error) { | ||
| return &acm.ListTagsForCertificateOutput{ | ||
| Tags: []types.Tag{ | ||
| {Key: aws.String("env"), Value: aws.String("prod")}, | ||
| }, | ||
| }, nil | ||
| }, | ||
| } | ||
|
|
||
| f := newTestFetcher([]string{"us-east-1"}, mock) | ||
| certs, err := f.FetchData(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(certs) != 1 { | ||
| t.Fatalf("expected 1 cert, got %d", len(certs)) | ||
| } | ||
|
|
||
| c := certs[0] | ||
| if c.CertificateArn != arn { | ||
| t.Errorf("arn: want %q, got %q", arn, c.CertificateArn) | ||
| } | ||
| if c.AccountID != "123456789012" { | ||
| t.Errorf("account_id: want %q, got %q", "123456789012", c.AccountID) | ||
| } | ||
| if c.Region != "us-east-1" { | ||
| t.Errorf("region: want %q, got %q", "us-east-1", c.Region) | ||
| } | ||
| if c.DomainName != "example.com" { | ||
| t.Errorf("domain_name: want %q, got %q", "example.com", c.DomainName) | ||
| } | ||
| if c.Status != "ISSUED" { | ||
| t.Errorf("status: want %q, got %q", "ISSUED", c.Status) | ||
| } | ||
| if c.NotAfter == nil || !c.NotAfter.Equal(notAfter) { | ||
| t.Errorf("not_after: want %v, got %v", notAfter, c.NotAfter) | ||
| } | ||
| if c.KeyAlgorithm != "RSA_2048" { | ||
| t.Errorf("key_algorithm: want %q, got %q", "RSA_2048", c.KeyAlgorithm) | ||
| } | ||
| if c.TransparencyLoggingPreference != "ENABLED" { | ||
| t.Errorf("transparency_logging_preference: want %q, got %q", "ENABLED", c.TransparencyLoggingPreference) | ||
| } | ||
| if len(c.DomainValidationOptions) != 1 || c.DomainValidationOptions[0].ValidationMethod != "DNS" { | ||
| t.Errorf("domain_validation_options: unexpected %v", c.DomainValidationOptions) | ||
| } | ||
| if len(c.InUseBy) != 1 { | ||
| t.Errorf("in_use_by: expected 1 entry, got %v", c.InUseBy) | ||
| } | ||
| if c.Tags["env"] != "prod" { | ||
| t.Errorf("tags: expected env=prod, got %v", c.Tags) | ||
| } | ||
| } | ||
|
|
||
| func TestFetchData_Pagination(t *testing.T) { | ||
| arns := []string{ | ||
| "arn:aws:acm:us-east-1:123456789012:certificate/aaa", | ||
| "arn:aws:acm:us-east-1:123456789012:certificate/bbb", | ||
| } | ||
| callCount := 0 | ||
|
|
||
| mock := &mockACMClient{ | ||
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | ||
| callCount++ | ||
| if callCount == 1 { | ||
| return &acm.ListCertificatesOutput{ | ||
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[0])}}, | ||
| NextToken: aws.String("page2"), | ||
| }, nil | ||
| } | ||
| return &acm.ListCertificatesOutput{ | ||
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[1])}}, | ||
| }, nil | ||
| }, | ||
| describeCertificate: func(_ context.Context, params *acm.DescribeCertificateInput, _ ...func(*acm.Options)) (*acm.DescribeCertificateOutput, error) { | ||
| return &acm.DescribeCertificateOutput{ | ||
| Certificate: &types.CertificateDetail{ | ||
| CertificateArn: params.CertificateArn, | ||
| DomainName: aws.String("example.com"), | ||
| Status: types.CertificateStatus("ISSUED"), | ||
| }, | ||
| }, nil | ||
| }, | ||
| listTagsForCertificate: func(_ context.Context, _ *acm.ListTagsForCertificateInput, _ ...func(*acm.Options)) (*acm.ListTagsForCertificateOutput, error) { | ||
| return &acm.ListTagsForCertificateOutput{}, nil | ||
| }, | ||
| } | ||
|
|
||
| f := newTestFetcher([]string{"us-east-1"}, mock) | ||
| certs, err := f.FetchData(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(certs) != 2 { | ||
| t.Errorf("expected 2 certs, got %d", len(certs)) | ||
| } | ||
| if callCount != 2 { | ||
| t.Errorf("expected 2 ListCertificates calls, got %d", callCount) | ||
| } | ||
| } | ||
|
|
||
| func TestFetchData_MultipleRegions(t *testing.T) { | ||
| var clientRegions []string | ||
|
|
||
| f := &DataFetcher{ | ||
| logger: hclog.NewNullLogger(), | ||
| config: &PluginConfig{Regions: []string{"us-east-1", "eu-west-1"}}, | ||
| newClient: func(_ context.Context, region string) (ACMClient, error) { | ||
| clientRegions = append(clientRegions, region) | ||
| return &mockACMClient{ | ||
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | ||
| return &acm.ListCertificatesOutput{}, nil | ||
| }, | ||
| }, nil | ||
| }, | ||
| } | ||
|
|
||
| _, err := f.FetchData(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(clientRegions) != 2 || clientRegions[0] != "us-east-1" || clientRegions[1] != "eu-west-1" { | ||
| t.Errorf("expected clients for us-east-1 and eu-west-1, got %v", clientRegions) | ||
| } | ||
| } | ||
|
|
||
| func TestFetchData_AccountIDFromARN(t *testing.T) { | ||
| cases := []struct { | ||
| arn string | ||
| accountID string | ||
| }{ | ||
| {"arn:aws:acm:us-east-1:123456789012:certificate/abc", "123456789012"}, | ||
| {"arn:aws:acm:eu-west-1:999999999999:certificate/xyz", "999999999999"}, | ||
| {"invalid", ""}, | ||
| } | ||
| for _, tc := range cases { | ||
| got := arnAccountID(tc.arn) | ||
| if got != tc.accountID { | ||
| t.Errorf("arnAccountID(%q): want %q, got %q", tc.arn, tc.accountID, got) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add explicit error-path tests for AWS API failures.
The suite currently validates happy paths; add at least one case each for ListCertificates, DescribeCertificate, and ListTagsForCertificate failures to lock down error propagation behavior.
Suggested test additions (skeleton)
+func TestFetchData_ListCertificatesError(t *testing.T) {
+ mock := &mockACMClient{
+ listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) {
+ return nil, errors.New("list failed")
+ },
+ }
+ f := newTestFetcher([]string{"us-east-1"}, mock)
+ if _, err := f.FetchData(context.Background()); err == nil {
+ t.Fatal("expected error, got nil")
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestFetchData_Empty(t *testing.T) { | |
| mock := &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| return &acm.ListCertificatesOutput{}, nil | |
| }, | |
| } | |
| f := newTestFetcher([]string{"us-east-1"}, mock) | |
| certs, err := f.FetchData(context.Background()) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(certs) != 0 { | |
| t.Errorf("expected 0 certs, got %d", len(certs)) | |
| } | |
| } | |
| func TestFetchData_SingleCertificate(t *testing.T) { | |
| arn := "arn:aws:acm:us-east-1:123456789012:certificate/abc-123" | |
| notAfter := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) | |
| mock := &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{ | |
| {CertificateArn: aws.String(arn)}, | |
| }, | |
| }, nil | |
| }, | |
| describeCertificate: func(_ context.Context, _ *acm.DescribeCertificateInput, _ ...func(*acm.Options)) (*acm.DescribeCertificateOutput, error) { | |
| return &acm.DescribeCertificateOutput{ | |
| Certificate: &types.CertificateDetail{ | |
| CertificateArn: aws.String(arn), | |
| DomainName: aws.String("example.com"), | |
| Status: types.CertificateStatus("ISSUED"), | |
| NotAfter: ¬After, | |
| KeyAlgorithm: types.KeyAlgorithm("RSA_2048"), | |
| Options: &types.CertificateOptions{ | |
| CertificateTransparencyLoggingPreference: types.CertificateTransparencyLoggingPreference("ENABLED"), | |
| }, | |
| DomainValidationOptions: []types.DomainValidation{ | |
| {DomainName: aws.String("example.com"), ValidationMethod: types.ValidationMethod("DNS")}, | |
| }, | |
| InUseBy: []string{"arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/foo/bar"}, | |
| }, | |
| }, nil | |
| }, | |
| listTagsForCertificate: func(_ context.Context, _ *acm.ListTagsForCertificateInput, _ ...func(*acm.Options)) (*acm.ListTagsForCertificateOutput, error) { | |
| return &acm.ListTagsForCertificateOutput{ | |
| Tags: []types.Tag{ | |
| {Key: aws.String("env"), Value: aws.String("prod")}, | |
| }, | |
| }, nil | |
| }, | |
| } | |
| f := newTestFetcher([]string{"us-east-1"}, mock) | |
| certs, err := f.FetchData(context.Background()) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(certs) != 1 { | |
| t.Fatalf("expected 1 cert, got %d", len(certs)) | |
| } | |
| c := certs[0] | |
| if c.CertificateArn != arn { | |
| t.Errorf("arn: want %q, got %q", arn, c.CertificateArn) | |
| } | |
| if c.AccountID != "123456789012" { | |
| t.Errorf("account_id: want %q, got %q", "123456789012", c.AccountID) | |
| } | |
| if c.Region != "us-east-1" { | |
| t.Errorf("region: want %q, got %q", "us-east-1", c.Region) | |
| } | |
| if c.DomainName != "example.com" { | |
| t.Errorf("domain_name: want %q, got %q", "example.com", c.DomainName) | |
| } | |
| if c.Status != "ISSUED" { | |
| t.Errorf("status: want %q, got %q", "ISSUED", c.Status) | |
| } | |
| if c.NotAfter == nil || !c.NotAfter.Equal(notAfter) { | |
| t.Errorf("not_after: want %v, got %v", notAfter, c.NotAfter) | |
| } | |
| if c.KeyAlgorithm != "RSA_2048" { | |
| t.Errorf("key_algorithm: want %q, got %q", "RSA_2048", c.KeyAlgorithm) | |
| } | |
| if c.TransparencyLoggingPreference != "ENABLED" { | |
| t.Errorf("transparency_logging_preference: want %q, got %q", "ENABLED", c.TransparencyLoggingPreference) | |
| } | |
| if len(c.DomainValidationOptions) != 1 || c.DomainValidationOptions[0].ValidationMethod != "DNS" { | |
| t.Errorf("domain_validation_options: unexpected %v", c.DomainValidationOptions) | |
| } | |
| if len(c.InUseBy) != 1 { | |
| t.Errorf("in_use_by: expected 1 entry, got %v", c.InUseBy) | |
| } | |
| if c.Tags["env"] != "prod" { | |
| t.Errorf("tags: expected env=prod, got %v", c.Tags) | |
| } | |
| } | |
| func TestFetchData_Pagination(t *testing.T) { | |
| arns := []string{ | |
| "arn:aws:acm:us-east-1:123456789012:certificate/aaa", | |
| "arn:aws:acm:us-east-1:123456789012:certificate/bbb", | |
| } | |
| callCount := 0 | |
| mock := &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| callCount++ | |
| if callCount == 1 { | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[0])}}, | |
| NextToken: aws.String("page2"), | |
| }, nil | |
| } | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[1])}}, | |
| }, nil | |
| }, | |
| describeCertificate: func(_ context.Context, params *acm.DescribeCertificateInput, _ ...func(*acm.Options)) (*acm.DescribeCertificateOutput, error) { | |
| return &acm.DescribeCertificateOutput{ | |
| Certificate: &types.CertificateDetail{ | |
| CertificateArn: params.CertificateArn, | |
| DomainName: aws.String("example.com"), | |
| Status: types.CertificateStatus("ISSUED"), | |
| }, | |
| }, nil | |
| }, | |
| listTagsForCertificate: func(_ context.Context, _ *acm.ListTagsForCertificateInput, _ ...func(*acm.Options)) (*acm.ListTagsForCertificateOutput, error) { | |
| return &acm.ListTagsForCertificateOutput{}, nil | |
| }, | |
| } | |
| f := newTestFetcher([]string{"us-east-1"}, mock) | |
| certs, err := f.FetchData(context.Background()) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(certs) != 2 { | |
| t.Errorf("expected 2 certs, got %d", len(certs)) | |
| } | |
| if callCount != 2 { | |
| t.Errorf("expected 2 ListCertificates calls, got %d", callCount) | |
| } | |
| } | |
| func TestFetchData_MultipleRegions(t *testing.T) { | |
| var clientRegions []string | |
| f := &DataFetcher{ | |
| logger: hclog.NewNullLogger(), | |
| config: &PluginConfig{Regions: []string{"us-east-1", "eu-west-1"}}, | |
| newClient: func(_ context.Context, region string) (ACMClient, error) { | |
| clientRegions = append(clientRegions, region) | |
| return &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| return &acm.ListCertificatesOutput{}, nil | |
| }, | |
| }, nil | |
| }, | |
| } | |
| _, err := f.FetchData(context.Background()) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(clientRegions) != 2 || clientRegions[0] != "us-east-1" || clientRegions[1] != "eu-west-1" { | |
| t.Errorf("expected clients for us-east-1 and eu-west-1, got %v", clientRegions) | |
| } | |
| } | |
| func TestFetchData_AccountIDFromARN(t *testing.T) { | |
| cases := []struct { | |
| arn string | |
| accountID string | |
| }{ | |
| {"arn:aws:acm:us-east-1:123456789012:certificate/abc", "123456789012"}, | |
| {"arn:aws:acm:eu-west-1:999999999999:certificate/xyz", "999999999999"}, | |
| {"invalid", ""}, | |
| } | |
| for _, tc := range cases { | |
| got := arnAccountID(tc.arn) | |
| if got != tc.accountID { | |
| t.Errorf("arnAccountID(%q): want %q, got %q", tc.arn, tc.accountID, got) | |
| } | |
| } | |
| } | |
| func TestFetchData_Empty(t *testing.T) { | |
| mock := &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| return &acm.ListCertificatesOutput{}, nil | |
| }, | |
| } | |
| f := newTestFetcher([]string{"us-east-1"}, mock) | |
| certs, err := f.FetchData(context.Background()) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(certs) != 0 { | |
| t.Errorf("expected 0 certs, got %d", len(certs)) | |
| } | |
| } | |
| func TestFetchData_SingleCertificate(t *testing.T) { | |
| arn := "arn:aws:acm:us-east-1:123456789012:certificate/abc-123" | |
| notAfter := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) | |
| mock := &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{ | |
| {CertificateArn: aws.String(arn)}, | |
| }, | |
| }, nil | |
| }, | |
| describeCertificate: func(_ context.Context, _ *acm.DescribeCertificateInput, _ ...func(*acm.Options)) (*acm.DescribeCertificateOutput, error) { | |
| return &acm.DescribeCertificateOutput{ | |
| Certificate: &types.CertificateDetail{ | |
| CertificateArn: aws.String(arn), | |
| DomainName: aws.String("example.com"), | |
| Status: types.CertificateStatus("ISSUED"), | |
| NotAfter: ¬After, | |
| KeyAlgorithm: types.KeyAlgorithm("RSA_2048"), | |
| Options: &types.CertificateOptions{ | |
| CertificateTransparencyLoggingPreference: types.CertificateTransparencyLoggingPreference("ENABLED"), | |
| }, | |
| DomainValidationOptions: []types.DomainValidation{ | |
| {DomainName: aws.String("example.com"), ValidationMethod: types.ValidationMethod("DNS")}, | |
| }, | |
| InUseBy: []string{"arn:aws:elasticloadbalancing:us-east-1:123456789012:loadbalancer/app/foo/bar"}, | |
| }, | |
| }, nil | |
| }, | |
| listTagsForCertificate: func(_ context.Context, _ *acm.ListTagsForCertificateInput, _ ...func(*acm.Options)) (*acm.ListTagsForCertificateOutput, error) { | |
| return &acm.ListTagsForCertificateOutput{ | |
| Tags: []types.Tag{ | |
| {Key: aws.String("env"), Value: aws.String("prod")}, | |
| }, | |
| }, nil | |
| }, | |
| } | |
| f := newTestFetcher([]string{"us-east-1"}, mock) | |
| certs, err := f.FetchData(context.Background()) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(certs) != 1 { | |
| t.Fatalf("expected 1 cert, got %d", len(certs)) | |
| } | |
| c := certs[0] | |
| if c.CertificateArn != arn { | |
| t.Errorf("arn: want %q, got %q", arn, c.CertificateArn) | |
| } | |
| if c.AccountID != "123456789012" { | |
| t.Errorf("account_id: want %q, got %q", "123456789012", c.AccountID) | |
| } | |
| if c.Region != "us-east-1" { | |
| t.Errorf("region: want %q, got %q", "us-east-1", c.Region) | |
| } | |
| if c.DomainName != "example.com" { | |
| t.Errorf("domain_name: want %q, got %q", "example.com", c.DomainName) | |
| } | |
| if c.Status != "ISSUED" { | |
| t.Errorf("status: want %q, got %q", "ISSUED", c.Status) | |
| } | |
| if c.NotAfter == nil || !c.NotAfter.Equal(notAfter) { | |
| t.Errorf("not_after: want %v, got %v", notAfter, c.NotAfter) | |
| } | |
| if c.KeyAlgorithm != "RSA_2048" { | |
| t.Errorf("key_algorithm: want %q, got %q", "RSA_2048", c.KeyAlgorithm) | |
| } | |
| if c.TransparencyLoggingPreference != "ENABLED" { | |
| t.Errorf("transparency_logging_preference: want %q, got %q", "ENABLED", c.TransparencyLoggingPreference) | |
| } | |
| if len(c.DomainValidationOptions) != 1 || c.DomainValidationOptions[0].ValidationMethod != "DNS" { | |
| t.Errorf("domain_validation_options: unexpected %v", c.DomainValidationOptions) | |
| } | |
| if len(c.InUseBy) != 1 { | |
| t.Errorf("in_use_by: expected 1 entry, got %v", c.InUseBy) | |
| } | |
| if c.Tags["env"] != "prod" { | |
| t.Errorf("tags: expected env=prod, got %v", c.Tags) | |
| } | |
| } | |
| func TestFetchData_Pagination(t *testing.T) { | |
| arns := []string{ | |
| "arn:aws:acm:us-east-1:123456789012:certificate/aaa", | |
| "arn:aws:acm:us-east-1:123456789012:certificate/bbb", | |
| } | |
| callCount := 0 | |
| mock := &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| callCount++ | |
| if callCount == 1 { | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[0])}}, | |
| NextToken: aws.String("page2"), | |
| }, nil | |
| } | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[1])}}, | |
| }, nil | |
| }, | |
| describeCertificate: func(_ context.Context, params *acm.DescribeCertificateInput, _ ...func(*acm.Options)) (*acm.DescribeCertificateOutput, error) { | |
| return &acm.DescribeCertificateOutput{ | |
| Certificate: &types.CertificateDetail{ | |
| CertificateArn: params.CertificateArn, | |
| DomainName: aws.String("example.com"), | |
| Status: types.CertificateStatus("ISSUED"), | |
| }, | |
| }, nil | |
| }, | |
| listTagsForCertificate: func(_ context.Context, _ *acm.ListTagsForCertificateInput, _ ...func(*acm.Options)) (*acm.ListTagsForCertificateOutput, error) { | |
| return &acm.ListTagsForCertificateOutput{}, nil | |
| }, | |
| } | |
| f := newTestFetcher([]string{"us-east-1"}, mock) | |
| certs, err := f.FetchData(context.Background()) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(certs) != 2 { | |
| t.Errorf("expected 2 certs, got %d", len(certs)) | |
| } | |
| if callCount != 2 { | |
| t.Errorf("expected 2 ListCertificates calls, got %d", callCount) | |
| } | |
| } | |
| func TestFetchData_MultipleRegions(t *testing.T) { | |
| var clientRegions []string | |
| f := &DataFetcher{ | |
| logger: hclog.NewNullLogger(), | |
| config: &PluginConfig{Regions: []string{"us-east-1", "eu-west-1"}}, | |
| newClient: func(_ context.Context, region string) (ACMClient, error) { | |
| clientRegions = append(clientRegions, region) | |
| return &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| return &acm.ListCertificatesOutput{}, nil | |
| }, | |
| }, nil | |
| }, | |
| } | |
| _, err := f.FetchData(context.Background()) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(clientRegions) != 2 || clientRegions[0] != "us-east-1" || clientRegions[1] != "eu-west-1" { | |
| t.Errorf("expected clients for us-east-1 and eu-west-1, got %v", clientRegions) | |
| } | |
| } | |
| func TestFetchData_AccountIDFromARN(t *testing.T) { | |
| cases := []struct { | |
| arn string | |
| accountID string | |
| }{ | |
| {"arn:aws:acm:us-east-1:123456789012:certificate/abc", "123456789012"}, | |
| {"arn:aws:acm:eu-west-1:999999999999:certificate/xyz", "999999999999"}, | |
| {"invalid", ""}, | |
| } | |
| for _, tc := range cases { | |
| got := arnAccountID(tc.arn) | |
| if got != tc.accountID { | |
| t.Errorf("arnAccountID(%q): want %q, got %q", tc.arn, tc.accountID, got) | |
| } | |
| } | |
| } | |
| func TestFetchData_ListCertificatesError(t *testing.T) { | |
| mock := &mockACMClient{ | |
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| return nil, errors.New("list failed") | |
| }, | |
| } | |
| f := newTestFetcher([]string{"us-east-1"}, mock) | |
| if _, err := f.FetchData(context.Background()); err == nil { | |
| t.Fatal("expected error, got nil") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/data_test.go` around lines 42 - 229, Add tests that assert FetchData
returns errors when underlying ACM calls fail by extending the test suite with
cases that inject failures via mockACMClient: create
TestFetchData_ListCertificatesError where mock.listCertificates returns an error
and ensure FetchData returns that error; TestFetchData_DescribeCertificateError
where listCertificates returns a certificate ARN but mock.describeCertificate
returns an error and FetchData propagates it; and
TestFetchData_ListTagsForCertificateError where describeCertificate succeeds but
mock.listTagsForCertificate returns an error and FetchData fails accordingly.
Use the existing newTestFetcher/DataFetcher setup, call
FetchData(context.Background()), and assert non-nil error and that the error
message or type matches the injected error to lock down propagation.
Comment on lines
+150
to
+160
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | ||
| callCount++ | ||
| if callCount == 1 { | ||
| return &acm.ListCertificatesOutput{ | ||
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[0])}}, | ||
| NextToken: aws.String("page2"), | ||
| }, nil | ||
| } | ||
| return &acm.ListCertificatesOutput{ | ||
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[1])}}, | ||
| }, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Assert request NextToken in the pagination mock.
Line 150 currently ignores the request input, so pagination can still “pass” even if FetchData never forwards NextToken.
Proposed test hardening
listCertificates: func(_ context.Context, in *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) {
callCount++
if callCount == 1 {
+ if in.NextToken != nil {
+ t.Fatalf("first page should not include NextToken, got %q", aws.ToString(in.NextToken))
+ }
return &acm.ListCertificatesOutput{
CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[0])}},
NextToken: aws.String("page2"),
}, nil
}
+ if aws.ToString(in.NextToken) != "page2" {
+ t.Fatalf("second page should include NextToken=page2, got %q", aws.ToString(in.NextToken))
+ }
return &acm.ListCertificatesOutput{
CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[1])}},
}, nil
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| listCertificates: func(_ context.Context, _ *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| callCount++ | |
| if callCount == 1 { | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[0])}}, | |
| NextToken: aws.String("page2"), | |
| }, nil | |
| } | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[1])}}, | |
| }, nil | |
| listCertificates: func(_ context.Context, in *acm.ListCertificatesInput, _ ...func(*acm.Options)) (*acm.ListCertificatesOutput, error) { | |
| callCount++ | |
| if callCount == 1 { | |
| if in.NextToken != nil { | |
| t.Fatalf("first page should not include NextToken, got %q", aws.ToString(in.NextToken)) | |
| } | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[0])}}, | |
| NextToken: aws.String("page2"), | |
| }, nil | |
| } | |
| if aws.ToString(in.NextToken) != "page2" { | |
| t.Fatalf("second page should include NextToken=page2, got %q", aws.ToString(in.NextToken)) | |
| } | |
| return &acm.ListCertificatesOutput{ | |
| CertificateSummaryList: []types.CertificateSummary{{CertificateArn: aws.String(arns[1])}}, | |
| }, nil | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/data_test.go` around lines 150 - 160, The pagination mock in the
test's listCertificates function ignores the incoming request, so the test won't
catch if FetchData fails to forward NextToken; update the mock to inspect its
input parameter (the *acm.ListCertificatesInput) and assert that input.NextToken
is nil/empty on the first call and equals "page2" on the second call (use
callCount to distinguish calls), and if the value is not as expected return a
descriptive error to fail the test; reference the listCertificates mock,
callCount and the FetchData behavior to locate where to add these checks.
Comment on lines
+209
to
+210
| if len(clientRegions) != 2 || clientRegions[0] != "us-east-1" || clientRegions[1] != "eu-west-1" { | ||
| t.Errorf("expected clients for us-east-1 and eu-west-1, got %v", clientRegions) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid order-coupled assertion for multi-region client creation.
Line 209 hard-codes region order; this makes the test fragile if region processing becomes concurrent.
Order-insensitive assertion
- if len(clientRegions) != 2 || clientRegions[0] != "us-east-1" || clientRegions[1] != "eu-west-1" {
- t.Errorf("expected clients for us-east-1 and eu-west-1, got %v", clientRegions)
- }
+ got := map[string]int{}
+ for _, r := range clientRegions {
+ got[r]++
+ }
+ if got["us-east-1"] != 1 || got["eu-west-1"] != 1 || len(clientRegions) != 2 {
+ t.Errorf("expected one client each for us-east-1 and eu-west-1, got %v", clientRegions)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(clientRegions) != 2 || clientRegions[0] != "us-east-1" || clientRegions[1] != "eu-west-1" { | |
| t.Errorf("expected clients for us-east-1 and eu-west-1, got %v", clientRegions) | |
| got := map[string]int{} | |
| for _, r := range clientRegions { | |
| got[r]++ | |
| } | |
| if got["us-east-1"] != 1 || got["eu-west-1"] != 1 || len(clientRegions) != 2 { | |
| t.Errorf("expected one client each for us-east-1 and eu-west-1, got %v", clientRegions) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/data_test.go` around lines 209 - 210, The test currently asserts
clientRegions in a specific order (clientRegions[0] == "us-east-1" etc.), which
is fragile; change the assertion in the test that checks multi-region client
creation to be order-insensitive by either converting clientRegions into a
set/map and asserting it contains "us-east-1" and "eu-west-1", or by sorting
clientRegions (e.g., sort.Strings(clientRegions)) and then comparing against the
expected slice, and update the t.Errorf message to report the actual slice when
the assertion fails; locate and update the assertion that references
clientRegions in the test (the if statement comparing length and indices) to use
one of these order-insensitive approaches.
| return CertificateContext{}, fmt.Errorf("ListTagsForCertificate: %w", err) | ||
| } | ||
|
|
||
| detail := descOut.Certificate |
There was a problem hiding this comment.
Add nil check for descOut.Certificate.
If DescribeCertificate returns successfully but with a nil Certificate field (edge case), subsequent accesses to detail.Options, detail.DomainValidationOptions, detail.InUseBy, detail.DomainName, detail.Status, detail.NotAfter, and detail.KeyAlgorithm will cause a nil pointer panic.
🛡️ Proposed fix to guard against nil Certificate
detail := descOut.Certificate
+ if detail == nil {
+ return CertificateContext{}, fmt.Errorf("DescribeCertificate returned nil certificate")
+ }
transparencyPref := ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| detail := descOut.Certificate | |
| detail := descOut.Certificate | |
| if detail == nil { | |
| return CertificateContext{}, fmt.Errorf("DescribeCertificate returned nil certificate") | |
| } | |
| transparencyPref := "" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/data.go` at line 127, After assigning detail := descOut.Certificate,
add a nil check for descOut.Certificate and handle it gracefully (e.g., return
an error or wrap with context) instead of proceeding to access detail.Options,
detail.DomainValidationOptions, detail.InUseBy, detail.DomainName,
detail.Status, detail.NotAfter, or detail.KeyAlgorithm; locate the assignment to
detail (using the symbols descOut and detail) in internal/data.go and if
descOut.Certificate == nil return a descriptive error like "DescribeCertificate
returned nil Certificate" so callers won't hit a nil pointer panic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ficate collection
Replaces the stub FetchData() with a full implementation that calls ListCertificates (paginated), DescribeCertificate, and ListTagsForCertificate for every configured region. The ACMClient interface enables unit tests with a mock client; AWS_ENDPOINT_URL is honoured automatically by the SDK for LocalStack compatibility (BCH-1294).
Summary by CodeRabbit
New Features
Tests