Address PR comments

backend/src/services/pki-sync/aws-certificate-manager/aws-certificate-manager-pki-sync-fns.ts

@@ -4,7 +4,6 @@ import RE2 from "re2";
 import { z } from "zod";
 
 import { BadRequestError, NotFoundError } from "@app/lib/errors";
-import { logger } from "@app/lib/logger";
 import { TAppConnectionDALFactory } from "@app/services/app-connection/app-connection-dal";
 import { AppConnection, AWSRegion } from "@app/services/app-connection/app-connection-enums";
 import { decryptAppConnectionCredentials } from "@app/services/app-connection/app-connection-fns";
@@ -237,6 +236,40 @@ export const awsCertificateManagerPkiSyncFactory = ({
   kmsService,
   appConnectionDAL
 }: TAwsCertificateManagerPkiSyncFactoryDeps) => {
+  const deleteCertificateFromAcm = async (
+    acm: AWS.ACM,
+    certificateArn: string,
+    operation: string,
+    syncId: string,
+    throwOnError = false
+  ): Promise<{ arn: string; success: boolean; error?: Error }> => {
+    try {
+      await withRateLimitRetry(() => acm.deleteCertificate({ CertificateArn: certificateArn }).promise(), {
+        operation,
+        syncId
+      });
+      return { arn: certificateArn, success: true };
+    } catch (error) {
+      const errorObj = error instanceof Error ? error : new Error("Unknown error");
+
+      if (throwOnError) {
+        throw new PkiSyncError({
+          message: `Failed to remove certificate from AWS Certificate Manager: ${errorObj.message}`,
+          cause: errorObj,
+          context: {
+            certificateArn,
+            operation
+          }
+        });
+      }
+
+      return {
+        arn: certificateArn,
+        success: false,
+        error: errorObj
+      };
+    }
+  };
   const $getAwsAcmCertificates = async (
     acm: AWS.ACM,
     syncId = "unknown"
@@ -252,7 +285,7 @@ export const awsCertificateManagerPkiSyncFactory = ({
 
       do {
         const listParams: AWS.ACM.ListCertificatesRequest = {
-          CertificateStatuses: ["ISSUED"], // Only get active certificates
+          CertificateStatuses: ["ISSUED"],
           NextToken: nextToken,
           MaxItems: 100
         };
@@ -290,7 +323,7 @@ export const awsCertificateManagerPkiSyncFactory = ({
           try {
             certificateContent = await acm.getCertificate({ CertificateArn: certSummary.CertificateArn }).promise();
           } catch (error) {
-            logger.error({ certificateArn: certSummary.CertificateArn, error }, "Cannot export certificate content");
+            // Certificate content cannot be imported
           }
         }
 
@@ -316,14 +349,14 @@ export const awsCertificateManagerPkiSyncFactory = ({
 
     const failedFetches = certificateResults.filter((result) => result.status === "rejected");
     if (failedFetches.length > 0) {
-      logger.warn(
-        {
-          syncId,
+      throw new PkiSyncError({
+        message: `Failed to fetch ${failedFetches.length} certificate details from AWS Certificate Manager`,
+        shouldRetry: true,
+        context: {
           failedCount: failedFetches.length,
           totalCount: certificateSummaries.length
-        },
-        "Some certificate details could not be fetched from AWS Certificate Manager"
-      );
+        }
+      });
     }
 
     const res: Record<
@@ -369,7 +402,8 @@ export const awsCertificateManagerPkiSyncFactory = ({
 
     const activeCertificateNames = Object.keys(certificateMap);
 
-    Object.entries(certificateMap).forEach(([certName, { cert, privateKey, certificateChain }]) => {
+    Object.entries(certificateMap).forEach(([certName, certData]) => {
+      const { cert, privateKey, certificateChain } = certData;
       const certificateName = generateCertificateName(certName, pkiSync);
 
       const existingCert = Object.values(acmCertificates).find((acmCert) =>
@@ -381,16 +415,14 @@ export const awsCertificateManagerPkiSyncFactory = ({
       try {
         validateCertificateContent(cert, privateKey);
       } catch (validationError) {
-        logger.error(
-          {
-            syncId: pkiSync.id,
-            certName,
+        throw new PkiSyncError({
+          message: `Certificate validation failed for ${certName}: ${validationError instanceof Error ? validationError.message : String(validationError)}`,
+          shouldRetry: false,
+          context: {
             certificateName,
-            error: validationError
-          },
-          "Certificate validation failed, skipping"
-        );
-        return;
+            certName
+          }
+        });
       }
 
       if (shouldUpdateCert) {
@@ -453,17 +485,6 @@ export const awsCertificateManagerPkiSyncFactory = ({
           return { key, name, success: true, response };
         } catch (error) {
           const errorMessage = error instanceof Error ? error.message : "Unknown error";
-          logger.error(
-            {
-              syncId: pkiSync.id,
-              certificateKey: key,
-              certificateName: name,
-              error,
-              errorMessage
-            },
-            "Failed to import certificate to AWS Certificate Manager"
-          );
-
           throw new PkiSyncError({
             message: `Failed to import certificate ${key} to AWS Certificate Manager: ${errorMessage}`,
             cause: error instanceof Error ? error : new Error(errorMessage),
@@ -489,26 +510,7 @@ export const awsCertificateManagerPkiSyncFactory = ({
     if (certificatesToRemove.length > 0) {
       removeResults = await executeWithConcurrencyLimit(
         certificatesToRemove,
-        async (certificateArn) => {
-          try {
-            await withRateLimitRetry(() => acm.deleteCertificate({ CertificateArn: certificateArn }).promise(), {
-              operation: "delete-certificate",
-              syncId: pkiSync.id
-            });
-            return { arn: certificateArn, success: true };
-          } catch (error) {
-            logger.error(
-              { error, syncId: pkiSync.id, certificateArn },
-              "Failed to remove expired/removed certificate from AWS Certificate Manager"
-            );
-
-            return {
-              arn: certificateArn,
-              success: false,
-              error: error instanceof Error ? error : new Error("Unknown error")
-            };
-          }
-        },
+        async (certificateArn) => deleteCertificateFromAcm(acm, certificateArn, "delete-certificate", pkiSync.id),
         { operation: "remove-certificates", syncId: pkiSync.id }
       );
 
@@ -517,17 +519,6 @@ export const awsCertificateManagerPkiSyncFactory = ({
       );
       removedCertificates = successfulRemovals.length;
       failedRemovals = removeResults.length - removedCertificates;
-
-      if (failedRemovals > 0) {
-        logger.warn(
-          {
-            syncId: pkiSync.id,
-            failedRemovals,
-            successfulRemovals: removedCertificates
-          },
-          "Some expired/removed certificates could not be removed from AWS Certificate Manager"
-        );
-      }
     }
 
     const details: {
@@ -549,15 +540,6 @@ export const awsCertificateManagerPkiSyncFactory = ({
           error: errorMessage
         };
       });
-
-      logger.error(
-        {
-          syncId: pkiSync.id,
-          failedUploads: details.failedUploads,
-          failedCount: failedUploads.length
-        },
-        "Some certificates failed to import to AWS Certificate Manager"
-      );
     }
 
     if (failedRemovals > 0 && removeResults.length > 0) {
@@ -576,15 +558,6 @@ export const awsCertificateManagerPkiSyncFactory = ({
         .filter((item): item is { name: string; error: string } => item !== null);
 
       details.failedRemovals = actualFailedRemovals;
-
-      logger.warn(
-        {
-          syncId: pkiSync.id,
-          failedRemovals: details.failedRemovals,
-          successfulRemovals: removedCertificates
-        },
-        "Some expired/removed certificates could not be removed from AWS Certificate Manager"
-      );
     }
 
     return {
@@ -626,30 +599,8 @@ export const awsCertificateManagerPkiSyncFactory = ({
 
     const results = await executeWithConcurrencyLimit(
       certificateArnsToRemove,
-      async (certificateArn) => {
-        try {
-          await withRateLimitRetry(() => acm.deleteCertificate({ CertificateArn: certificateArn }).promise(), {
-            operation: "delete-specific-certificate",
-            syncId: pkiSync.id
-          });
-
-          return { arn: certificateArn, success: true };
-        } catch (error) {
-          logger.error(
-            { error, syncId: pkiSync.id, certificateArn },
-            "Failed to remove specific certificate from AWS Certificate Manager"
-          );
-
-          throw new PkiSyncError({
-            message: `Failed to remove certificate from AWS Certificate Manager: ${(error as Error)?.message || "Unknown error"}`,
-            cause: error as Error,
-            context: {
-              certificateArn,
-              region: (pkiSync.destinationConfig as TAwsCertificateManagerPkiSyncConfig).region
-            }
-          });
-        }
-      },
+      async (certificateArn) =>
+        deleteCertificateFromAcm(acm, certificateArn, "delete-specific-certificate", pkiSync.id, true),
       { operation: "remove-specific-certificates", syncId: pkiSync.id }
     );
 

docs/documentation/platform/pki/certificate-syncs/aws-certificate-manager.mdx

@@ -7,7 +7,6 @@ description: "Learn how to configure an AWS Certificate Manager Certificate Sync
 
 - Set up and configure a [Certificate Authority](/documentation/platform/pki/overview)
 - Create an [AWS Connection](/integrations/app-connections/aws)
-- Ensure your network security policies allow incoming requests from Infisical to this certificate sync provider, if network restrictions apply.
 
 <Note>
   The AWS Certificate Manager Certificate Sync requires the following ACM permissions to be set on the IAM user/role
@@ -125,7 +124,6 @@ Your AWS Certificate Manager Certificate Sync will:
 - **Automatic Deployment**: Deploy new certificates issued by your PKI subscriber to AWS Certificate Manager
 - **Certificate Updates**: Update certificates in AWS Certificate Manager when renewals occur
 - **Expiration Handling**: Optionally remove expired certificates from AWS Certificate Manager (if enabled)
-- **Format Preservation**: Maintain certificate format and metadata during sync operations
 - **Tagging**: Automatically tag certificates with an InfisicalCertificate tag for easy identification and management
 
 <Note>

docs/integrations/app-connections/aws.mdx

@@ -205,7 +205,7 @@ Infisical supports two methods for connecting to AWS.
                                 <Note>
                                     - **ListCertificates**: Lists all certificates in the account
                                     - **ImportCertificate**: Imports certificates from Infisical into AWS Certificate Manager
-                                    - **ExportCertificate**: Exports certificates for synchronization (only works with imported certificates)
+                                    - **ExportCertificate**: Exports certificates for synchronization
                                     - **DeleteCertificate**: Removes certificates that are no longer managed by Infisical
                                     - **DescribeCertificate** and **GetCertificate**: Retrieves certificate details for comparison during sync
                                     - Tag-related permissions: Manages certificate tags for identification and organization
@@ -421,7 +421,7 @@ Infisical supports two methods for connecting to AWS.
                                 <Note>
                                     - **ListCertificates**: Lists all certificates in the account
                                     - **ImportCertificate**: Imports certificates from Infisical into AWS Certificate Manager
-                                    - **ExportCertificate**: Exports certificates for synchronization (only works with imported certificates)
+                                    - **ExportCertificate**: Exports certificates for synchronization
                                     - **DeleteCertificate**: Removes certificates that are no longer managed by Infisical
                                     - **DescribeCertificate** and **GetCertificate**: Retrieves certificate details for comparison during sync
                                     - Tag-related permissions: Manages certificate tags for identification and organization