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

[full-ci] Add TLS options for all reva grpc services #4798

Merged
merged 5 commits into from
Oct 25, 2022

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Oct 12, 2022

Description

This adds options for enabling TLS for reva grpc services. By default TLS is still disabled.

@update-docs
Copy link

update-docs bot commented Oct 12, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@ownclouders
Copy link
Contributor

ownclouders commented Oct 12, 2022

💥 Acceptance test Core-API-Tests-ocis-storage-9 failed. Further test are cancelled...

@rhafer rhafer force-pushed the grpc-tls branch 2 times, most recently from 99f2dca to 3189895 Compare October 13, 2022 09:30
services/notifications/pkg/config/config.go Outdated Show resolved Hide resolved
services/thumbnails/pkg/config/config.go Outdated Show resolved Hide resolved
services/webdav/pkg/config/config.go Outdated Show resolved Hide resolved
@rhafer rhafer force-pushed the grpc-tls branch 5 times, most recently from a54da3f to a0ce343 Compare October 19, 2022 09:50
@rhafer
Copy link
Contributor Author

rhafer commented Oct 19, 2022

Hm, from time to time ocis is faliing to start. Like here: https://drone.owncloud.com/owncloud/ocis/16249/53/4
I don't think I have seen it outside the CI yet, but there seems to be a race somewhere.

@rhafer rhafer force-pushed the grpc-tls branch 2 times, most recently from 0794195 to f94d03c Compare October 19, 2022 13:55
@rhafer
Copy link
Contributor Author

rhafer commented Oct 19, 2022

Hm, from time to time ocis is faliing to start. Like here: https://drone.owncloud.com/owncloud/ocis/16249/53/4 I don't think I have seen it outside the CI yet, but there seems to be a race somewhere.

I hope cs3org/reva#3377 helps with this.

@rhafer rhafer changed the title Enable (insecure) TLS for all reva grpc services [full-ci] Add TLS options for all reva grpc services Oct 20, 2022
@rhafer rhafer force-pushed the grpc-tls branch 4 times, most recently from 2c5cdb3 to e8bbfe6 Compare October 20, 2022 15:47
@rhafer rhafer marked this pull request as ready for review October 20, 2022 15:47
@rhafer rhafer requested review from C0rby and micbar October 20, 2022 15:47
@mmattel
Copy link
Contributor

mmattel commented Oct 20, 2022

Just recognizing this for all items: OCIS_GRPC_TLS_KEY and OCIS_GRPC_TLS_CERTIFICATE both start with the text: File name of the TLS server certificate. I guess that it is meant Path/File name and not the filename alone or?

@mmattel
Copy link
Contributor

mmattel commented Oct 20, 2022

The transport protocol of the GPRC service.
Added doc strings to the TLS options now.

I cant find any change in the descriptions...? did I miss something?

Consolidate all services to use the Reva config struct for the shared package.
This works because all services (except 'notifications', 'thumbnails' and
'webdav') where using the same config keys and environment variables for
setting the reva gateway.
This is needed as the permission server (provided by the settings service)
is not TLS enabled yet.
The commit of unifying the Reva Client config introduced some backwards
incompatible changes to the config structures and yaml config tags. For
the "thumbnails", "webdav" and "notifications" service. This reverts the
changes on the service and introduces TLS options in a backwards
compatible manner.
@rhafer
Copy link
Contributor Author

rhafer commented Oct 24, 2022

Just recognizing this for all items: OCIS_GRPC_TLS_KEY and OCIS_GRPC_TLS_CERTIFICATE both start with the text: File name of the TLS server certificate. I guess that it is meant Path/File name and not the filename alone or?

@mmattel This is the same docstring that we use for many other ENV var where certificates can be configured. If you think this needs to be addressed I'd rather see that happening in a separate PR (this one is already big enough). Would you mind opening a separate issue?

@mmattel
Copy link
Contributor

mmattel commented Oct 24, 2022

If you think this needs to be addressed I'd rather see that happening in a separate PR

all good, I just expressed my frustration as there are so many OCIS_ envs which slip thru automatic documantation. The issue is already addressed.

@rhafer
Copy link
Contributor Author

rhafer commented Oct 24, 2022

The transport protocol of the GPRC service.
Added doc strings to the TLS options now.

I cant find any change in the descriptions...? did I miss something?

Probably not. I interpreted your comment as if if was for the TLS related settings. Sorry for being a bit picky here. But I'd really like this PR to be focussed on the TLS related change (to keep it reviewable). The Transport Protocol settings (which I currently even don't exactly know about what is does) is not part of that. I agree it needs to be documented (or maybe even removed if we don't need it?). It should happen in a separate PR however.

@sonarcloud
Copy link

sonarcloud bot commented Oct 24, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
51.2% 51.2% Duplication

@rhafer rhafer requested a review from wkloucek October 25, 2022 08:50
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