-
Notifications
You must be signed in to change notification settings - Fork 180
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
[full-ci] Introduce TLS Settings for go-micro based grpc services and clients #4901
Conversation
8594125
to
1f36279
Compare
I see a problem there, not with the implementation or the env description texts. The problem is, that none of the envs starting with This impacts that we have important envs to configure but nobody knows... Referncing #3917 (Harmonize env variable naming for those which are not bound to a particular service) @dragonchaser fyi |
This is not what is happening here. Actually the env vars are assigned to specific services. Even if the struct are defined on a "global" level (mainly to avoid to much duplication and copy and pasting the same thing all over the code base, also because currently there is no need to override these settings on a per service base). Each service that needs those settings instantiates that struct in it's own config struct. And the documentation will get properly generated. Exactly the same thing was done in: #4798 (e.g. for the REVA_GATEWAY_TLS_MODE and REVA_GATEWAY_TLS_CACERT settings) and the documentation is properly generated for those. See https://owncloud.dev/services/users/configuration/ |
99272bd
to
2ce07b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the verious places where you configure the GRPC default client:
err = ogrpc.Configure(ogrpc.GetClientOptions(cfg.GRPCClientTLS)...)
That only works because GRPCClientTLS is a *shared.GRPCClientTLS
that is filled by EnsureDefaults()
, only has a single OCIS_GRPC_CLIENT_TLS_*
set of env vars and no service specific config.
While it looks fragile to me and I think we may need to add service specific certificates in the future, this is a great step in the right direction.
This comment was marked as resolved.
This comment was marked as resolved.
I set
now getting:
🤔 |
my bad it has to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the PR needs a rebase on / merge of master as some of the expected test lines look shifted.
Yes. And I did that on purpose. Mainly to avoid spreading even more code duplication across all the services'
Two questions:
|
BTW, you can even leave OCIS_GRPC_TLS_CERTIFICATE and OCIS_GRPC_TLS_KEY empty. It will just generate (insecure) certificates in-memory (at every start of ocis). |
A sentence could be added to the description reflecting this behaviour. |
|
Added some commitable suggestions, from a docs pov, looks fine to me though I would be happy to have an answer to #4901 (comment) for a general improvment of that env descriptions - which can be fixed post merging this PR but should be considered. |
TLS for the services can be configure by setting the OCIS_MICRO_GRPC_TLS_ENABLED" "OCIS_MICRO_GRPC_TLS_CERTIFICATE" and "OCIS_MICRO_GRPC_TLS_KEY" enviroment variables. TLS for the clients can configured by setting the "OCIS_MICRO_GRPC_CLIENT_TLS_MODE" and "OCIS_MICRO_GRPC_CLIENT_TLS_CACERT" variables. By default TLS is disabled. Co-authored-by: Martin <github@diemattels.at>
All grpc service (whether they're based on reva) or go-micro use the same set of config vars now. TLS for the services can be configure by setting the OCIS_GRPC_TLS_ENABLED, OCIS_GRPC_TLS_CERTIFICATE and OCIS_GRPC_TLS_KEY enviroment variables. TLS for the clients can configured by setting the OCIS_GRPC_CLIENT_TLS_MODE and OCIS_MICRO_GRPC_CLIENT_TLS_CACERT variables. There are no individual per service config vars currently. If really needed, per service tls configurations can be specified via config file. Co-authored-by: Martin <github@diemattels.at>
Kudos, SonarCloud Quality Gate passed! |
Reusing a shared config requires initializing it with EnsureDefaults ... I'm just beginning to hold a grudge against all the boilerplate, which I consider fragile when trying to create a new service. At the moment I think it is still nearld impossible to add a service and get the config and env var parsing right ... not really a critique af this PR, I admit.
Yeah, I had the same thought but did not really follow up on it. Thank you for driving the nail into the coffin by confirming it. |
Author: Ralf Haferkamp <rhaferkamp@owncloud.com> Date: Thu Nov 3 10:17:08 2022 +0100 [full-ci] Introduce TLS Settings for go-micro based grpc services and clients (#4901) * Introduce TLS Settings for go-micro based grpc services and clients TLS for the services can be configure by setting the OCIS_MICRO_GRPC_TLS_ENABLED" "OCIS_MICRO_GRPC_TLS_CERTIFICATE" and "OCIS_MICRO_GRPC_TLS_KEY" enviroment variables. TLS for the clients can configured by setting the "OCIS_MICRO_GRPC_CLIENT_TLS_MODE" and "OCIS_MICRO_GRPC_CLIENT_TLS_CACERT" variables. By default TLS is disabled. Co-authored-by: Martin <github@diemattels.at> * Unify TLS configuration for all grpc services All grpc service (whether they're based on reva) or go-micro use the same set of config vars now. TLS for the services can be configure by setting the OCIS_GRPC_TLS_ENABLED, OCIS_GRPC_TLS_CERTIFICATE and OCIS_GRPC_TLS_KEY enviroment variables. TLS for the clients can configured by setting the OCIS_GRPC_CLIENT_TLS_MODE and OCIS_MICRO_GRPC_CLIENT_TLS_CACERT variables. There are no individual per service config vars currently. If really needed, per service tls configurations can be specified via config file. Co-authored-by: Martin <github@diemattels.at> Co-authored-by: Martin <github@diemattels.at>
TLS for the services can be configure by setting the OCIS_GRPC_TLS_ENABLED" "OCIS_GRPC_TLS_CERTIFICATE" and "OCIS_GRPC_TLS_KEY" enviroment variables.
TLS for the clients can configured by setting the "OCIS_GRPC_CLIENT_TLS_MODE" and "OCIS_GRPC_CLIENT_TLS_CACERT" variables.
By default TLS is disabled.