Add per-node and global BGP config to KDD #468

robbrockbank · 2017-07-03T17:13:20Z

Adds per-node and global BGP config to KDD. Most importantly the global BGP config option to enabled/disable the node mesh is present.

This PR includes some changes to the client and etcd code too:
Reason:

etcd exposes the BGP config parameters in an odd way:
capitalization differs from the Felix "standard"
the node-2-node mesh is a json blob rather than a single boolean value.

Changes:

Hide the etcd oddities in the compat driver - so on the KVPair interface we'll just define camelcase options AsNumber NodeMeshEnabled (this is what Felix uses, should we? ... or should I change AsNumber to ASNumber?) parameters.
compat module will convert these to the original as_num and node_mesh.
In addition, the NodeMeshEnabled will be exposed in the KVPair as a boolean string - in compat we'll convert this to the json blob.

This allows the KVPair interface to expose sensible config names and values with consistent format as used by Felix. I think this is only used by etcd so I'm wondering if we should just be more explicit about that and do all conversion in that module.

Converting KVPairs / Keys in the compat module is a bit of a PITA because error responses from the etcd driver will have the modified Key as the Identifier -- so that means we have to jump through hoops to adjust the Identifier in the error back to the original Key. Nothing terrible, but a side effect of adding the identifier inforamation in the backend when it should really be handed by the client. Anyhoo.

caseydavenport

Ok, first round done!

caseydavenport · 2017-07-06T20:53:56Z

lib/backend/k8s/resources/globalbgpconfig.go

+}
+
+func (_ GlobalBgpConfigConverter) NameToKey(name string) (model.Key, error) {
+	return nil, fmt.Errorf("Mapping of Name to Key is not possible for global config")


Should probably say global bgp config

caseydavenport · 2017-07-06T21:58:48Z

lib/backend/compat/compat.go

 				Name:     "network_v6",
 			},
 		},
 	}

 	return primary, optional
 }
+
+// toDatastoreGlobalBGPConfigKey modifies the Global BGP Config key to the one required by
+// the datastore (for back-compatibility).


could you update the comments here so it says "for back-compatibility with what is expected in the etcdv2 datastore driver" or similar?

That would make it more clear why we need to do this.

caseydavenport · 2017-07-06T22:05:56Z

lib/backend/compat/compat.go

+func toDatastoreGlobalBGPConfig(d model.KVPair) *model.KVPair {
+	// Copy the KVPair, so we aren't modifying the original.
+	k := d.Key.(model.GlobalBGPConfigKey)
+	d.Key = toDatastoreGlobalBGPConfigKey(k)


Hm, so we're copying it so that toDatastoreGlobalBGPConfigKey doesn't modify the original, but then we're overwriting the original in the same line?

Are we just keeping this so that we can use k.Name below? If so, why don't we either:

Switch on the "datastore" representation of the name, OR

Just save off the same and use that in the switch below.

Or, am I misunderstanding?

Oh the Copy KVPair comment is bogus. It's already copied by virtue of us not passing in a pointer.

caseydavenport · 2017-07-06T22:11:44Z

lib/backend/k8s/k8s.go

@@ -397,15 +415,19 @@ func (c *KubeClient) Delete(d *model.KVPair) error {
 	log.Debugf("Performing 'Delete' for %+v", d)
 	switch d.Key.(type) {
 	case model.GlobalConfigKey:
-		return c.globalConfigClient.Delete(d)
+		return c.globalFelixConfigClient.Delete(d)


Hm, so this is nice because it makes it clear that it's for felix, however the model is called GlobalConfig, so it might be nice to keep this in-sync with what the model is called.

WDYT?

Yeah don't mind either way. Makes it a little less verbose and easy to rename.

caseydavenport · 2017-07-06T22:13:42Z

lib/backend/k8s/resources/globalbgpconfig.go

+		restClient:      r,
+		name:            GlobalBgpConfigTPRName,
+		resource:        GlobalBgpConfigResourceName,
+		description:     "Calico Global Configuration",


Calico Global BGP Configuration?

caseydavenport · 2017-07-06T22:15:27Z

lib/backend/k8s/resources/globalbgpconfig.go

+}
+
+func (_ GlobalBgpConfigConverter) ListInterfaceToKey(l model.ListInterface) model.Key {
+	pl := l.(model.GlobalBGPConfigListOptions)


What is pl short for? Could we just do name := l.(model.GlobalBGPConfigListOptions).Name?

caseydavenport · 2017-07-06T22:30:45Z

lib/backend/model/bgpconfig.go


 	"github.com/projectcalico/libcalico-go/lib/errors"
 )

 var (
-	typeGlobalBGPConfig = rawStringType
-	typeHostBGPConfig   = rawStringType
+	matchGlobalBGPConfig = regexp.MustCompile("^/?calico/bgp/v1/global/(.+)$")


Hm, what are the changes to this file for?

We didn't previously support list operations for config. I was adding that in, although for the life of me I can't remember why.

Oh I think it was for "no surprises" later on -- was thinking ahead about moving away from confd and using a common set of library function for both backends to configure bird. Strictly not required for this PR though.

caseydavenport · 2017-07-06T22:47:30Z

lib/client/config_e2e_test.go

@@ -89,6 +89,89 @@ var _ = testutils.E2eDatastoreDescribe("with config option API tests", testutils
 		Expect(cs).To(Equal(client.ConfigLocationGlobal))
 	})

+	It("should handle node IP in IP tunnel address", func() {


Right, so this fails with KDD because KDD gets the tunnel address from the Node.PodCIDR field, and we needed to move it to an etcd-only set of tests, yeah? Just making sure I understand.

Yup - and that is why I didn't just add node-specific Felix config in this PR. It's not required for KDD routing, and needs a little more thought regarding interactions with this field.

caseydavenport · 2017-07-06T22:48:29Z

lib/client/config_e2e_test.go

+		Expect(ip).To(BeNil())
+	})
+
+	It("should handle per-node felix config", func() {


This should work now in KDD right?

Nope - as with the above, I haven't added per-node Felix config to KDD. It isn't required for KDD routing.

I think we should add it soon, and then we should be feature complete (in terms of what can be configured through calicoctl).

caseydavenport · 2017-07-06T22:50:30Z

lib/errors/errors.go

+		return nil
+	}
+
+	switch e := err.(type) {


Hm, why do we need to create a new var e and then re-assign to err?

Couldn't we just do err.Identifier= id for each of these?

No we cannot.

It's a golang thang:

Firstly err is of type "error" so you can't reference the Identifier field. Secondly, when we do e := err.(type) we actually create a copy of the exception struct - so you are modifying a copy of rather than the original.

caseydavenport

A couple of minor nits left, but otherwise feel free to merge.

caseydavenport · 2017-07-07T16:15:25Z

lib/backend/k8s/k8s.go

 	go func() { done <- c.globalConfigClient.EnsureInitialized() }()
+	go func() { done <- c.globalBgpConfigClient.EnsureInitialized() }()

 	// Wait for all 4 registrations to complete and keep track of the last


Comment is out-of-date

robbrockbank changed the title ~~Add per-node and global BGP config to KDD~~ [DO NOT REVIEW] Add per-node and global BGP config to KDD Jul 3, 2017

robbrockbank changed the title ~~[DO NOT REVIEW] Add per-node and global BGP config to KDD~~ Add per-node and global BGP config to KDD Jul 3, 2017

robbrockbank mentioned this pull request Jul 5, 2017

Expose per-node listing as external interfaces #469

Merged

robbrockbank changed the title ~~Add per-node and global BGP config to KDD~~ [WIP] Add per-node and global BGP config to KDD Jul 5, 2017

robbrockbank force-pushed the bgp-config-types branch 2 times, most recently from 04b0ba3 to 2704959 Compare July 5, 2017 20:55

robbrockbank changed the title ~~[WIP] Add per-node and global BGP config to KDD~~ Add per-node and global BGP config to KDD Jul 5, 2017

robbrockbank requested a review from caseydavenport July 5, 2017 21:48

robbrockbank assigned caseydavenport Jul 5, 2017

robbrockbank force-pushed the bgp-config-types branch 3 times, most recently from 86856cb to 8b8987c Compare July 6, 2017 18:39

caseydavenport requested changes Jul 6, 2017

View reviewed changes

caseydavenport approved these changes Jul 7, 2017

View reviewed changes

Add remaining BGP configuration types

b26b26e

robbrockbank force-pushed the bgp-config-types branch from f582b2b to b26b26e Compare July 7, 2017 16:43

robbrockbank merged commit 4ca18ea into projectcalico:master Jul 7, 2017

robbrockbank deleted the bgp-config-types branch July 7, 2017 16:50

caseydavenport added this to the Calico v2.4.0 milestone Jul 11, 2017

lcnsir mentioned this pull request Sep 6, 2017

node_mesh set to enable: false by default in kubernetes cluster on Openstack projectcalico/calico#1081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add per-node and global BGP config to KDD #468

Add per-node and global BGP config to KDD #468

robbrockbank commented Jul 3, 2017 •

edited

Loading

caseydavenport left a comment

caseydavenport Jul 6, 2017

caseydavenport Jul 6, 2017

caseydavenport Jul 6, 2017

robbrockbank Jul 6, 2017

caseydavenport Jul 6, 2017

robbrockbank Jul 6, 2017

caseydavenport Jul 6, 2017

caseydavenport Jul 6, 2017

caseydavenport Jul 6, 2017

robbrockbank Jul 6, 2017

robbrockbank Jul 6, 2017

caseydavenport Jul 6, 2017

robbrockbank Jul 6, 2017

caseydavenport Jul 6, 2017

robbrockbank Jul 6, 2017

caseydavenport Jul 6, 2017

robbrockbank Jul 6, 2017 •

edited

Loading

caseydavenport left a comment

caseydavenport Jul 7, 2017

Add per-node and global BGP config to KDD #468

Add per-node and global BGP config to KDD #468

Conversation

robbrockbank commented Jul 3, 2017 • edited Loading

caseydavenport left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbrockbank Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

caseydavenport left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbrockbank commented Jul 3, 2017 •

edited

Loading

robbrockbank Jul 6, 2017 •

edited

Loading