tempo-operator icon indicating copy to clipboard operation
tempo-operator copied to clipboard

[WIP] Protect query frontend using oauth proxy

Open rubenvp8510 opened this issue 1 year ago • 6 comments

rubenvp8510 avatar Jul 31 '24 00:07 rubenvp8510

Codecov Report

Attention: Patch coverage is 74.32950% with 67 lines in your changes missing coverage. Please review.

Project coverage is 73.06%. Comparing base (8008b1f) to head (6357fe1).

Files Patch % Lines
internal/manifests/monolithic/build.go 4.16% 45 Missing and 1 partial :warning:
internal/manifests/config/build.go 58.33% 3 Missing and 2 partials :warning:
internal/manifests/manifestutils/probes.go 0.00% 4 Missing :warning:
internal/manifests/oauthproxy/oauth_proxy.go 94.11% 3 Missing and 1 partial :warning:
internal/manifests/queryfrontend/query_frontend.go 94.44% 2 Missing and 2 partials :warning:
internal/manifests/manifestutils/volumes.go 60.00% 1 Missing and 1 partial :warning:
internal/manifests/manifestutils/containers.go 80.00% 1 Missing :warning:
internal/manifests/monolithic/configmap.go 0.00% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
- Coverage   73.16%   73.06%   -0.10%     
==========================================
  Files         106      109       +3     
  Lines        6606     6735     +129     
==========================================
+ Hits         4833     4921      +88     
- Misses       1480     1516      +36     
- Partials      293      298       +5     
Flag Coverage Δ
unittests 73.06% <74.32%> (-0.10%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 31 '24 01:07 codecov-commenter

@rubenvp8510 it would be great to provide some high level overview with notable changes in the PR description. E.g. two oauth-proxies are used in the query-frontend etc. Are they enabled by defalt?

pavolloffay avatar Jul 31 '24 09:07 pavolloffay

@rubenvp8510 there seems to be inconsistent behaviour for oauth-proxy for jaeger and tempo.

The proxy is enabled for Tempo always, however for jaeger only when the route is enabled:

kubectl apply -f - <<EOF
apiVersion: tempo.grafana.com/v1alpha1
kind: TempoStack
metadata:
  name: simplest
spec:
  storage:
    secret:
      name: minio-test
      type: s3
  storageSize: 1Gi
  template:
    queryFrontend:
      jaegerQuery:
        enabled: true
        ingress:
          type: 
EOF

Deploys oauth-proxy for Tempo, but not for Jaeger. I think we should be consistent and deploy the proxy for Jaeger and Tempo when the same configuration is applied. e.g. if we defaulting to enable proxy automatically we should do it for both components.

pavolloffay avatar Jul 31 '24 09:07 pavolloffay

@rubenvp8510 did you try configuring multiple upstreams https://github.com/openshift/oauth-proxy?tab=readme-ov-file#upstream-configuration? This way we can deploy a single proxy for both jaeger and tempo.

Maybe we can be explicit for Jaeger and set the upstream to http://127.0.0.1:8080/foo/ and forward everything else to Tempo.

pavolloffay avatar Jul 31 '24 09:07 pavolloffay

Can you check if the query frontend listens to localhost only (I think the tempo config must be updated)? Otherwise it'll be still accessible.

You can check the open ports of a pod with

# On the host, find the PID of the application running inside the container
ps ax | grep my-application

# List open TCP ports
sudo nsenter -t <pid> -n ss -lntp

if you use KinD or Minikube.

Ohh you're right! is still listening on all the interfaces! Thanks, I'll check this part.

rubenvp8510 avatar Jul 31 '24 14:07 rubenvp8510

@rubenvp8510 there seems to be inconsistent behaviour for oauth-proxy for jaeger and tempo.

The proxy is enabled for Tempo always, however for jaeger only when the route is enabled:

kubectl apply -f - <<EOF
apiVersion: tempo.grafana.com/v1alpha1
kind: TempoStack
metadata:
  name: simplest
spec:
  storage:
    secret:
      name: minio-test
      type: s3
  storageSize: 1Gi
  template:
    queryFrontend:
      jaegerQuery:
        enabled: true
        ingress:
          type: 
EOF

Deploys oauth-proxy for Tempo, but not for Jaeger. I think we should be consistent and deploy the proxy for Jaeger and Tempo when the same configuration is applied. e.g. if we defaulting to enable proxy automatically we should do it for both components.

Yeah this was the behaviour previously, so I think some code of that is still there. I will fix this.

rubenvp8510 avatar Jul 31 '24 14:07 rubenvp8510