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

BREAKING CHANGE - Align content bucket name with url #394

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

cat5inthecradle
Copy link
Contributor

This change aligns the name of the Content Bucket with the name of the URL and CDN used to access. This is a common practice for S3-backed domains, and helps to eliminate the one use of IsDevCondition in our template.

Deployment

Deployed as-is, this will attempt to delete the existing content bucket. Here is a possible plan for deploying this change:

  1. First, deploy a change that adds DeletionPolicy: Retain to the ContentBucket resource
  2. Then, deploy this change. The new bucket should be created and wired up correctly, and the old bucket should be left untouched and now disconnected from javabuilder.
  3. Copy all files from the old content bucket to the new one, restoring all content to the expected location

This would result in service degradation between step 2 and the completion of step 3, because some content would be missing until it is restored.

BREAKING CHANGE: requires bucket replacement, must be deployed carefully and with downtime.
Copy link
Contributor

@molly-moen molly-moen left a comment

Choose a reason for hiding this comment

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

FWIW, the way the content bucket is used is very short-term. When a user makes a request to run code, we upload their sources/assets to the bucket, then run their code. Once that run is done, we no longer care about the content that was in the bucket. So the service degradation would be very brief, and I don't think we would need to copy over any content from the old bucket to the new bucket.

@cat5inthecradle
Copy link
Contributor Author

cat5inthecradle commented May 24, 2023

@molly-moen Awesome! I haven't looked at the code to see if we're already doing it, but perhaps we should add some lifecycle rules to clean up old content.

edit: we do!

      LifecycleConfiguration:
        Rules:
          - Id: ExpirationRule
            Status: Enabled
            ExpirationInDays: 1

@cat5inthecradle cat5inthecradle marked this pull request as draft June 21, 2023 23:21
@cat5inthecradle
Copy link
Contributor Author

Worked on this for a while today and discovered some issues. The new bucket name is not compatible with our method of applying permissions. Converting to a draft.

We define the Roles used by the lambdas up in the iam.yml template, which is applied once per AWS Account. those roles are granted permission to all buckets with names like cdo-*javabuilder*-content. This worked for all environments because of how we named the content bucket. The new scheme, ${subdomain}.${basedomain}-content is not safely targeted with wildcards. We could do something like *-content, but that feels too broad.

Ideally, we would grant permissions for the bucket in the same place that we create the bucket, in the app template. This branch as of this comment attempts to create the IAM policy alongside the bucket and apply it to the roles. Unfortunately, the Cloudformation service role that deploys the template does not have IAM putRolePolicy permission.

I do believe that the policy and role should be created in the app template (therefore limiting each lambda to only access it's own environment's content bucket), but this will require further troubleshooting with IAM roles and pipeline permissions.

NEXT STEPS

  • See if we can delete the old LambdaExecution Role
  • Try to grant appropriate permission to the role being used to deploy the cloudformation template (requires iam.yml deployment)
  • Move the role and policy to the app template
  • Test by deploying to a dev environment

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