-
Notifications
You must be signed in to change notification settings - Fork 8.1k
cloudfront update with lambda function for redirects #15151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cloudfront update with lambda function for redirects #15151
Conversation
✅ Deploy Preview for docsdocker ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
0191320 to
86b99f9
Compare
|
Labs environment is ready and is using Cloudfront and Lambda Edge to handle redirections. Thanks @VictorBersy!! $ curl -s -D - https://docs-labs.docker.com/get-started/part2/ -o /dev/null
HTTP/2 301
content-length: 0
server: CloudFront
date: Mon, 08 Aug 2022 15:08:46 GMT
location: /get-started/02_our_app/
x-cache: LambdaGeneratedResponse from cloudfront
via: 1.1 f41c2361062c4fc74c645f4e4fddd2de.cloudfront.net (CloudFront)
x-amz-cf-pop: CDG3-C2
x-amz-cf-id: MpXwuPV4N_Kd8sVy_AGNc2Rbi2oniKOVRMDUrBsTFIcbxtiH4Sunbw==$ curl -s -D - https://docs-labs.docker.com/ee/sqdsqdsqdsqds -o /dev/null
HTTP/2 301
content-length: 0
server: CloudFront
date: Mon, 08 Aug 2022 15:10:30 GMT
location: /
x-cache: LambdaGeneratedResponse from cloudfront
via: 1.1 9891f2220bf61a27cb1f26085ab3703c.cloudfront.net (CloudFront)
x-amz-cf-pop: CDG3-C2
x-amz-cf-id: Fl9PI6vyWYRIy5kjHpjOEbtJI1kTCf7Onenhtm9wsXxN2_JrFuOQRw==I will update the PR to reflect changes made on lab branch. |
4ecf7b9 to
3d212a2
Compare
|
Ready for review. I have just enabled OIDC auth and cloudfront redirects with lambda edge func for our lab environment as it's the only one supporting it atm. Next step I will open a PR to prepare the work for stage and prod environment so that we will be ready when @VictorBersy has created the new stage and prod environments that support OIDC auth and Cloudfront with Lambda Edge redirections as well. @VictorBersy Let me know when we can catch up on this by giving me the IAM role ARN for OIDC auth and cloudfront id with lambda edge function name as well to use for stage and prod. |
3d212a2 to
0f4039a
Compare
|
We finally managed to make it work with Terraform on the lab branch. Next week I will be working on deploying this to stage. Thanks a lot @crazy-max for your support and help 🙇 |
0f4039a to
c694e2d
Compare
|
As discussed with @VictorBersy, I have created a dedicated workflow called |
.github/workflows/deploy.yml
Outdated
| if [ "${{ github.ref }}" = "refs/heads/master" ]; then | ||
| DOCS_URL="https://docs-stage2.docker.com" | ||
| DOCS_AWS_IAM_ROLE="" | ||
| DOCS_S3_BUCKET="" | ||
| DOCS_S3_CONFIG="s3-config.json" | ||
| DOCS_CLOUDFRONT_ID="" | ||
| DOCS_LAMBDA_FUNCTION_REDIRECTS="" | ||
| DOCS_SLACK_MSG="Successfully deployed docs-stage2 from master branch. https://docs-stage2.docker.com/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VictorBersy Let me know when you have the IAM role, bucket, cloudfront ID and lambda name to use for stage env. I already set DOCS_URL="https://docs-stage2.docker.com" as discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the requested information:
DOCS_AWS_IAM_ROLE="arn:aws:iam::710015040892:role/stage-docs-docker.github.io-20220816140248629900000003"
DOCS_S3_BUCKET="stage-docs-docker.github.io"
DOCS_CLOUDFRONT_ID="E1R7CSW3F0X4H8"
DOCS_LAMBDA_FUNCTION_REDIRECTS="DockerDocsRedirectFunction-stage"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Added stage aws config in last commit. Let me know when you have prod ready and I update the config for it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And for prod:
DOCS_AWS_IAM_ROLE="arn:aws:iam::710015040892:role/prod-docs-docker.github.io-20220816161549883800000001"
DOCS_S3_BUCKET="prod-docs-docker.github.io"
DOCS_CLOUDFRONT_ID="E228TTN20HNU8F"
DOCS_LAMBDA_FUNCTION_REDIRECTS="DockerDocsRedirectFunction-prod"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VictorBersy Thanks, I updated the config for prod. Is https://docs2.docker.com the right domain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is, we use docs2.docker.com for now.
|
Perfect thanks @VictorBersy! Looks like we are ready to merge. Can you PTAL @thaJeztah? Thanks |
thaJeztah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall this looks great! Left some comments/questions
| if [ "${{ github.ref }}" = "refs/heads/master" ]; then | ||
| DOCS_URL="https://docs-stage2.docker.com" | ||
| DOCS_AWS_IAM_ROLE="arn:aws:iam::710015040892:role/stage-docs-docker.github.io-20220816140248629900000003" | ||
| DOCS_S3_BUCKET="stage-docs-docker.github.io" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably too late now (?) to rename these buckets but we should really try to get rid of that docker.github.io everywhere, and use docs.docker.com (also rename the repository back to docs or documentation or docs.docker.com at some point)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's to match the name of the repository. Am I right @VictorBersy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was to match the repo name, but I don't mind renaming them. It's not too late for that.
I can name everything as docs.docker.com. Ideally, it would match the future name of the repository. Let me know 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a final "verdict" on the repository name (but anything else than docker.github.io 😂)
docker/docs would work for me (with the caveat that we used to have a repository with that name that's now archived; https://github.com/docker-archive/docs.docker.com
Oh, LOL, and which apparently also was renamed to docs.docker.com 😂 so either way we would break a redirect (not really important, that stuff is pre-historic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know when you have one, ideally before the production release, as it would make the migration more tedious 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VictorBersy if it's ok with you that potentially the name of the bucket doesn't match the name of the repository, I'm inclined to name the buckets after the domain they're for (docs.docker.com / docs-stage.docker.com etc) as those are unlikely to change.
@crazy-max WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh absolutely, no problem with that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with it
|
Oh! Take note that GitHub is hiding some comments (so may need a "click to load more comments") |
7a60be2 to
8a95cd1
Compare
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
8a95cd1 to
1b93493
Compare
|
Should be good, thanks for the review @thaJeztah |
thaJeztah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (to the extend of my knowledge 😅)
|
okay; let's get this one in. @VictorBersy for a follow-up; perhaps you have an answer on #15151 (comment)
|
|
So far so good for stage2: https://github.com/docker/docker.github.io/runs/7880300358?check_suite_focus=true |
|
docs2 has been deployed too: https://github.com/docker/docker.github.io/runs/7901183427?check_suite_focus=true#step:9:229 |
|
ooh, and now I'm wondering if we could optimize things further; That's a (default?) redirect from s3 to redirect URLs with no trailing However, in case of the above URL, that means a double redirect; So
Would be cool if our redirect function would be able to skip the first one. |
Yes indeed it seems to be returned by s3 and not cloudfront so cannot be handled by our redirect function: See I guess it needs some tuning on s3 but looks like this is the same behavior currently right?: |
|
Actually I see that we could use our redirect function for this: https://stackoverflow.com/questions/67967925/how-to-redirect-internal-urls-to-end-with-trailing-slashes-with-s3-websites But I'm afraid we could potentially break access to assets (images, etc..) |
Yes, so I was wondering; it could possibly still work as long as we base our redirects on the https://docs.docker.com/redirects.json; in that case we can look if there's a match for the source URL (after appending a The redirects.json doesn't have redirects for assets (at least not currently) |
Hum I see, yes that could work. |
|
I changed the redirect function to also check for uri without trailing slash but it doesn't pass through 😞 As I can see from the request, Cloudfront cache that request because it handles itself the redirection without going to the viewer request function. So this is not an S3 config but something in the Cloudfront distribution to change. As Cloudfront manages this redirection and handles its internal cache for them I think this is fine but we could improve this in a follow-up and let our lambda function do this redirection. |
|
Ah, that's a pity; thanks for trying! |
follow-up and take 2 of #15118
As replace directive for an s3 bucket is limited to 50 we can't use them. It's also really slow to propagate each of them with the AWS CLI (~30m).
This pr adds a new function to
releaserthat will update a lambda edge function to handle redirects directly in our cloudfront distribution.I have created a
labbranch on my fork and also a S3 bucket with a Lambda Edge function attached to a new Cloudfront distribution on my personal AWS account to test this feature and so far so good:Also looking at #15133, I have added a new
redirects-prefixes.jsonfile that handles redirects with prefixes so we can get rid of replaces in our s3 bucket with hardcoded domain inside it and have everything in place in our lambda edge function:Keeping in draft for now as we have to create the Lambda Edge function and additional perms on AWS as well as getting the current cloudfront ID for each environment (cc @StefanScherer).
Signed-off-by: CrazyMax crazy-max@users.noreply.github.com