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

Move S3 auth & cloudfront_signer config validation to init #1302

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

gerrod3
Copy link
Contributor

@gerrod3 gerrod3 commented Sep 14, 2023

Moved the validation in get_default_settings to be inside __init__ and check the final instantiated values, not just the values from settings. So even if the values in settings are incorrect, you can still create a S3 class by passing in the right values.

@gerrod3
Copy link
Contributor Author

gerrod3 commented Sep 18, 2023

@jschneier Wdyt about this refactor?

@jschneier
Copy link
Owner

I almost pushed something like this myself. Can you make it explicit what situation you are solving for?

@gerrod3
Copy link
Contributor Author

gerrod3 commented Sep 19, 2023

I want to make sure that if you are instantiating your own storage classes and providing every possible value for it, it shouldn't be blocked by what is configured in settings. In our project, Pulp, we have a multi-tenancy feature that allows users to create their own domains with their own custom storage backends separate from the default storage. [0] These domains must not use or be affected by the default settings.

[0] https://docs.pulpproject.org/pulpcore/workflows/domains-multi-tenancy.html

@jschneier jschneier merged commit d8a097d into jschneier:master Sep 29, 2023
15 checks passed
@gerrod3 gerrod3 deleted the s3-better-init branch October 4, 2023 18:00
@terencehonles
Copy link
Contributor

This change seems reasonable, but we were relying on cloudfront_signer=None which this change forces it to be truthy and a falsy value allowed disabling the signing but still serving via cloudfront. I fixed this in #1326

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