operator: Configure gateway to honor the global TLS security profile #6870

aminesnow · 2022-08-10T17:04:56Z

What this PR does / why we need it:
This PR enables the gateway to be configured with the minimum TLS version and ciphers defined in the global apiservers configuration on the OpenShift Platform.

Checklist

Documentation added
Tests updated
Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

grafanabot · 2022-08-10T17:11:28Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Red-GV

Looks good so far!

A few comments on this one:

Red-GV · 2022-08-25T20:19:18Z

operator/internal/handlers/lokistack_create_or_update.go

I wonder if the better option here is to expand the Options structure to also have an optional TLS configuration pieces from the apiServer object.

Also, returning an error here causes an issue in base K8s, no? We should only return a degraded error if we are on an OpenShift cluster.

I agree that adding it to Options is probably a better deal. We can also convert the TLSSecurityProfile to the information we actually need (minimum TLS version and cipher list) before passing it into the manifest generation.

The request to get the APIServer resource can only be done on OpenShift and will fail on other clusters. But even on OpenShift it can not be assumed that the TLSSecurityProfile is configured. This leads to the question what a sensible fallback is, I'd say either we go with the intermediate profile like the PR implementation already does or we skip configuring the TLS version and ciphers like we did before.

Red-GV · 2022-08-25T20:20:21Z

operator/internal/manifests/gateway_tenants_test.go

Maybe move this to some helper function to reference rather than put the entire list here? Perhaps there's a way to grab all the cipher-suites and filter them by tls version allowed?

An alternative would be to configure some "dummy values" just for testing.

Oh! I hadn't noticed this was the test file. Yeah, maybe some dummy values would be best.

periklis

This is a great start into supporting a TLS profile for our managed components in Loki. This work is really awesome 🚀

In general the code looks clear and concise for me. However, we need to polish out this feature for k8s first and make the automation piggypacking on openshiftv1.ApiServers CRD only optional. Thus I suggest to address this like this:

First let's add a configuration option in the operator configuration for selecting one of the three named profiles. All three are general names from Mozilla's work on groups TLS versions and cipher suites.
Add a feature flag for OpenShift that overwrites per default the operator config for TLSProfile with the values from the APIServer CRD.
In general pass anything from the CRD or the operator configuration to the custom manifest.Options with new fields and minimize the exposure to optional CRDs.

WDYT? (cc @xperimental @Red-GV )

grafanabot · 2022-08-30T08:07:19Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

periklis

This looks way better and more decoupled than the first version. However I suggest to decouple from using enumerations from the openshift/api package to remove the burden of maintaining version compatibility in general (as per both our code base and openshift/api depend on k8s).

I suggest simply to create an enumeration for three TLS Profiles (https://wiki.mozilla.org/Security/Server_Side_TLS) and use it across our code base. In addition I suggest to replace DisableAPIServerTLSProfile with a simple TLSProfile field so non-openshift users can declare what profile they want to choose directly.

aminesnow · 2022-08-30T09:19:48Z

@periklis Thanks for the feedback!
There is actually a 4th enum value Custom, and it allows the user to define a custom profile. I would like to keep supporting it in addition to the 3 main ones.
Also, DisableAPIServerTLSProfile is defined in the openshift options because by default, when using openshift, we get the profile from the api server, and this allows the user to disable it so that they can define their own setting. So I'm confused by your comment regarding this..

periklis · 2022-08-30T09:27:28Z

@periklis Thanks for the feedback! There is actually a 4th enum value Custom, and it allows the user to define a custom profile. I would like to keep supporting it in addition to the 3 main ones.

Supporting Custom would require to add a set of two fields one for TLSMinVersion and one for TLSCipherSuites in the operator project config. I would like to skip this extra options if nobody asks for them.

Also, DisableAPIServerTLSProfile is defined in the openshift options because by default, when using openshift, we get the profile from the api server, and this allows the user to disable it so that they can define their own setting. So I'm confused by your comment regarding this..

An Options with DisableAPIServer... as a prefix is quite confusing for normal k8s users. I even doubt if this feature is well known and understood in the OpenShift ecosystem. Thus I would like to lower the confusion by making this field the input enumeration to select one of the three public listed profiles (See link to Mozilla). Our project config for OpenShift users will let this field empty and use the ApiServer CRD to fetch the profile.

aminesnow · 2022-08-30T09:30:49Z

@periklis understood, thanks for the explanation!

grafanabot · 2022-08-31T09:26:00Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-08-31T16:03:36Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-09-01T15:53:12Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0.1%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

operator/CHANGELOG.md

operator/go.mod

operator/internal/handlers/lokistack_create_or_update.go

operator/internal/handlers/lokistack_create_or_update_test.go

operator/internal/manifests/gateway.go

operator/internal/manifests/options.go

grafanabot · 2022-09-02T14:50:53Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-09-02T15:05:39Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
-              logql	-0.4%
+               loki	0%

operator/cmd/loki-broker/main.go

operator/internal/handlers/internal/tlsprofile/tlsprofile.go

operator/internal/handlers/lokistack_create_or_update.go

operator/internal/handlers/lokistack_create_or_update_test.go

operator/internal/manifests/options.go

operator/internal/manifests/gateway_tenants.go

operator/internal/handlers/internal/tlsprofile/tlsprofile.go

grafanabot · 2022-09-05T10:19:13Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-09-06T08:47:11Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

periklis

Minor suggestions and we are good to go

operator/internal/manifests/gateway_test.go

operator/internal/handlers/lokistack_create_or_update_test.go

operator/internal/handlers/internal/tlsprofile/tlsprofile_test.go

grafanabot · 2022-09-06T13:08:17Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-09-06T14:27:58Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

…rafana#6870)

pull-request-size bot added the size/L label Aug 10, 2022

github-actions bot added the sig/operator label Aug 10, 2022

aminesnow changed the title ~~Operator: Configure loki-operator to honor the global tlsSecurityProfile on Openshift~~ Operator: Configure gateway to honor the global tlsSecurityProfile on Openshift Aug 10, 2022

aminesnow marked this pull request as ready for review August 10, 2022 17:06

aminesnow requested a review from a team as a code owner August 10, 2022 17:06

Red-GV reviewed Aug 25, 2022

View reviewed changes

periklis requested changes Aug 29, 2022

View reviewed changes

aminesnow force-pushed the tls_policy branch from afcafe8 to 07b5d6a Compare August 29, 2022 14:35

periklis requested changes Aug 30, 2022

View reviewed changes

aminesnow force-pushed the tls_policy branch from 4cba0be to 79b18d7 Compare September 1, 2022 15:46

periklis requested changes Sep 1, 2022

View reviewed changes

Mohamed-Amine Bouqsimi added 6 commits September 2, 2022 16:41

add tls policy mgmt to gateway/opa

178d9fb

changelog

83d20f8

added option config

5052b15

some more refactoring

ef48db3

tlsProfile param fix

f4a665e

refactor

f8ff3cd

aminesnow force-pushed the tls_policy branch from ac19f70 to f8ff3cd Compare September 2, 2022 14:46

go mod

c1fa62f

periklis requested changes Sep 5, 2022

View reviewed changes

add UT + refactor

68d248b

enable tls profile to all tenants

cd3c462

periklis reviewed Sep 6, 2022

View reviewed changes

operator/internal/manifests/gateway_test.go Outdated Show resolved Hide resolved

operator/internal/handlers/lokistack_create_or_update_test.go Outdated Show resolved Hide resolved

operator/internal/handlers/internal/tlsprofile/tlsprofile_test.go Outdated Show resolved Hide resolved

ut to table tests

86897a9

changelog

8347c2c

periklis approved these changes Sep 6, 2022

View reviewed changes

periklis changed the title ~~Operator: Configure gateway to honor the global tlsSecurityProfile on Openshift~~ operator: Configure gateway to honor the global TLS security profile Sep 6, 2022

periklis merged commit a068c12 into grafana:main Sep 6, 2022

periklis mentioned this pull request Sep 9, 2022

Operator: Configure kube-rbac-proxy sidecar to use Intermediate TLS security profile in OCP #7092

Merged

4 tasks

lxwzy pushed a commit to lxwzy/loki that referenced this pull request Nov 7, 2022

operator: Configure gateway to honor the global TLS security profile (g…

91a6ea3

…rafana#6870)

operator: Configure gateway to honor the global TLS security profile #6870

operator: Configure gateway to honor the global TLS security profile #6870

Uh oh!

Conversation

aminesnow commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grafanabot commented Aug 10, 2022

Uh oh!

Red-GV left a comment

Choose a reason for hiding this comment

Uh oh!

Red-GV Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

xperimental Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Red-GV Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

xperimental Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

Red-GV Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

periklis left a comment

Choose a reason for hiding this comment

Uh oh!

grafanabot commented Aug 30, 2022

Uh oh!

periklis left a comment

Choose a reason for hiding this comment

Uh oh!

aminesnow commented Aug 30, 2022

Uh oh!

periklis commented Aug 30, 2022

Uh oh!

aminesnow commented Aug 30, 2022

Uh oh!

grafanabot commented Aug 31, 2022

Uh oh!

grafanabot commented Aug 31, 2022

Uh oh!

grafanabot commented Sep 1, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grafanabot commented Sep 2, 2022

Uh oh!

grafanabot commented Sep 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grafanabot commented Sep 5, 2022

Uh oh!

grafanabot commented Sep 6, 2022

Uh oh!

periklis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

grafanabot commented Sep 6, 2022

Uh oh!

aminesnow commented Aug 10, 2022 •

edited

Loading

xperimental Aug 26, 2022 •

edited

Loading