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

Add an ingress uid label to tunnels #293

Merged
merged 1 commit into from
Aug 23, 2023
Merged

Conversation

jrobsonchase
Copy link
Contributor

Resolves #291

What

Add a k8s.ngrok.com/ingress-uid label to tunnels created by the
ingress controller to uniquely identify them among multiple ingress
controller instances.

How

Plumb the full ingress object down to the method to create ngrok tunnel
lables, and add the UID from that. The ingress UID should be stable as
long as the object exists in k8s (citation needed?), so we don't have to
worry about unexpected recreate thrashing as a result of it changing.

Breaking Changes

Shouldn't be any. After the ingress controller is redeployed, a
difference should be detected between the desired and actual tunnel
state, and things should be recreated with the new labels as expected.

@jrobsonchase jrobsonchase requested a review from a team as a code owner August 18, 2023 14:51
@jrobsonchase
Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions github-actions bot added area/controller Issues dealing with the controller area/helm-chart Issues dealing with the helm chart labels Aug 18, 2023
Copy link
Contributor

@bobzilladev bobzilladev left a comment

Choose a reason for hiding this comment

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

Hopefully r.statusID wasn't actually ever nil there.

Copy link
Contributor

@euank euank left a comment

Choose a reason for hiding this comment

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

Just so we're aware, this does have a functional difference that could matter for limits.

To explain what I mean, let's look at the following scenario:
You have one service (my-app) listening on port 80, and you have 5 ingresses for it (ing1.myapp.ngrok.io through ing5.myapp.ngrok.io).
Before, that would result in 1 tunnel with the labels namespace: default, service: my-app, port: 80).
After this change, since there's N different ingresses, it's now 5 tunnels, which could get you closer to your session tunnel limit.

An alternative to this would be using the namespace UID, or using the service UID, both of which would be shared in more cases.
I feel like the service UID matches most closely with the existing intent of the labels.

All that said, I think this is probably fine, and like longer term we probably need to rethink this a bit regardless (i.e. the coupling of service and tunnel is a slightly off abstraction, and really we need to be pulling the EdgeID out of the tunnel header we get sent by the server).
I think the service UID probably fits a little better, but I could also be missing something there

@jrobsonchase
Copy link
Contributor Author

The ingress UID maps most cleanly to our current tunnel ownership model. We're also setting the tunnel owner as the ingress, so I don't think it was ever the intended behavior for a tunnel to be usable by multiple ingresses that just so happened to have the same labels.

@bobzilladev Well... it was, and I was getting panics 😅 Didn't dig much deeper into the underlying cause though.

@jrobsonchase jrobsonchase merged commit 94bfa69 into main Aug 23, 2023
@jrobsonchase jrobsonchase deleted the josh/ingress-uid-label branch August 23, 2023 14:19
@nikolay-ngrok
Copy link
Contributor

This actually breaks the case where we have multiple ingresses in the same namespaces, defining routes leading to the same service. This is because we are creating multiple https edge routes, but a single tunnel, so the last one that marks the tunnel wins, leaving the rest unroutable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Issues dealing with the controller area/helm-chart Issues dealing with the helm chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tunnel mix-up with multiple ngrok Instances
4 participants