Apply namespace scoping to Gateway servers in samples #34035
Conversation
istio-policy-bot
added
area/networking
release-notes-none
google-cla
bot
added
the
cla: yes
istio-testing
added
the
size/M
There was a problem hiding this comment.
Love these changes! Thanks for doing this @kailun-qin.
We would need to change the istio.io examples/tasks too or else the docs and bundled sample resources will be out of sync?
|
We should also give though to whether it makes it more complicated to understand by looking at the samples, as these are some of the things we use to teach Istio to people. |
| @@ -11,7 +11,7 @@ spec: | |||
| name: http | |||
| protocol: HTTP | |||
| hosts: | |||
| - "*" | |||
| - "default/*" | |||
There was a problem hiding this comment.
Problem with this is the YAML itself is not namespace bound. So they can apply anywhere, and then this won't work.
Maybe we should have allowed ./* in the code...
There was a problem hiding this comment.
Yep, an alternative is to explicitly specify the namespaces for the YAMLs so that they can be bound together. The namespace to bind should align w/ the sample READMEs.
In this way, when kubectl apply w/ no explicit ns set, the resources will by default be applied to the exact ns specified in the Pod spec. And when kubectl apply w/ explicit -n <ns_name>, there'll be an error returned to users when they try to apply it elsewhere (message like the namespace from the provided object "istio-system" does not match the namespace "default". You must pass '--namespace=istio-system' to perform this operation.), which is guaranteed by kubectl.
Does this approach make sense to you @howardjohn? Thanks!
There was a problem hiding this comment.
Some of these resources (e.g., bookinfo ones) are intended to be used in whatever namespace the user wants. Even the Istio tasks use different namepaces (e.g., some use default, others use ns bookinfo). Outside blogs and other docs also rely on the namespace flexibility, so this would be unacceptable breaking change IMO.
There was a problem hiding this comment.
Thanks for the comments!
If it's the case, I think maybe we should have allowed * in the code. We should only apply ns scoping to the samples where the YAMLs have already bound w/ specific ns, e.g. samples/bookinfo/networking/certmanager-gateway.yaml, samples/multicluster etc.
WDYT @frankbu @howardjohn?
There was a problem hiding this comment.
We should only apply ns scoping to the samples where the YAMLs have already bound w/ specific ns
That makes sense but I don't think there are very many of those. The samples/multicluster east-west gateways are not always in istio-system. For example, see the external control plane doc: https://preliminary.istio.io/latest/docs/setup/install/external-controlplane/#setup-east-west-gateways
There was a problem hiding this comment.
btw, another recommended best practice is to use fully-qualified hosts in VirtualServices (e.g., reviews.default.svc.cluster.local insead of just reviews), but we haven't done that in the samples either, for the same reason.
Maybe we need to add more best practices to the docs and put some tips in the tasks/examples that point to them.
Thanks! Yes, we do need to update the related if this change itself makes sense. |
Agree. IMO this looks fine for readability. Additional doc/comments can be updated if not clear enough. |
Gateways allow namespace bound virtual service selection (e.g., foo/foo.example.com). This is a good security best practice. This patch spreads this usage to all samples to restrict the set of virtual services that can bind to a gateway server. Fixes istio#33977 Signed-off-by: Kailun Qin <kailun.qin@intel.com>
|
/retest |
Signed-off-by: Kailun Qin <kailun.qin@intel.com>
istio-policy-bot
added
the
lifecycle/stale
|
🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2021-07-15. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions. Created by the issue and PR lifecycle manager. |
istio-policy-bot
added
the
lifecycle/automatically-closed
Gateways allow namespace bound virtual service selection (e.g.,
foo/foo.example.com). This is a good security best practice.
This patch spreads this usage to all samples to restrict the set of
virtual services that can bind to a gateway server.
Fixes #33977
Signed-off-by: Kailun Qin kailun.qin@intel.com
[ ] Configuration Infrastructure
[x] Docs
[ ] Installation
[x] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure
Pull Request Attributes
Please check any characteristics that apply to this pull request.
[x] Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.