Conversation
api/v1/installation_types.go
Outdated
| // confd and BIRD program that route. When ClusterRoutingMode is Felix, it is expected that Felix will program that route. | ||
| // Felix always programs such routes for IP Pools with vxlanMode: Always or vxlanMode: CrossSubnet. [Default: BIRD] | ||
| // +optional | ||
| // +kubebuilder:validation:Enum=BIRD;Felix |
There was a problem hiding this comment.
I think we usually put the Enum validation on the enum type so it can be re-used wherever the enum is used.
| reqLogger logr.Logger, | ||
| ) (bool, error) { | ||
| updated := false | ||
| desiredValue := "Enabled" |
There was a problem hiding this comment.
Are there constants we should be using for this?
There was a problem hiding this comment.
No, for these options in the BGPConfig and FelixConfig we use string type, and we rely on kubebuilder to validate inputs.
We use both approach though. But it's cleaner to declare constants. I'll address it separately since needs merging a few PRs.
| desiredValue = "Disabled" | ||
| } | ||
|
|
||
| if bgpConfig.Spec.ProgramClusterRoutes == nil || *bgpConfig.Spec.ProgramClusterRoutes != desiredValue { |
There was a problem hiding this comment.
Hm, we usually try to avoid overwriting end-user modifications of FelixConfig, etc using an annotation. Technically I suppose we should do it here... but we're actually pretty inconsistent about it.
I've started a prototype of how to make this more easily implemented: #4523
But I don't want to block this PR on it. So I'll just make sure to incorporate this into my changes.
There was a problem hiding this comment.
Thanks for the input and including the case in your PR.
| func (r *ReconcileInstallation) setClusterRoutingOnFelixConfiguration( | ||
| ctx context.Context, | ||
| install *operatorv1.Installation, | ||
| fc *v3.FelixConfiguration, | ||
| reqLogger logr.Logger, | ||
| ) (bool, error) { | ||
| updated := false | ||
| desiredValue := "Disabled" |
There was a problem hiding this comment.
ctx is unused in this function — can be removed.
| func (r *ReconcileInstallation) setClusterRoutingOnBGPConfiguration( | ||
| ctx context.Context, | ||
| install *operatorv1.Installation, | ||
| bgpConfig *v3.BGPConfiguration, | ||
| reqLogger logr.Logger, | ||
| ) (bool, error) { | ||
| updated := false | ||
| desiredValue := "Enabled" |
There was a problem hiding this comment.
ctx is unused here too.
Also, both this and setClusterRoutingOnFelixConfiguration above have stale doc comments — they both say setDefaultOnBGPConfiguration which looks like a copy-paste leftover. Should match the actual function names.
| // In BIRD cluster routing mode, IPIP currently requires BGP to be running in order to program routes. | ||
| if birdClusterRoutingMode && | ||
| (instance.Spec.CalicoNetwork.BGP == nil || *instance.Spec.CalicoNetwork.BGP == operatorv1.BGPDisabled) { | ||
| return fmt.Errorf("IPIP encapsulation requires that BGP is enabled") |
There was a problem hiding this comment.
The error message still just says "IPIP encapsulation requires that BGP is enabled" — now that there's a way to fix this without BGP (by setting ClusterRoutingMode: Felix), should the error message mention that option? Same for the unencapsulated case below. Otherwise users won't know about the escape hatch.
| Expect(*bgpConfig.Spec.ProgramClusterRoutes).To(Equal("Disabled")) | ||
| }) | ||
|
|
||
| It("should set vxlanVNI to 10000 when provider is DockerEE", func() { |
There was a problem hiding this comment.
Good coverage for the happy paths. Might be worth adding a test for the case where the default BGPConfiguration doesn't already exist — to verify the create-on-patch path in PatchBGPConfiguration works correctly (or to verify we get an appropriate error if the prerequisite calico PR hasn't landed).
There was a problem hiding this comment.
Interesting. Is create-on-patch a feature of Operator? I was trying to figure out how felixconfiguration is created.
There was a problem hiding this comment.
I see! it's part of the patching logic.
| // Verify Calico settings, if specified. | ||
| if instance.Spec.CalicoNetwork != nil { | ||
| bpfDataplane := instance.Spec.CalicoNetwork.LinuxDataplane != nil && *instance.Spec.CalicoNetwork.LinuxDataplane == operatorv1.LinuxDataplaneBPF | ||
| birdClusterRoutingMode := !felixProgramsClusterRoutes(instance) |
There was a problem hiding this comment.
nit: I find this a bit confusing with the negation.
| birdClusterRoutingMode := !felixProgramsClusterRoutes(instance) | |
| felixClusterRouting := felixProgramsClusterRoutes(instance) |
3 participants
Description
Currently, configuring cluster routing mode is exposed to users through
programClusterRoutesin both FelixConfig and BGPConfig.This PR introduces a new config option to the installation resource, named
clusterRoutingModeto provide better UX for users to set cluster routing mode. Supported values areBIRD(default) andFelix.When set to :
BIRD, Calico route engine, i.e. BIRD, is responsible for programming IPIP and no-encap routes. This effectively updatesprogramClusterRoutesinFelixConfigtoDisabledand inBGPConfigtoEnabledFelix, Felix will program those routes instead. As suchprogramClusterRoutesis updated inFelixConfigtoDisabledand inBGPConfigtoEnabledThis PR assumes
BGPConfigurationresources is always available and only performs patch operation. As such The following PR is need which created the defaultBGPConfigurationresource on startup:projectcalico/calico#12045
Operator now needs to create
bgpconfigurationresources which is handed in this PR: projectcalico/calico#12080Release Note
For PR author
make gen-filesmake gen-versionsFor PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bugif this is a bugfix.kind/enhancementif this is a a new feature.enterpriseif this PR applies to Calico Enterprise only.