fix(provisioning): stop unwrapping CloudFront OriginGroups in drift readback (#876)

Author: go-to-k

docs/_generated/integ-coverage.json

@@ -509,7 +509,8 @@
           "l2"
         ],
         "s3-cloudfront": [
-          "l2"
+          "l2",
+          "literal"
         ]
       }
     },

docs/_generated/integ-last-run.tsv

@@ -136,4 +136,4 @@ opensearch-domain-getatt	2026-06-14T10:54:56Z	PASS	2000	verify.sh	CC-API GetAtt
 s3-vectors	2026-06-14T13:39:25Z	PASS	60	verify.sh	Tags backfill + in-place Tags update() (TagResource/UntagResource) re-run after create dedup; 0 orphans
 dynamodb-ondemand	2026-06-14T14:54:16Z	PASS	120	verify.sh	BillingMode/ProvisionedThroughput in-place UPDATE + backfills; 0 orphans
 glue-update-hardening	2026-06-15T01:57:37Z	PASS		verify.sh	rc ok, orph clean (re-run after review fix; deploy+update+destroy clean)
-s3-cloudfront	2026-06-15T04:03:59Z	PASS		verify.sh	rc ok, orph clean (#872 re-run after review fixes; bucket policy + distribution clean; clean destroy)
+s3-cloudfront	2026-06-15T04:27:23Z	PASS		verify.sh	rc ok, orph clean (#873 OriginGroups drift clean + distribution/bucket-policy clean; clean destroy)

docs/changelog-cdkd.md

@@ -18,6 +18,8 @@ The CLAUDE.md `## Known Limitations` section retains the load-bearing summary
 
 **Recently Implemented** (2026-06-15):
 
+- ✅ **Fix: stop unwrapping `AWS::CloudFront::Distribution.OriginGroups` in drift readback (closes the OriginGroups phantom drift, issue [#873](https://github.com/go-to-k/cdkd/issues/873))** — `src/provisioning/providers/cloudfront-distribution-provider.ts`. **Bug:** `OriginGroups` was listed in `QUANTITY_ITEM_FIELDS` (the top-level fields that are a bare array in CFn and a `{ Quantity, Items }` wrapper in the SDK, so cdkd wraps them on write and unwraps them on read). But unlike `Origins` / `CacheBehaviors` / `Aliases` / `CustomErrorResponses` (genuinely bare lists in CFn), the CFn `DistributionConfig.OriginGroups` property is ITSELF a `{ Quantity, Items }` object — confirmed by synthesizing a distribution with an origin group (`Quantity` + `Items[]`, each item carrying inner `{ Quantity, Items }` `Members` and `FailoverCriteria.StatusCodes`). So `convertToCfnFormat` was unwrapping the top-level OriginGroups to a bare array, which no longer matched the template's `{ Quantity, Items }`, firing phantom drift on every `cdkd drift` for any distribution with an origin group. (#871 had suppressed OriginGroups via `getDriftUnknownPaths` as a stopgap, under the mistaken assumption — from that PR's review — that the inner Members/StatusCodes were bare arrays needing a `revertOriginGroup`; the synth evidence shows the whole subtree is `{ Quantity, Items }`, so the real fix is the opposite: don't touch it at all.) **Fix:** remove `OriginGroups` from `QUANTITY_ITEM_FIELDS` so it passes through UNTOUCHED in both directions (create still works — `wrapWithQuantity` was a no-op on the already-`{ Quantity, Items }` value), and remove `DistributionConfig.OriginGroups` from `getDriftUnknownPaths` so origin-group drift is now actually reported. 1 new unit test (OriginGroups `{ Quantity, Items }` with inner Members/StatusCodes passes through byte-equal, not unwrapped); `getDriftUnknownPaths` test updated. Validated against real AWS: the `s3-cloudfront` fixture gains an `OriginGroup` (primary S3-OAI origin + HTTP fallback + failover status codes), and `verify.sh` asserts the distribution carries exactly 1 OriginGroup AND that the whole distribution (OriginGroups now included) reports clean on a fresh deploy.
+
 - ✅ **Fix: canonicalize CloudFront OAI grant principals in `S3BucketPolicyProvider.readCurrentState` (closes the phantom drift on an S3 BucketPolicy granting an OAI, issue [#872](https://github.com/go-to-k/cdkd/issues/872))** — `src/provisioning/providers/s3-bucket-policy-provider.ts`. **Bug:** a bucket policy statement that grants a CloudFront Origin Access Identity (OAI) read access stores its principal as the OAI's S3 canonical user id (`{ CanonicalUser: <64-hex> }`, what CDK's `Fn::GetAtt [<OAI>, S3CanonicalUserId]` resolves to). But `s3:GetBucketPolicy` returns that same principal in TWO other unstable forms over the policy's lifetime: a transient `{ AWS: <IAM-unique-id> }` (e.g. `AIDA…`) right after `PutBucketPolicy`, then the settled `{ AWS: arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity <id> }`. The drift comparator saw these three equivalent representations as different and fired a guaranteed false positive on every `cdkd drift` run for any stack with an OAI + S3 bucket policy (the deploy-time `observedProperties` capture caught the transient unique-id form; the later drift read got the ARN form — neither matched the template's canonical user id). **Fix:** `readCurrentState` now canonicalizes every recognizable OAI principal back to `{ CanonicalUser: <id> }`. For the settled `arn:…:cloudfront:user/…<oaiId>` form it maps the OAI id to its `S3CanonicalUserId`, **preferring the same-stack sibling OAI resource's already-read state attribute** (zero AWS call / no `cloudfront:GetCloudFrontOriginAccessIdentity` IAM grant — the OAI's `S3CanonicalUserId` is a readOnly attribute cdkd already resolved at deploy time, threaded into the drift read via the new `ReadCurrentStateContext.siblings[].attributes` + `.physicalId` fields) and falling back to `GetCloudFrontOriginAccessIdentity(<oaiId>)` only when the OAI is NOT a same-stack sibling (an imported / external OAI). The transient bare-IAM-unique-id form (which carries no recoverable link to the OAI) is canonicalized only by matching the corresponding template statement (same Effect / Action / Resource) carrying a `{ CanonicalUser }` principal — safe because a user cannot author a bare IAM unique id as a bucket-policy principal, so it only ever fires for AWS's transient rendering. Both the deploy-time capture and the drift read run through the same normalization, so both converge to the canonical user id and compare equal; a genuinely-different OAI resolves to a different canonical id so real drift is still detected (strict reconcile — never a blanket suppress). Non-OAI principals (`*`, service principals, normal role ARNs) are untouched. The `ReadCurrentStateContext.siblings` shape gained optional `physicalId` + `attributes` (populated by `buildReadCurrentStateContext` in `src/cli/commands/drift.ts`) so any provider can reconcile a sibling's computed identity from already-read state without an extra AWS call. 13 unit tests (ARN→CanonicalUser via sibling state attribute with NO CloudFront call; strict-reconcile: a DIFFERENT OAI resolves to a DIFFERENT canonical id so real drift is NOT suppressed; AWS-array principal + ambiguous-template-match left unchanged; ARN→CanonicalUser via `GetCloudFrontOriginAccessIdentity` fallback when not a sibling; bare-unique-id→template-match; no-template-match left unchanged; best-effort ARN-lookup-failure left unchanged; non-OAI principals untouched; one CloudFront call cached across statements). Validated against real AWS by extending the `s3-cloudfront` integ's `verify.sh` to assert the `AWS::S3::BucketPolicy` reports clean (in the `clean` drift bucket, not `drifted`) on a fresh deploy — the distribution-scoping caveat the CloudFront PR (#871) added is removed now that the bucket policy no longer phantom-drifts. (Approach mirrors the sibling cdk-real-drift project's fix per the #872 cross-note: read the OAI's `S3CanonicalUserId` from already-read state rather than re-fetching it.)
 
 **Recently Implemented** (2026-06-14):

docs/integ-coverage.md

@@ -57,7 +57,7 @@ Registered without an integ fixture, with an explicit `// allow-no-integ: <ratio
 | `AWS::CertificateManager::Certificate` | [`acm-certificate`](../tests/integration/acm-certificate/) (l2) |
 | `AWS::CloudFormation::Stack` | [`export-nested-stack`](../tests/integration/export-nested-stack/) (literal)<br>[`import-nested-stack`](../tests/integration/import-nested-stack/) (literal)<br>[`nested-stack`](../tests/integration/nested-stack/) (literal)<br>[`nested-stack-3level`](../tests/integration/nested-stack-3level/) (literal)<br>[`nested-stack-deep`](../tests/integration/nested-stack-deep/) (literal) |
 | `AWS::CloudFront::CloudFrontOriginAccessIdentity` | [`s3-cloudfront`](../tests/integration/s3-cloudfront/) (l2) |
-| `AWS::CloudFront::Distribution` | [`bench-cdk-sample`](../tests/integration/bench-cdk-sample/) (l2)<br>[`cloudfront-function-url`](../tests/integration/cloudfront-function-url/) (l2)<br>[`local-start-cloudfront`](../tests/integration/local-start-cloudfront/) (l2)<br>[`s3-cloudfront`](../tests/integration/s3-cloudfront/) (l2) |
+| `AWS::CloudFront::Distribution` | [`bench-cdk-sample`](../tests/integration/bench-cdk-sample/) (l2)<br>[`cloudfront-function-url`](../tests/integration/cloudfront-function-url/) (l2)<br>[`local-start-cloudfront`](../tests/integration/local-start-cloudfront/) (l2)<br>[`s3-cloudfront`](../tests/integration/s3-cloudfront/) (l2,literal) |
 | `AWS::CloudTrail::Trail` | [`infra-security`](../tests/integration/infra-security/) (l2,literal) |
 | `AWS::CloudWatch::Alarm` | [`cloudwatch`](../tests/integration/cloudwatch/) (l2,literal)<br>[`composite-stack`](../tests/integration/composite-stack/) (l2,literal)<br>[`deep-getatt-chains`](../tests/integration/deep-getatt-chains/) (l1)<br>[`full-stack-demo`](../tests/integration/full-stack-demo/) (l2)<br>[`log-pipeline`](../tests/integration/log-pipeline/) (l2)<br>[`scheduled-task`](../tests/integration/scheduled-task/) (l2) |
 | `AWS::CodeBuild::Project` | [`ci-cd`](../tests/integration/ci-cd/) (l2,literal) |

src/provisioning/providers/cloudfront-distribution-provider.ts

@@ -28,16 +28,21 @@ import type {
 } from '../../types/resource.js';
 
 /**
- * Fields in the DistributionConfig that follow the { Quantity, Items } pattern.
- * The CDK template may provide just Items (an array); we wrap it with Quantity.
+ * Top-level `DistributionConfig` fields that are a BARE ARRAY in the CFn
+ * template but a `{ Quantity, Items }` wrapper in the SDK shape — so
+ * `convertToSdkFormat` wraps them and `convertToCfnFormat` unwraps them.
+ *
+ * `OriginGroups` is deliberately NOT here (issue #873): unlike `Origins` /
+ * `CacheBehaviors` / `Aliases` / `CustomErrorResponses` (bare lists in CFn), the
+ * CFn `DistributionConfig.OriginGroups` property is ITSELF a `{ Quantity, Items }`
+ * object, and so are its nested `Members` and `FailoverCriteria.StatusCodes`.
+ * CDK synthesizes that full `{ Quantity, Items }` shape and `GetDistributionConfig`
+ * returns the same, so OriginGroups must pass through UNTOUCHED in both
+ * directions — unwrapping it (it was incorrectly listed here before) turned the
+ * read-back OriginGroups into a bare array that no longer matched the template's
+ * `{ Quantity, Items }`, firing phantom drift.
  */
-const QUANTITY_ITEM_FIELDS = [
-  'Origins',
-  'CacheBehaviors',
-  'CustomErrorResponses',
-  'Aliases',
-  'OriginGroups',
-];
+const QUANTITY_ITEM_FIELDS = ['Origins', 'CacheBehaviors', 'CustomErrorResponses', 'Aliases'];
 
 /**
  * Nested fields inside each CacheBehavior / DefaultCacheBehavior that use
@@ -437,27 +442,18 @@ export class CloudFrontDistributionProvider implements ResourceProvider {
    *   logging bucket to its `<bucket>.s3.amazonaws.com` regional domain on
    *   read, which never matches the bare bucket domain a template may
    *   carry; treat it as drift-unknown to avoid a guaranteed mismatch.
-   * - `DistributionConfig.OriginGroups`: each origin group carries its own
-   *   inner `{ Quantity, Items }` wrappers (`Members`,
-   *   `FailoverCriteria.StatusCodes`) that `convertToCfnFormat` does not yet
-   *   unwrap symmetrically (only the top-level OriginGroups list is unwrapped;
-   *   there is no `revertOriginGroup` pass, and `convertToSdkFormat` likewise
-   *   does not descend into them). Against the observed baseline both sides go
-   *   through the same `readCurrentState` so this never false-positives on a
-   *   normal deploy, but against the `properties`-fallback baseline (older
-   *   state without `observedProperties`) the raw `{ Quantity, Items }` shape
-   *   would diff the bare-array template form. Suppress it until a dedicated
-   *   fix lands a `revertOriginGroup` + a real OriginGroups integ fixture
-   *   (there is no OriginGroups fixture to verify the inner shape today). See
-   *   the PR #871 review thread.
+   *
+   * NOTE: `DistributionConfig.OriginGroups` is NO LONGER suppressed (issue
+   * #873). The earlier suppression worked around `convertToCfnFormat`
+   * incorrectly unwrapping the top-level OriginGroups `{ Quantity, Items }`
+   * wrapper; the real fix (dropping OriginGroups from `QUANTITY_ITEM_FIELDS` so
+   * it passes through untouched in both directions) makes OriginGroups compare
+   * equal, so it is now drift-checked normally and a real OriginGroups change
+   * IS reported.
    */
   getDriftUnknownPaths(resourceType: string): string[] {
     if (resourceType !== 'AWS::CloudFront::Distribution') return [];
-    return [
-      'DistributionConfig.CallerReference',
-      'DistributionConfig.Logging.Bucket',
-      'DistributionConfig.OriginGroups',
-    ];
+    return ['DistributionConfig.CallerReference', 'DistributionConfig.Logging.Bucket'];
   }
 
   /**

tests/integration/s3-cloudfront/lib/s3-cloudfront-stack.ts

@@ -43,12 +43,24 @@ export class S3CloudFrontStack extends cdk.Stack {
       })
     );
 
+    // Primary S3-via-OAI origin, plus an OriginGroup with an HTTP fallback so
+    // the distribution carries an `AWS::CloudFront::Distribution.OriginGroups`
+    // block — exercises CloudFrontDistributionProvider drift normalization of
+    // the OriginGroups inner `{ Quantity, Items }` wrappers (Members /
+    // FailoverCriteria.StatusCodes), issue #873.
+    const s3Origin = origins.S3BucketOrigin.withOriginAccessIdentity(bucket, {
+      originAccessIdentity: oai,
+    });
+    const originGroup = new origins.OriginGroup({
+      primaryOrigin: s3Origin,
+      fallbackOrigin: new origins.HttpOrigin('fallback.example.com'),
+      fallbackStatusCodes: [500, 502, 503, 504],
+    });
+
     // Create CloudFront Distribution
     const distribution = new cloudfront.Distribution(this, 'Distribution', {
       defaultBehavior: {
-        origin: origins.S3BucketOrigin.withOriginAccessIdentity(bucket, {
-          originAccessIdentity: oai,
-        }),
+        origin: originGroup,
         viewerProtocolPolicy: cloudfront.ViewerProtocolPolicy.REDIRECT_TO_HTTPS,
       },
       defaultRootObject: 'index.html',

tests/integration/s3-cloudfront/verify.sh

@@ -103,6 +103,18 @@ if [ -z "${DIST_ARN}" ] || [ "${DIST_ARN}" = "None" ]; then
   exit 1
 fi
 
+# --- Assertion: the distribution actually carries an OriginGroup -------
+# Proves the OriginGroups drift path (#873) is genuinely exercised — if a
+# future fixture change drops the origin group, this fails loudly rather than
+# silently stop testing OriginGroups normalization.
+OG_QTY=$(aws cloudfront get-distribution-config --id "${DIST_ID}" --region "${REGION}" \
+  --query 'DistributionConfig.OriginGroups.Quantity' --output text 2>/dev/null)
+if [ "${OG_QTY}" != "1" ]; then
+  echo "FAIL: distribution does not carry exactly 1 OriginGroup (got Quantity=${OG_QTY}) — the #873 drift path is not being exercised" >&2
+  exit 1
+fi
+echo "    OK: distribution carries an OriginGroup (exercises #873 OriginGroups drift normalization)"
+
 # --- Assertion: AWS reflects the two CDK Tags -------------------------
 TAGS_JSON=$(aws cloudfront list-tags-for-resource --resource "${DIST_ARN}" --region "${REGION}" \
   --query 'Tags.Items' --output json)

tests/unit/provisioning/cloudfront-distribution-provider.test.ts

@@ -945,6 +945,45 @@ describe('CloudFrontDistributionProvider', () => {
       expect(result!['DistributionConfig']).toEqual({ Enabled: true, Comment: '' });
     });
 
+    it('passes OriginGroups through UNCHANGED — it is {Quantity,Items} in both CFn and the SDK (#873)', async () => {
+      // The CFn DistributionConfig.OriginGroups property (+ its inner Members /
+      // FailoverCriteria.StatusCodes) is ITSELF a { Quantity, Items } object, so
+      // it must NOT be unwrapped like Origins/CacheBehaviors/Aliases — the
+      // template and the AWS read-back are identical and compare equal.
+      const originGroups = {
+        Quantity: 1,
+        Items: [
+          {
+            Id: 'group1',
+            FailoverCriteria: { StatusCodes: { Quantity: 4, Items: [500, 502, 503, 504] } },
+            Members: {
+              Quantity: 2,
+              Items: [{ OriginId: 'origin1' }, { OriginId: 'origin2' }],
+            },
+          },
+        ],
+      };
+      mockSend.mockResolvedValueOnce({
+        DistributionConfig: { CallerReference: 'ref', Enabled: true, Comment: '', OriginGroups: originGroups },
+      });
+      mockSend.mockResolvedValueOnce({
+        Distribution: { ARN: 'arn:aws:cloudfront::111122223333:distribution/EDFDVBD6EXAMPLE' },
+      });
+      mockSend.mockResolvedValueOnce({ Tags: { Items: [] } });
+
+      const result = await provider.readCurrentState(
+        'EDFDVBD6EXAMPLE',
+        'MyDistribution',
+        'AWS::CloudFront::Distribution'
+      );
+
+      // OriginGroups is byte-equal to the input { Quantity, Items } shape — not
+      // unwrapped to a bare array (which is what caused the #873 phantom drift).
+      expect((result!['DistributionConfig'] as Record<string, unknown>)['OriginGroups']).toEqual(
+        originGroups
+      );
+    });
+
     it('still returns DistributionConfig when the tag read fails', async () => {
       mockSend.mockResolvedValueOnce({
         DistributionConfig: { CallerReference: 'ref', Enabled: true },
@@ -995,15 +1034,10 @@ describe('CloudFrontDistributionProvider', () => {
   });
 
   describe('getDriftUnknownPaths', () => {
-    it('excludes CallerReference + Logging.Bucket + OriginGroups for CloudFront::Distribution', () => {
+    it('excludes CallerReference + Logging.Bucket for CloudFront::Distribution (OriginGroups is now drift-checked, #873)', () => {
       expect(provider.getDriftUnknownPaths('AWS::CloudFront::Distribution')).toEqual([
         'DistributionConfig.CallerReference',
         'DistributionConfig.Logging.Bucket',
-        // OriginGroups inner {Quantity,Items} (Members / FailoverCriteria.
-        // StatusCodes) is not unwrapped by convertToCfnFormat yet, so it is
-        // suppressed to avoid a properties-fallback-baseline false positive
-        // until a dedicated revertOriginGroup + fixture lands.
-        'DistributionConfig.OriginGroups',
       ]);
     });