Enable securing query component by a sidecar #1105
Conversation
Enable and configure a sidecar with arguments by the Jaeger crd and the service will be redirected. The UI of the query component has no authentication measure. Resolves: #1104 Signed-off-by: Bastian Schlegel <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1105 +/- ##
==========================================
+ Coverage 88.22% 88.28% +0.06%
==========================================
Files 86 86
Lines 5351 5379 +28
==========================================
+ Hits 4721 4749 +28
Misses 466 466
Partials 164 164
Continue to review full report at Codecov.
|
|
Thank you for this PR! I think it's a great first step and this is something we indeed wanted to implement. We have a logic already about adding OAuth proxies, which is currently active only when the operator is running in an OpenShift cluster instead of Kubernetes. This code should probably be adapted to use the Keycloak Gatekeeper when the platform isn't OpenShift. Ideally, we could even configure it automatically if the Keycloak Operator is found and a suitable Keycloak instance is found ;-) jaeger-operator/pkg/inject/oauth_proxy.go Lines 22 to 38 in e7ad3f0 jaeger-operator/pkg/strategy/controller.go Lines 93 to 101 in e7ad3f0 |
To enable using a sidecar for securing query component (UI) the existing oauth-proxy function is adapted and expanded to use it also for a none-OpenShift platform. Resolves: #1104 Signed-off-by: Bastian Schlegel <[email protected]>
Some smaller reverts of my first pull request which are not necessary anymore Resolves: #1104 Signed-off-by: Bastian Schlegel <[email protected]>
Again a small revert of change from my first pull request which are not necessary anymore Resolves: #1104 Signed-off-by: Bastian Schlegel <[email protected]>
|
Hello Mr. Krohling, Thanks for your feedback to my pull request! At jaeger.spec.query.oauthProxy.image you´re able to configure another sidecar - default is Could you check the code and give me feedback again what is needed to confirm my pull request |
|
Unfortunately the e2e (es-otel) got a timeout - It worked with the same code already in my other (temp-)branch: |
|
The same test failed on #1111. It might not be related to the PR at all. cc @kevinearls |
There was a problem hiding this comment.
This looks great! There are a couple of comments, but I like the direction this is going. I also love that this is quite generic, not really tied to Keycloak specifically.
@stianst, do you have someone on your team that could review this one?
| @@ -200,6 +200,21 @@ type JaegerQuerySpec struct { | |||
|
|
|||
| // +optional | |||
| JaegerCommonSpec `json:",inline,omitempty"` | |||
|
|
|||
| // +optional | |||
| OauthProxy JaegerQueryOauthProxySpec `json:"oauthProxy,omitempty"` | |||
There was a problem hiding this comment.
This can be a pointer, so that an empty object isn't persisted in case the user does not specify it.
pkg/inject/oauth_proxy_test.go
Outdated
| jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) | ||
| jaeger.Spec.Ingress.Security = v1.IngressSecurityOAuthProxy | ||
| viper.Set("platform", "kubernetes") |
There was a problem hiding this comment.
This shouldn't be required for the test to pass.
pkg/inject/oauth_proxy_test.go
Outdated
| // see https://github.com/openshift/oauth-proxy/issues/95 | ||
| jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) | ||
| jaeger.Spec.Ingress.Security = v1.IngressSecurityOAuthProxy | ||
| viper.Set("platform", "openshift") |
There was a problem hiding this comment.
Most of the tests add this as the first thing in the test, implying that this is set before anything we call ourselves.
pkg/inject/oauth_proxy_test.go
Outdated
| @@ -213,6 +289,40 @@ func TestOAuthProxyResourceLimits(t *testing.T) { | |||
| assert.Equal(t, *resource.NewQuantity(512, resource.DecimalSI), dep.Spec.Template.Spec.Containers[1].Resources.Requests[corev1.ResourceRequestsEphemeralStorage]) | |||
| } | |||
|
|
|||
| func TestOpenShiftOAuthProxyResourceLimits(t *testing.T) { | |||
There was a problem hiding this comment.
If this is exactly like the previous test, consider making it a table test, so that it's clear what's different between the tests.
pkg/strategy/controller_test.go
Outdated
| @@ -157,12 +157,21 @@ func TestSetSecurityToOAuthProxyByDefaultOnOpenShift(t *testing.T) { | |||
| assert.Equal(t, v1.IngressSecurityOAuthProxy, jaeger.Spec.Ingress.Security) | |||
| } | |||
|
|
|||
| func TestSetSecurityToNoneOnNonOpenShift(t *testing.T) { | |||
| func TestSetSecurityOnNonOpenShift(t *testing.T) { | |||
There was a problem hiding this comment.
This is not dependent on being on OpenShift anymore, so, this can be renamed, TestSetExplicitSecurity could be appropriate.
|
@abstractj could you take a look at this? |
|
@stianst sure, no problem |
Addressed most review comments and updated example with new format. Resolves: #1104 Signed-off-by: Nils Böhling <[email protected]>
|
Hello everybody, |
|
@nilsboeh thank you for your work in this PR! There are still quite a few items open, and I would love to re-review this and give it a try once they are addressed. |
| # The redirection url, essentially the site url, note: /oauth/callback is added at the end | ||
| redirection-url: "https://jaeger-gatekeeper.example.com" | ||
| # Role restrictions for URL. User must have specified role in order to access. | ||
| resources: "uri:/admin/*|white-listed=true" |
There was a problem hiding this comment.
Have you tested this configuration? I'm not sure this will work. We don't have a /admin in the Jaeger UI.
There was a problem hiding this comment.
My bad... The /admin does not belong there.
There was a problem hiding this comment.
I corrected this but found another problem, especially with the resources option. The Gatekeeper accepts this option multiple times so you can define it for different subpages:
--enable-default-deny=true \
--resources="uri=/admin*|roles=test1,test2" \
--resources="uri=/backend*|roles=test1" \
Our current implementation with translation from yaml options to args only accepts this once and other notations (as map or multiple uri's separated with pipes) do not work properly.
There was a problem hiding this comment.
We enabled adding volumeMounts to the oauth-proxy to configure it via a configuration file placed in a secret
Addressed recent review comments by changing default ContainerPort to 9091 and correcting the resources option in the example. Resolves: #1104 Signed-off-by: Nils Böhling <[email protected]>
- OauthProxy object is now a pointer. - OauthProxy configuration via config file in volumeMount enabled - OauthProxy tests optimized - OauthProxy ports changed for none-OpenShift platforms - deploy example updated Resolves: #1104 Signed-off-by: Bastian Schlegel <[email protected]>
|
Hi everyone, we tried to solve the remaining review comments. Please have a look. |
|
Hi, |
|
@basch255 because we need all commits to be signed-off, I can't just resolve them via GitHub's web interface, but looking at the conflict, the resolution is quite simple: just remove the cmd.Flags().String("jaeger-es-rollover-image", "jaegertracing/jaeger-es-rollover", "The Docker image for the Jaeger Elasticsearch Rollover")
cmd.Flags().String("oauth-proxy-image", "quay.io/keycloak/keycloak-gatekeeper:10.0.0", "The Docker image location definition for the Default OAuth Proxy")
cmd.Flags().String("openshift-oauth-proxy-image", "openshift/oauth-proxy:latest", "The Docker image location definition for the OpenShift OAuth Proxy") |
|
@jpkrohling Thanks for your reply and the resolution. I wasn't sure I should merge the commits. Now the conflict is solved and my branch got an update from the master. |
There was a problem hiding this comment.
Thanks for your hard work, I think we are very close.
Here's a list of things I need before merging:
- instructions on how to setup an OAuth Proxy (Keycloak Gatekeeper) for testing. Without that, I don't know how to test this PR, and would make it hard for us to document and write e2e tests.
make checkfixing the formatting issues
Things that I would find awesome to have in this PR:
- e2e tests
- a PR against the documentation repo, showing how to get this working
- having the OpenShift OAuth Proxy as the first example of override for the new generic (default: Keycloak) OAuth proxy. I left a comment about it inline in the review.
|
|
||
| // +optional | ||
| // +listType=set | ||
| VolumeMounts []v1.VolumeMount `json:"volumeMounts,omitempty"` |
There was a problem hiding this comment.
The indentation here looks odd, but the CI didn't catch this problem, apparently. Running make check locally did report this as a problem.
Anyway: could you please run make check and update the PR?
| @@ -20,23 +20,59 @@ const defaultProxySecret = "ncNDoqLGrayxXzxTn5ANbOXZp3qXd0LA" | |||
|
|
|||
| // OAuthProxy injects an appropriate proxy into the given deployment | |||
| func OAuthProxy(jaeger *v1.Jaeger, dep *appsv1.Deployment) *appsv1.Deployment { | |||
| if jaeger.Spec.Ingress.Security != v1.IngressSecurityOAuthProxy { | |||
| return dep | |||
| if jaeger.Spec.Ingress.Security == v1.IngressSecurityOAuthProxy { | |||
There was a problem hiding this comment.
The old code was doing a short-circuit here: if the security isn't OAuth Proxy, then just return the dep. Everything else would be handled outside of an if statement. I find that approach more readable, but you can keep it this way if you prefer it.
|
|
||
| ports := []corev1.ContainerPort{ | ||
| { | ||
| ContainerPort: 9091, |
There was a problem hiding this comment.
A future improvement: allow the port to be customized, just like the image.
| jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) | ||
| jaeger.Spec.Ingress.Security = v1.IngressSecurityOAuthProxy | ||
| defer viper.Reset() |
There was a problem hiding this comment.
There's no viper.Set in this method, it doesn't need a Reset.
| return dep | ||
| } | ||
|
|
||
| func getOAuthProxyContainer(jaeger *v1.Jaeger) corev1.Container { |
There was a problem hiding this comment.
One thing I just realized while reviewing this PR: we want the generic OAuth feature to allow users to specify their own OAuth Proxies, so that we are not tied to Keycloak specifically.
With that in mind, the first consumer that overrides the default OAuth Proxy could be the operator itself, when running on OpenShift. Meaning: the code path for the generic OAuth proxy could be used for both OpenShift and non-OpenShift, with the OpenShift path only customizing the OAuth parts (image/args).
I know it might sound a bit frustrating to hear this at this point in the PR, but it just clicked with me. For some reason, I believe you'll also realize that :)
| @@ -5,6 +5,8 @@ import ( | |||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |||
| "k8s.io/apimachinery/pkg/util/intstr" | |||
|
|
|||
| "github.com/spf13/viper" | |||
There was a problem hiding this comment.
This import belongs to the second (first in this case) import group.
|
Hi, |
|
I think the docs / examples should not refer to Keycloak Gatekeeper if this is approved: Sunsetting Louketo Project louketo/louketo-proxy#683 |
|
Back to the drawing board, I guess. I'm closing this, but I'm really looking forward for a new proposal. |
|
@jpkrohling FYI, I manually set up a sidecar for our jaeger (we have a few customized the deployment a bit) with https://github.com/oauth2-proxy/oauth2-proxy/ connected to keycloak (using keycloak-operator + patches, pending review..) Here is the kustomize.yaml patch i used for a POC: apiVersion: apps/v1
kind: Deployment
metadata:
name: jaeger-query
spec:
template:
spec:
containers:
- name: oauth-proxy
image: quay.io/oauth2-proxy/oauth2-proxy:v6.1.1
args:
- --cookie-secret=AAAAAAAAAAAAAAAA # has different length requirements, 16/32 chars, compared to sidecar in openshift now
- --tls-cert-file=/etc/tls/private/tls.crt # please note "-file" difference compared to openshift sidecar
- --tls-key-file=/etc/tls/private/tls.key
- --provider-ca-file=/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem # hack, I injected this using a configmap w/ config.openshift.io/inject-trusted-cabundle: "true". Just note that this is not a RHEL based image
- --https-address=:8443
- --upstream=http://localhost:16686
- --provider=keycloak
- --email-domain=$DOMAIN
- --client-id=jaeger-ci
- --client-secret=...hard-coded-for-testing..
- --login-url=https://keycloak.internal/auth/realms/$REALM/protocol/openid-connect/auth
- --redeem-url=https://keycloak.internal/auth/realms/$REALM/protocol/openid-connect/token
- --validate-url=https://keycloak.internal/auth/realms/$REALM/protocol/openid-connect/userinfo
# - --keycloak-group=<user_group>One issue is that the KeycloakClient creates a client secret object, which must be mounted here, but I don't think the keycloak operator looks for KeycloakClients in other namespaces than where keycloak-operator is installed. So there is a dependency to a secret in another namespace. Unless I missed a flag for making it look in all namespaces, I hope so! :) |
Enable and configure a sidecar with arguments by the Jaeger crd and
the service will be redirected.
The UI of the query component has no authentication measure.
Resolves: #1104
Signed-off-by: Bastian Schlegel [email protected]