Define ServiceScopeConfig in ServiceSettings #3464
Conversation
|
Skipping CI for Draft Pull Request. |
istio-testing
added
do-not-merge/work-in-progress
|
/test all |
istio-testing
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
This looks like what we got general consensus on in https://docs.google.com/document/d/1Wg6sx9ZUJL4AsHj5wM1kMx3E436s5wg2qoMqoI-bqbQ/edit?tab=t.0 across multiple TOC and WG meetings so I'm approving
| @@ -444,6 +444,47 @@ message MeshConfig { | |||
| // | |||
| // For example: foo.bar.svc.cluster.local, *.baz.svc.cluster.local | |||
| repeated string hosts = 2; | |||
|
|
|||
| // Scope configuration to be applied to matching services. | |||
There was a problem hiding this comment.
What is the relation between this new setting and the existing hosts/settings?
There was a problem hiding this comment.
My thought was that users set one or the other. A oneOf might be better to express that
There was a problem hiding this comment.
At this point ServiceScopes will only apply to ambient multicluster. In traditional multicluster today the default availability is global. For ambient multicluster, the default availability is local. For alpha, I think it is unnecessary to support ServiceScopes for both configurations operating with different default availabilities
There was a problem hiding this comment.
Is it possible there could be conflict where the host is foo.bar.svc.cluster.local and the namespace or service has istio.io/global label enabled?
Also, do we need to support both namespace and service level? what if they have conflicts?
mesh/v1alpha1/config.proto
Outdated
| // | ||
| // ```yaml | ||
| // serviceSettings: | ||
| // serviceScopes: |
There was a problem hiding this comment.
| // serviceScopes: | |
| // - serviceScopes: |
serviceSettings is a list of MeshConfig_ServiceSettings. Not entirely sure we need the double nested list?
There was a problem hiding this comment.
@keithmattix do you have a preference? I am in favor of it not being double nested
There was a problem hiding this comment.
Hmm looking at the proto, I almost feel like ServiceScopes should just be a sibling to ServiceSettings. The other fields in service settings don't make sense for service scope, and eventually, I think the latter will supplant the former
There was a problem hiding this comment.
Might be harder to enforce oneof though...
There was a problem hiding this comment.
Fair enough...in that case yeah we don't want the extra nesting
There was a problem hiding this comment.
I had another thought: I'm not sure if we can do oneOf because new versions of istiod reading old meshconfig would no longer be able to parse it correctly...
istio-testing
added
the
needs-rebase
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
jaellio
force-pushed
the
jaellio/srvcapandexportapi
branch
from
ad386cf to
c9111e4
Compare
istio-testing
removed
the
needs-rebase
istio-policy-bot
added
the
lifecycle/stale
jaellio
removed
the
lifecycle/stale
istio-policy-bot
added
the
lifecycle/stale
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
|
@louiscryan Could you please take a look? |
istio-policy-bot
removed
the
lifecycle/stale
Signed-off-by: Jackie Elliott <jaellio@microsoft.com>
|
cc @stevenctl to review as well. @louiscryan is on vacation hopefully back soon! |
Part of adding support for Ambient mulitcluster.
Based on https://docs.google.com/document/d/1Wg6sx9ZUJL4AsHj5wM1kMx3E436s5wg2qoMqoI-bqbQ/edit?tab=t.0#heading=h.nehszyorutrv
#3463