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

Chart: optional sidecar containers for dashboard in chart #5125

Closed
pierluigilenoci opened this issue May 15, 2020 · 8 comments
Closed

Chart: optional sidecar containers for dashboard in chart #5125

pierluigilenoci opened this issue May 15, 2020 · 8 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@pierluigilenoci
Copy link
Contributor

What would you like to be added

I want to have the option to specify a sidecar container as a configurable parameter in the chart

Why is this needed

Many time I need to deploy the dashboard with a sidecar container (ex: an SSO/auth proxy)

Comments

Please take a look at the Prometheus Helm chart for reference.

I had already opened the ticket in September 2019 but it was rejected #4355.

I had promised to try to do it but I did not yet have time. I move the feature request here as it is now on this side that the chart will be kept. If anyone wants to bother to do it, they are welcome. Otherwise, when I have time I'll do it.

@pierluigilenoci pierluigilenoci added the kind/feature Categorizes issue or PR as related to a new feature. label May 15, 2020
@floreks
Copy link
Member

floreks commented May 17, 2020

I don't see how we could add something generic enough to support all possible sidecar containers. Some might require external storage, other some service or ingress with custom configuration, a config map, or a secret. There is no way to simply add a chart with optional generic container to support many options. This sounds like something tailored just for your configuration.

@pierluigilenoci
Copy link
Contributor Author

Obviously, a generic solution will not be possible to obtain immediately but it will also be necessary to start somewhere. Leaving out something (for example "external storage") a simple enough generic solution is reachable.

@floreks
Copy link
Member

floreks commented May 18, 2020

I don't think this is a good approach. Some people might prefer using a separate deployment instead of sidecar containers. Preparing similar generic optional deployment for them would also be out of scope for us. There are just too many possible configurations to try and cover all of them in a single chart deployment. We don't want to have a configuration that is so tightly coupled with something. It's better to use another chart to deploy extra software or prepare your own custom version that extends our base chart.

@Eddman
Copy link
Contributor

Eddman commented May 18, 2020

Somehow I agree with @floreks. 99% of things can be solved via separate deployments....

In your case auth proxy should be done via Ingress external auth. If you have Dashboard configured correctly then accessing it via different channel is rejected, cause token is required. Note that it is always wrong to have cluster-admin assigned to Dashboard in production use case.

A correct setup would be

  1. Do not grant any privileges to Dashboard
  2. Configure Kubernetes API server to consume auth tokens.
  3. Configure Ingress to use auth proxy before calling your Dashboard with following annotations (just an example):
    # Configure to call our OIDC proxy running on the same hostname
    nginx.ingress.kubernetes.io/auth-url: "https://$host/my_oidc_proxy/auth"
    nginx.ingress.kubernetes.io/auth-signin: "https://$host/my_oidc_proxy/start?redirect_uri=https://$host$request_uri$is_args$args" # URL will be different for different proxies
    
    # Send token to Dashboard
    nginx.ingress.kubernetes.io/configuration-snippet: |
      # Proxy Authentication header to Dashboard
      # adds authorization header for kubernetes-dashboard
      auth_request_set $token $upstream_http_authorization;
      proxy_set_header Authorization $token;
    

@Eddman
Copy link
Contributor

Eddman commented May 18, 2020

Also described here - #5105. And many other issues.

@pierluigilenoci
Copy link
Contributor Author

Thank you @Eddman, I will try.

@floreks
Copy link
Member

floreks commented May 28, 2020

Closing this as it is out of our scope

/close

@k8s-ci-robot
Copy link
Contributor

@floreks: Closing this issue.

In response to this:

Closing this as it is out of our scope

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants