Skip to content

Experiment - revert envoy update to attempt to fix integration tests#619

Closed
dubious90 wants to merge 2 commits intoenvoyproxy:mainfrom
dubious90:fix-integration-tests
Closed

Experiment - revert envoy update to attempt to fix integration tests#619
dubious90 wants to merge 2 commits intoenvoyproxy:mainfrom
dubious90:fix-integration-tests

Conversation

@dubious90
Copy link
Copy Markdown
Contributor

Integration tests failing on main branch. Seeing if reverting the latest envoy update is the root cause.

dubious90 and others added 2 commits February 5, 2021 12:36
Pull latest into my fork
Signed-off-by: Nathan Perry <nbperry@google.com>
@oschaaf
Copy link
Copy Markdown
Member

oschaaf commented Feb 5, 2021

Hmm. is the tracking of https cipher usage broke (again)?
I think there's been a recent change in Envoy that could be related.
But then it is really weird that CI didn't catch this before we merged.

@oschaaf
Copy link
Copy Markdown
Member

oschaaf commented Feb 5, 2021

If my hunch is correct (which might very well be not!) the upstream changes that broke us are in these lines:
https://github.com/envoyproxy/envoy/blob/c16a033ddf81f5366cc0e590c67326e6696d5249/source/extensions/transport_sockets/tls/context_impl.cc#L329-L344

@dubious90
Copy link
Copy Markdown
Contributor Author

Hmm. is the tracking of https cipher usage broke (again)?
I think there's been a recent change in Envoy that could be related.
But then it is really weird that CI didn't catch this before we merged.

Tbh I haven't fully investigated yet. The Docker failure threw a small wrench in my investigation efforts. But the failures started shortly after I merged in the latest envoy update, so it was my first suspect.

Agree that it's weird that the CI didn't catch the problem. Let's see how the system operates after we merge in the docker fix

@mum4k
Copy link
Copy Markdown
Collaborator

mum4k commented Feb 6, 2021

Are we planning to merge this PR, or is it just an experiment to execute the CI? Asking, because merging it would prevent the next Envoy update that is already in review:
#621

If it is just an experiment, we could mark it as draft or say something like "DO NOT MERGE" in the PR description.

@mum4k
Copy link
Copy Markdown
Collaborator

mum4k commented Feb 6, 2021

Fyi, this is what fixed the last docker issue, this one looks related:

#610

@mum4k
Copy link
Copy Markdown
Collaborator

mum4k commented Feb 6, 2021

Sorry, reading emails in sequence - now I saw that we already merged a fix. Assuming we will be discarding this PR, however if merging it is necessary - let's discuss our options first.

@dubious90 dubious90 closed this Feb 8, 2021
@dubious90 dubious90 deleted the fix-integration-tests branch April 26, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants