-
Notifications
You must be signed in to change notification settings - Fork 24
Fix service mesh auto config after migration to gRPC #756
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
Conversation
|
Hey team, this change should be invisible, but it's probably worth mentioning in release notes, where do you recommend I mention this? Do you figure any additional testing is needed? |
|
I think the best place we have for notes about changes specific to deployment types are these old pages: https://sourcegraph.com/docs/admin/updates/kubernetes#notes-2 Pretty good testing imo, although I wouldn't mind if @keegancsmith laid eyes upon this change |
keegancsmith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please announce it somewhere. It could actually break if someone, but I think in practice that is so tiny that we should rather fix it like you are doing. Thanks!
|
|
> It could actually break if someone, but I think in practice
> that is so tiny that we should rather fix it like you are
> doing.
@keegancsmith I think I'm missing something after "if someone,"
am I?
@marcleblanc2 if someone targetted the names / configured istio
(or something else) based on the names? I think that is possible
but unlikely / easy to fix for the admin.
|
Noted, thank you! Deep Search provided similar feedback, not likely, but some self-hosted customers may require manual intervention as part of their upgrade:
I'll include these in an update to https://sourcegraph.com/docs/admin/updates/kubernetes#notes-2 |
|
Hey @DaedalusG, I've applied the same fixes to the remaining services which the customer found broken, and to match, and have made some minor formatting adjustments for readability. I've re-requested your review in case you'd like to. |
|
PR for doc update here: sourcegraph/docs#1407 |
Customer was blocked on an issue with our Helm chart + Istio
Istio uses the first part of a port name, and service definitions, to configure its service mesh sidecar proxy containers, based on the expected traffic type; docs: https://istio.io/latest/docs/ops/configuration/traffic-management/protocol-selection/#explicit-protocol-selection
The
http-prefix in the port name wasn't updated when we switched to gRPC, so Istio was auto-configuring for http, and failing to send the gRPC trafficThere was an old
unusedport in the gitserver service definition, which I couldn't find any history for, it's been there for 7+ years, so I added gRPC port 3178 in its placeMaking these updates automagically fixes Istio issues, no longer requiring the Envoy patch in charts/sourcegraph/examples/envoy
Helpful background on Istio here https://istio.io/latest/docs/ops/deployment/architecture/
Checklist
This change should be invisible, except for customers using the Envoy patch for gitserver, so it's worth mentioning in release notes, where do you recommend I mention this?
Test plan
Developed and tested with a customer, on their self-hosted instance with Istio
Tested on a k3s instance:
Tested on an AWS EKS instance