Make allow-directory-listing and provide-index-page compatible

[AyodeAwe]

This PR follows the discussion that happened in this issue.

The PR enables automatic static index.html rendering alongside the existing directory listing functionality provided by the library.

Changes:

• Architecture becomes thus: initial request is made to get an index.html page. If this page is found, it is rendered, otherwise a simple directory listing is done.
• Feature only works if PROVIDE_INDEX_PAGE variable is set to true.

Note: Due to being an NVIDIA employee I am supposed to request files I modify have an NVIDIA copyright - this explains the included copyright headers.

[dekobon]

Thank you @AyodeAwe for your contribution. Unfortunately, I cannot accept contributions that retain their NVIDIA copyright. Honestly, I am disappointed about that, and I don't like this copyright game we are all stuck in, yet I've got to follow our policy. We also could try to set up a company-to-company contributor agreement, but that's a lot of work for a few number of changes.

Perhaps, this PR can inspire another person to draft their own set of changes without a copyright attached.

Seriously, myself and the NGINX crew are quite grateful for the contributions. By any chance, do you work with @indianwhocodes or @KodyKantor ?

Kindly,
Elijah

By the way are you working with

[AyodeAwe]

Hey @dekobon I received permission to remove the copyright headers, so I have added a commit to that effect.

To answer your question, I don't recall having worked with either of the persons you mentioned.

[dekobon]

Hey @dekobon I received permission to remove the copyright headers, so I have added a commit to that effect.

Wow, thank you! I will work on the next steps for getting this merged.

[ajschmidt8]

@dekobon, just checking in. any updates here?

[dekobon]

I've started to review this PR and I'm concerned that the automated tests are failing. I've yet to get enough focus time to determine why they are failing, so if @AyodeAwe or @ajschmidt8 can do this, it would be quite helpful in moving this PR forward.

[AyodeAwe]

@dekobon The issue with the failing automated tests should be fixed now. Kindly re-approve workflow.

[AyodeAwe]

Just rebased this PR on top of the latest remote commit (af0b2e9). Might need a workflow re-run to see if any errors still come up.

[AyodeAwe]

cc: @dekobon ^^

[dekobon]

It looks like the tests are now failing after rebasing. I'll be out of town next week, so I will not be able to make progress. If anyone wants to jump in and offer a fix, that would be appreciated.

[AyodeAwe]

Fixed and tested locally. Kindly approve workflow run @dekobon

[AyodeAwe]

Just checking on this PR @dekobon. Happy to help if any more work can be done to help move this PR forward.

[dekobon]

Right now, I'm working on testing. I've got the PR merged with latest master and it is passing all the tests on my test system. It is just a manner of me getting a solid chunk of time to go through it. My apologies for the delay.

[dekobon]

Hi @AyodeAwe

I've made some modifications to the code for this PR (squashing and making a docs change). I've pushed it to: https://github.com/dekobon/nginx-s3-gateway/tree/render-static-index

I would really like to keep the conversation history for this PR in a single place. Would you mind pulling this changes and force pushing them over the branch you have associated with for this PR?

Next, I have an ask regarding the commit titled "allow HEAD requests." Can we either:

1. Remove the commit
2. Fix the gateway such that when doing HEAD requests to subdirectories it does not return a 404 for subdirectories that exist.

If I recall correctly, we do not support subdirectories because I wasn't able (or didn't have the time) to get them to return without doing a 404.

Lastly, I have one more question. Did you test these changes with the v2 signatures enabled?

[ajschmidt8]

@dekobon, just a heads up. @AyodeAwe is a colleague of mine and he is out this week. so we may not hear from him until next week.

[AyodeAwe]

Ok. I’ve pulled in your changes and removed the allow HEAD requests commit.

Regarding testing with v2, it looks like Signature Version 2 is being turned off (deprecated) by AWS so all requests are naturally now failing. See warning note here.

[AyodeAwe]

cc: @dekobon ^^

[AyodeAwe]

Note @dekobon the allow HEAD requests commit (which you asked to remove) fixes the currently failing test case; but this failure should be irrelevant seeing that signature v2 requests are turned off by AWS. All v4 tests pass.

The failing test is this line (fails when HEAD requests are made to the root / directory). It passes when we update the conditional in this line to also check for HEAD requests and not only GETs.

If we must have this case (specifically root directory HEAD requests - as other requests in that case assert correctly) pass (even though it is testing with the now deprecated v2 signature), we may need to update the if statement as described above.

Thoughts?

[dekobon]

Hi @AyodeAwe, my apologies for misunderstanding how the allow HEAD requests commit worked to mitigate the test failure.

Just as you say, changing s3gateway.js:323 to:

    if (isDirectoryListing && (r.method === 'GET' || r.method === 'HEAD')) {

makes the tests pass.

I think if we add, we will be good to go. Thank you for bearing with me on this.

As for v2 signatures, if it passes the integration tests, I think that is good enough for now. I do not want to deprecate v2 signatures because I've found that many non-AWS S3 interfaces use them exclusively.

[AyodeAwe]

Thank you @dekobon, the commit has been re-added.