-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CNI isn't required to be installed in kube-system on OpenShift #50984
base: master
Are you sure you want to change the base?
Conversation
In theory we can deploy CNI in any namespace now. I'm not sure if this is safe regarding upgrades though. If yes, I guess we need a release note for this? If it will bring problems during upgrades, we can discard this PR and stick with kube-system forever. @luksa @howardjohn thoughts? |
Last time I tried - cni in kube-system and istio-system fought with each
other to install as last component in the cni config. Not sure if it
changed.
Since all installs and tests are on istio-system - it is worth migrating or
using that for new clusters. But you first must uninstall kube-system cni -
if you can.
The current ambient docs are written from fresh install on clean cluster
perspective - I assume next release we'll focus on existing clusters and
such cases.
Will be a large problem in GKE where we have auto-managed CNI in
kube-system for ASM users. Please keep me in the loop.
…On Fri, May 10, 2024, 06:27 Jonh Wendell ***@***.***> wrote:
In theory we can deploy CNI in any namespace now.
I'm not sure if this is safe regarding upgrades though. If yes, I guess we
need a release note for this?
If it will bring problems during upgrades, we can discard this PR and
stick with kube-system forever.
@luksa <https://github.com/luksa> @howardjohn
<https://github.com/howardjohn> thoughts?
—
Reply to this email directly, view it on GitHub
<#50984 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2QBJZWX6QMZKKTT2PDZBTDMTAVCNFSM6AAAAABHQUI5USVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBUGYYTAMJXGU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break upgrades in a terrible way (constantly reconciling over each other). I don't think we should do this without more work somehow to make it safe
Fortunately helm will not install because the cluster resources are already
present.
The only safe upgrade I found so far is by using affinity and node labels -
coincidentally this is also a good way to canary and upgrade ztunnel. But
needs few changes to the charts - we should discuss in the WG.
There are some ideas to make it more flexible - we do a lot more than we
should in the node ( considering how much the pod can do), but it's a very
risky are - code is complex and many things can break. IMO best choice is
to consider splitting things up a bit, in particular the installed
component which is the real issue.
…On Fri, May 10, 2024, 07:17 John Howard ***@***.***> wrote:
***@***.**** commented on this pull request.
This will break upgrades in a terrible way (constantly reconciling over
each other). I don't think we should do this without more work somehow to
make it safe
—
Reply to this email directly, view it on GitHub
<#50984 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2SSFDEUBMWJV7NA4XDZBTJFZAVCNFSM6AAAAABHQUI5USVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJQGE3TMNZSHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
It is not safe to install the same daemonset 2x in k8s as a general rule - that's why it's a daemonset (arguably).
Yes, that's what I'd expect.
We can, but we cannot deploy multiple CNI instances to different namespaces at the same time - that's both not allowed by Helm, and not really rational K8S practice - and not something we will ever support, I think. See also #49009 |
On Tue, May 14, 2024, 10:09 Ben Leggett ***@***.***> wrote:
It is not safe to install the same daemonset 2x in k8s as a general rule -
that's why it's a daemonset.
Right. But you can have a daemon set that is only installed on select
nodes, and another one selecting a different node pool.
Unfortunately we have no way to detect another istio-derived cni in a
different namespace, or another CNI doing 'stuff'.
Fortunately helm will not install because the cluster resources are already
present.
Yes, that's what I'd expect.
Istioctl or helm template doesn't have those problems...
In theory we can deploy CNI in any namespace now.
We can, but we cannot deploy multiple CNIs to different namespaces at the
same time - that's both not allowed by Helm, and not really rational K8S
practice.
It's more complex - for example in GKE we auto install in kube-system, and
with helm template.
Others may do the same. Depending on order you may or may not be broken.
Also it's not a problem if each CNI targets different pods. There is a
project ( forgot the name) allowing pods to select one or more cni sets (
to enable multiple net interfaces in different protocols).
We need to gradually improve our install story from 'hope only one cni is
messing with network'
—
… Reply to this email directly, view it on GitHub
<#50984 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2T44EYYTSKRQJMPO2LZCJAM3AVCNFSM6AAAAABHQUI5USVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJQG4ZDQMRZGA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Agreed, that's our special problem :D
Auto-installing to a namespace in a cluster with an existing Istio install without checking if you should is probably a vendor caveat-emptor situation.
I've seen proposals like that, but in general I think the UX of selecting CNI at the pod level sucks, is not a user concern, and is a bad idea (it's convenient for some kinds of operators but it doesn't solve any real user-facing problems and creates quite a few user-facing footguns)
I don't think we need to improve it from 'only one |
On Tue, May 14, 2024 at 11:11 AM Ben Leggett ***@***.***> wrote:
Istioctl or helm template doesn't have those problems...
Agreed, that's our special problem :D
I think our real problem is not 'helm template not accidentally protecting
us' ( as helm install does ).
The code in istio-cni dealing with install is ancient and over complicated
- creating jwt tokens, not doing checks
if other versions are installed, etc. I tried to do some changes but gave
up - IMO best option is to rewrite the installer
part and what runs on the host.
One of the best things about the ambient changes in CNI is that the CNI pod
can enter namespaces and fix,
it doesn't depend on the host portion of the code doing 'stuff' - we can
have a tiny program that just sends
the params over UDS to one or more sockets. Then we can have as many CNI
pods as we want - as long
as they use different labels or check if someone else already did the
setup, or apply incremental/independent
changes.
Unfortunately the code used for sidecars is even more complicated and
slightly different from what ambient
is using... So I'm still trying to find a simple solution - on some
environments users can't just install their own
CNI ( GKE autopilot for example, probably others that have strict
restrictions on the host access ).
It's more complex - for example in GKE we auto install in kube-system, and
with helm template. Others may do the same. Depending on order you may or
may not be broken.
Auto-installing to a namespace in a cluster with an existing Istio install
without checking if you should is probably a vendor caveat-emptor situation.
The problem is users attempting to install while a vendor already did that.
If the vendor doesn't block it - bad things happen.
Perhaps we should add more warnings and checks to the docs.
Also it's not a problem if each CNI targets different pods. There is a
project ( forgot the name) allowing pods to select one or more cni sets (
to enable multiple net interfaces in different protocols).
I've seen proposals like that, but in general I think the UX of selecting
CNI at the pod level sucks, is not a user concern, and is a bad idea
(though it is convenient for some types of operators)
I don't disagree. Messing with CNI should not be possible for a user IMO,
but be part of the platform and heavily locked.
We need to gradually improve our install story from 'hope only one cni is
messing with network'
I don't think we need to improve it from 'only one istio-cni MAY mess
with the network at a time', unless I'm missing something
The definition of "one istio-cni" is a bit unclear. There are forks of
Istio, other projects may reuse the code or model, vendors may
have their own install ( GKE autopilot installs in kube-system in some
cases - at least it also blocks any other install )
|
No description provided.