Skip to content
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

Ozone cdn invalidation #2087

Merged
merged 9 commits into from
Feb 29, 2024
Merged

Ozone cdn invalidation #2087

merged 9 commits into from
Feb 29, 2024

Conversation

dholms
Copy link
Collaborator

@dholms dholms commented Jan 24, 2024

Move CDN invalidation to Ozone. Invalidation is pushed out after the takedown event is pushed to appview.

Comment on lines 492 to 494
const paths = (this.cdnPaths ?? []).map((path) =>
path.replace('%s', subject.did),
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not noticing a place where the cid is also inserted into the path. Pretty sure the invalidators themselves don't do this, so I think we'd want it around here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm yup you're totally right i thought the img invalidator took care of that tbh 😅

Comment on lines +15 to +19
const {
BunnyInvalidator,
CloudfrontInvalidator,
MultiImageInvalidator,
} = require('@atproto/aws')
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may need to adjust the service's package.json and dockerfile to include @atproto/aws.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Easiest thing to check/catch this is to make a build and run it, even if it's unconfigured.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup yup good point 👍

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Looks solid, just a couple small things to peek at.

await Promise.allSettled(
(subject.blobCids ?? []).map((cid) => {
const paths = (this.cdnPaths ?? []).map((path) =>
path.replace('%s', subject.did).replace('%s', cid),
Copy link
Collaborator

@devinivy devinivy Feb 2, 2024

Choose a reason for hiding this comment

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

No issue here, but node's util.format() does this exact printf-style replacement! Would look something like util.format(path, subject.did, cid).

@devinivy devinivy merged commit 81370d7 into main Feb 29, 2024
11 checks passed
@devinivy devinivy deleted the ozone-cdn-invalidation branch February 29, 2024 20:28
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.

2 participants