Skip to content

Conversation

@sicarul
Copy link
Contributor

@sicarul sicarul commented Oct 22, 2025

This reverts PR #16341

The website is failing to load multiple pages

Copy link
Contributor

@foot foot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@claude
Copy link
Contributor

claude bot commented Oct 22, 2025

Review of PR #16357

This PR reverts the OAC (Origin Access Control) implementation from #16341, restoring the previous S3 website endpoint configuration.

Infrastructure Changes Review

✅ Revert appears technically sound:

  • Removes OAC resource (originAccessControl) at lines 208-214
  • Removes CloudFront Function (indexRewriteFunction) that was needed for directory path handling when using OAC with REST API endpoint (lines 441-461)
  • Restores S3 website endpoint origin configuration at lines 526-536 with customOriginConfig using http-only protocol
  • Simplifies bucket policy back to denying s3:ListBucket to non-account principals (lines 210-229)
  • Removes function association from cache behavior (lines 462 in old code)

Code Quality:

  • No syntax errors or TypeScript issues detected
  • Resource dependencies are properly maintained
  • The originBucketPolicy resource ordering is correct (moved before CloudFront distribution definition, as it no longer depends on the CDN ARN)

⚠️ Observations:

  1. Policy Version Discrepancy (line 214): The reverted bucket policy uses Version: "2008-10-17" while the OAC version used Version: "2012-10-17". This is likely intentional (restoring previous state), but "2012-10-17" is the current recommended version for S3 bucket policies. Consider updating this in a future change.

  2. Security Posture: This revert moves from the modern OAC approach (which AWS recommends) back to public S3 website endpoint + bucket policy. The original OAC implementation was attempting to improve security by:

    • Using AWS-recommended OAC instead of legacy approaches
    • Restricting bucket access to only CloudFront via service principal
    • Using HTTPS to S3 via REST API endpoint

    The reverted configuration relies on:

    • Public S3 website endpoint (HTTP-only between CloudFront and S3)
    • Bucket policy denying only ListBucket to non-account users
    • Objects are still publicly accessible via the website endpoint
  3. PR Description: The PR body only states "This reverts PR..." Consider adding context about:

    • Why the revert is needed
    • What issues were encountered with OAC
    • Whether this is temporary or permanent
    • Any follow-up actions planned

Recommendation:
The revert is technically correct and should safely restore the previous working configuration. However, I recommend adding a comment in the PR explaining the reason for the revert for future reference.


Mention me (@claude) if you'd like additional review or have questions about this feedback.

@sicarul sicarul enabled auto-merge (squash) October 22, 2025 21:04
@pulumi-bot
Copy link
Collaborator

@sicarul sicarul merged commit ace899b into master Oct 22, 2025
17 of 20 checks passed
@sicarul sicarul deleted the revert-oac-website-change branch October 22, 2025 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants