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

[WIP] Protect query frontend using oauth proxy #997

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

rubenvp8510
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2024

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 ⚠️
internal/manifests/config/build.go 58.33% 3 Missing and 2 partials ⚠️
internal/manifests/manifestutils/probes.go 0.00% 4 Missing ⚠️
internal/manifests/oauthproxy/oauth_proxy.go 94.11% 3 Missing and 1 partial ⚠️
internal/manifests/queryfrontend/query_frontend.go 94.44% 2 Missing and 2 partials ⚠️
internal/manifests/manifestutils/volumes.go 60.00% 1 Missing and 1 partial ⚠️
internal/manifests/manifestutils/containers.go 80.00% 1 Missing ⚠️
internal/manifests/monolithic/configmap.go 0.00% 0 Missing and 1 partial ⚠️
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%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Ruben Vargas <[email protected]>
@pavolloffay
Copy link
Collaborator

@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
Copy link
Collaborator

pavolloffay commented Jul 31, 2024

@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
Copy link
Collaborator

pavolloffay commented Jul 31, 2024

@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.

Copy link
Collaborator

@andreasgerstmayr andreasgerstmayr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rubenvp8510
Copy link
Collaborator Author

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
Copy link
Collaborator Author

@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.

Signed-off-by: Ruben Vargas <[email protected]>
@rubenvp8510 rubenvp8510 force-pushed the protect_query_frontend_oauthproxy branch from 3c7a6ad to 1eea353 Compare July 31, 2024 16:15
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