-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Feat: cilium routing mode setting #12588
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
base: master
Are you sure you want to change the base?
Feat: cilium routing mode setting #12588
Conversation
Some users want independent variables and hope to be able to configure them like `tunnelProtocol`. Signed-off-by: ChengHao Yang <17496418+tico88612@users.noreply.github.com>
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhkarimi1383, tico88612 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
At this point can't we just have a values-overrides thing which we'd feed directly to cilium cli ? |
If we have tunnel_mode we should have routing_mode too since they are related, But we need some place to be able to pass some values/params in raw |
|
I'm unable to use kubespray without custom patch applied every time It is a needed thing to set in some cases and it makes the kubespray unusable |
| identityAllocationMode: {{ cilium_identity_allocation_mode }} | ||
|
|
||
| routingMode: {{ cilium_routing_mode }} | ||
|
|
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.
if cilium_routing_mode is native, we should skip routingMode: {{ cilium_routing_mode }} entirely, otherwise, cilium complains.
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.
I this should be allowed here and should be passed directly to cilium, for example if cilium introduce a new feature or something like that we can't handle, I think keeping this simple is a better approach
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.
@ayoubfaouzi Why? If you skip this, that's not working. routingMode default is tunnel.
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.
Oh sorry, what I wanted to say was, when I deployed kubespray with cilium as CNI, I want to use native routing mode, so no tunneling. I modified cilium_tunnel_mode to disabled as in the doc.
However cilium kept complaining about disabled, looks like it did not get it (should be only vxlan or geneve), so I had to edit the cm and completely get rid of cilium_tunnel_mode and add routing-mode: native.
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.
Or maybe you try an empty string? the cilium docs said cilium_tunnel_mode only accepted "", "vxlan", "geneve"
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.
Yes, I am on same boat. cilium_tunnel_mode only accept vxlan or geneve. I tried empty string which started throwing errors.
For now I just kept tunnel-protocol to vxlan. Hope its acceptable value with routing mode native.
|
I don't understand why we'd need this when we already have |
It is needed as we have a separate variable for |
|
The fact that one variable exists is not justification enough to add another one. From what I can see, the values.yml.j2 has already started duplicating the values of the helm chart, which is a nightmare for maintainability. @tico88612 Am I missing something here ? The file feeded to cilium-cli looks like it has very little logic in it, basically just translate ansible variables into cilium-cli values. (In that case we should IMHO stop adding to it and just use the extra values stuff which is already there). |
|
To the best of my knowledge, for the existing variable That said, there are still some alternative approaches, which would end up looking like this: TBH, I'm actually quite hesitant about this point, and I don't want it to become another copied helm values. |
|
I have used following setting to activate routing mode native. This is what my current config view looks Question, does this confuse cilium that routing mode is native but tunnel-protocol is vxlan so routing may create issue in future? I didn't find a way to delete tunnel-protocol flag using kubespray. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Some users want independent variables and hope to be able to configure them like
tunnelProtocol.Which issue(s) this PR fixes:
Fixes #12541
Does this PR introduce a user-facing change?: