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

[pkg] add env var to dashboard deplyoment for kubernetes oidc auth #1465

Closed
wants to merge 1 commit into from

Conversation

dymart
Copy link

@dymart dymart commented Aug 2, 2022

What does this PR do?

Allow for non dex oidc auth providers in managed kubernetes deployments by setting env vars in che-dashboard deployment if present. Currently che-dashboard has no way to change these variables so no way to us non dex auth. With these changes it will be possible to set all of these env vars from CheCluster. Making it easier to use che with managed kubernetes.

Add CHE_INFRA_KUBERNETES_PORT, CHE_INFRA_KUBERNETES_PORT_443_TCP_ADDR, CHE_INFRA_KUBERNETES_PORT_443_TCP, CHE_INFRA_KUBERNETES_SERVICE_HOST to the che-dashboard deployment for kubernetes if the variables are set.

spec:
  components:
    cheServer:
      extraProperties:
        CHE_INFRA_KUBERNETES_PORT:
        CHE_INFRA_KUBERNETES_PORT_443_TCP_ADDR:
        CHE_INFRA_KUBERNETES_PORT_443_TCP: 
        CHE_INFRA_KUBERNETES_SERVICE_HOST:

This way non dex oidc auth providers can be used ( ex: gke with external auth). This significantly simplifies deployment and can make auth consistent and easy to use with managed kubernetes clusters where the kube api cannot be set or changed.

These are then used as env vars in the che-dashboard deployment for kubernetes. With changes to the dashboard in another pr (eclipse-che/che-dashboard#596 ), if these env vars are present in the deployment the dashboard will use them to set KUBERNETES_PORT, KUBERNETES_PORT_443_TCP_ADDR, KUBERNETES_PORT_443_TCP, KUBERNETES_SERVICE_HOST. Allowing for the configuration of a non dex oidc auth provider.

What issues does this PR fix or reference?

eclipse-che/che#21260

How to test this PR?

Test platform: managed kubernetes with non dex oidc auth.

cat EOF << > /tmp/patch.yaml
spec:
  components:
    cheServer:
      extraProperties:
        # needed but not added in this pr
        CHE_INFRA_KUBERNETES_MASTER__URL=https://gke-oidc-envoy.anthos-identity-service
        # added in this pr
        CHE_INFRA_KUBERNETES_PORT:tcp://<gke-oidc-envoy LB IP>:443
        CHE_INFRA_KUBERNETES_PORT_443_TCP_ADDR:<gke-oidc-envoy LB IP>
        CHE_INFRA_KUBERNETES_PORT_443_TCP:tcp://<gke-oidc-envoy LB IP>:443
        CHE_INFRA_KUBERNETES_SERVICE_HOST:<gke-oidc-envoy LB IP>
EOF

chectl server:deploy \
     --installer operator \
     --platform k8s \
     --che-operator-image <CUSTOM_OPERATOR_IMAGE> \
     --che-operator-cr-patch-yaml /tmp/patch.yaml

Docs pr: eclipse-che/che-docs#2413

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@openshift-ci
Copy link

openshift-ci bot commented Aug 2, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dymart

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link

openshift-ci bot commented Aug 2, 2022

Hi @dymart. Thanks for your PR.

I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@tolusha
Copy link
Contributor

tolusha commented Aug 2, 2022

@dymart
Thank you for contribution.
As far as I understand, in general you need a way to mount arbitrary environment variables into Eclipse Che containers.
If so, let me open a PR to provide more common way to do so.

@tolusha
Copy link
Contributor

tolusha commented Aug 2, 2022

@dymart
Copy link
Author

dymart commented Aug 3, 2022

Hi @tolusha,

The potential solution you provided would not solve the problem. Being able to configure the properties through the CheCluster CR would be the best solution.

Just to clarify the problem is specifically related to che-dashboard needing these environment variables to be set and used to be able to work with a kubernetes cluster that is managed by one of the large cloud providers ( as one cannot change the kube api there). It would be one of the last hurdles to allow che to run on one of the large cloud providers. ex: eclipse-che/che#21304

The issues you reference as similar seem to deal with configuring oauth proxy and allowing more configuration than is currently possible and while that would be very nice to have the two issue may be of different severity. The issue this pr resolves renders che unusable on kubernetes cluster that are managed by cloud providers like gke. Allowing this would enable others to install che on these could providers as well.

Please also consider that others would like to use this functionality in the mean time as a more general solution is being worked on. I understand that reworking this might not be a priority for the project currently and that this current solution may not be as general as you would like but it would be a simple and clean solution that could easily be removed when the project has time to implement a more general solution.

thanks

@tolusha
Copy link
Contributor

tolusha commented Aug 4, 2022

@dymart

Being able to configure the properties through the CheCluster CR would be the best solution.

Pls have a look at [1] and comment if needed. I've already started working on this issue and I will provide a PR soon.
[1] eclipse-che/che#21605

@tolusha
Copy link
Contributor

tolusha commented Aug 4, 2022

#1468

@dymart
Copy link
Author

dymart commented Aug 7, 2022

Great,

thank you @tolusha that pr looks great!

Really appreciate it, just didn't want this issue to get backlogged for a long time. Sorry if my last message came across poorly.

@dymart dymart closed this Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants