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

Updating Nginx Reverse Proxy documentation #743

Merged
merged 2 commits into from Mar 6, 2023
Merged

Updating Nginx Reverse Proxy documentation #743

merged 2 commits into from Mar 6, 2023

Conversation

ravindk89
Copy link
Collaborator

Closes #727

Staged: http://192.241.195.202:9000/staging/DOCS-727/integrations/setup-nginx-proxy-with-minio.html

Summary

With the websocket changes for MinIO Console, the existing nginx reverse proxy instructions need updating.

We also have run into a lot of users who have confusion on hosting MinIO on subpaths, which breaks the S3 signature calculation.

This PR handles both areas:

  • Proxying MinIO based on subdomains of a single hostname
  • Proxying MinIO based on subpath of a single hostname
  • Including the websocket upload logic

These docs make no reference on the nginx structure, because in my latest testing the whole sites-available setup has vanished entirely for a simpler single default file you modify. We are comfortable letting Users take on the responsibility of being experts at nginx.

@ravindk89 ravindk89 added the Review Assigned Reviewers Must LGTM To Close label Feb 24, 2023
@ravindk89 ravindk89 self-assigned this Feb 24, 2023
- A DNS hostname which uniquely identifies the MinIO deployment

There are two models for proxying requests to the MinIO Server API and the MinIO Console:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Choosing to not deal with :443 or https here. Not sure if that makes a difference.

Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

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

A few wording suggestions and some styling things that didn't come out correctly in the staged render.
I'd like to see it again after an engineering pass.

This documentation assumes the following:

- An existing `Nginx <http://nginx.org/en/download.html>`__ deployment
- An existing :ref:`MinIO <minio-installation>` deployment
Copy link
Collaborator

Choose a reason for hiding this comment

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

This didn't resolve to a link. Is the reference correct?

Copy link
Collaborator Author

@ravindk89 ravindk89 Feb 27, 2023

Choose a reason for hiding this comment

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

Fixed. I think I needed to re-update staging


The following location blocks provide a template for further customization in your unique environment:

.. code-block:: shell
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pygments has nginx as a valid language format. I'd use that instead of shell for better highlighting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

listen [::]:80;
server_name minio.example.net;

# To allow special characters in headers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drop the To in this and the other comment lines in the code sample.

Allow special characters in headers is fine.

}
}

The S3 API signature calculation algorithm does *not* support proxy schemes where you host either the MinIO Server API or Console GUI on a subpath, e.g. ``example.net/s3/`` or ``example.net/s3/console``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we drop the latinism e.g. and use the plain English for example or that is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

such as could also work here to avoid repeating example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


The following location blocks provide a template for further customization in your unique environment:

.. code-block:: shell
Copy link
Collaborator

Choose a reason for hiding this comment

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

nginx for the language instead of shell.


.. tab-item:: Subdomain

Create or configure a unique subdomain for both the MinIO Server S3 API and MinIO Console Web GUI.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say for each of the here.
"for both" risks implying one subdomain is all that is needed.

Maybe

Create or configure separate, unique subdomains for the MinIO Server S3 API and for the MinIO Console Web GUI.

Separate and unique may be too much, but I think making subdomains plural really emphasizes they need two subdomains. I'll leave that to you to decide.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Taking your suggestion!

@djwfyi
Copy link
Collaborator

djwfyi commented Feb 28, 2023

Consider updating the NGINX link on this page to point to this topic.
https://min.io/docs/minio/linux/operations/install-deploy-manage/expand-minio-deployment.html#networking-and-firewalls

@ravindk89
Copy link
Collaborator Author

@djwfyi back to you - I have internal signoff on the engineering side.

Copy link
Collaborator

@djwfyi djwfyi left a comment

Choose a reason for hiding this comment

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

LGTM
Merge it!

@ravindk89 ravindk89 merged commit 732b6c3 into main Mar 6, 2023
@ravindk89 ravindk89 deleted the DOCS-727 branch March 6, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review Assigned Reviewers Must LGTM To Close
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update nginx reverse proxy documentation
2 participants