Revert STS from 1.4 with cherry picks of integration and stress test fixes. #180
Conversation
This reverts commit 063eeb9.
This reverts commit 37dbbd4.
This reverts commit ab59731.
This reverts commit 02901d0.
This reverts commit 3eb2101.
…envoyproxy#9101)" This reverts commit ec6b907.
This reverts commit 03ecfad.
…o#162) Backport envoyproxy/envoy-wasm#422 and its prerequisite (envoyproxy#10009). * Plumb the flaky flag from envoy_cc_test to the native.cc_test (envoyproxy#10009) Signed-off-by: Yan Avlasov <yavlasov@google.com> * ci: mark //test/integration:protocol_integration_test as flaky. (envoyproxy#422) Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: gargnupur <gargnupur@google.com>
Signed-off-by: gargnupur <gargnupur@google.com>
| std::unordered_map<uint32_t, std::unique_ptr<GrpcCallHandlerBase>> grpc_calls_; | ||
| std::unique_ptr<GrpcStreamHandlerBase> cur_grpc_stream_; |
There was a problem hiding this comment.
You shouldn't be reverting those changes, see: #175 (comment)
There was a problem hiding this comment.
But we didn't ship that in 1.4.6. I think instead we should get release-1.4 into a state that looks like the 1.4.6 release we just shipped, tag it, then reapply any fixes which can go out in 1.4.7
@howardjohn CC
There was a problem hiding this comment.
That's a valid point. OK, fair enough, no one should be using Wasm in 1.4 anyway...
cc @mandarjog @kyessenov @bianpengyuan for visibility.
There was a problem hiding this comment.
I disagree that we need to get release-1.4 branch into a state that looks like 1.4.6. We did not ship from 1.4 branch, as in reality we created a private 1.4.6 branch that was developed independently from 1.4 branch. Now that 1.4.6 branch is done, we should simply merge 1.4.6 branch into 1.4 branch (and that was exceptional due to secrecy around the patches).
|
I disagree that we need to get release-1.4 branch into a state that looks
like 1.4.6
I agree. We need only to get release-1.4 branch into a state of what we
want to get shipped in 1.4.7.
…On Wed, Mar 4, 2020 at 6:14 PM Kuat ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In api/wasm/cpp/proxy_wasm_api.h
<#180 (comment)>:
> std::unordered_map<uint32_t, std::unique_ptr<GrpcCallHandlerBase>> grpc_calls_;
- std::unique_ptr<GrpcStreamHandlerBase> cur_grpc_stream_;
I disagree that we need to get release-1.4 branch into a state that looks
like 1.4.6. We did not ship from 1.4 branch, as in reality we created a
private 1.4.6 branch that was developed independently from 1.4 branch. Now
that 1.4.6 branch is done, we should simply merge 1.4.6 branch into 1.4
branch (and that was exceptional due to secrecy around the patches).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#180?email_source=notifications&email_token=AAEYGXJY2VCPQSFEODJ7T3TRF4DGXA5CNFSM4LAWICCKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYBF7MY#discussion_r388043518>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXIKVCSGZSZ6X464O5DRF4DGXANCNFSM4LAWICCA>
.
|
|
OK, I can agree with that. And John's point is accurate - we just need to get it into a state for what we want shipped in 1.4.7. That means at this point that if there are pure bug fix SHAs they should stay in the branch. So @kyessenov @bianpengyuan send them in a PR or list the SHAs here and I'll get them back into the branch. But if there are features mixed in with the SHAs, then we are still having a discussion with the community about whether we can include them in 1.4.7 |
|
I think the relevant stake holders are the release managers for 1.4, who have already looked into those PRs (in master or other branches sometimes and cherry-picked). |
|
@kyessenov the issue is that these are features going into a patch release. I am within my rights to object to that. Also, I can't find a place where STS is called a google-specific extension. |
vendor specificity is an issue for OSs istio-proxy repo, but we can set that aside for now. The primary issue then is feature addition in 1.4.5 of Istio. Since this code has never been shipped in a OSS release we can let it all roll back. |
|
I get that features in a patch release is an issue, and I'm OK with pretending those changes never were approved. I just don't think we should be using the CVE release as the reason to revert those changes. It's making a bad precedent to have a bunch of changes reverted post-factum, and creates a lot of work figuring out which change corresponds to which feature. Clearly, we do want some important bug fixes in 1.4.7. If I were to patch 1.4 six months from now (like what I had to do for 1.2), I'd be enormously confused by what happened in the commit history. |
|
#185 for the bug fix reverted by this pr. |
This is an alternate to #175. It cherry picks some integration and stress test fixes from envoyproxy/envoy-wasm in addition to the STS reverts to try to get builds passing.