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

Support an alternative proxy implementation: KubeIngressProxy with an external k8s Ingress controller #1673

Closed
wants to merge 69 commits into from

Conversation

remche
Copy link
Contributor

@remche remche commented May 18, 2020

The goal of this PR is to enable the use of native k8s ingress controller as described in #1642.

Changes :

  1. Adds a way to skip proxy deployment :

    • adds proxy.chp.enabled value (default to true)
    • templates files to avoid deployment of any proxy ressources
    • pass value to hub configmap to allow hub configuration in jupyterhub_config.py
  2. Patch the Role manifest when externalIngressController is enabled to allow (get, watch, list, create, delete, patch) (endpoints, services, ingresses).

@remche remche changed the title [WIP] optional chp deployment and externalIngressController option Optional chp deployment and externalIngressController option May 19, 2020
@remche
Copy link
Contributor Author

remche commented May 20, 2020

Here is a minimal config file for testing :

hub:
  image:
    name: remche/k8s-hub
    tag: ingress-extv1b1
  redirectToServer: false
  extraConfig:
    ingress_proxy.py: |
      c.JupyterHub.proxy_class = 'kubespawner.proxy.KubeIngressProxy'
      c.KubeIngressProxy.should_start = False
proxy:
  externalIngressController: true
  chp:
    enabled: false

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR! I haven't had time to try it yet, but what are your impressions of using it so far? Are you planning to use it in production?

proxy.externalIngressController modifies the hub RBAC role but based on your configuration you still need to manually set c.JupyterHub.proxy_class = 'kubespawner.proxy.KubeIngressProxy'. Can you explain why you made this choice?

There's some ongoing work to refactor the C2JH CI system #1664
Do you have any thoughts on adding a test scenario for externalIngressController after the CI work is completed?

{{- if .Values.proxy.externalIngressController }}
- apiGroups: [""] # "" indicates the core API group
resources: ["endpoints", "services"]
verbs: ["get", "watch", "list", "create", "delete", "patch"]
Copy link
Member

@manics manics May 30, 2020

Choose a reason for hiding this comment

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

To help with review could you say whether you figured these out by going through the K8s API calls, or is it a guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additive hardworking way. I added permission according to hub error logs.

@remche
Copy link
Contributor Author

remche commented Jun 1, 2020

Thanks for opening this PR! I haven't had time to try it yet, but what are your impressions of using it so far? Are you planning to use it in production?

I used it on test deployment with both traefik and nginx ingress controller with no pb so far. We would use it in production if merged.

proxy.externalIngressController modifies the hub RBAC role but based on your configuration you still need to manually set c.JupyterHub.proxy_class = 'kubespawner.proxy.KubeIngressProxy'. Can you explain why you made this choice?

That's indeed a good question ;)
It was my testing setup, but you're right I should've put this in jupyterhub_config.py !
The other question is, should we keep the proxy.chp.enabled.

There's some ongoing work to refactor the C2JH CI system #1664
Do you have any thoughts on adding a test scenario for externalIngressController after the CI work is completed?

I need a deeper look but my first feeling is that I can add a test scenario for that case.

@remche
Copy link
Contributor Author

remche commented Jun 26, 2020

I simplified logic as @manics suggested, relying only on proxy.externalIngressController value. New minimal config file :

hub:
  image:
    name: remche/k8s-hub
    tag: ingress-extv1b1
proxy:
  externalIngressController: true

@consideRatio consideRatio changed the title Optional chp deployment and externalIngressController option Support an alternative proxy implementation: KubeIngressProxy with an external k8s Ingress controller Sep 30, 2020
@consideRatio
Copy link
Member

Thanks for thorough and interesting work @remche!

I'd like to see the Helm chart configuration option be more obvious, as the boolean externalIngressController doesn't convey much unless you have a very clear understanding about what it already does in my mind. By configuring proxy.type: chp to proxy.type: kip it becomes a bit clearer though that we are replacing one approach with another.

I'm not great a reviewing this PR without any experience of using the KubeIngressProxy myself, but I think this can be merged without too much consideration if there is agreement to maintain support for this. I don't have a clear idea of this, and would like to know if @yuvipanda who has worked on KubeIngressProxy thinks this is worthwhile to support in Z2JH. What do you think @yuvipanda?

My overview of the PR

This PRs goal is to let the JupyterHub Helm chart support using another proxy implementation than the default.

In the default proxy implementation, we run a configurable-http-proxy within the k8s deployment named proxy, and let it be controlled through the ConfigurableHTTPProxy class.

To use another proxy implementation, we update both:

  • c.JupyterHub.proxy_class to use another Proxy class, which is JupyterHub's interface to communicate it needs for certain network routes to be updated for the spawned user pods.
  • The k8s proxy Deployment resource and its associated resources that are no longer needed.

Overall, I think what is important for this PR to get merged is that we lock in sensible configuration. I'd also like to see documentation to help future maintainers maintain this as part of Z2JH without for example being active users of this proxy implementation themselves. I think this comment takes us a bit along the way, but it would be great to have a section that introduce this configuration option properly in the z2jh.jupyter.org documentation both for users and maintainers.

My technical considerations

  • Does KubeIngressProxy support adding JupyterHub services and such as well?
  • How does usage of KubeIngressProxy influence usage of autohttps? How can KubeIngressProxy handle that through changes to the Ingress resources etc?
  • Does this Helm chart need to pass through additional configuration to configure the KubeIngressProxy class?

@Id2ndR
Copy link

Id2ndR commented Feb 17, 2021

Thank you for this interesting work @remche. I tested it, and here are the remaining problems I found:

  • KubeIngressProxy spawn jupyter--2f-route ingress that route to the hub just like jupyterhub ingress (the last one is created by the z2jh chart when ingress.enabled is true). Using the chart ingress however still is necessary to add annotations (like cert-manager / ingress class ones).
  • KubeIngressProxy's spawned services and ingresses are not cleaned up when the chart is uninstalled
  • configuration of KubeIngressProxy is not possible yet

IMHO, this PR mainly depends of jupyterhub/kubespawner#406 and should be reworked after the latest is merged.

@consideRatio
Copy link
Member

@remche I'll opt to close this PR at this point as I perceive it as KubeIngressProxy isn't a well enough maintained project for z2jh to support integration against it with all the complexity that follows of testing, documenting, and adding to the general maintenance burden of the z2jh repo.

Thank you for your work on this nevertheless!

@remche
Copy link
Contributor Author

remche commented May 9, 2022

@consideRatio I agree and I'm really sorry not to have much time to spend on this.

@consideRatio
Copy link
Member

No worries at all, thank you for pioneering work on this in the open!!

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.

6 participants