From 85a34516b364d0e831c0df7294c71388cc826d00 Mon Sep 17 00:00:00 2001 From: Christian Bell Date: Tue, 14 Feb 2017 16:48:07 -0800 Subject: [PATCH] First stab at extending the "uid" configmap to store firewall rule information. --- controllers/gce/main.go | 117 +++++++++++++++------ controllers/gce/storage/configmaps.go | 59 ++++++----- controllers/gce/storage/configmaps_test.go | 57 +++++++--- controllers/gce/utils/utils.go | 41 +++++++- 4 files changed, 200 insertions(+), 74 deletions(-) diff --git a/controllers/gce/main.go b/controllers/gce/main.go index 0f1f8e981..ba5f3594f 100644 --- a/controllers/gce/main.go +++ b/controllers/gce/main.go @@ -120,6 +120,9 @@ var ( healthzPort = flags.Int("healthz-port", lbApiPort, `Port to run healthz server. Must match the health check port in yaml.`) + + firewallName = flags.String("firewall-name", "", + `Name to use for firewall rule names in lieu of cluster-uid.`) ) func registerHandlers(lbc *controller.LoadBalancerController) { @@ -213,7 +216,7 @@ func main() { if *inCluster || *useRealCloud { // Create cluster manager - namer, err := newNamer(kubeClient, *clusterName) + namer, err := newNamer(kubeClient, *clusterName, *firewallName) if err != nil { glog.Fatalf("%v", err) } @@ -245,32 +248,98 @@ func main() { } } -func newNamer(kubeClient client.Interface, clusterName string) (*utils.Namer, error) { +func newNamer(kubeClient client.Interface, clusterName string, fwName string) (*utils.Namer, error) { name, err := getClusterUID(kubeClient, clusterName) if err != nil { return nil, err } + fw_name, err := getFirewallName(kubeClient, fwName, clusterName) + if err != nil { + return nil, err + } - namer := utils.NewNamer(name) - vault := storage.NewConfigMapVault(kubeClient, api.NamespaceSystem, uidConfigMapName) + namer := utils.NewNamerWithFirewall(name, fw_name) + uid_vault := storage.NewConfigMapVault(kubeClient, api.NamespaceSystem, uidConfigMapName) // Start a goroutine to poll the cluster UID config map // We don't watch because we know exactly which configmap we want and this // controller already watches 5 other resources, so it isn't worth the cost // of another connection and complexity. go wait.Forever(func() { - uid, found, err := vault.Get() - existing := namer.GetClusterName() - if found && uid != existing { - glog.Infof("Cluster uid changed from %v -> %v", existing, uid) - namer.SetClusterName(uid) - } 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() + if !found || err != nil { + glog.Errorf("Can't read uidConfigMap %v", uidConfigMapName) + return + } + + for key, val := range vaultmap { + switch key { + case storage.UidDataKey: + if uid := namer.GetClusterName(); uid != val { + glog.Infof("Cluster uid changed from %v -> %v", val, uid) + namer.SetClusterName(uid) + } + case storage.FirewallRuleKey: + if fw_name := namer.GetFirewallName(); fw_name != val { + glog.Infof("Cluster firewall name changed from %v -> %v", val, fw_name) + namer.SetClusterFirewallName(fw_name) + } + } } }, 5*time.Second) return namer, nil } +// getFlagOrLookupVault returns the name to use associated to a flag and configmap. +// The returned value follows this priority: +// If the provided 'name' is not empty, that name is used. +// 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) { + if name != "" { + glog.Infof("Using user provided %v %v", cm_key, name) + // Don't save the uid in the vault, so users can rollback through + // setting the accompany flag to "" + return name, nil + } + vaultmap, found, err := cfgVault.Get() + if found { + if val, found := vaultmap[cm_key]; found { + glog.Infof("Using saved %v %q", cm_key, val) + return val, nil + } + } + + if err != nil { + // This can fail because of: + // 1. No such config map - found=false, err=nil + // 2. No such key in config map - found=false, err=nil + // 3. Apiserver flake - found=false, err!=nil + // It is not safe to proceed in 3. + return "", fmt.Errorf("Failed to retrieve %v: %v, using %q as name", cm_key, err, name) + } + // Not found but safe to proceed. + return "", nil +} + +// getFirewallName returns the firewall rule name to use for this cluster. For +// backwards compatibility, the firewall name will default to the cluster UID. +// Use getFlagOrLookupVault to obtain a stored or overridden value for the firewall name. +// else, use the cluster UID as a backup (this retains backwards compatibility). +func getFirewallName(kubeClient client.Interface, name string, cluster_uid string) (string, error) { + cfgVault := storage.NewConfigMapVault(kubeClient, api.NamespaceSystem, uidConfigMapName) + if fw_name, err := getFlagOrLookupVault(cfgVault, storage.FirewallRuleKey, name); err != nil { + return "", err + } else if fw_name != "" { + return fw_name, nil + } else { + glog.Infof("Using cluster UID %v as firewall name", cluster_uid) + return cluster_uid, nil + } +} + // getClusterUID returns the cluster UID. Rules for UID generation: // If the user specifies a --cluster-uid param it overwrites everything // else, check UID config map for a previously recorded uid @@ -279,26 +348,12 @@ func newNamer(kubeClient client.Interface, clusterName string) (*utils.Namer, er // else, allocate a new uid func getClusterUID(kubeClient client.Interface, name string) (string, error) { cfgVault := storage.NewConfigMapVault(kubeClient, api.NamespaceSystem, uidConfigMapName) - if name != "" { - glog.Infof("Using user provided cluster uid %v", name) - // Don't save the uid in the vault, so users can rollback through - // --cluster-uid="" + if name, err := getFlagOrLookupVault(cfgVault, storage.UidDataKey, name); err != nil { + return "", err + } else if name != "" { return name, nil } - existingUID, found, err := cfgVault.Get() - if found { - glog.Infof("Using saved cluster uid %q", existingUID) - return existingUID, nil - } else if err != nil { - // This can fail because of: - // 1. No such config map - found=false, err=nil - // 2. No such key in config map - found=false, err=nil - // 3. Apiserver flake - found=false, err!=nil - // It is not safe to proceed in 3. - return "", fmt.Errorf("Failed to retrieve current uid: %v, using %q as name", err, name) - } - // Check if the cluster has an Ingress with ip ings, err := kubeClient.Extensions().Ingresses(api.NamespaceAll).List(api.ListOptions{LabelSelector: labels.Everything()}) if err != nil { @@ -309,10 +364,10 @@ func getClusterUID(kubeClient client.Interface, name string) (string, error) { if len(ing.Status.LoadBalancer.Ingress) != 0 { c := namer.ParseName(loadbalancers.GCEResourceName(ing.Annotations, "forwarding-rule")) if c.ClusterName != "" { - return c.ClusterName, cfgVault.Put(c.ClusterName) + return c.ClusterName, cfgVault.Put(map[string]string{storage.UidDataKey: c.ClusterName}) } glog.Infof("Found a working Ingress, assuming uid is empty string") - return "", cfgVault.Put("") + return "", cfgVault.Put(map[string]string{storage.UidDataKey: ""}) } } @@ -327,7 +382,7 @@ func getClusterUID(kubeClient client.Interface, name string) (string, error) { return "", err } uid := fmt.Sprintf("%x", b) - return uid, cfgVault.Put(uid) + return uid, cfgVault.Put(map[string]string{storage.UidDataKey: uid}) } // getNodePort waits for the Service, and returns it's first node port. diff --git a/controllers/gce/storage/configmaps.go b/controllers/gce/storage/configmaps.go index cfed347fc..11585eff1 100644 --- a/controllers/gce/storage/configmaps.go +++ b/controllers/gce/storage/configmaps.go @@ -27,15 +27,11 @@ import ( client "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" ) -// UIDVault stores UIDs. -type UIDVault interface { - Get() (string, bool, error) - Put(string) error - Delete() error -} - -// uidDataKey is the key used in config maps to store the UID. -const uidDataKey = "uid" +const ( + // UidDataKey is the key used in config maps to store the UID. + UidDataKey = "uid" + FirewallRuleKey = "fwName" +) // ConfigMapVault stores cluster UIDs in config maps. // It's a layer on top of ConfigMapStore that just implements the utils.uidVault @@ -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) { key := fmt.Sprintf("%v/%v", c.namespace, c.name) item, found, err := c.ConfigMapStore.GetByKey(key) if err != nil || !found { - return "", false, err + return nil, false, err } cfg := item.(*api.ConfigMap) - if k, ok := cfg.Data[uidDataKey]; ok { - return k, true, nil - } - return "", false, fmt.Errorf("Found config map %v but it doesn't contain uid key: %+v", key, cfg.Data) + return cfg.Data, true, nil } -// 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 { + if len(keyvals) == 0 { + return nil + } apiObj := &api.ConfigMap{ ObjectMeta: api.ObjectMeta{ Name: c.name, Namespace: c.namespace, }, - Data: map[string]string{uidDataKey: uid}, + Data: keyvals, } cfgMapKey := fmt.Sprintf("%v/%v", c.namespace, c.name) item, exists, err := c.ConfigMapStore.GetByKey(cfgMapKey) if err == nil && exists { data := item.(*api.ConfigMap).Data - if k, ok := data[uidDataKey]; ok && k == uid { - return nil - } else if ok { - glog.Infof("Configmap %v has key %v but wrong value %v, updating", cfgMapKey, k, uid) + + updated_needed := false + + // 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) + data[key] = val + updated_needed = true + } else if val_stored != val { + glog.Infof("Configmap %v has key %v but wrong value %v, updating to %v", cfgMapKey, key, val_stored, val) + data[key] = val + updated_needed = true + } } - if err := c.ConfigMapStore.Update(apiObj); err != nil { - return fmt.Errorf("Failed to update %v: %v", cfgMapKey, err) + if updated_needed { + if err := c.ConfigMapStore.Update(apiObj); err != nil { + return fmt.Errorf("Failed to update %v: %v", cfgMapKey, err) + } } + } else if err := c.ConfigMapStore.Add(apiObj); err != nil { return fmt.Errorf("Failed to add %v: %v", cfgMapKey, err) } - glog.Infof("Successfully stored uid %q in config map %v", uid, cfgMapKey) + glog.Infof("Successfully stored map in config map %v", cfgMapKey) return nil } diff --git a/controllers/gce/storage/configmaps_test.go b/controllers/gce/storage/configmaps_test.go index 3b8404b89..5f732faf8 100644 --- a/controllers/gce/storage/configmaps_test.go +++ b/controllers/gce/storage/configmaps_test.go @@ -17,6 +17,7 @@ limitations under the License. package storage import ( + "reflect" "testing" "k8s.io/kubernetes/pkg/api" @@ -24,31 +25,61 @@ import ( func TestConfigMapUID(t *testing.T) { vault := NewFakeConfigMapVault(api.NamespaceSystem, "ingress-uid") - uid := "" - k, exists, err := vault.Get() + //uid := "" + keyvals, exists, err := vault.Get() if exists { - t.Errorf("Got a key from an empyt vault") + t.Errorf("Got keyvals from an empty vault") } - vault.Put(uid) - k, exists, err = vault.Get() + + // Store empty value for UidDataKey. + uidmap := map[string]string{UidDataKey: ""} + vault.Put(uidmap) + keyvals, exists, err = vault.Get() if !exists || err != nil { t.Errorf("Failed to retrieve value from vault") } - if k != "" { + if val, ok := keyvals[UidDataKey]; !ok { + t.Errorf("Failed to retried UidDataKey from vault") + } else if val != "" { t.Errorf("Failed to store empty string as a key in the vault") } - vault.Put("newuid") - k, exists, err = vault.Get() + + // Store actual value in key. + uidmap[UidDataKey] = "newuid" + vault.Put(uidmap) + keyvals, exists, err = vault.Get() if !exists || err != nil { t.Errorf("Failed to retrieve value from vault") + } else if val, ok := keyvals[UidDataKey]; !ok { + t.Errorf("Failed to retried UidDataKey from vault") + } else if val != "newuid" { + t.Errorf("Failed to store empty string as a key in the vault") } - if k != "newuid" { - t.Errorf("Failed to modify uid") - } + + // Delete value. if err := vault.Delete(); err != nil { t.Errorf("Failed to delete uid %v", err) } - if uid, exists, _ := vault.Get(); exists { - t.Errorf("Found uid %v, expected none", uid) + if keyvals, exists, _ = vault.Get(); exists { + t.Errorf("Found uid but expected none after deletion") } + + // Ensure Keystore is not wiped on second update. + uidmap[UidDataKey] = "newuid" + uidmap[FirewallRuleKey] = "fwrule" + vault.Put(uidmap) + keyvals, exists, err = vault.Get() + if !exists || err != nil || len(keyvals) != 2 { + t.Errorf("Failed to retrieve value from vault") + } + uidmap[UidDataKey] = "newnewuid" + vault.Put(uidmap) + keyvals, exists, err = vault.Get() + if !exists || err != nil || len(keyvals) != 2 { + t.Errorf("Failed to retrieve value from vault") + } + if !reflect.DeepEqual(keyvals, uidmap) { + t.Errorf("Failed to provide equal maps from vault after a partial update") + } + } diff --git a/controllers/gce/utils/utils.go b/controllers/gce/utils/utils.go index 33525ffa3..af0a07612 100644 --- a/controllers/gce/utils/utils.go +++ b/controllers/gce/utils/utils.go @@ -92,8 +92,9 @@ const ( // Namer handles centralized naming for the cluster. type Namer struct { - clusterName string - nameLock sync.Mutex + clusterName string + firewallName string + nameLock sync.Mutex } // NewNamer creates a new namer. @@ -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 { + namer := &Namer{} + namer.SetClusterName(clusterName) + namer.SetClusterFirewallName(firewallName) + return namer +} + // NameComponents is a struct representing the components of a a GCE resource // name constructed by the namer. The format of such a name is: // k8s-resource---uid @@ -123,6 +132,14 @@ func (n *Namer) SetClusterName(name string) { n.clusterName = name } +func (n *Namer) SetClusterFirewallName(firewall_name string) { + // Retain backwards compatible behavior where firewallName == clusterName. + n.nameLock.Lock() + defer n.nameLock.Unlock() + glog.Infof("Changing cluster %v firewall name to %v", n.clusterName, firewall_name) + n.firewallName = firewall_name +} + // GetClusterName returns the UID/name of this cluster. func (n *Namer) GetClusterName() string { n.nameLock.Lock() @@ -130,6 +147,20 @@ func (n *Namer) GetClusterName() string { return n.clusterName } +// GetClusterNameAndFirewall returns the UID/name and firewall/name of this cluster. +func (n *Namer) GetClusterAndFirewallNames() (string, string) { + n.nameLock.Lock() + defer n.nameLock.Unlock() + return n.clusterName, n.firewallName +} + +// GetFirewallName returns the firewall name of this cluster. +func (n *Namer) GetFirewallName() string { + n.nameLock.Lock() + defer n.nameLock.Unlock() + return n.firewallName +} + // Truncate truncates the given key to a GCE length limit. func (n *Namer) Truncate(key string) string { if len(key) > nameLenLimit { @@ -216,12 +247,12 @@ func (n *Namer) IGName() string { // FrSuffix constructs the glbc specific suffix for the FirewallRule. func (n *Namer) FrSuffix() string { - clusterName := n.GetClusterName() + firewallName := n.GetFirewallName() // The entire cluster only needs a single firewall rule. - if clusterName == "" { + if firewallName == "" { return globalFirewallSuffix } - return n.Truncate(fmt.Sprintf("%v%v%v", globalFirewallSuffix, clusterNameDelimiter, clusterName)) + return n.Truncate(fmt.Sprintf("%v%v%v", globalFirewallSuffix, clusterNameDelimiter, firewallName)) } // FrName constructs the full firewall rule name, this is the name assigned by