Clarify Ingress awaiter success condition and improve error message #292
Conversation
pkg/await/extensions_ingress.go
Outdated
| @@ -327,7 +327,9 @@ func (iia *ingressInitAwaiter) errorMessages() []string { | |||
|
|
|||
| if !iia.ingressReady { | |||
| messages = append(messages, | |||
| "Ingress was not allocated an IP address; does your cloud provider support this?") | |||
| "Ingress .status.loadBalancer field was not updated with a hostname/IP address; "+ | |||
| "If you're using the Traefik ingress-controller, make sure it is configured with the "+ | |||
There was a problem hiding this comment.
While helpful, I'm not sure we should be guessing about specific causes like this unless we think those are 90% cases. Can we generalize this at all to make it suggestive of the same root cause and fix, but while applying a larger class of causes? Or should we start maintaining a Kubernetes provider troubleshooting docs page where we can include this kind of helpful text in longer form and link to it from the error messages here?
There was a problem hiding this comment.
Curious @hausdorff thoughts on this as well.
There was a problem hiding this comment.
For example - could we add something like "Ensure that your ingress controller is configured to update the status section of ingress objects"? (Inspired by the text at https://docs.traefik.io/configuration/backends/kubernetes/)
There was a problem hiding this comment.
Yeah, that's reasonable. One of the previous platforms I worked on would put a link in the error message to a knowledge base page we maintained. That way you could still get detailed troubleshooting info like that, but it was easier to maintain.
There was a problem hiding this comment.
Yeah - we have https://pulumi.io/reference/troubleshooting.html which is used for this purpose by the CLI for core CLI errors. We could extend that to have sections for other providers to add error message help as well, or could create dedicated troubleshooting pages under the /kubernetes section.
There was a problem hiding this comment.
It sounds like we all agree about what to do here -- but to make it explicit, my preference is also to make error messages like this one follow a pattern:
- Factual explanation of what is wrong (i.e., ingress controller not configured to emit status)
- Link to a docs/playbooks/whatever
There are many reasons why I think this is a useful pattern -- lets us evolve recommended mitigations gradually and completely independently as we mature; not tractable to explain all mitigations in the error message; docs site experience is going to be way better for most complex issues; etc.
|
I've opened pulumi/docs#672 and am in the process of registering a URL shortening service for |
|
PR to merge doc changes to the prod website: pulumi/docs#685 |
|
Here's the updated error: |
|
Ok, this is ready to go. Here's the working shortlink: pulumi.io/help/k8s-ingress-lbstatus |
There was a problem hiding this comment.
One minor thing, in other parts of the CLI we're not putting < and > around the URLs. I propose to pick one form and use it everywhere, with a slight preference for the way we're doing it now, because it makes it easier to ctrl-click.
But if there are compelling reasons to do it your way I don't think that's a tragedy.
|
@hausdorff Ah, I didn't realize we were already doing this elsewhere. I'll update to look more like https://github.com/pulumi/pulumi-service/blob/507e45bf80b8b6e0b744cc34e446ea04eb703430/cmd/service/services/cloud/managed_backend_updates.go#L114 FWIW, ctrl-click does work with the angle brackets in iterm2. |
|
Opened pulumi/docs#689 and will push an updated link here in a few mins. |
lblackstone
force-pushed
the
lblackstone/ingress-wait-message
branch
from
2a27cec
to
861dc8a
Compare
|
|
Fixes #290
Here's what it looks like after the fix:
