-
Notifications
You must be signed in to change notification settings - Fork 150
cluster routing mode #4511
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
cluster routing mode #4511
Changes from all commits
c2091dd
4f2a631
b33923f
8aa8c7b
f074258
1e389f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1144,9 +1144,34 @@ func (r *ReconcileInstallation) Reconcile(ctx context.Context, request reconcile | |
| if err != nil { | ||
| return false, err | ||
| } | ||
| return u || u2, nil | ||
|
|
||
| // Configure cluster routing mode. | ||
| u3, err := setClusterRoutingOnFelixConfiguration(instance, fc, reqLogger) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| updated := u || u2 || u3 | ||
| return updated, nil | ||
| }) | ||
| if err != nil { | ||
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
| // Set any non-default BGPConfiguration values that we need. | ||
| _, err = utils.PatchBGPConfiguration(ctx, r.client, func(bgpConfig *v3.BGPConfiguration) (bool, error) { | ||
| // Configure cluster routing mode. | ||
| u, err := setClusterRoutingOnBGPConfiguration(instance, bgpConfig, reqLogger) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| return u, nil | ||
| }) | ||
| if err != nil { | ||
| // Since, programClusterRoutes in FelixConfiguration is already updated earlier, | ||
| // failure in updating programClusterRouting in BGPConfiguration, essentially results in inconsistency | ||
| // between the configuration of BIRD and Felix in programming cluster routes, until the next reconcile convergence | ||
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
|
|
@@ -2011,6 +2036,59 @@ func (r *ReconcileInstallation) setDefaultsOnFelixConfiguration(ctx context.Cont | |
| return updated, nil | ||
| } | ||
|
|
||
| // setClusterRoutingOnFelixConfiguration sets programClusterRoutes in the FelixConfiguration resource | ||
| // based on the value of clusterRoutingMode in the install config. | ||
| func setClusterRoutingOnFelixConfiguration( | ||
| install *operatorv1.Installation, | ||
| fc *v3.FelixConfiguration, | ||
| reqLogger logr.Logger, | ||
| ) (bool, error) { | ||
| updated := false | ||
| desiredValue := "Disabled" | ||
| if felixProgramsClusterRoutes(install) { | ||
| desiredValue = "Enabled" | ||
| } | ||
|
|
||
| if fc.Spec.ProgramClusterRoutes == nil || *fc.Spec.ProgramClusterRoutes != desiredValue { | ||
| fc.Spec.ProgramClusterRoutes = &desiredValue | ||
| updated = true | ||
| reqLogger.Info("Patching FelixConfiguration", "programClusterRoutes", desiredValue) | ||
| } | ||
|
|
||
| return updated, nil | ||
| } | ||
|
|
||
| // setClusterRoutingOnBGPConfiguration sets programClusterRoutes in the BGPConfiguration resource | ||
| // based on the value of clusterRoutingMode in the install config. | ||
| func setClusterRoutingOnBGPConfiguration( | ||
| install *operatorv1.Installation, | ||
| bgpConfig *v3.BGPConfiguration, | ||
| reqLogger logr.Logger, | ||
| ) (bool, error) { | ||
| updated := false | ||
| desiredValue := "Enabled" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there constants we should be using for this?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, for these options in the BGPConfig and FelixConfig we use string type, and we rely on kubebuilder to validate inputs. |
||
|
|
||
| if felixProgramsClusterRoutes(install) { | ||
| desiredValue = "Disabled" | ||
| } | ||
|
|
||
| if bgpConfig.Spec.ProgramClusterRoutes == nil || *bgpConfig.Spec.ProgramClusterRoutes != desiredValue { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the input and including the case in your PR. |
||
| bgpConfig.Spec.ProgramClusterRoutes = &desiredValue | ||
| updated = true | ||
| reqLogger.Info("Patching BGPConfiguration", "programClusterRoutes", desiredValue) | ||
| } | ||
|
|
||
| return updated, nil | ||
| } | ||
|
|
||
| func felixProgramsClusterRoutes(install *operatorv1.Installation) bool { | ||
| if install.Spec.CalicoNetwork != nil && install.Spec.CalicoNetwork.ClusterRoutingMode != nil && | ||
| *install.Spec.CalicoNetwork.ClusterRoutingMode == operatorv1.ClusterRoutingModeFelix { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // setBPFUpdatesOnFelixConfiguration will take the passed in fc and update any BPF properties needed | ||
| // based on the install config and the daemonset. | ||
| func (r *ReconcileInstallation) setBPFUpdatesOnFelixConfiguration(ctx context.Context, install *operatorv1.Installation, fc *v3.FelixConfiguration, reqLogger logr.Logger) (bool, error) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1427,6 +1427,92 @@ var _ = Describe("Testing core-controller installation", func() { | |
| Expect(pullSecret.Kind).To(Equal("Installation")) | ||
| }) | ||
|
|
||
| It("should correctly patch FelixConfig and BGPConfig with ClusterRouteMode not set", func() { | ||
| cr.Spec.CalicoNetwork = &operator.CalicoNetworkSpec{} | ||
| Expect(c.Create(ctx, cr)).NotTo(HaveOccurred()) | ||
| _, err := r.Reconcile(ctx, reconcile.Request{}) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| fc := &v3.FelixConfiguration{} | ||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, fc) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| Expect(fc.Spec.ProgramClusterRoutes).NotTo(BeNil()) | ||
| Expect(*fc.Spec.ProgramClusterRoutes).To(Equal("Disabled")) | ||
|
|
||
| bgpConfig := &v3.BGPConfiguration{} | ||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, bgpConfig) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| Expect(bgpConfig.Spec.ProgramClusterRoutes).NotTo(BeNil()) | ||
| Expect(*bgpConfig.Spec.ProgramClusterRoutes).To(Equal("Enabled")) | ||
| }) | ||
|
|
||
| It("should correctly patch FelixConfig and BGPConfig with ClusterRouteMode set to BIRD", func() { | ||
| bird := operator.ClusterRoutingModeBIRD | ||
| cr.Spec.CalicoNetwork = &operator.CalicoNetworkSpec{ClusterRoutingMode: &bird} | ||
| Expect(c.Create(ctx, cr)).NotTo(HaveOccurred()) | ||
| _, err := r.Reconcile(ctx, reconcile.Request{}) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| fc := &v3.FelixConfiguration{} | ||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, fc) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| Expect(fc.Spec.ProgramClusterRoutes).NotTo(BeNil()) | ||
| Expect(*fc.Spec.ProgramClusterRoutes).To(Equal("Disabled")) | ||
|
|
||
| bgpConfig := &v3.BGPConfiguration{} | ||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, bgpConfig) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| Expect(bgpConfig.Spec.ProgramClusterRoutes).NotTo(BeNil()) | ||
| Expect(*bgpConfig.Spec.ProgramClusterRoutes).To(Equal("Enabled")) | ||
| }) | ||
|
|
||
| It("should correctly patch FelixConfig and BGPConfig with ClusterRouteMode set to Felix", func() { | ||
| felix := operator.ClusterRoutingModeFelix | ||
| cr.Spec.CalicoNetwork = &operator.CalicoNetworkSpec{ClusterRoutingMode: &felix} | ||
| Expect(c.Create(ctx, cr)).NotTo(HaveOccurred()) | ||
| _, err := r.Reconcile(ctx, reconcile.Request{}) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| fc := &v3.FelixConfiguration{} | ||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, fc) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| Expect(fc.Spec.ProgramClusterRoutes).NotTo(BeNil()) | ||
| Expect(*fc.Spec.ProgramClusterRoutes).To(Equal("Enabled")) | ||
|
|
||
| bgpConfig := &v3.BGPConfiguration{} | ||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, bgpConfig) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| Expect(bgpConfig.Spec.ProgramClusterRoutes).NotTo(BeNil()) | ||
| Expect(*bgpConfig.Spec.ProgramClusterRoutes).To(Equal("Disabled")) | ||
| }) | ||
|
|
||
| It("should create the default BGPConfig and FelixConfig with ClusterRoutingMode set", func() { | ||
| bgpConfig := &v3.BGPConfiguration{} | ||
| err := c.Get(ctx, types.NamespacedName{Name: "default"}, bgpConfig) | ||
| Expect(err).Should(HaveOccurred()) | ||
|
|
||
| fc := &v3.FelixConfiguration{} | ||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, fc) | ||
| Expect(err).Should(HaveOccurred()) | ||
|
|
||
| felix := operator.ClusterRoutingModeFelix | ||
| cr.Spec.CalicoNetwork = &operator.CalicoNetworkSpec{ClusterRoutingMode: &felix} | ||
| Expect(c.Create(ctx, cr)).NotTo(HaveOccurred()) | ||
| _, err = r.Reconcile(ctx, reconcile.Request{}) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, fc) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| Expect(fc.Spec.ProgramClusterRoutes).NotTo(BeNil()) | ||
| Expect(*fc.Spec.ProgramClusterRoutes).To(Equal("Enabled")) | ||
|
|
||
| bgpConfig = &v3.BGPConfiguration{} | ||
| err = c.Get(ctx, types.NamespacedName{Name: "default"}, bgpConfig) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| Expect(bgpConfig.Spec.ProgramClusterRoutes).NotTo(BeNil()) | ||
| Expect(*bgpConfig.Spec.ProgramClusterRoutes).To(Equal("Disabled")) | ||
| }) | ||
|
|
||
| It("should set vxlanVNI to 10000 when provider is DockerEE", func() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. Is create-on-patch a feature of Operator? I was trying to figure out how felixconfiguration is created.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see! it's part of the patching logic. |
||
| cr.Spec.KubernetesProvider = operator.ProviderDockerEE | ||
| Expect(c.Create(ctx, cr)).NotTo(HaveOccurred()) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.