Finishing touches

Author: rramkumar1

cmd/glbc/app/clients.go

@@ -99,7 +99,7 @@ func NewGCEClient() *gce.GCECloud {
 			// Configure GCE rate limiting
 			rl, err := ratelimit.NewGCERateLimiter(flags.F.GCERateLimit.Values())
 			if err != nil {
-				glog.Errorf("Error in configuring rate limiting: %v", err)
+				glog.Fatalf("Error configuring rate limiting: %v", err)
 			}
 			cloud.SetRateLimiter(rl)
 			// If this controller is scheduled on a node without compute/rw

pkg/flags/flags.go

@@ -59,7 +59,7 @@ var (
 
 func init() {
 	F.NodePortRanges.ports = []string{DefaultNodePortRange}
-	F.GCERateLimit.specs = []string{}
+	F.GCERateLimit.specs = []string{"alpha.Operations.Get,qps,10,100", "beta.Operations.Get,qps,10,100", "ga.Operations.Get,qps,10,100"}
 }
 
 // Register flags with the command line parser.
@@ -91,7 +91,11 @@ associated Ingress is deleted.`)
 		`Optional, can be used to rate limit certain GCE API calls. Example usage:
 --gce-ratelimit=ga.Addresses.Get,qps,1.5,5
 (limit ga.Addresses.Get to maximum of 1.5 qps with a burst of 5).
-Use the flag more than once to rate limit more than one call.`)
+Use the flag more than once to rate limit more than one call. If you do not
+specify this flag, the default is to rate limit Operations.Get for all versions.
+If you do specify this flag one or more times, this default will be overwritten.
+If you want to still use the default, simply specify it along with your other
+values.`)
 	flag.StringVar(&F.HealthCheckPath, "health-check-path", "/",
 		`Path used to health-check a backend service. All Services must serve a
 200 page on this path. Currently this is only configurable globally.`)
@@ -122,6 +126,7 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
 
 type RateLimitSpecs struct {
 	specs []string
+	isSet bool
 }
 
 // Part of the flag.Value interface.
@@ -131,6 +136,12 @@ func (r *RateLimitSpecs) String() string {
 
 // Set supports the flag being repeated multiple times. Part of the flag.Value interface.
 func (r *RateLimitSpecs) Set(value string) error {
+	// On first Set(), clear the original defaults
+	// On subsequent Set()'s, append.
+	if !r.isSet {
+		r.specs = []string{}
+		r.isSet = true
+	}
 	r.specs = append(r.specs, value)
 	return nil
 }

pkg/ratelimit/ratelimit.go

@@ -31,14 +31,14 @@ import (
 // GCERateLimiter implements cloud.RateLimiter
 type GCERateLimiter struct {
 	// Map a RateLimitKey to its rate limiter implementation.
-	rateLimitImpls map[*cloud.RateLimitKey]flowcontrol.RateLimiter
+	rateLimitImpls map[cloud.RateLimitKey]flowcontrol.RateLimiter
 }
 
 // NewGCERateLimiter parses the list of rate limiting specs passed in and
 // returns a properly configured cloud.RateLimiter implementation.
 // Expected format of specs: {"[version].[service].[operation],[type],[param1],[param2],..", "..."}
 func NewGCERateLimiter(specs []string) (*GCERateLimiter, error) {
-	rateLimitImpls := make(map[*cloud.RateLimitKey]flowcontrol.RateLimiter)
+	rateLimitImpls := make(map[cloud.RateLimitKey]flowcontrol.RateLimiter)
 	// Within each specification, split on comma to get the operation,
 	// rate limiter type, and extra parameters.
 	for _, spec := range specs {
@@ -91,7 +91,7 @@ func (l *GCERateLimiter) rateLimitImpl(key *cloud.RateLimitKey) flowcontrol.Rate
 	// Since the passed in key will have the ProjectID field filled in, we need to
 	// create a copy which does not, so that retreiving the rate limiter implementation
 	// through the map works as expected.
-	keyCopy := &cloud.RateLimitKey{
+	keyCopy := cloud.RateLimitKey{
 		ProjectID: "",
 		Operation: key.Operation,
 		Version:   key.Version,
@@ -101,21 +101,23 @@ func (l *GCERateLimiter) rateLimitImpl(key *cloud.RateLimitKey) flowcontrol.Rate
 }
 
 // Expected format of param is [version].[service].[operation]
-func constructRateLimitKey(param string) (*cloud.RateLimitKey, error) {
+func constructRateLimitKey(param string) (cloud.RateLimitKey, error) {
+	var retVal cloud.RateLimitKey
 	params := strings.Split(param, ".")
 	if len(params) != 3 {
-		return nil, fmt.Errorf("Must specify operation in [version].[service].[operation] format.")
+		return retVal, fmt.Errorf("Must specify rate limit in [version].[service].[operation] format: %v", param)
 	}
 	// TODO(rramkumar): Add another layer of validation here?
 	version := meta.Version(params[0])
 	service := params[1]
 	operation := params[2]
-	return &cloud.RateLimitKey{
+	retVal = cloud.RateLimitKey{
 		ProjectID: "",
 		Operation: operation,
 		Version:   version,
 		Service:   service,
-	}, nil
+	}
+	return retVal, nil
 }
 
 // constructRateLimitImpl parses the slice and returns a flowcontrol.RateLimiter

pkg/ratelimit/ratelimit_test.go

@@ -25,6 +25,7 @@ func TestConfigureGCERateLimiting(t *testing.T) {
 		[]string{"ga.Addresses.Get,qps,1.5,5"},
 		[]string{"ga.Addresses.List,qps,2,10"},
 		[]string{"ga.Addresses.Get,qps,1.5,5", "ga.Firewalls.Get,qps,1.5,5"},
+		[]string{"ga.Operations.Get,qps,10,100"},
 	}
 	invalidTestCases := [][]string{
 		[]string{"gaAddresses.Get,qps,1.5,5"},