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

feat: specify custom transport agent parameter #1104

Merged
merged 8 commits into from
Apr 26, 2023

Conversation

aldy505
Copy link
Contributor

@aldy505 aldy505 commented Mar 4, 2023

This PR provides an optional parameter of transportAgent that can be provided at the init of the Client class to provide custom http.Agent (https://nodejs.org/api/http.html#class-httpagent) or https.Agent (https://nodejs.org/api/https.html#class-httpsagent). In Go, this is the same as providing a parameter for custom http.Transport (https://pkg.go.dev/net/http#Transport).

If not provided, it will default to the globalAgent (https://nodejs.org/api/https.html#httpsglobalagent for secure == true, https://nodejs.org/api/http.html#httpglobalagent otherwise).

This Agent class is backward compatible as it's available from Nodejs v0.3.4

@prakashsvmx
Copy link
Member

@aldy505 http and https are supported internally. could you explain the need for transportAgent may be with an example ?

@aldy505
Copy link
Contributor Author

aldy505 commented Mar 6, 2023

If you're using a self signed CA certificate, or if your Minio instance is behind a reverse proxy that requires mTLS, every connection will be rejected.

@prakashsvmx
Copy link
Member

@aldy505 thank you.
So it is for a client certificate based authentication ?
Please update with an example and possible test cases.

@aldy505
Copy link
Contributor Author

aldy505 commented Mar 6, 2023

It's actually much more than client authentication/mTLS. You can go ahead and read Node's documentation on the Agent class.

I will update with example and tests later this week. I suppose I don't need to do integration test with a real Minio server running on the CI process?

@aldy505
Copy link
Contributor Author

aldy505 commented Mar 10, 2023

@prakashsvmx I've update the example. I'm not sure on testing this one, as this would not disturb current behaviour.

@harshavardhana harshavardhana requested review from prakashsvmx and removed request for prakashsvmx April 8, 2023 04:23
Copy link
Member

@prakashsvmx prakashsvmx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aldy505
Copy link
Contributor Author

aldy505 commented Apr 12, 2023

@prakashsvmx is there any blocker for this to get merged? I have a project that depends on a custom TLS that uses this library

@harshavardhana harshavardhana merged commit 14474b5 into minio:master Apr 26, 2023
@aldy505 aldy505 deleted the feat/custom-transport-agent branch April 26, 2023 23:25
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