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

Refactor existing_arrikto into a community supported config #599

Closed
yanniszark opened this issue Nov 1, 2019 · 18 comments
Closed

Refactor existing_arrikto into a community supported config #599

yanniszark opened this issue Nov 1, 2019 · 18 comments

Comments

@yanniszark
Copy link
Contributor

There seems to be a community agreement that Kubeflow needs a config in the lines of
existing_arrikto.
Since our intention was for it to be a community config from the start, we refactor it into a community-supported config.

The proposed new name is kfctl_istio_dex.yaml.

What is included:

  • Kubeflow components: the intention is for this config to be a batteries-included config, with all the Kubeflow components (Notebooks, Pipelines, TFJobs, etc.).
  • Service Mesh: Istio is used and installed by default.
  • Auth: OIDC with Dex as the OIDC Provider and the oidc-authservice as the OIDC Client.
  • Exposing Kubeflow: Kubeflow is exposed on HTTPS, with a LoadBalancer with a self-signed cert by default.
  • Restrictions: Must not depend on any cloud-specific feature. This is targeted at existing, on-prem clusters with no access to cloud-specific services.

/cc @krishnadurai @ddutta @jlewi @johnugeorge @vkoukis @cvenets

@krishnadurai
Copy link
Contributor

Thanks, @yanniszark for filing this issue. Let me link @jlewi's comment which summarizes this:

Currently kfctl_k8s_istio.yaml is a configuration that works for everyone but is not identity aware and doesn't have a good story for how you access it from outside the cluster (kubectl port-forward works but isn't great).

Is the proposal to change that to

  • Exposing the istio ingress gateway on an IP address (which could be either internal or public depending on the "Cloud" Provider's implementation of service type loadbalancer and possibly additional service annotations)
  • Always including DEX for auth
    • Defaulting to basic auth with username and password set by environment variables
  • Using a self sign certificate to do https termination

Adding advanced options to

  • Use LetsEncrypt to generate a signed certificate and/or upload a signed certificate
    • I don't think we want to use LetsEncrypt by default because it requires an external DNS name and public ip
    • LetsEncrypt ACME challenge is also slow
  • Specify a DOMAIN that would be registered in some external registrar
    • I don't think we want to require this by default because that requires users to perform another manual step in a cloud/infra dependent way

Originally posted by @jlewi in #560 (comment)

@krishnadurai
Copy link
Contributor

It follows from here that we would stop supporting kfctl_k8s_istio.yaml and replace it with kfctl_k8s_istio_dex.yaml

@jlewi
Copy link
Contributor

jlewi commented Nov 1, 2019

Thanks @yanniszark; I'd like to propose one additional

  • Basic Auth use Dex basic auth by default

    • kfctl should look for environment variables KUBEFLOW_USERNAME and KUBEFLOW_PASSWORD and use them to configure basic auth

    • This is what GCP basic auth does today so we could refactor that code into a generic kfctl plugin

The motivation is as follows

  • On cloud providers a Service of type load balancer creates a public IP address so we need some form of Auth
  • Using an OIDC provider (e.g. GitHub, Google, Microsoft etc...) requires the user to create an OAuthClient with the identity provider
  • I think its better if to get started we don't have dependencies on external systems

@krishnadurai
Copy link
Contributor

krishnadurai commented Nov 1, 2019

One important assumption we need to note is that with 0.7 release the identity at Kubeflow gateway and Kubernetes' API are assumed to be the same for a user.
The reference architecture needs to support the same Identity at both ends.

@krishnadurai
Copy link
Contributor

krishnadurai commented Nov 14, 2019

We've thought of introducing Istio 1.3.1 with this config.
Add E2E tests for Istio 1.3.1: kubeflow/kubeflow#4480
Upgrade kfctl_istio_dex kfdef to use Istio 1.3.1 and update tests: kubeflow/kubeflow#4501

@jlewi
Copy link
Contributor

jlewi commented Jan 21, 2020

@krishnadurai and @yanniszark any update on this ? What is the remaining work that needs to be done for 1.0?

@krishnadurai
Copy link
Contributor

@jlewi Here's the status:

Overall, I'm looking to complete the implementation by sometime next week.

@jlewi
Copy link
Contributor

jlewi commented Jan 27, 2020

Thanks @krishnadurai; how's this coming?

We've cut the 1.0-branch so changes will need to be pulled into that branch either via cherry-picks or rebasing the branch

@jlewi
Copy link
Contributor

jlewi commented Feb 4, 2020

@krishnadurai and @yanniszark can we close this out or is there more work to be done?

@yanniszark
Copy link
Contributor Author

@jlewi I believe @krishnadurai wanted for the Dex plugin to make it into 1.0.
I'm not sure if that is realistic given that kubeflow/kfctl#214 is not implemented yet.

In addition, there is this issue for updating the docs: kubeflow/website#1592

@acesir
Copy link

acesir commented Feb 9, 2020

I am closely looking at this issue as it is something we need in our AWS environment, particularly referring to the option of passing the DOMAIN envar in order to generate a valid cert based on the domain/load-balancer exposure with the deployment. Does PR #600 actually implement the functionality described here or is there further work to be done on Dex @krishnadurai

@krishnadurai
Copy link
Contributor

@acesir

* Dex Plugin implementation needs to be updated with Unit Tests. [kubeflow/kfctl#130](https://github.com/kubeflow/kfctl/pull/130)

* Manifests for issuing certificates for 'DOMAIN' or LoadBalancer IP needs to be implemented.

Both of the above tasks will look to provide 'DOMAIN' environment variable. Expect this to be completed within a few weeks. We are not looking for this to be a part of 1.0.

@jlewi
Copy link
Contributor

jlewi commented Feb 11, 2020

@krishnadurai and @yanniszark any update on this? How close are we to closing this?

@jlewi
Copy link
Contributor

jlewi commented Feb 14, 2020

@krishnadurai and @yanniszark ping?

@krishnadurai
Copy link
Contributor

krishnadurai commented Feb 14, 2020

@jlewi
For 1.0 we should be able to release this config as-is and without the Dex Plugin. We need to get the docs changed to reflect the new name according to this issue:

kubeflow/website#1592

I'll send in a PR for this.

@jlewi
Copy link
Contributor

jlewi commented Feb 14, 2020

Thanks @krishnadurai so it sounds like we can close this as soon as the docs are submitted.

@krishnadurai
Copy link
Contributor

/close

We'll create a separate issue for Dex plugin implementation.

@k8s-ci-robot
Copy link
Contributor

@krishnadurai: Closing this issue.

In response to this:

/close

We'll create a separate issue for Dex plugin implementation.

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
Projects
None yet
Development

No branches or pull requests

5 participants