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

added tls options #84

Merged
merged 4 commits into from
Mar 3, 2021
Merged

added tls options #84

merged 4 commits into from
Mar 3, 2021

Conversation

chiararelandini
Copy link
Contributor

added tls options for https protocol to makeCall in serviceBuilder

@coveralls
Copy link

coveralls commented Mar 1, 2021

Coverage Status

Coverage remained the same at 97.546% when pulling 173d484 on feature/tlsOptions into eaa569e on master.

Copy link
Member

@fredmaggiowski fredmaggiowski left a comment

Choose a reason for hiding this comment

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

The implementation is fine, I'd just try and fix the lint issues raised by GitHub on the test files (we could disable them at all for the test file).

Just one question: with this PR we provide the feature of creating a single serviceProxy with custom certificates/etc; could we implement this feature also on a library-wide scale? This way a user could configure the library once and then build all the desired services automatically inheriting the configuration. Maybe managed via some environment variable specificying where the files are located 🤔 ?

@chiararelandini
Copy link
Contributor Author

@fredmaggiowski I don't know if using the same settings for all services would work. Certificates and Certificate Authority might change as the contacted server changes. Or the kind of authorization might change, it is not necessarily true that all service proxies use https configurations if one does.

If you just refer to having an environment variable to save certificate and key files location, this can be certainly done. Would you like me to implement it?

@fredmaggiowski
Copy link
Member

fredmaggiowski commented Mar 2, 2021

@chiararelandini I agree with your comment:

Certificates and Certificate Authority might change as the contacted server changes.

this is true, especially if the lib is used in a microservice that contacts different external services, their certification authorities will often (if not always) differ.

however there could still be a case where my microservice will only contact internal services that share the same certification authorities and I'd like my lib to be configured once for all the instances of a serviceProxy I might create. I think that this would be a useful commodity, we could implement it later on if you prefer.

@fredmaggiowski
Copy link
Member

Speaking with @chiararelandini we resolved with a good solution that will be implemented in another PR, it is tracked in #85

@fredmaggiowski
Copy link
Member

LGTM @davidebianchi any comment?

@davidebianchi
Copy link
Member

LGTM

@davidebianchi davidebianchi merged commit d12a202 into master Mar 3, 2021
@davidebianchi davidebianchi deleted the feature/tlsOptions branch March 3, 2021 09:37
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.

4 participants