support exportTo specific namespaces #24443
Conversation
Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io>
|
/hold |
Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io>
|
/hold cancel |
|
@istio/wg-networking-maintainers-pilot this is now ready for review |
|
/test integ-security-k8s-tests_istio |
pilot/pkg/model/push_context.go
Outdated
| @@ -71,10 +71,14 @@ type PushContext struct { | |||
| // Service related | |||
| // TODO: move to a sub struct | |||
|
|
|||
| // privateServices are reachable within the same namespace. | |||
| // privateServices are reachable within the same namespace, with exportTo . | |||
There was a problem hiding this comment.
nit: can you please put . -> "." so that we do not get confused with end of sentence?
Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io>
pilot/pkg/model/push_context.go
Outdated
| // check the target service's namespace for public rules | ||
| if svcNs != "" && ps.namespaceExportedDestRules[svcNs] != nil { | ||
| // check the target service's namespace for exported rules | ||
| if svcNs != "" && ps.exportedDestRulesByNamespace[svcNs] != nil { |
There was a problem hiding this comment.
We store destination rule by the ns it resides, not by function scope. So it seems we are missing check for its function scope in the first step in L721.
There was a problem hiding this comment.
what do you mean by function scope? The algorithm is look in proxy's namespace first (doesn't matter if the service does not belong ot the proxy's namespace. if there is a dest rule in proxy's namespace for that service, pick it up first). Then look in the target service's namespace for public rules or rules that are explicitly exported to the proxy's namespace (in either case, this dest rule is "visible" to the proxy's namespace).
And then finally look in config root namespace.
| } | ||
| } else if len(rule.ExportTo) == 1 && exportToMap[visibility.Private] { |
There was a problem hiding this comment.
nit: concate with if condition
There was a problem hiding this comment.
nit: i mean the if else conditions can be merged
| @@ -69,6 +85,11 @@ func mergeVirtualServicesIfNeeded(vServices []Config) (out []Config) { | |||
| // delegate not found, ignore only the current HTTP route | |||
| continue | |||
| } | |||
| // make sure that the delegate is visible to root virtual service's namespace | |||
| exportTo := delegatesExportToMap[key(route.Delegate.Name, route.Delegate.Namespace)] | |||
| if !exportTo[visibility.Public] && !exportTo[visibility.Instance(root.Namespace)] { | |||
| } | ||
| } else { | ||
| // if this is a serviceEntry, allow ~ in exportTo | ||
| if isServiceEntry && visibility.Instance(e) == visibility.None { |
There was a problem hiding this comment.
I am not sure if we will have a definitive use case for this but if we ever do, the only config that can support ~ is the serviceEntry - think of the scenario where you have a cronjob with some dummy service assigned to it, and you want to make sure no one can talk to the cronjob.
There was a problem hiding this comment.
Then the question comes, if it is not intended to be accessed, why even create a SE/service for it?
There was a problem hiding this comment.
For the proxies that implement that service - so that the GetProxyServiceInstances() doesn't return empty. Also, if a service is present, then metrics will be present. E.g., a cron job that calls a bunch of internal APIs every 10s..
| default: | ||
| return fmt.Errorf("only . or * is allowed in the exportTo in the current release") | ||
| if !labels.IsDNS1123Label(string(v)) { |
There was a problem hiding this comment.
Actually, this is not right. The allowed length is 63 here. But in k8s the allowed name length is <= 253
There was a problem hiding this comment.
I have found many misuse of IsDNS1123Label in the whole repo
There was a problem hiding this comment.
Are you sure kubernetes namespaces can be >63 characters? If the name is 253 characters, then no one can create a service inside a namespace as the total DNS name length will be >255 characters.
There was a problem hiding this comment.
You are right, i am looking at something wrong
|
issue for what? exportTo to specific namespaces is part of the networking items to tackle for 1.7 (marked as P1 IIRC). |
Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io>
|
I will need to extend the Do you have yaml files making use of this that you use for tests or as samples? If so I would like to use them to write analysis test cases. (It not I will create my own.) |
|
@esnible there is no syntactic change. Here is one sample YAML |
|
also, cc @Stono - this is something you asked for a long time ago :). |
|
We have an analyzer that verifies that every As a user, if I made the mistake below, I would want a hint from |
|
Wooooo cool |
* support exportTo specific namespaces Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io> * tests and bug fixes Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io> * enclose dot in quotes to differentiate from full stop Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io> * nits Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io> * update merge logic comments Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io> * merge if Signed-off-by: Shriram Rajagopalan <rshriram@tetrate.io>
It is now possible to use exportTo to export to specific namespaces. Was fixed in istio/istio#24443
Enhancing from the existing . and * to allow users to export to specific set of namespaces.
[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[x ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure