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

[KubeIngressProxy] Add KubeIngressProxy.ingress_specifications #667

Merged
merged 2 commits into from
Nov 11, 2022

Conversation

dolfinus
Copy link
Contributor

@dolfinus dolfinus commented Nov 8, 2022

This allows to add host field to ingress rules, and also set tls field in ingress specification. Alternative implementation of #406, should fix #404.

Next step of dividing #648 into smaller pieces

@dolfinus dolfinus force-pushed the ingress_host branch 3 times, most recently from e986ede to 6d6b21d Compare November 8, 2022 11:13

rules = [
V1IngressRule(
host=host or spec.get("host"),
Copy link
Member

Choose a reason for hiding this comment

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

Ah now I see this involved a reference to host.

Should this be spec.get("host") or host instead though? I'm not sure at all, just wanted to make sure that at least one of us is sure about what makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this is currently weird, considering how spec["host"] is referenced in the tls section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is complicated.

From the one hand, routespec can contain hostname only if Jupyterhub.subdomain_host is not empty. If Jupyterhub.subdomain_host="example.com" then Hub is running on example.com, services on services.example.com and servers on {username}.example.com/{server}, and all these domains are set in the routespec.

From another hand, Ingress tls and host can contain wildcards:
https://kubernetes.io/docs/concepts/services-networking/ingress/#hostname-wildcards

I'm not sure which option has higher priority

Copy link
Member

Choose a reason for hiding this comment

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

Since this only affects the KubeIngressProxy part etc, I figure we go with what you think makes sense here - but maybe you could add a comment about this as this is apparently quite tricky?

Copy link
Member

Choose a reason for hiding this comment

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

Overall, this PR LGTM, but this makes me uncertain.

I don't overview when host is None, or if there are implicit assumptions about what ingress_specification can and can't be, such as setting just tlsSecret and not the host etc, and if passing host=None is acceptable or not here.

If a comment clarifies it, even though its a comment indicating that we are not sure, that is a clear improvement on managing this complexity I think.

Copy link
Contributor Author

@dolfinus dolfinus Nov 10, 2022

Choose a reason for hiding this comment

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

IMHO, the best way here is to merge host from routespec with items from ingress_specifications to cover all the cases. But I'm still not sure about the way of merging these values.

For example, routespec=https://my.example.com/me/service and ingress_specifications=[{"host": "*.example.com", "tlsSecretName": "some-secret"}]. Do we need to create:

spec:
  rules:
    - host: my.example.com
      path: /me/service
    - host: *.example.com
      path: /me/service
  tls:
    hosts:
      - *.example.com
    secretName: my-secret

or

spec:
  rules:
    - host: *.example.com
      path: /me/service
  tls:
    hosts:
      - *.example.com
    secretName: my-secret

or

spec:
  rules:
    - host: my.example.com
      path: /me/service
  tls:
    hosts:
      - *.example.com
    secretName: my-secret

?

Copy link
Contributor Author

@dolfinus dolfinus Nov 10, 2022

Choose a reason for hiding this comment

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

Options 1 and 2 can lead to the following issue.

Example:

c.Jupyterhub.subdomain_host="example.com"
c.KubeIngressProxy.ingress_specifications = [{"host": "*.example.com", "tlsSecretName": "secret-name"}]

For user1 and user2 Jupyterhub will create routespec="https://user1.example.com/default-server" and routespec="https://user2.example.com/default-server" respectively.

But then both ingress rules will look like:

spec:
  rules:
  - host: *.example.com
    path: /default-server
  tls:
    hosts:
    - *.example.com
    secretName: secret-name

and path resolution will be ambiguous.

So I've implemented option 3 which produces rules like:

spec:
  rules:
  - host: user1.example.com
    path: /default-server
  tls:
    hosts:
    - *.example.com
    secretName: secret-name

and added some tests covering these cases.

tests/test_objects.py Outdated Show resolved Hide resolved
@consideRatio consideRatio added KubeIngressProxy KubeIngressProxy is a separate class from KubeSpawner! new labels Nov 9, 2022
@dolfinus dolfinus marked this pull request as draft November 10, 2022 15:38
@dolfinus dolfinus force-pushed the ingress_host branch 14 times, most recently from 503e4d4 to 75ffefd Compare November 10, 2022 17:43
@dolfinus dolfinus marked this pull request as ready for review November 10, 2022 17:54
@consideRatio consideRatio merged commit 0f22abf into jupyterhub:main Nov 11, 2022
@consideRatio
Copy link
Member

❤️ 🎉 🌻 thanks @dolfinus for helping me understand this better and providing comments so that maintenance in the future is simplified! This LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
KubeIngressProxy KubeIngressProxy is a separate class from KubeSpawner! new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ingress rules w/o host wont match
3 participants