FIX: Ensure CORS rules exist for S3 using rake task (PR #14802)

This commit introduces a new s3:ensure_cors_rules rake task that is run as a prerequisite to s3:upload_assets. This rake task calls out to the S3CorsRulesets class to ensure that the 3 relevant sets of CORS rules are applied, depending on site settings:

  • assets
  • direct S3 backups
  • direct S3 uploads

This works for both Global S3 settings and Database S3 settings (the latter set directly via SiteSetting).

As it is, only one rule can be applied, which is generally the assets rule as it is called first. This commit changes the ensure_cors! method to be able to apply new rules as well as the existing ones.

This commit also slightly changes the existing rules to cover direct S3 uploads via uppy, especially multipart, which requires some more headers.

GitHub

Would like to clarify why we need GET and HEAD as compared to just PUT previously.

    return if !SiteSetting.s3_install_cors_rule
    return if ! (GlobalSetting.use_s3? || SiteSetting.enable_s3_uploads)

Maybe I’m getting older but I feel like this is much easier to understand.

Hmm does this mean that we’ll need to redeploy when SiteSetting.backup_location is changed to S3 before the CORS rules are applied?

Same concern as above where I think we might need to redeploy before the CORS rules are applied when this site setting is changed.

  def self.build_from_config(use_db_s3_config: false, for_backup: false, s3_client: nil)
    klass = use_db_s3_config ? SiteSetting : GlobalSetting
    
    bucket, options = [
      for_backup ? klass.s3_backup_bucket.downcase : klass.s3_upload_bucket.downcase,
      S3Helper.s3_options(klass)
    ]
    
    options[:client] = s3_client if s3_client.present?

    S3Helper.new(bucket, '', options)
  end

Minor but perhaps we can assert for the results returned from sync_rules here? Otherwise, it seems strange that there are no assertion in this test.

I kind of feel like all these tests are not necessary since they are eventually covered by the “applies all rules if everything is set up” test below.

Hmm do we need this when the global settings have already been configured?

No you’re right, not sure why I committed the sin of using unless XD

Yes fair enough

Yes we do, see:

I agree it is a little strange, confused me a bit why that was needed but the backup bucket stuff is not loaded into GlobalSetting

I would prefer to get rid of the “applies all rules” test then, I want to have explicit tests for each of the “configured” rules cause they have distinct logic.

Will discuss this internally as well, but it is because this will be needed for direct S3 backup multipart uploads, and I would prefer to apply all these new rules at the same time.

@tgxworld just addressing these concerns:

Hmm does this mean that we’ll need to redeploy when SiteSetting.backup_location is changed to S3 before the CORS rules are applied?

Same concern as above where I think we might need to redeploy before the CORS rules are applied when this site setting is changed.

I don’t think this will be a problem on our hosting. I think we would rarely if ever change the backup location (we use S3 for all hosting) and the direct S3 uploads site setting is currently somewhat experimental; once it is more stable I can see us enabling that by default on our hosting as well.

If either of them do change, the operator can just run the s3:ensure_cors rake task to even the rules out.

Unfortunately this does not quite work because the S3 bucket property is slightly different between global + site:

image

Will try to clean it up a bit though.

If either of them do change, the operator can just run the s3:ensure_cors rake task to even the rules out.

I think my concern here is that the syncing here is based on site settings which are usually done via the admin interface but for it to work we need to run a rake task manually. For people managing their own setups, I think we either need to indicate that in the site setting description and change the CORS rule when the site setting changes.

For people managing their own setups, I think we either need to indicate that in the site setting description and change the CORS rule when the site setting changes.

Yes agreed we need to do something slightly different for self hosters, that can be a future PR though. To counter this for self hosters, I will just revert this change to not call ensure_cors! inside the S3BackupStore:

Later on we can take that out in favour of the auto-change on site setting change.