-
Notifications
You must be signed in to change notification settings - Fork 41
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
Updates to Envoy 1.17 and Istio 1.8 #177
Conversation
istio test (TestConnectsToMockPilotAsAGateway) on macOS flaked twice, so I'll look into it prior to merge |
As Envoy 1.16.3 doesn't pass istio (XDS) tests, I had to move this to latest Envoy 1.17.1 |
@@ -1,21 +0,0 @@ | |||
-----BEGIN CERTIFICATE----- |
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.
these files were dead (never read). when we get to doing cert tests, we can add relevant ones but don't need to store dead files meanwhile
// https://github.com/istio/istio/blob/1.8.4/istio.deps -> | ||
// https://github.com/istio/proxy/blob/4cc266a75a84435b26613da6df6c32b4a2df4f3e/WORKSPACE -> | ||
// https://github.com/istio/envoy/commit/5b0c5f7b21f84b6ab86e5f416bdaf6bb0fbc2a32 -> ~ 1.16 | ||
// However, 1.16 fails to complete startup when XDS is configured (it blocks on 503/PRE_INITIALIZING). |
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.
here's the note about test failure
@@ -102,6 +103,7 @@ func generateIstioConfig(e *envoy.Runtime) meshconfig.ProxyConfig { | |||
cfg.EnvoyAccessLogService = &meshconfig.RemoteService{Address: e.Config.ALSAddresss} | |||
// Required: Defaults to MUTUAL_TLS, but we don't configure auth, yet, so it has to be set to NONE | |||
cfg.ControlPlaneAuthPolicy = meshconfig.AuthenticationPolicy_NONE | |||
cfg.Tracing = nil // Prevent server from hanging on unavailable Zipkin cluster |
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.
there were two reasons why the istio test hung, one was some bug in envoy with XDS, the other was this. Envoy wouldn't become available because there was no zipkin host ready.
// First, ensure r.Config.XDSAddress ended up written to config as the discoveryAddress | ||
require.Contains(t, string(serverInfo), fmt.Sprintf(`"discoveryAddress": "%s"`, pilotGrpc)) | ||
|
||
// Next, ensure a connection discoveryAddress was made (Envoy -> Pilot's XDS) |
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.
While I can't commit to this now, you can probably see how this test could now be refactored into a unit test and an e2e test, where only the latter needs to run istio. We can mock the XDS service here in other words and doing so will massively clean up the dependency tree.
locally on OS/X I get a failure like this. I wonder if that means anything to anyone...
|
there's no more failure, because I changed the test config to not try to resolve. however, the panic probably should be fixed upstream |
Istio 1.7 is EOL. This migrates to floor versions implied in Istio 1.8. As Envoy 1.16.3 doesn't pass tests, this moves to latest Envoy 1.17.1 Signed-off-by: Adrian Cole <adrian@tetrate.io>
ff3cfe7
to
0e4652a
Compare
// Technically what happens is bootstrap results in a "xds-grpc" cluster pointed at Pilot's gRPC address. During | ||
// bootstrap, Envoy invokes "/envoy.service.discovery.v3.AggregatedDiscoveryService/StreamAggregatedResources", which | ||
// reads configuration in pilot (from testdata/configs.yaml). This test passes when that configuration merges with the | ||
// bootstrap configuration generated by Istio. |
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.
sounds like you are an Istio/Envoy expert 😄 Thanks for the comment here!
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.
Thank you!!
Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
…stio Signed-off-by: Adrian Cole <adrian@tetrate.io>
I moved the disable zipkin hack to test code so that the default template still works as advertised. Also, added a comment because it seems hard-coding might end in istio 1.10 istio/istio#31553 (comment) |
Given that we have only been supported Istio 1.2 before you worked on the refactoring work here, I am thinking that we could eliminate entire Istio stuff from GetEnvoy considering the maintenance work.. |
@mathetake I'm game for removal and will raise a PR post merge (since this works now, at least it will be in source history in case someone wants to try again). I agree this istio topic alone has cost days of effort not including weekend time, and it is likely not in use. |
sg |
#178 on removing istio |
Istio 1.7 is EOL. This migrates to floor versions implied in Istio 1.8.
As Envoy 1.16.3 doesn't pass tests, this moves to latest Envoy 1.17.1