Merge pull request #101 from bowei/cleanup

Cleanup

build/build.sh

@@ -39,5 +39,5 @@ fi
 
 go install                                                         \
     -installsuffix "static"                                        \
-    -ldflags "-X ${PKG}/pkg/version.VERSION=${VERSION}"            \
+    -ldflags "-X ${PKG}/pkg/version.Version=${VERSION}"            \
     ./...

cmd/glbc/app/clients.go

@@ -21,19 +21,22 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
+	"os"
 	"time"
 
 	"github.com/golang/glog"
 
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
-	"k8s.io/ingress-gce/pkg/utils"
 	"k8s.io/kubernetes/pkg/cloudprovider"
 	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
 
 	// Register the GCP authorization provider.
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
+
+	"k8s.io/ingress-gce/pkg/flags"
+	"k8s.io/ingress-gce/pkg/utils"
 )
 
 const (
@@ -43,7 +46,7 @@ const (
 
 // NewKubeClient returns a Kubernetes client given the command line settings.
 func NewKubeClient() (kubernetes.Interface, error) {
-	if Flags.InCluster {
+	if flags.F.InCluster {
 		glog.V(0).Infof("Using in cluster configuration")
 		config, err := rest.InClusterConfig()
 		if err != nil {
@@ -52,41 +55,48 @@ func NewKubeClient() (kubernetes.Interface, error) {
 		return kubernetes.NewForConfig(config)
 	}
 
-	glog.V(0).Infof("Using APIServerHost=%q, KubeConfig=%q", Flags.APIServerHost, Flags.KubeConfigFile)
-	config, err := clientcmd.BuildConfigFromFlags(Flags.APIServerHost, Flags.KubeConfigFile)
+	glog.V(0).Infof("Using APIServerHost=%q, KubeConfig=%q", flags.F.APIServerHost, flags.F.KubeConfigFile)
+	config, err := clientcmd.BuildConfigFromFlags(flags.F.APIServerHost, flags.F.KubeConfigFile)
 	if err != nil {
 		return nil, err
 	}
 	return kubernetes.NewForConfig(config)
 }
 
-// NewGCEClient returns a client to the GCE environment.
-func NewGCEClient(config io.Reader) *gce.GCECloud {
-	getConfigReader := func() io.Reader { return nil }
+// NewGCEClient returns a client to the GCE environment. This will block until
+// a valid configuration file can be read.
+func NewGCEClient() *gce.GCECloud {
+	var configReader func() io.Reader
+	if flags.F.ConfigFilePath != "" {
+		glog.Infof("Reading config from path %q", flags.F.ConfigFilePath)
+		config, err := os.Open(flags.F.ConfigFilePath)
+		if err != nil {
+			glog.Fatalf("%v", err)
+		}
+		defer config.Close()
 
-	if config != nil {
 		allConfig, err := ioutil.ReadAll(config)
 		if err != nil {
-			glog.Fatalf("Error while reading entire config: %v", err)
+			glog.Fatalf("Error while reading config (%q): %v", flags.F.ConfigFilePath, err)
 		}
-		glog.V(4).Infof("Using cloudprovider config file: %q", string(allConfig))
+		glog.V(4).Infof("Cloudprovider config file contains: %q", string(allConfig))
 
-		getConfigReader = func() io.Reader {
+		configReader = func() io.Reader {
 			return bytes.NewReader(allConfig)
 		}
 	} else {
 		glog.V(2).Infof("No cloudprovider config file provided, using default values.")
+		configReader = func() io.Reader { return nil }
 	}
 
 	// Creating the cloud interface involves resolving the metadata server to get
 	// an oauth token. If this fails, the token provider assumes it's not on GCE.
 	// No errors are thrown. So we need to keep retrying till it works because
 	// we know we're on GCE.
 	for {
-		provider, err := cloudprovider.GetCloudProvider("gce", getConfigReader())
+		provider, err := cloudprovider.GetCloudProvider("gce", configReader())
 		if err == nil {
 			cloud := provider.(*gce.GCECloud)
-
 			// If this controller is scheduled on a node without compute/rw
 			// it won't be allowed to list backends. We can assume that the
 			// user has no need for Ingress in this case. If they grant

cmd/glbc/app/handlers.go

@@ -25,7 +25,9 @@ import (
 
 	"github.com/golang/glog"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
+
 	"k8s.io/ingress-gce/pkg/controller"
+	"k8s.io/ingress-gce/pkg/flags"
 )
 
 func RunHTTPServer(lbc *controller.LoadBalancerController) {
@@ -44,8 +46,8 @@ func RunHTTPServer(lbc *controller.LoadBalancerController) {
 		lbc.Stop(true)
 	})
 
-	glog.V(0).Infof("Running http server on :%v", Flags.HealthzPort)
-	glog.Fatal(http.ListenAndServe(fmt.Sprintf(":%v", Flags.HealthzPort), nil))
+	glog.V(0).Infof("Running http server on :%v", flags.F.HealthzPort)
+	glog.Fatal(http.ListenAndServe(fmt.Sprintf(":%v", flags.F.HealthzPort), nil))
 }
 
 func RunSIGTERMHandler(lbc *controller.LoadBalancerController, deleteAll bool) {

cmd/glbc/app/init.go

@@ -28,26 +28,28 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
+
 	"k8s.io/ingress-gce/pkg/annotations"
 	"k8s.io/ingress-gce/pkg/backends"
+	"k8s.io/ingress-gce/pkg/flags"
 )
 
 func DefaultBackendServicePort(kubeClient kubernetes.Interface) *backends.ServicePort {
 	// TODO: make this not fatal
-	if Flags.DefaultSvc == "" {
+	if flags.F.DefaultSvc == "" {
 		glog.Fatalf("Please specify --default-backend")
 	}
 
 	// Wait for the default backend Service. There's no pretty way to do this.
-	parts := strings.Split(Flags.DefaultSvc, "/")
+	parts := strings.Split(flags.F.DefaultSvc, "/")
 	if len(parts) != 2 {
 		glog.Fatalf("Default backend should take the form namespace/name: %v",
-			Flags.DefaultSvc)
+			flags.F.DefaultSvc)
 	}
 	port, nodePort, err := getNodePort(kubeClient, parts[0], parts[1])
 	if err != nil {
 		glog.Fatalf("Could not configure default backend %v: %v",
-			Flags.DefaultSvc, err)
+			flags.F.DefaultSvc, err)
 	}
 
 	return &backends.ServicePort{

cmd/glbc/app/namer.go

@@ -17,8 +17,8 @@ limitations under the License.
 package app
 
 import (
+	"crypto/rand"
 	"fmt"
-	"os"
 	"time"
 
 	"github.com/golang/glog"
@@ -36,8 +36,11 @@ import (
 const (
 	// Key used to persist UIDs to configmaps.
 	uidConfigMapName = "ingress-uid"
+	// uidByteLength is the length in bytes for the random UID.
+	uidByteLength = 8
 )
 
+// NewNamer returns a new naming policy given the state of the cluster.
 func NewNamer(kubeClient kubernetes.Interface, clusterName string, fwName string) (*utils.Namer, error) {
 	name, err := getClusterUID(kubeClient, clusterName)
 	if err != nil {
@@ -51,26 +54,26 @@ func NewNamer(kubeClient kubernetes.Interface, clusterName string, fwName string
 	namer := utils.NewNamer(name, fw_name)
 	uidVault := storage.NewConfigMapVault(kubeClient, metav1.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.
+	// 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() {
-		for _, key := range [...]string{storage.UidDataKey, storage.ProviderDataKey} {
+		for _, key := range [...]string{storage.UIDDataKey, storage.ProviderDataKey} {
 			val, found, err := uidVault.Get(key)
 			if err != nil {
 				glog.Errorf("Can't read uidConfigMap %v", uidConfigMapName)
 			} else if !found {
 				errmsg := fmt.Sprintf("Can't read %v from uidConfigMap %v", key, uidConfigMapName)
-				if key == storage.UidDataKey {
+				if key == storage.UIDDataKey {
 					glog.Errorf(errmsg)
 				} else {
 					glog.V(4).Infof(errmsg)
 				}
 			} else {
 
 				switch key {
-				case storage.UidDataKey:
+				case storage.UIDDataKey:
 					if uid := namer.UID(); uid != val {
 						glog.Infof("Cluster uid changed from %v -> %v", uid, val)
 						namer.SetUID(val)
@@ -87,48 +90,49 @@ func NewNamer(kubeClient kubernetes.Interface, clusterName string, fwName string
 	return namer, nil
 }
 
-// useDefaultOrLookupVault returns either a 'default_name' or if unset, obtains a name from a ConfigMap.
-// The returned value follows this priority:
-// If the provided 'default_name' is not empty, that name is used.
-//       This is effectively a client override via a command line flag.
-// else, check cfgVault with 'cm_key' as a key and if found, use the associated value
+// useDefaultOrLookupVault returns either a 'defaultName' or if unset, obtains
+// a name from a ConfigMap.  The returned value follows this priority:
+//
+// If the provided 'defaultName' is not empty, that name is used.
+//     This is effectively a client override via a command line flag.
+// else, check cfgVault with 'configMapKey' 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 useDefaultOrLookupVault(cfgVault *storage.ConfigMapVault, cm_key, default_name string) (string, error) {
-	if default_name != "" {
-		glog.Infof("Using user provided %v %v", cm_key, default_name)
-		// Don't save the uid in the vault, so users can rollback through
-		// setting the accompany flag to ""
-		return default_name, nil
+func useDefaultOrLookupVault(cfgVault *storage.ConfigMapVault, configMapKey, defaultName string) (string, error) {
+	if defaultName != "" {
+		glog.Infof("Using user provided %v %v", configMapKey, defaultName)
+		// Don't save the uid in the vault, so users can rollback
+		// through setting the accompany flag to ""
+		return defaultName, nil
 	}
-	val, found, err := cfgVault.Get(cm_key)
+	val, found, err := cfgVault.Get(configMapKey)
 	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, returning empty name", cm_key, err)
+		return "", fmt.Errorf("failed to retrieve %v: %v, returning empty name", configMapKey, err)
 	} else if !found {
 		// Not found but safe to proceed.
 		return "", nil
 	}
-	glog.Infof("Using %v = %q saved in ConfigMap", cm_key, val)
+	glog.Infof("Using %v = %q saved in ConfigMap", configMapKey, val)
 	return val, 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 kubernetes.Interface, name, cluster_uid string) (string, error) {
+func getFirewallName(kubeClient kubernetes.Interface, name, clusterUID string) (string, error) {
 	cfgVault := storage.NewConfigMapVault(kubeClient, metav1.NamespaceSystem, uidConfigMapName)
-	if fw_name, err := useDefaultOrLookupVault(cfgVault, storage.ProviderDataKey, name); err != nil {
+	if firewallName, err := useDefaultOrLookupVault(cfgVault, storage.ProviderDataKey, name); err != nil {
 		return "", err
-	} else if fw_name != "" {
-		return fw_name, cfgVault.Put(storage.ProviderDataKey, fw_name)
+	} else if firewallName != "" {
+		return firewallName, cfgVault.Put(storage.ProviderDataKey, firewallName)
 	} else {
-		glog.Infof("Using cluster UID %v as firewall name", cluster_uid)
-		return cluster_uid, cfgVault.Put(storage.ProviderDataKey, cluster_uid)
+		glog.Infof("Using cluster UID %v as firewall name", clusterUID)
+		return clusterUID, cfgVault.Put(storage.ProviderDataKey, clusterUID)
 	}
 }
 
@@ -140,7 +144,7 @@ func getFirewallName(kubeClient kubernetes.Interface, name, cluster_uid string)
 // else, allocate a new uid
 func getClusterUID(kubeClient kubernetes.Interface, name string) (string, error) {
 	cfgVault := storage.NewConfigMapVault(kubeClient, metav1.NamespaceSystem, uidConfigMapName)
-	if name, err := useDefaultOrLookupVault(cfgVault, storage.UidDataKey, name); err != nil {
+	if name, err := useDefaultOrLookupVault(cfgVault, storage.UIDDataKey, name); err != nil {
 		return "", err
 	} else if name != "" {
 		return name, nil
@@ -158,23 +162,25 @@ func getClusterUID(kubeClient kubernetes.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(storage.UidDataKey, c.ClusterName)
+				return c.ClusterName, cfgVault.Put(storage.UIDDataKey, c.ClusterName)
 			}
 			glog.Infof("Found a working Ingress, assuming uid is empty string")
-			return "", cfgVault.Put(storage.UidDataKey, "")
+			return "", cfgVault.Put(storage.UIDDataKey, "")
 		}
 	}
 
-	// Allocate new uid
-	f, err := os.Open("/dev/urandom")
+	uid, err := randomUID()
 	if err != nil {
 		return "", err
 	}
-	defer f.Close()
-	b := make([]byte, 8)
-	if _, err := f.Read(b); err != nil {
+	return uid, cfgVault.Put(storage.UIDDataKey, uid)
+}
+
+func randomUID() (string, error) {
+	b := make([]byte, uidByteLength)
+	if _, err := rand.Read(b); err != nil {
 		return "", err
 	}
 	uid := fmt.Sprintf("%x", b)
-	return uid, cfgVault.Put(storage.UidDataKey, uid)
+	return uid, nil
 }