Skip to content

Revert STS changes from 1.4 branch #175

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

Revert STS changes from 1.4 branch #175

wants to merge 8 commits into from

Conversation

duderino
Copy link

@duderino duderino commented Mar 3, 2020

This reverts all STS changes except for e0ecba3 which I'm still figuring out how to handle. Will address that in a subsequent PR.

duderino added 8 commits March 3, 2020 13:10
@duderino
Revert "fix opencensus tracer (istio#155)"
8d3fb7c 
This reverts commit 063eeb9.
@duderino
Revert "Add x-goog-user-proj header for sts credential (istio#152)"
7cf7f62 
This reverts commit 37dbbd4.
@duderino
Revert "Update GrpcService to add StsService. (envoyproxy#411)"
c665445 
This reverts commit ab59731.
@duderino
Revert "fix tracer ssl credential (istio#151)"
7a48ec8 
This reverts commit 02901d0.
@duderino
Revert "remove url validation as it is not implemented"
f0c8b8c 
This reverts commit 3eb2101.
@duderino
Revert "Use gRPC Security Token Service (STS) to get call credentials (…
4e1e29f 
…envoyproxy#9101)"

This reverts commit ec6b907.
@duderino
Revert "[release-1.4] Use sts for call credential when STS_PORT is pr…
6bdbf7a 
…ovided in node metadata istio#144 (istio#148)"

This reverts commit 7081e43.
@duderino
Revert "Upgrade gRPC to 1.25 which has gRPC STS feature (istio#145)"
bd68bad 
This reverts commit 03ecfad.
@duderino
Copy link
Author

duderino commented Mar 3, 2020 •
edited
Loading

And I think reverting e0ecba3 is actually unnecessary. It seems to be a merge of the prior commit ab59731 from jplev. Both ab59731 and e0ecba3 have the identical changes.

I did revert ab59731 and that removed the changes visible in both commits from the branch

@duderino
Copy link
Author

duderino commented Mar 3, 2020 •
edited
Loading

@JimmyCYJ @kyessenov @bianpengyuan @jplevyak heads up, we (@duderino @mandarjog @howardjohn and @PiotrSikora) want to remove all STS changes from OSS Istio release-1.4 (but not master and release-1.5) and keep them Google internal (we don't care that OSS sees them, but we think they are too much code to be introduced into an OSS patch release). So I reverting all STS commits made by you in this PR.

Once this is merged we will cherry pick in the commits from today's 1.4.6 security release.

I'll also do something similar for the istio/proxy repo.

@kyessenov
Copy link

Heads up that some changes are actually bug fixes (e.g. gRPC metadata is not working in 1.4).

@bianpengyuan
Copy link

bianpengyuan commented Mar 3, 2020 •
edited
Loading

This file contains a fix for CreatingInitialMetadata: 7cf7f62#diff-3367fe470b910c37aac0a6fd5856a936 (I should have separated it into a difference PR, but anyway..) I can send a PR with only that fix after this reverting is merged.

@duderino
Copy link
Author

duderino commented Mar 3, 2020

@bianpengyuan @kyessenov none of the commits I reverted were shipped in OSS Istio 1.4.6, so if you want them to go out in 1.4.7, please readd them later.

Once this is merged, then we want to cherrypick in the security fixes, tag it 1.4.6, and then you can send your PRs

@duderino
Copy link
Author

duderino commented Mar 3, 2020

/test test-tsan_envoy_release-1.4

@duderino
Copy link
Author

duderino commented Mar 3, 2020

/test test-asan_envoy_release-1.4

@duderino
Copy link
Author

duderino commented Mar 3, 2020

@PiotrSikora looks like the wasm stress test is still present in this branch and its complaining. I find that a bit surprising because it should be passing now that I've effectively rolled the branch back in time, but I could also just remove the stress test anyways. Thoughts?

@duderino
Copy link
Author

duderino commented Mar 3, 2020

I get it, I have reverted commits to effectively bring us back to #133 ... which was before istio/envoy ran automated tests. So I shouldn't expect these tests to pass ;)

@istio-testing
Copy link

@duderino: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
test-release_envoy_release-1.4 bd68bad link /test test-release_envoy_release-1.4
test-asan_envoy_release-1.4 bd68bad link /test test-asan_envoy_release-1.4
test-tsan_envoy_release-1.4 bd68bad link /test test-tsan_envoy_release-1.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@duderino duderino mentioned this pull request Mar 3, 2020
Merged
@duderino
Copy link
Author

duderino commented Mar 4, 2020

Closing this in favor of #180 which cherry picked additional fixes for the failing integration and stress tests

@duderino duderino closed this Mar 4, 2020
aruntony001 pushed a commit to vmware-allspark/envoy that referenced this pull request Jun 12, 2023
@alyssawilk @jpsim
build: remove alwayslink
72f7fa6 
Fixes istio#175
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JimmyCYJ JimmyCYJ Awaiting requested review from JimmyCYJ

@kyessenov kyessenov Awaiting requested review from kyessenov

@bianpengyuan bianpengyuan Awaiting requested review from bianpengyuan

@jplevyak jplevyak Awaiting requested review from jplevyak

@PiotrSikora PiotrSikora Awaiting requested review from PiotrSikora

Assignees
No one assigned
Labels
cla: yes size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@duderino @kyessenov @bianpengyuan @istio-testing @googlebot