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

Add option to specify a prefix to be prepended to object paths #190

Merged
merged 4 commits into from
Dec 15, 2023

Conversation

hoffmanr-cshs
Copy link
Contributor

This draft PR adds an optional configuration environment variable S3_OBJECT_PREFIX. If defined, this prefix is prepended to all S3 object paths. This is intended as a simple way to allow only some subset of a bucket to be served by the gateway. When used in combination with the existing STRIP_LEADING_DIRECTORY_PATH option, this effectively allows the leading prefix to be replaced, rather than just removed.

I don't know if this will be of value to any others, or if there are potentially any "gotchas" to be aware of implementing it in this way. But just in case it's useful for anyone, I wanted to share!

@4141done
Copy link
Collaborator

4141done commented Dec 9, 2023

Hi @hoffmanr-cshs , thank you so much for your contribution!
I'm going to set aside some time to review this in the coming week. Based on my initial read it looks very straightforward. Mainly I need to think through this addition in relation to the rest of the growing list of configuration variables.

@hoffmanr-cshs hoffmanr-cshs marked this pull request as ready for review December 12, 2023 17:45
Copy link
Collaborator

@4141done 4141done left a comment

Choose a reason for hiding this comment

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

Thanks for your patience while I took a look at this. I think this change works really well with STRIP_LEADING_DIRECTORY_PATH as you mentioned in the description. Having the ability to strip, prepend, or replace initial object paths is great flexibility.

I have a few comments:

  1. What do you think about choosing a name for the configuration that more closely groups the two configuration options? Maybe something like PREPEND_LEADING_DIRECTORY_PATH? Your name matches the S3 terminology better but I'm trying to group configuration options as much as possible.

  2. I have a suggestion for an alternate approach to adding the prefix. See comment in code

  3. Adding tests for this one is a little tricky since it prefixes everything and out integration test suite is in need of some love. I think this PR will be what pushes me to finally refactor it into javascript but for now I can help add some basic tests to this PR in a hacky way. I can add those once we decide on an approach for (2) and (3)

common/etc/nginx/include/s3gateway.js Outdated Show resolved Hide resolved
@hoffmanr-cshs
Copy link
Contributor Author

Thanks for your reply - and no rush! To your points:

  1. I'd like to suggest PREFIX_LEADING_DIRECTORY_PATH as the name - this still follows the existing variable naming conventions, but "prepend" and "prefix" can be synonyms, and with all other things being equal I think it's probably good to hew closer to S3 jargon and keep the word "prefix" in there. (Just from my own experience, when I was first reading the docs to see if this was already possible, I know that "prefix" was the first thing I searched!)
  2. I think that looks great! I defaulted to doing it in JS just because I'm more familiar with it than with nginx configs, but I agree this makes (probably much more) sense.
  3. Not sure what I can add here since the tests will necessarily depend on what's in your test bucket, but hopefully some very simple tests will be enough to validate it.

I've taken a pass at implementing and validating these changes in my application, and it all seems to be working well! I've just pushed a reversion of s3gateway.js and a commit with the suggested changes - take a look, and me know if you have any more thoughts!

@4141done
Copy link
Collaborator

  1. I think your suggested name is perfect. Great point about the importance of keyword searchability as we gain more config options.
  2. Cool! You changes look perfect.
  3. I'm going to try to commit a simple addition to the tests to give this some coverage, then tackle this issue to expand things a bit.

Once the tests are in I think we are good to merge. My final request to you would be to test the current iteration of this PR against your use case to make sure it still works for you after the change in approach.

@4141done
Copy link
Collaborator

@hoffmanr-cshs tests are in, please let me know if you think I missed anything critical. At this point once you give me the 👍 that you are happy with how this looks we will merge 🐳

@hoffmanr-cshs
Copy link
Contributor Author

all looks great to me - thank you!

@4141done 4141done merged commit bb03e88 into nginxinc:master Dec 15, 2023
2 checks passed
@4141done
Copy link
Collaborator

All done! Thank you once again for your contribution!

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