Allow to configure CPLB interfaces with mac addr #6369
Conversation
juanluisvaladas
force-pushed
the
cplb-mac-nic
branch
5 times, most recently
from
fec3171 to
4be3b5f
Compare
pkg/apis/k0s/v1beta1/cplb.go
Outdated
| } | ||
| case VRRPInterfaceTypeMAC: | ||
| if len(k.VRRPInstances[i].InterfaceMACAddresses) == 0 { | ||
| errs = append(errs, errors.New("InterfaceMACAddresses must be defined when InterfaceType is mac")) |
There was a problem hiding this comment.
TODO: The PR is large already, so I want to deal with this in a separate PR, but the validation could be better, we should use k8s.io/apimachinery/pkg/util/validation/field like we do in workerprofile.go
Also we could do better error checking in the test for this function, again, we should improve it like we do in workerprofile.go...
There was a problem hiding this comment.
I'd prefer to use those errors right away. We'll never get around to refactor this once this lands. We can just omit the proper field path propagation for now, and use something like this?
| errs = append(errs, errors.New("InterfaceMACAddresses must be defined when InterfaceType is mac")) | |
| path := field.NewPath("interfaceMACAddresses") | |
| err := field.Required(path, `must be defined when interfaceType is "mac"`) | |
| errs = append(errs, err) |
Also, it's a good practice to use the JSON field names in the error messages, as this is what users will be facing.
|
What is the reasoning to support multiple MAC addresses? If we could get away with a single, we could maybe let the interface: 00:00:5E:00:53:00 |
It was requested by k0rdent people so that they could have the same configuration for all the control plane nodes. If they have 3 control plane nodes with MAC addresses X, Y and Z, they'll define X, Y and Z in the configuration file and only the correct value will be used. Apparently providing different configuration per control plane node is major change for them.
That was actually the initial implementation. |
I don't think this is a good argument for allowing multiple interfaces. Can we come up with some reasoning that makes sense for k0s itself to support MAC lists for a single node? |
docs/configuration.md
Outdated
| | `virtualIPs` | List of virtual IP address used by the VRRP instance. Each virtual IP must be a CIDR as defined in RFC 4632 and RFC 4291.A list of the CIDRs handled by the VRRP instance. | | ||
| | `interfaceType` | The type of the interface. If not specified, k0s will use `interface`. Valid values are `interface` and `mac`. If `interfaceType` is `mac`, k0s will use the field `interfaceMACAddresses`, otherwise it will use the field `interface`. Default: `interface`. | | ||
| | `interface` | The NIC used by the virtual router. If not specified, k0s will use the interface that owns the default route. | | ||
| | `interfaceMACAddresses` | A list of MAC addresses for the interface. MAC addresses are evaluated in order and the first match will be chosen. If none of the MAC addresses match, an error is returned and k0s will not start. This field is only used when `interfaceType` is set to `mac`. | |
There was a problem hiding this comment.
If we have two separate fields for this, we don't need the type field IMO in this case. We can make it either interface or interfaceMACAddresses. I reckon this will save some code/docs.
pkg/apis/k0s/v1beta1/cplb.go
Outdated
| } | ||
| case VRRPInterfaceTypeMAC: | ||
| if len(k.VRRPInstances[i].InterfaceMACAddresses) == 0 { | ||
| errs = append(errs, errors.New("InterfaceMACAddresses must be defined when InterfaceType is mac")) |
There was a problem hiding this comment.
I'd prefer to use those errors right away. We'll never get around to refactor this once this lands. We can just omit the proper field path propagation for now, and use something like this?
| errs = append(errs, errors.New("InterfaceMACAddresses must be defined when InterfaceType is mac")) | |
| path := field.NewPath("interfaceMACAddresses") | |
| err := field.Required(path, `must be defined when interfaceType is "mac"`) | |
| errs = append(errs, err) |
Also, it's a good practice to use the JSON field names in the error messages, as this is what users will be facing.
pkg/apis/k0s/v1beta1/cplb.go
Outdated
| k.VRRPInstances[i].Interface = iface | ||
| } | ||
| case VRRPInterfaceTypeMAC: | ||
| if len(k.VRRPInstances[i].InterfaceMACAddresses) == 0 { |
There was a problem hiding this comment.
We might want to distinguish nil from an empty slice. Would be different errors (required vs. invalid).
Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <jvaladas@mirantis.com>
juanluisvaladas
force-pushed
the
cplb-mac-nic
branch
from
4be3b5f to
4288463
Compare
Description
Allow to configure CPLB using interface mac address instead of interface name.
Type of change
How Has This Been Tested?
Checklist