Safer SE API changes#3729
Conversation
Signed-off-by: Ian Rudie <ian.rudie@solo.io>
|
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
|
😊 Welcome @ilrudie! This is either your first contribution to the Istio api repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
the
size/M
| // external admission validation. When unset, ServiceEntry behaves as it does | ||
| // today (published cluster-wide; ambient ignores `exportTo`). See the | ||
| // ServiceEntryPublishing message for details. | ||
| ServiceEntryPublishing service_entry_publishing = 72; |
There was a problem hiding this comment.
nit: ServiceEntryVisibility might be a better name
There was a problem hiding this comment.
Thanks for the suggestion, visibility is good.
There was a problem hiding this comment.
Updated with visibility instead of publish. Thanks for the feedback. This suggestion allowed cleaning up the surrounding verbiage as well. Much appreciated.
| // Policies are evaluated in order; the first policy whose rules all match sets | ||
| // the ServiceEntry's publish scope. If no policy matches, `defaultPublishScope` | ||
| // applies. When this entire message is absent from `MeshConfig`, every | ||
| // ServiceEntry is published `CLUSTER_LOCAL`, preserving current Istio behavior. |
There was a problem hiding this comment.
What does this mean publishing to CLUSTER_LOCAL mean? In sidecar world, if you create a ServiceEntry in Primary/Config cluster, it is visible to dataplanes in all clusters
There was a problem hiding this comment.
The intention was to avoid the term "global" per some of the feedback during discussions of the proposal in WG. This was to avoid making the impression that it would be visible across a multi-primary cluster boundary. The behavior seems like it should be "visible to all things programmed by this cluster" in a shared primary.
I'm very open to suggestions here for either a clarifying rename, or a new visibility. CONTROL_LOCAL perhaps, meaning everything programmed by this control plane has it.
There was a problem hiding this comment.
I guess one question to nail down, if we have something which means "visible to everything programmed by this control plane", does a cluster visibility provide much utility any more? It seems like those can be combined without much loss of expressiveness.
There was a problem hiding this comment.
I think when we have some thing like visible to everything programmed by a specific control plane, cluster visibility IMO does not make sense. May be another visibility type like "MESH" expresses that it is visible to all clusters managed by the control plane?
There was a problem hiding this comment.
I was thinking about "mesh" too, but to my mind it would mean the whole multi-cluster mesh (as in distributed to all the other primaries as well). That's not what we plan to implement in this unit of work at least. I suppose tbd if we land there in future which is part of why this api shape is flexible for being able to express something like that.
| // external admission validation. When unset, ServiceEntry behaves as it does | ||
| // today (published cluster-wide; ambient ignores `exportTo`). See the | ||
| // ServiceEntryPublishing message for details. | ||
| ServiceEntryPublishing service_entry_publishing = 72; |
|
|
||
| // Visible to the entire cluster. Equivalent to the legacy `exportTo: "*"`. | ||
| CLUSTER_LOCAL = 3; | ||
| } |
There was a problem hiding this comment.
could these two be combined to local vs remote?
There was a problem hiding this comment.
I don't think I understand. Is the suggestion to a visibily like cluster_local and cluster_remote to be able to distinguish visibility in the shared primary approach?
Signed-off-by: Ian Rudie <ian.rudie@solo.io>
|
@ilrudie: The following test failed, say
DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
5 participants
This adds the API changes to mesh config discussed in the Safer ServiceEntry proposal.
TL;DR: Allows a mesh administrator to control which namespaces are allowed to "publish" a service entry at a scope above "namespace" while offering flexibility for future matching options to further constrain this publishing, such as "address within cidr" or "hostname re/suffix/exact").
This is the shape discussed, however we did not bikeshed the precise wording of the api in the proposal.