Support external frr-k8s #483
Conversation
fedepaol
force-pushed
the
cnodeploysfrrk8s
branch
3 times, most recently
from
65f3ec1
to
678466c
Compare
Signed-off-by: Federico Paolinelli <[email protected]>
fedepaol
force-pushed
the
cnodeploysfrrk8s
branch
from
678466c
to
b37023d
Compare
| @@ -103,6 +104,8 @@ type MetalLBSpec struct { | |||
| type FRRK8SConfig struct { | |||
| // A list of cidrs we want always to block for incoming routes | |||
| AlwaysBlock []string `json:"alwaysBlock,omitempty"` | |||
| // The namespace frr-k8s is running on in case of frr-k8s external mode | |||
There was a problem hiding this comment.
mind adding something about what happens when this is empty but mode is "external" here?
There was a problem hiding this comment.
We need to provide a default value to avoid such scenario.
controllers/metallb_controller.go
Outdated
| @@ -76,6 +79,9 @@ var FRRK8SChartPath = FRRK8SChartPathController | |||
| // +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch | |||
| // +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=create;delete;get;update;patch;list;watch | |||
| // +kubebuilder:rbac:groups="",resources=secrets,verbs=create;delete;get;update;patch;list;watch | |||
| // +kubebuilder:rbac:groups=metallb.io,resources=metallbs,verbs=get;list;watch;update; | |||
There was a problem hiding this comment.
isn't this redundant with the first one?
// +kubebuilder:rbac:groups=metallb.io,resources=metallbs,verbs=get;list;watch;create;update;patch;delete
There was a problem hiding this comment.
yep probably the result of a copy-paste
pkg/params/params.go
Outdated
| res.MustDeployFRRK8sFromCNO = true | ||
| } | ||
|
|
||
| res.FRRK8sDefaultNamespace = os.Getenv("FRRK8S_DEFAULT_NAMESPACE") |
There was a problem hiding this comment.
not sure I understand the rationale (and the naming - something that is "default" being fed from outside) of this env var when there's the namespace in the crd. mind expanding a bit? also what happens when it is empty as the charts don't allow external + empty ns:
- --frrk8s-namespace={{ required "namespace is required when frrk8s is external" .Values.frrk8s.namespace }}
There was a problem hiding this comment.
yep we probably need to define the default better
| logger := r.Log.WithName("syncMetalLBResources") | ||
| logger.Info("Start") | ||
|
|
||
| bgpType := params.BGPType(config, r.EnvConfig.IsOpenshift) | ||
| if r.EnvConfig.MustDeployFRRK8sFromCNO && r.EnvConfig.IsOpenshift && (bgpType == metallbv1beta1.FRRK8sExternalMode) { |
There was a problem hiding this comment.
not sure I understand the MustDeployFRRK8sFromCNO env var, I thought "external" in openshift always comes from cno?
There was a problem hiding this comment.
nope, the user will deploy metallb as always. This is to force the openshift specific behaviour only in ocp and only when the user didn't opt-in to a specific mode.
fedepaol
force-pushed
the
cnodeploysfrrk8s
branch
3 times, most recently
from
8bd8abf
to
b76a91f
Compare
| return err | ||
| } | ||
| } | ||
|
|
||
| err := config.Validate() |
There was a problem hiding this comment.
should we add a validation for external + ns must be specified together?
There was a problem hiding this comment.
I guess, with the premise that with the current defaults, to get into this scenario the user must willingly remove the default env variable and shoot himself in the foot.
We allow setting the external mode from the MetalLB crd, and we also allow to specify the namespace it is running on. Signed-off-by: Federico Paolinelli <[email protected]>
Starting from 4.17, the Cluster Network Operator will deploy frr-k8s. Here we switch external to be the default deployment method in openshift, and add the code to instruct CNO to deploy frr-k8s (and to wait until the right version is available). Signed-off-by: Federico Paolinelli <[email protected]>
Signed-off-by: Federico Paolinelli <[email protected]>
fedepaol
force-pushed
the
cnodeploysfrrk8s
branch
from
b76a91f
to
fb55bcb
Compare
|
obsoleted by #487 |
Is this a BUG FIX or a FEATURE ?:
What this PR does / why we need it:
Special notes for your reviewer:
Release note: