Extend ConfigMap to store fwrule names #278

csbell · 2017-02-15T01:06:21Z

GLBC users can store a providerUid which is used in lieu of Uid when constructing l7 firewall rule names. This can be leveraged to resolve issues seen in Kubernetes #37306.

Backwards compatibility is retained by using Uid for l7 firewall rules unless providerUid is set.

k8s-reviewable · 2017-02-15T01:07:25Z

This change is

coveralls · 2017-02-15T01:25:51Z

Coverage decreased (-0.03%) to 45.94% when pulling 85a3451 on csbell:fw-name into a8b8967 on kubernetes:master.

bprashanth

Made a first pass, I think the structure of the code will change if you have the Get/Put methods take k/v pairs instead of entire maps so I'll wait for you to upload another pr or reply to existing comments before more review.

bprashanth · 2017-02-15T18:27:59Z

controllers/gce/main.go

-		} else if err != nil {
-			glog.Errorf("Failed to reconcile cluster uid %v, currently set to %v", err, existing)
+		// First look for changes in firewallConfigMap.
+		vaultmap, found, err := uid_vault.Get()


Please avoid "_" in names, https://golang.org/doc/effective_go.html#mixed-cap

bprashanth · 2017-02-15T18:30:49Z

controllers/gce/main.go

+		// First look for changes in firewallConfigMap.
+		vaultmap, found, err := uid_vault.Get()
+		if !found || err != nil {
+			glog.Errorf("Can't read uidConfigMap %v", uidConfigMapName)


Why eat this error insted of logging it?

bprashanth · 2017-02-15T18:52:31Z

controllers/gce/storage/configmaps.go

@@ -50,46 +46,59 @@ type ConfigMapVault struct {
 // If this method returns an error, it's guaranteed to be apiserver flake.
 // If the error is a not found error it sets the boolean to false and
 // returns and error of nil instead.
-func (c *ConfigMapVault) Get() (string, bool, error) {
+func (c *ConfigMapVault) Get() (map[string]string, bool, error) {


The problem with returning maps is the following:

package main import ( "fmt" "sync" "time" ) type mapStore struct { mapValue map[string]string l sync.Mutex } func (m *mapStore) getMap() map[string]string { m.l.Lock() defer m.l.Unlock() return m.mapValue } func (m *mapStore) set(k, v string) { m.l.Lock() defer m.l.Unlock() m.mapValue[k] = v } func main() { m := mapStore{mapValue: map[string]string{"foo": "bar"}} retrievedMap := m.getMap() var wg sync.WaitGroup wg.Add(1) go func() { for i := 0; i < 1000; i++ { fmt.Printf("%v", retrievedMap) } wg.Done() }() for i := 0; i < 1000; i++ { m.set("foo", fmt.Sprintf("%d", i)) time.Sleep(10) } wg.Wait() fmt.Printf("%v\n", retrievedMap) }

running with go run -race main.go reveals a race. Even if the race is not going to impact the actual program, go will complain about it in CI and cause tests to fail, and every programmer reading this code will probably need to convince themselves it's not racy.

Suggest passing the key down into this func, retrieving the string value under a lock and returning the string.

Thanks for the example -- I think the summary is just that we'll be issuing more configmap reads but won't be exposing the map which I think per go is fundamentally not threadsafe.

bprashanth · 2017-02-15T19:04:35Z

controllers/gce/storage/configmaps.go

-// Put stores the given UID in the cluster config map.
-func (c *ConfigMapVault) Put(uid string) error {
+// Put stores the given UID and EUID in the cluster config map.
+func (c *ConfigMapVault) Put(keyvals map[string]string) error {


Same thing here, suggest Put(k, v string) if possible, to reduce the risk of a map clobber

map was not being clobbered but we are moving to key->val puts.

bprashanth · 2017-02-15T19:04:50Z

controllers/gce/storage/configmaps_test.go

 	"testing"

 	"k8s.io/kubernetes/pkg/api"
 )

 func TestConfigMapUID(t *testing.T) {
 	vault := NewFakeConfigMapVault(api.NamespaceSystem, "ingress-uid")
-	uid := ""
-	k, exists, err := vault.Get()
+	//uid := ""


why is this commendte out?

bprashanth · 2017-02-15T19:13:15Z

controllers/gce/utils/utils.go

+func (n *Namer) GetFirewallName() string {
+	n.nameLock.Lock()
+	defer n.nameLock.Unlock()
+	return n.firewallName


Suggest defaulting this to clusterName if firewallName is unset, so we don't inadvertently break non-fed deployments by changing the name of the firewall and leaking it.

bprashanth · 2017-02-15T19:15:12Z

controllers/gce/storage/configmaps.go

+		updated_needed := false
+
+		// Dump everything from keyvals into data.
+		for key, val := range keyvals {


Yeah batch storing k,v pairs makes it harder to reason about consistency. Ideally the call sites that care about the fw name should only get/update the one key. That way, if there's a conflict, we know we own that keyspace.

bprashanth · 2017-02-15T19:18:17Z

controllers/gce/storage/configmaps.go

 }

-// Put stores the given UID in the cluster config map.
-func (c *ConfigMapVault) Put(uid string) error {
+// Put stores the given UID and EUID in the cluster config map.


The comment conflicts with the code a little, i.e we take a random map of any values not just UID and EUID right?

right, fixed.

bprashanth · 2017-02-15T19:20:01Z

controllers/gce/storage/configmaps.go

+		// Dump everything from keyvals into data.
+		for key, val := range keyvals {
+			if val_stored, ok := data[key]; !ok {
+				glog.Infof("Configmap %v will be updated with %v = %v", cfgMapKey, key, val)


This log line is misleading, it makes me think only the given k/v pair is updated but we actually plug the entire keyval map into the apiObj right?

Right. N/A now with key/val puts

bprashanth · 2017-02-15T19:23:13Z

controllers/gce/main.go

-		glog.Infof("Using saved cluster uid %q", existingUID)
-		return existingUID, nil
-	} else if err != nil {
+		if val, found := vaultmap[cm_key]; found {


s/vaultmap/vaultMap

csbell · 2017-02-16T03:33:43Z

Thanks for the first round of comments -- I've re-pushed the CL. The major differences are key/val-based puts and adding some unit testing.

However, I still plan on holding off until I can adequately integrate this with federation and test out the functionality.

coveralls · 2017-02-16T03:44:15Z

Coverage increased (+0.04%) to 46.014% when pulling c25aebca924449bdcac34326f309a8166c125e2a on csbell:fw-name into 9bba947 on kubernetes:master.

csbell · 2017-02-23T00:24:49Z

Kubernetes driver for this is here: kubernetes/kubernetes#41942

PTAL @nicksardo

coveralls · 2017-02-23T00:28:41Z

Coverage increased (+0.004%) to 46.167% when pulling f498b1c on csbell:fw-name into 592814b on kubernetes:master.

nicksardo · 2017-02-28T03:04:13Z

controllers/gce/utils/utils.go

@@ -103,6 +104,14 @@ func NewNamer(clusterName string) *Namer {
 	return namer
 }

+// NewNamer creates a new namer with a Firewall Name
+func NewNamerWithFirewall(clusterName string, firewallName string) *Namer {


NewNamer is only used in tests; perhaps we should add the firewall name there instead of creating NewNamerWithFirewall. Also, the first string type can be omitted.

rule information.

nicksardo · 2017-02-28T18:51:40Z

controllers/gce/main.go

+//       This is effectively a client override via a command line flag.
+// else, check configmap under 'configmap_name' as a key and if found, use the associated value
+// else, return an empty 'name' and pass along an error iff the configmap lookup is erroneous.
+func getFlagOrLookupVault(cfgVault *storage.ConfigMapVault, cm_key string, name string) (string, error) {


I'm not a fan of this func name since you're supplying the override and doing nothing flag specific.

Furthermore, I see there's a clusterName flag but not a firewallName flag. Do we want one?

The name reflects the time where there was a flag fore firewallName. Since talking to beeps, there's no need to introduce a new flag since there will be a new configration mechanism coming up over the next few months that will make flags obsolete.

nicksardo · 2017-02-28T18:55:32Z

controllers/gce/main.go

-		glog.Infof("Using saved cluster uid %q", existingUID)
-		return existingUID, nil
+		glog.Infof("Using %v = %q saved in ConfigMap", cm_key, val)
+		return val, nil
 	} else if err != nil {


This is an master-existing nitpick of mine. Could we reorder the err != nil before the if found?

Thank you for fixing it in the newNamer! https://github.com/kubernetes/ingress/blob/master/controllers/gce/main.go#L269

csbell

PTAL.

csbell · 2017-02-28T23:56:32Z

controllers/gce/main.go

+//       This is effectively a client override via a command line flag.
+// else, check configmap under 'configmap_name' as a key and if found, use the associated value
+// else, return an empty 'name' and pass along an error iff the configmap lookup is erroneous.
+func getFlagOrLookupVault(cfgVault *storage.ConfigMapVault, cm_key string, name string) (string, error) {


The name reflects the time where there was a flag fore firewallName. Since talking to beeps, there's no need to introduce a new flag since there will be a new configration mechanism coming up over the next few months that will make flags obsolete.

csbell · 2017-02-28T23:56:42Z

controllers/gce/main.go

-		glog.Infof("Using saved cluster uid %q", existingUID)
-		return existingUID, nil
+		glog.Infof("Using %v = %q saved in ConfigMap", cm_key, val)
+		return val, nil
 	} else if err != nil {


csbell · 2017-02-28T23:57:01Z

controllers/gce/utils/utils.go

@@ -103,6 +104,14 @@ func NewNamer(clusterName string) *Namer {
 	return namer
 }

+// NewNamer creates a new namer with a Firewall Name
+func NewNamerWithFirewall(clusterName string, firewallName string) *Namer {


nicksardo · 2017-03-01T22:15:59Z

/lgtm

k8s-ci-robot · 2017-03-01T22:16:00Z

@nicksardo: you can't LGTM a PR unless you are an assignee.

In response to this comment:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

csbell · 2017-03-02T00:48:11Z

Need another lgtm

nicksardo · 2017-03-02T01:14:20Z

/lgtm

csbell · 2017-03-02T04:48:33Z

With /approve ?

nicksardo · 2017-03-02T05:07:13Z

/approve

coveralls · 2017-03-02T19:47:49Z

Coverage increased (+0.04%) to 45.393% when pulling 68097e9 on csbell:fw-name into f5f9f5e on kubernetes:master.

@madhusudancs

Automatic merge from submit-queue Add ProviderUid support to Federated Ingress This PR (along with GLBC support [here](kubernetes/ingress-nginx#278)) is a proposed fix for #39989. The Ingress controller uses a configMap reconciliation process to ensure that all underlying ingresses agree on a unique UID. This works for all of GLBC's resources except firewalls which need their own cluster-unique UID. This PR introduces a ProviderUid which is maintained and synchronized cross-cluster much like the UID. We chose to derive the ProviderUid from the cluster name (via md5 hash). Testing here is augmented to guarantee that configMaps are adequately propagated prior to Ingress creation. ```release-note Federated Ingress over GCE no longer requires separate firewall rules to be created for each cluster to circumvent flapping firewall health checks. ``` cc @madhusudancs @quinton-hoole

@madhusudancs

Automatic merge from submit-queue Add ProviderUid support to Federated Ingress This PR (along with GLBC support [here](kubernetes/ingress-nginx#278)) is a proposed fix for #39989. The Ingress controller uses a configMap reconciliation process to ensure that all underlying ingresses agree on a unique UID. This works for all of GLBC's resources except firewalls which need their own cluster-unique UID. This PR introduces a ProviderUid which is maintained and synchronized cross-cluster much like the UID. We chose to derive the ProviderUid from the cluster name (via md5 hash). Testing here is augmented to guarantee that configMaps are adequately propagated prior to Ingress creation. ```release-note Federated Ingress over GCE no longer requires separate firewall rules to be created for each cluster to circumvent flapping firewall health checks. ``` cc @madhusudancs @quinton-hoole

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 15, 2017

aledbf assigned bprashanth Feb 15, 2017

aledbf added the backend/gce label Feb 15, 2017

bprashanth reviewed Feb 15, 2017

View reviewed changes

csbell force-pushed the fw-name branch from 85a3451 to c25aebc Compare February 16, 2017 03:25

csbell force-pushed the fw-name branch from c25aebc to f498b1c Compare February 23, 2017 00:09

csbell mentioned this pull request Feb 23, 2017

Add ProviderUid support to Federated Ingress kubernetes/kubernetes#41942

Merged

nicksardo reviewed Feb 28, 2017

View reviewed changes

First stab at extending the "uid" configmap to store firewall

b259c9b

rule information.

nicksardo reviewed Feb 28, 2017

View reviewed changes

csbell force-pushed the fw-name branch from f498b1c to 8433338 Compare February 28, 2017 23:55

csbell commented Feb 28, 2017

View reviewed changes

nicksardo self-assigned this Mar 2, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2017

Better logging and address review comments

68097e9

csbell force-pushed the fw-name branch from 8433338 to 68097e9 Compare March 2, 2017 19:24

nicksardo merged commit a6e3822 into kubernetes:master Mar 6, 2017

csbell mentioned this pull request Mar 6, 2017

Need new GLBC release for Kubernetes Federation fixes #381

Closed

madhusudancs mentioned this pull request Mar 13, 2017

GCE Ingress controller (GLBC) should install a firewall rule per Ingress object per cluster kubernetes/kubernetes#37306

Closed

ghost mentioned this pull request Apr 26, 2017

Federated ingress creates flapping backends and health checks kubernetes/kubernetes#36327

Closed

irfanurrehman mentioned this pull request Oct 30, 2017

Federated ingress creates flapping backends and health checks kubernetes-retired/federation#79

Closed

nicksardo mentioned this pull request Jan 24, 2018

Delegate creating firewall rule to ingress-gce GoogleCloudPlatform/k8s-multicluster-ingress#122

Open

Extend ConfigMap to store fwrule names #278

Extend ConfigMap to store fwrule names #278

Conversation

csbell commented Feb 15, 2017 • edited Loading

k8s-reviewable commented Feb 15, 2017

coveralls commented Feb 15, 2017

bprashanth 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

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

csbell commented Feb 16, 2017

coveralls commented Feb 16, 2017

csbell commented Feb 23, 2017

coveralls commented Feb 23, 2017

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

csbell 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

nicksardo commented Mar 1, 2017

k8s-ci-robot commented Mar 1, 2017

csbell commented Mar 2, 2017

nicksardo commented Mar 2, 2017

csbell commented Mar 2, 2017

nicksardo commented Mar 2, 2017

coveralls commented Mar 2, 2017

csbell commented Feb 15, 2017 •

edited

Loading