Conversation
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
…try_view Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
costinm
left a comment
There was a problem hiding this comment.
/lgtm (but some cosmetic doc requests).
I'm not sure it's a good idea to try to implement everything in 1.1 - I would rather have a well tested
implementation at namespace level, without workload selector ( which is trickier - in particular if you consider runtime changes in labels )
| // in a namespace will be able to reach other workloads in the same | ||
| // namespace. To declare dependencies on workloads in other namespaces, a | ||
| // NetworkScope resource has to be specified in the current | ||
| // namespace. *_Each namespace can have only one NetworkScope |
There was a problem hiding this comment.
How about defining a name ("default") - to make it easier to write RBAC rules - instead of leaving it 'arbitrary name'?
| // - namespace: egress | ||
| // ``` | ||
| // | ||
| // In a mesh where the default network scope is set to CURRENT_NAMESPACE |
There was a problem hiding this comment.
I'm not sure - if NetworkScope is specified in a namespace, it should be respected regardless of the default.
So it doesn't matter what the default is.
There was a problem hiding this comment.
Have the same doubt: is cluster scoped NetworkScope enough?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: costinm, rshriram 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 |
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
mesh/v1alpha1/config.proto
Outdated
|
|
||
| // Configure routes to services in the same namespace as the sidecar | ||
| // in addition to namespaces specified in sharedNamespaces. | ||
| CURRENT_NAMESPACE = 2; |
There was a problem hiding this comment.
Won't OWN_NAMESPACE be better? If yes, change throughout the files.
There was a problem hiding this comment.
OWN_NAMESPACE is ambiguous because own has two meanings: "my own" or "owned by". Maybe SAME_NAMESPACE is a better choice?
| // expected to talk to, in addition to other workloads in the same | ||
| // namespace. Dependencies describe the properties of outbound traffic from | ||
| // a given workload. | ||
| repeated Dependency dependencies = 1; |
There was a problem hiding this comment.
why dependencies and imports are an array type at the same time?
Only one array can indicate the import dependency.
There was a problem hiding this comment.
Dependency has other things, like source pod selectors, but I guess that's going to be added in a future release.
There was a problem hiding this comment.
Because workload labels will be added to dependencies in future..
frankbu
left a comment
There was a problem hiding this comment.
I would consider changing CURRENT_NAMESPACE to SAME_NAMESPACE, but otherwise looks good
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
| } | ||
|
|
||
| // REQUIRED: The default import setting for every workload in the mesh. | ||
| Mode import_mode = 1; |
There was a problem hiding this comment.
Curious why the default isn't 0, ALL_NAMESPACES?
It stated in other place: "By default, the service mesh
established by Istio will have a full mesh connectivity - i.e. every
workload will have proxy configuration required to reach every other
workload in the mesh."
There was a problem hiding this comment.
The 1 is a proto field number, not a value.
Signed-off-by: Shriram Rajagopalan shriramr@vmware.com