config/crds/bases/external-secrets.io_clustersecretstores.yaml (1)

645-652: Enum update for AWS service to include CertificateManager looks correct

Adding CertificateManager to the aws.service enum is consistent with the new ACM provider and simply broadens allowed values without breaking existing manifests. One optional follow‑up you might consider (in this or a later PR) is adjusting the nearby aws description string (Line 469) so it no longer reads only as “Secret Manager provider,” since the service can now be SecretsManager, ParameterStore, or CertificateManager.

providers/v1/aws/provider.go (1)

17-18: Update the package comment to include CertificateManager.

The package documentation should reflect all supported services, including the newly added Certificate Manager.

🔎 Apply this diff to update the package comment:

-// Package aws implements AWS provider interfaces for External Secrets Operator,
-// supporting SecretManager and ParameterStore services.
+// Package aws implements AWS provider interfaces for External Secrets Operator,
+// supporting SecretsManager, ParameterStore, and CertificateManager services.

go.mod (1)

405-405: Consider updating to the latest ACM SDK version.

v1.37.13 is not the latest version of the module. While no known security vulnerabilities exist for this version and the package maintains healthy release cadence, a more recent version may be available. Review the latest release notes to determine if an update is warranted.

config/crds/bases/external-secrets.io_secretstores.yaml (1)

645-652: ACM enum addition looks correct; consider aligning the AWS provider description.

Adding CertificateManager to spec.provider.aws.service.enum is consistent with the new ACM provider and will let v1 SecretStore objects validate correctly. No schema issues here.

Since aws.service now supports SecretsManager, ParameterStore, and CertificateManager, the aws description ("sync secrets using AWS Secret Manager provider") above is slightly misleading; consider updating it to reflect that this block configures AWS as a provider and the specific service is selected via the service field.

providers/v1/aws/certificatemanager/certificatemanager_test.go (1)

166-189: Rename successCases and consider using subtests.

The slice successCases contains both success and error scenarios, making the name misleading. Additionally, using numeric indices in error messages makes debugging difficult.

🔎 Consider these improvements:

1. Rename the slice:

-	successCases := []*certificateManagerTestCase{
+	testCases := []*certificateManagerTestCase{

2. Add descriptive names and use subtests for better isolation:

testCases := []struct {
	name string
	tc   *certificateManagerTestCase
}{
	{"valid certificate with all fields", makeValidCertificateManagerTestCaseCustom(setValidCertificate)},
	{"certificate only", makeValidCertificateManagerTestCaseCustom(setCertificateOnly)},
	{"certificate and chain", makeValidCertificateManagerTestCaseCustom(setCertificateAndChain)},
	{"no data returned error", makeValidCertificateManagerTestCaseCustom(setNoDataReturned)},
	{"export certificate failure", makeValidCertificateManagerTestCaseCustom(setExportCertFail)},
	{"describe certificate failure", makeValidCertificateManagerTestCaseCustom(setDescribeCertFail)},
	{"empty ARN error", makeValidCertificateManagerTestCaseCustom(setEmptyARN)},
}

for _, tt := range testCases {
	t.Run(tt.name, func(t *testing.T) {
		// test logic here using tt.tc
	})
}

This improves debuggability by showing which specific test case failed by name rather than index.

providers/v1/aws/certificatemanager/resolver.go (1)

37-49: Avoid shadowing the url package name.

Line 40 declares a variable named url that shadows the imported url package. While this code works, it reduces readability and could cause confusion.

🔎 Apply this diff to use a more descriptive variable name:

 func (c customEndpointResolver) ResolveEndpoint(ctx context.Context, params acm.EndpointParameters) (smithyendpoints.Endpoint, error) {
 	endpoint := smithyendpoints.Endpoint{}
 	if v := os.Getenv(ACMEndpointEnv); v != "" {
-		url, err := url.Parse(v)
+		parsedURL, err := url.Parse(v)
 		if err != nil {
 			return endpoint, fmt.Errorf("failed to parse acm endpoint %s: %w", v, err)
 		}
-		endpoint.URI = *url
+		endpoint.URI = *parsedURL
 		return endpoint, nil
 	}
 	defaultResolver := acm.NewDefaultEndpointResolverV2()
 	return defaultResolver.ResolveEndpoint(ctx, params)
 }

providers/v1/aws/certificatemanager/certificatemanager.go (2)

32-49: Unused field and interface methods.

• The prefix field (line 37) is stored but never used in any method implementation.
• ACMInterface defines ListCertificates, GetCertificate, AddTagsToCertificate, and RemoveTagsFromCertificate, but none of these are called by the provider.

Consider removing unused code to reduce maintenance overhead.

🔎 Suggested diff

 type CertificateManager struct {
 	cfg          *aws.Config
 	client       ACMInterface
 	referentAuth bool
-	prefix       string
 }

 // ACMInterface defines the subset of ACM API methods used by the provider.
 // see: https://docs.aws.amazon.com/sdk-for-go/api/service/acm/
 type ACMInterface interface {
 	DescribeCertificate(ctx context.Context, input *acm.DescribeCertificateInput, opts ...func(*acm.Options)) (*acm.DescribeCertificateOutput, error)
 	ExportCertificate(ctx context.Context, input *acm.ExportCertificateInput, opts ...func(*acm.Options)) (*acm.ExportCertificateOutput, error)
-	ListCertificates(ctx context.Context, input *acm.ListCertificatesInput, opts ...func(*acm.Options)) (*acm.ListCertificatesOutput, error)
-	GetCertificate(ctx context.Context, input *acm.GetCertificateInput, opts ...func(*acm.Options)) (*acm.GetCertificateOutput, error)
-	AddTagsToCertificate(ctx context.Context, input *acm.AddTagsToCertificateInput, opts ...func(*acm.Options)) (*acm.AddTagsToCertificateOutput, error)
-	RemoveTagsFromCertificate(ctx context.Context, input *acm.RemoveTagsFromCertificateInput, opts ...func(*acm.Options)) (*acm.RemoveTagsFromCertificateOutput, error)
 }

---

84-99: Consider supporting remoteRef.Property for component selection.

The current implementation concatenates all components (certificate, chain, private key) into a single value. For Kubernetes TLS secrets, users typically need separate tls.crt and tls.key fields.

Supporting ref.Property would allow users to extract individual components:

• certificate → just the certificate
• chain → just the certificate chain
• privateKey → just the private key
• empty/default → all concatenated (current behavior)

This would improve flexibility for secret templating.

🔎 Example implementation

switch ref.Property {
case "certificate":
    if exportOut.Certificate != nil {
        return []byte(*exportOut.Certificate), nil
    }
    return nil, fmt.Errorf("certificate not available")
case "chain":
    if exportOut.CertificateChain != nil {
        return []byte(*exportOut.CertificateChain), nil
    }
    return nil, fmt.Errorf("certificate chain not available")
case "privateKey":
    if exportOut.PrivateKey != nil {
        return []byte(*exportOut.PrivateKey), nil
    }
    return nil, fmt.Errorf("private key not available")
default:
    // Current behavior - return all concatenated
}

Configuration used: defaults

Review profile: CHILL

Plan: Pro

providers/v1/aws/certificatemanager/certificatemanager_test.go (1)

173-197: Consider renaming test cases array and using subtests.

The variable successCases (line 173) contains both success and error test cases (lines 177-181 are error scenarios), which is misleading. Additionally, using subtests with t.Run would improve test output clarity and make it easier to identify which specific scenario fails.

💡 Suggested improvements

Rename the test cases array:

-	successCases := []*certificateManagerTestCase{
+	testCases := []*certificateManagerTestCase{

Consider restructuring with subtests for better test organization:

testCases := map[string]struct {
	setup          func(*certificateManagerTestCase)
	expectedSecret string
	expectError    string
}{
	"valid certificate with all fields": {
		setup:          setValidCertificate,
		expectedSecret: testCertWithAll,
	},
	"certificate only": {
		setup:          setCertificateOnly,
		expectedSecret: testCertificate + "\n",
	},
	// ... other cases
}

for name, tc := range testCases {
	t.Run(name, func(t *testing.T) {
		// test logic
	})
}

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

providers/v1/aws/provider_test.go (1)

474-488: LGTM! Consider adding CertificateManager client creation test.

The test case correctly validates the CertificateManager service with a valid region, following the established pattern. However, TestProvider (lines 39-153) lacks a test case for creating a CertificateManager client, unlike SecretsManager and ParameterStore which have dedicated cases verifying expType. Consider adding:

{
    test:    "should create certificate manager client",
    expErr:  false,
    expType: &certificatemanager.CertificateManager{},
    store: &esv1.SecretStore{
        Spec: esv1.SecretStoreSpec{
            Provider: &esv1.SecretStoreProvider{
                AWS: &esv1.AWSProvider{
                    Service: esv1.AWSServiceCertificateManager,
                },
            },
        },
    },
},

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro