-
Notifications
You must be signed in to change notification settings - Fork 268
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
Add multiple ingress controller support + traefik #5943
Conversation
c3faab8
to
66eabcd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5943 +/- ##
===========================================
+ Coverage 9.77% 25.82% +16.05%
===========================================
Files 32 32
Lines 2712 2734 +22
===========================================
+ Hits 265 706 +441
+ Misses 2425 1981 -444
- Partials 22 47 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
0d0377a
to
5431573
Compare
// prepare a list of items to disable, based on all valid components. | ||
// we have to use an intermediate set because the flag interface | ||
// doesn't allow us to remove flag values once added. | ||
disabledCharts := sets.Set[string]{} |
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 wonder why we need a set instead of a slice. If I understand correclty, the "values" are always slices that we are hardcoding with rke2 components in "pkg/rke2/rke2.go", e.g. CNIItems
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 made it a set so that I can easily remove entries from it by value, without having to find their indexes in the slice and elide them. It'd be much more boilerplate code than just the Insert()
and Delete()
functions that sets provide. I could use a map[string]struct{}
and insert/delete keys but generic sets are implemented as a map[comparable]struct{}
under the hood anyway:
https://github.com/kubernetes/apimachinery/blob/master/pkg/util/sets/set.go#L24-L25
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.
aaah ok, I was thinking on the property that no copies are possible in sets by default and that did not make sense here. Thanks
@@ -105,7 +106,7 @@ func (p *PEBinaryConfig) Bootstrap(ctx context.Context, nodeConfig *config.Node, | |||
} | |||
|
|||
if p.IsServer { | |||
return bootstrap.UpdateManifests(p.Resolver, nodeConfig, cfg) | |||
return bootstrap.UpdateManifests(p.Resolver, p.IngressController, nodeConfig, cfg) |
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 have just realized that if we ever reach this line, something would be broken because windows can never be the server. What if we use this PR to add an error message instead saying that this is not supported?
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 wired it up here in case we ever do support windows server nodes. I don't think it needs to be an error, it's just not a code path that is reachable at the moment.
ec51387
to
6f53be4
Compare
Signed-off-by: Brad Davidson <[email protected]>
* Add new --ingress-controller CLI flag * Refactor --ingress-controller and --cni flags to use common helper for disabling all unused charts * Wire first ingress controller name into global.systemDefaultIngressClass chart variable Signed-off-by: Brad Davidson <[email protected]>
Signed-off-by: Brad Davidson <[email protected]>
e461844
to
e56bb43
Compare
Waiting on:
Proposed Changes
--ingress-controller=none
will disable all of them, same as--cni=none
.Note: while multiple ingress controllers can be selected via the CLI flag, and the first one will be set as the default ingress class, additional user-provided configuration will be necessary to allow both ingress controllers to run in parallel , due to both charts defaulting to deploying a DaemonSet that binds to ports 80 and 443. For this reason, while allowed by the CLI flag, we will not be supporting parallel installation of multiple ingress controllers.
If someone did want to do it on a cluster with a LoadBalancer controller capable of binding multiple LoadBalancer services on the same ports, something like this would work to reconfigure both controllers to use Deployment+LoadBalancer instead of Daemonset+HostPort:
Types of Changes
ADR; packaged component
Verification
ingressclass.kubernetes.io/is-default-class: "true"
; others are false or absent.Testing
Linked Issues
User-Facing Change
Further Comments