Fixes istio/istio#12873. Add field Sidecar.OutboundTrafficPolicy to o… #887

robertpanzer · 2019-03-29T14:14:40Z

…verride outbound traffic policy per sidecar

This PR tries to solve https://github.com/istio/istio/12873.
For that it adds a field Sidecar.OutboundTrafficPolicy that is currently a copy of the same field in the Mesh configuration.

I actually wanted to reuse that type and move it to network/v1alpha3, but the build has another opinion on this and fails.
Would it work to move the type mesh/v1alpha1.MeshConfig_OutboundTrafficPolicy to network/v1alpha3/OutboundTrafficPolicy?
In my understanding this should result in the same Wire format, so that it should still work to run a newer Galley with an older Pilot or vice versa.
Are there any opinions on this?
And if it would be ok to move this, how can I do it? Currently make fails when moving this type.

googlebot · 2019-03-29T14:14:43Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

istio-testing · 2019-03-29T14:14:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: robertpanzer
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: kyessenov

If they are not already assigned, you can assign the PR to them by writing /assign @kyessenov in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

istio-testing · 2019-03-29T14:14:56Z

Hi @robertpanzer. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

googlebot · 2019-03-29T14:18:43Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mandarjog · 2019-03-29T14:30:05Z

/ok-to-test

rshriram · 2019-04-01T18:58:02Z

networking/v1alpha3/sidecar.proto

+    ALLOW_ANY = 1;
+
+    reserved 2;
+    reserved "VIRTUAL_SERVICE_ONLY";


you dont need these two here.. since you are copying the struct

Awesome, thank you!

So you mean I can remove the struct from v1alpha1?
Or do you mean I can remove the reserved 2; and the following line?

I dont think you can remove the struct from the mesh/v1alpha1 without causing some form of circular dependency thing IIRC.. though @ozevren would have to attest to that. For the purpose of this PR though, I would suggest that you remove the reserved 2 and the reserved VIRTUAL.. line as this is an entirely new message introduced in this file..

Ok, I'll do that. Thanks for the clarification.

My original intent was to have this struct only in networking.v1alpha3 and remove the one in mesh.v1alpha1.
There is already one dependency in that direction; adding a reference to mesh.v1alpha1.OutboundTrafficPolicy from networking.v1alpha3 would create a cyclic dependency.

@rshriram I removed the reserved field and updated the PR by amending the commit (To avoid the breaking change in proto.lock in the commit history).

…verride outbound traffic policy per sidecar

rshriram

cc @louiscryan thoughts?

costinm · 2019-04-05T16:11:47Z

Please revert and move to master - AFAIK we are not supposed to make API changes in 1.1 ( or it needs to go trough release manager, review, etc )

release-1.1 branch is released every week - so the TOC review can be too late.

costinm · 2019-04-05T16:12:02Z

@duderino

costinm · 2019-04-05T16:46:00Z

On the content of the API change: I don't think this is the best way, it has multiple problems:

like public/private, it is too narrow
not aligned with 'egress' and 'exportTo' - where for internal traffic we provide a list
not aligned with ServiceEntry - which is normally used to control egress, and has different syntax

It is also quite confusing - what is 'outbound traffic' ?

Egress is used in most places in the docs. What is different between egress and outbound traffic and why a new term used here ?
traffic to other Istio services is also outbond traffic. Is it affected ? Why ? Why not ?
what happens when we want to allow specific ports, or http hosts ?

robertpanzer · 2019-04-05T16:51:02Z

Sorry for creating this.
OutboundTrafficPolicy is already part of Istio's Mesh configuration and I agree that it is confusing.
But all that configuration does is to allow application owners to refine this configuration for their workloads.

I think though that the term "external" refers to MESH_EXTERNAL.

…icy to override outbound traffic policy per sidecar (#887)" (#894) API changes to released Istio versions should go through TOC review first, before being submitted. This reverts commit cf912ce.

* regenerate pb.go with 1.2.0 Signed-off-by: Lizan Zhou <lizan@tetrate.io> * go mod tidy Signed-off-by: Lizan Zhou <lizan@tetrate.io> * fix istio/api to 1.1.0-snapshot.5 Signed-off-by: Lizan Zhou <lizan@tetrate.io> * fix fmt Signed-off-by: Lizan Zhou <lizan@tetrate.io> Mirrored from https://github.com/tetrateio/tetrate @ de742a09f874e00c44445410478d458c260641ed

istio-testing requested review from ozevren and rshriram March 29, 2019 14:14

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Mar 29, 2019

istio-testing added the needs-ok-to-test label Mar 29, 2019

robertpanzer force-pushed the fix-12873-sidecar-outbound-traffic-policy branch from f146f74 to 22c670f Compare March 29, 2019 14:18

googlebot added cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Mar 29, 2019

robertpanzer mentioned this pull request Mar 29, 2019

Fixes #12873. Allow Sidecar to override Outbound Traffic Policy istio/istio#12910

Closed

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Mar 29, 2019

ozevren requested review from costinm and andraxylia and removed request for ozevren March 29, 2019 18:08

rshriram reviewed Apr 1, 2019

View reviewed changes

robertpanzer force-pushed the fix-12873-sidecar-outbound-traffic-policy branch from 22c670f to c877e20 Compare April 2, 2019 07:47

Fixes istio/istio#12873. Add field Sidecar.OutboundTrafficPolicy to o…

4b363cd

…verride outbound traffic policy per sidecar

robertpanzer force-pushed the fix-12873-sidecar-outbound-traffic-policy branch from c877e20 to 4b363cd Compare April 2, 2019 07:56

rshriram approved these changes Apr 2, 2019

View reviewed changes

rshriram approved these changes Apr 5, 2019

View reviewed changes

rshriram merged commit cf912ce into istio:release-1.1 Apr 5, 2019

robertpanzer deleted the fix-12873-sidecar-outbound-traffic-policy branch April 5, 2019 16:51

ozevren mentioned this pull request Apr 5, 2019

Revert Github #887 (SHA cf912ceffb25353871734e526c8ae099827b2823) #894

Merged

robertpanzer restored the fix-12873-sidecar-outbound-traffic-policy branch July 2, 2019 14:39

robertpanzer mentioned this pull request Jul 2, 2019

Fixes #12873. Add property Sidecar.OutboundTrafficPolicy to configure… #964

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes istio/istio#12873. Add field Sidecar.OutboundTrafficPolicy to o… #887

Fixes istio/istio#12873. Add field Sidecar.OutboundTrafficPolicy to o… #887

robertpanzer commented Mar 29, 2019 •

edited

Loading

googlebot commented Mar 29, 2019

istio-testing commented Mar 29, 2019

istio-testing commented Mar 29, 2019

googlebot commented Mar 29, 2019

mandarjog commented Mar 29, 2019

rshriram Apr 1, 2019

robertpanzer Apr 1, 2019

rshriram Apr 1, 2019

robertpanzer Apr 2, 2019

robertpanzer Apr 2, 2019

rshriram left a comment

costinm commented Apr 5, 2019

costinm commented Apr 5, 2019

costinm commented Apr 5, 2019

robertpanzer commented Apr 5, 2019

Fixes istio/istio#12873. Add field Sidecar.OutboundTrafficPolicy to o… #887

Fixes istio/istio#12873. Add field Sidecar.OutboundTrafficPolicy to o… #887

Conversation

robertpanzer commented Mar 29, 2019 • edited Loading

googlebot commented Mar 29, 2019

istio-testing commented Mar 29, 2019

istio-testing commented Mar 29, 2019

googlebot commented Mar 29, 2019

mandarjog commented Mar 29, 2019

rshriram Apr 1, 2019

Choose a reason for hiding this comment

robertpanzer Apr 1, 2019

Choose a reason for hiding this comment

rshriram Apr 1, 2019

Choose a reason for hiding this comment

robertpanzer Apr 2, 2019

Choose a reason for hiding this comment

robertpanzer Apr 2, 2019

Choose a reason for hiding this comment

rshriram left a comment

Choose a reason for hiding this comment

costinm commented Apr 5, 2019

costinm commented Apr 5, 2019

costinm commented Apr 5, 2019

robertpanzer commented Apr 5, 2019

robertpanzer commented Mar 29, 2019 •

edited

Loading