Replace 'scope' with 'export_to' namespace#758
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: louiscryan If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
|
We do implement scoping for destination rules in a very intricate way. So I suggest adding the exportTo to destination rules as well. Other than that, I would also add a temporary restriction that for this release, only . and * are allowed. [there is a bunch of code that needs to change to incorporate selective exports]. |
| // be overridden by explicitly listing the namespaces that are allowed | ||
| // to refer to this declaration. | ||
| // | ||
| // The value "." is reserved and defines an export to the namespace where |
There was a problem hiding this comment.
Let's clearly define the defaults - we have the same problem for Sidecar.
- if field is missing ( no entries ) - we clearly have to support the backward compat mode ( export to all )
- if it is present - it must have at least one entry ( we can't have an empty list represented in proto). I assume "." will be equivalent with the former "private".
- not clear what is the use case for '' - I assume it makes sense only if we define a mesh config indicating 'default export_to', allowing transition of the default from '' to '.' (or some other explicit list).
There was a problem hiding this comment.
Tidied up the language a bit to match that. I kept asterisk as a reserved value even if the use case is equivalent to the empty list case.
| // The value "." is reserved and defines an export to the namespace where | ||
| // the service is declared, similarly the value "*" is reserved and | ||
| // defines an export to all namespaces. | ||
| repeated string export_to = 7; |
There was a problem hiding this comment.
Also worth noting that export_to still require an explicit 'egress' in the destination namespace.
There was a problem hiding this comment.
Why? I thought no egress mean all public (i.e., with export_to my namespace) services in any other namespace.
There was a problem hiding this comment.
If no Sidecar is defined and we have the old behavior the export_to can still restrict it. Its an additive feature of the API so doing that does not impose any backward compatability risks
|
That behavior is not scalable - users can explicitly enable it with */*,
but it's not something we want to encourage.
If a Sidecar is used - we want to default to explicit imports ( and if
they are ok with slow performance or have low scale - can use */* ).
We need to stop all the 'all/random namespaces' behaviors - and Sidecar is
the way to opt in, at some point we'll have a global flag and
figure out how to completely deprecate the old behavior. For 1.1 - no
Sidecar means the old way for backward compat, Sidecar means
user understands they need explicit import and isolation.
…On Tue, Jan 15, 2019 at 11:38 AM Frank Budinsky ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In networking/v1alpha3/service_entry.proto
<#758 (comment)>:
> @@ -429,10 +428,13 @@ message ServiceEntry {
// One or more endpoints associated with the service.
repeated Endpoint endpoints = 6;
- // The visibility setting associated with this service entry. Set to
- // PRIVATE if this service should not be visible outside the namespace
- // where the service entry was added. The default scope is public,
- // i.e. the service added by the service entry will be visible to
- // workloads in the entire mesh.
- ConfigScope config_scope = 7;
+ // The namespaces to which this service is exported. By default
+ // services are exported to all namespaces. This behavior can
+ // be overridden by explicitly listing the namespaces that are allowed
+ // to refer to this declaration.
+ //
+ // The value "." is reserved and defines an export to the namespace where
+ // the service is declared, similarly the value "*" is reserved and
+ // defines an export to all namespaces.
+ repeated string export_to = 7;
Why? I thought no egress mean all public (i.e., with export_to my
namespace) services in any other namespace.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#758 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6mng4iVCVdEhi6MQfzVUfiZdQS6gks5vDi5QgaJpZM4Z_-ts>
.
|
175cfe6 to
661175b
Compare
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
8edc9a6 to
68e10f0
Compare
|
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. |
68e10f0 to
f9f0de9
Compare
|
/retest |
f9f0de9 to
47cd7fa
Compare
|
CLAs look good, thanks! |
|
what is the rational of choosing export_to over just export? |
rshriram
left a comment
There was a problem hiding this comment.
please make the format of exportTo consistent (comma separated string or repeated string).
Also, if you have time, could you please clean up the language for the locality_lb_setting in the mesh config?
|
Also missing is the root namespace flag. |
If we just said 'export' it would read more like a boolean, 'export to' implies a specification of the receiver. |
This can't be in config.proto as we'll read config.proto from that namespace. It has to be an install time flag for the control plane. |
I should say that there are two choices here. Either the control plane reads config.proto from it's own namespace or from the root namespace. I'm fine with either and it might be that reading config.proto from the control-plane namespace makes the most sense |
a270321 to
3726c91
Compare
|
@rshriram PTAL |
| istio.networking.v1alpha3.TLSSettings tls_settings = 2; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
? I was asking if you would be able to clean up the language in this locality weighted routing stuff. This is the code moved from the destination rule and shoved into the global config. I can clean it up later as well.
There was a problem hiding this comment.
ah, ok. Updating
|
PTAL |
| // The value "." is reserved and defines an export to the same namespace that | ||
| // the virtual service is declared in, similarly the value "*" is reserved and | ||
| // defines an export to all namespaces. | ||
| // |
There was a problem hiding this comment.
Its not clear that 'none' has any utility on VirtualService or ServiceEntry beyond what 'this namespace only' provides.
This is different from the use of 'none' on the referring side
Add flags to control scopeTo defaults Update doc for locality weighted LB
f34eef0 to
1ea813d
Compare
Currently 'scope' is only implemented on service and virtual service but not destination rule. This change also aligns with the revised naming conventions for referring to namespaces that have been adopted in the Sidecar API.