Strip leading directory path from request #158
Conversation
|
@elJosho Thank you for the contribution! We are going to review it soon. |
There was a problem hiding this comment.
Thank you for your contribution and I apologize for the delay - I'm a new maintainer and it took me a little more time to look over your suggested change. Also congratulations on making your first public contribution!
In general I think this looks like a good use case, and we actually have an existing parameter that does this for the directory listing function: DIRECTORY_LISTING_PATH_PREFIX.
I think the two configuration options would make a good pair.
STRIP_LEADING_DIRECTORY_PATH -> For retrieving individual files
DIRECTORY_LISTING_PATH_PREFIX -> For listing directories
Before we can merge there are a few things I'd like to add:
- I think you need a
defaultclause in your newmapstatement. - otherwise all requests without the prefix will fail if you have a prefix set. (see comment) - Can you think of a name that you that might more closely match
DIRECTORY_LISTING_PATH_PREFIXthat accurately describes your feature? That way others can recognize them as related. - Can you add some usage instructions next to those for
DIRECTORY_LISTING_PATH_PREFIX(link)
I'd also like to add some integration tests but I'm still wrapping my head around the setup. I'm looking in to learning the ropes by writing some against your change. I'll have news on that as we work through the above items.
| # Remove a portion of request URL (if configured) | ||
| map $uri_full_path $uri_path { | ||
| "~^$STRIP_LEADING_DIRECTORY_PATH(.*)" $1; | ||
| } |
There was a problem hiding this comment.
I'm in the process of trying to test this with the project's test suite, but I think this might need a default clause.
map $uri_full_path $uri_path {
"~^$STRIP_LEADING_DIRECTORY_PATH(.*)" $1;
default $uri_full_path;
}
The code in the PR will work when theSTRIP_LEADING_DIRECTORY_PATH is not specified, and for /mybucket/a/b/foo.txt if STRIP_LEADING_DIRECTORY_PATH=/mybucket is specified.
However, if STRIP_LEADING_DIRECTORY_PATH=/mybucket is specified, then uris that do not include that prefix I think will not match and then the value of $uri_path will be empty.
It's very possible I'm missing something but would like to get your thoughts.
docs/getting_started.md
Outdated
| | `CORS_ALLOWED_ORIGIN` | No | | | value to set to be returned from the CORS `Access-Control-Allow-Origin` header. This value is only used if CORS is enabled. (default: \*) | | ||
|
|
||
| | `CORS_ALLOWED_ORIGIN` | No | | | value to set to be returned from the CORS `Access-Control-Allow-Origin` header. This value is only used if CORS is enabled. (default: \*) | ||
| | `STRIP_LEADING_DIRECTORY_PATH` | No | | | Removes a portion of the path in the requested URL (if configured). Useful when deploying to an ALB under a folder (eg. www.mysite.com/mybucket). |
There was a problem hiding this comment.
💚 Thank you for the nice documentation entry
|
I worked through how to add your configuration option to the integration test suite. You can view the patch here: You can either add it to your branch (I suggest first rebasing on the upstream When running those integration tests I have confirmed the necessity of a |
|
Thanks so much for looking at this! I've had a bit of a week so haven't had the chance to address your comments but will as soon as I can. |
Sounds good! We appreciate you taking the time to contribute. We can work together to get this merged on a timeline that works for you 🐳 |
* chore: small fixes to test scripts and README for clarity
… on MacOS by supporting `md5` (nginxinc#162) # What Fix error messages for missing dependencies, allow test suite to run on MacOS by supporting `md5` ## Dependency Checks There were some checks like this: ```bash curl_cmd="$(command -v curl)" if ! [ -x "${curl_cmd}" ]; then e "required dependency not found: curl not found in the path or not executable" exit ${no_dep_exit_code} fi ``` However, `command -v` exits with an error if the command does not exist. This results in the nice error message not being shown. The solution is to avoid the assignment of the variable erroring and instead relying on the executability check to make sure we have the right thing ## `md5` compatibility The test script depends on `md5sum` which is not available on MacOS. Thus, tests would fail at the start when run on MacOS. MacOS has a tool called `md5`, which when called with the `-r` flag which reverses the format of the output.
This PR fixes the issue nginxinc#164 ## Expectations This solution assumes that the following configuration options are set to true * `APPEND_SLASH_FOR_POSSIBLE_DIRECTORY` * `PROVIDE_INDEX_PAGE` * `ALLOW_DIRECTORY_LIST` Given a folder `test` **with** an `index.html` present in the directory, the `index.html` should be served for: * `/test` * `/test/` * `/test?foo=bar` * `/test/?foo=bar` Given a folder `test` **WITHOUT** an `index.html` present in the directory, files in the directory should be listed for: * `/test` * `/test/` * `/test?foo=bar` * `/test/?foo=bar` ## Notes * The `@trailslash` was rewriting to `$request_uri` with a trailing slash on the end. In the case of `/test?foo=bar` we'd wind up with `/test?foo=bar/` which when combined with the other changes led to a rewrite loop * Changed the check for directory or file to consider the path without anchor or querystring * Added yet another integration test suite and shuffled around the conditionals that maybe make the tests even more confusing - but do cover this case. Co-authored-by: Javier Evans <j.evans@f5.com> * make the isDirectory check simpler and more inclusive of error states * Allow useful output from curl and enable timeouts This change adds three new flags when using curl to hit endpoints as part of integration tests: --connect-timeout 3 --max-time 30 --no-progress-meter --------- Co-authored-by: Javier Evans <j.evans@f5.com>
…ginxinc#176) # What Fixes an issue where comments in the `/etc/resolv.conf` would get into the dns resolvers list and cause an error when starting the s3 gateway. ## How Following a suggestion on the issue, the logic from [this file](https://github.com/nginxinc/docker-nginx/blob/master/entrypoint/15-local-resolvers.envsh) in the official NGINX docker container was ported over to replace the existing logic. Given a `/etc/resolv.conf` that looks like this: ``` nameserver 127.0.0.11 nameserver 8.8.4.4 nameserver 94.198.184.14 nameserver 94.198.184.34 # NOTE: the libc resolver may not support more than 3 nameservers. # The nameservers listed below may not be recognized. nameserver 8.8.8.8 options ndots:0 ``` The existing code would produce this: ``` 127.0.0.11 8.8.4.4 94.198.184.14 94.198.184.34 [NOTE:] The 8.8.8.8 ``` With this change applied it looks like this: ``` 127.0.0.11 8.8.4.4 94.198.184.14 94.198.184.34 8.8.8.8 ``` The startup printout looks like this with the change applied: ``` Origin: http://bucket-1.minio:9000 Region: us-east-1 Addressing Style: virtual AWS Signatures Version: v2 DNS Resolvers: 127.0.0.11 8.8.4.4 94.198.184.14 94.198.184.34 8.8.8.8 Directory Listing Enabled: false Directory Listing Path Prefix: Provide Index Pages Enabled: Append slash for directory enabled: Stripping the following headers from responses: x-amz-; CORS Enabled: 0 /docker-entrypoint.sh: Launching /docker-entrypoint.d/10-listen-on-ipv6-by-default.sh 10-listen-on-ipv6-by-default.sh: info: Getting the checksum of /etc/nginx/conf.d/default.conf ``` ## Notes Ideally we should be incorporating the entrypoint scripts from the official nginx docker image. For now we're just porting key logic to get the issue solved quickly. The integration of these scripts will have some other concerns. An issue has been filed for this work: nginxinc#175
# What Adds the ability to generate a website with reference documentation based on JSDoc annotations in the Javascript code. Annotations are also added. This will help you if you use an editor that knows how to parse JSDoc annotations. **This change adds a workflow that depends on NodeJS** however, normal usage of the gateway will not require NodeJs, nor will any development workflows. In the future, development workflows will require NodeJs. Discussion on this topic here: nginxinc#163 ## Usage To generate the documentation run: ``` make jsdoc ``` It will be located in the `./reference` folder at the root of the project To generate and view the documentation in a browser, run: ``` make jsdoc-open ``` To remove generated documentation, run: ``` make clean ```
|
Hi, sorry again for the delay on this, health problems. I rebased my branch off master and attempted to apply your patch, however it failed on patching test.sh, likely b/c I've waited too long. Would you be able to apply it to my fork as you mentioned? Also, any suggestions on a different name for the property? I'm terrible at naming stuff, it look me forever to come up with STRIP_LEADING_DIRECTORY_PATH but would happily rename to whatever you think makes sense. Thanks! |
|
Hi @elJosho I'm sorry to hear about your health and hope you are well. It looks like the updates against master made things a little confusing so I pulled it out into another branch here #185 Would you mind testing it to make sure that it works for your use case? Upon regaining my context on this feature, I think the name of the configuration variable actually sounds fine. Once we're sure it works for you then I think we can just merge the other branch with some small docs additions. Your authorship will be maintained since I cherry picked the commits over from your fork. |
|
Pulled down your branch and it works for me. Thanks so much again for all your help with this! |
|
Closing in favor of #185 which is the same but resolved some merge issues. |
|
@elJosho Thank you so much for your effort with this pull request and the considered feature proposal. It has been merged to mainline and new images containing the change will be available shortly. |
|
@4141done thank you for your help with this, I really appreciate it! |
This will strip the leading directory from a request if configured. It's useful when you're trying to expose a bucket at a specific path on an ALB. For example, if I have an ALB rule that forwards all traffic from "www.mysite.com/mybucket" to the S3 gateway, and I want to request a file in the root of my bucket, "myfile.txt". Making a request to "www.mysite.com/mybucket/myfile.txt" will fail as the S3 gateway will look for the file inside a folder named "mybucket", rather than at the root of the bucket.
After this change, setting the new env var to
STRIP_LEADING_DIRECTORY_PATH = /mybucketwill send all requests to the root of the bucket.
Hoping this helps someone, it worked well for me. Please be gentle if I did anything especially stupid, this is my first time submitting a PR to a public repo.