From 87d360afda7b73344ec7a7d08126d3ad492ccadf Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Fri, 11 Mar 2016 14:25:14 -0700 Subject: [PATCH 1/4] fix(manager): refactor REST API to make it modular This modifies the startup and initial handling of the REST API to make it modular, remove the init() methods, and remove reliance on global vars. --- Makefile | 2 +- cmd/manager/deployments.go | 89 ++++++++----- cmd/manager/main.go | 97 ++++++-------- cmd/manager/router/encoder.go | 121 +++++++++++++++++ cmd/manager/router/encoder_test.go | 110 ++++++++++++++++ cmd/manager/router/router.go | 203 +++++++++++++++++++++++++++++ cmd/manager/router/router_test.go | 72 ++++++++++ glide.lock | 8 +- glide.yaml | 1 + 9 files changed, 614 insertions(+), 89 deletions(-) create mode 100644 cmd/manager/router/encoder.go create mode 100644 cmd/manager/router/encoder_test.go create mode 100644 cmd/manager/router/router.go create mode 100644 cmd/manager/router/router_test.go diff --git a/Makefile b/Makefile index ef888f5bf..6ec99023a 100644 --- a/Makefile +++ b/Makefile @@ -56,7 +56,7 @@ container: all .PHONY: test-unit test-unit: @echo Running tests... - go test -v $(GO_PKGS) + go test -race -v $(GO_PKGS) .PHONY: test-flake8 test-flake8: diff --git a/cmd/manager/deployments.go b/cmd/manager/deployments.go index 993dc1678..e71a2418e 100644 --- a/cmd/manager/deployments.go +++ b/cmd/manager/deployments.go @@ -18,7 +18,7 @@ package main import ( "encoding/json" - "flag" + "errors" "fmt" "io" "io/ioutil" @@ -32,11 +32,11 @@ import ( "github.com/ghodss/yaml" "github.com/gorilla/mux" - "github.com/kubernetes/deployment-manager/cmd/manager/manager" "github.com/kubernetes/deployment-manager/cmd/manager/repository" "github.com/kubernetes/deployment-manager/cmd/manager/repository/persistent" "github.com/kubernetes/deployment-manager/cmd/manager/repository/transient" + "github.com/kubernetes/deployment-manager/cmd/manager/router" "github.com/kubernetes/deployment-manager/pkg/common" "github.com/kubernetes/deployment-manager/pkg/registry" "github.com/kubernetes/deployment-manager/pkg/util" @@ -65,57 +65,84 @@ var deployments = []Route{ {"GetCredential", "/credentials/{credential}", "GET", getCredentialHandlerFunc, ""}, } -var ( - maxLength = flag.Int64("maxLength", 1024, "The maximum length (KB) of a template.") - expanderName = flag.String("expander", "expandybird-service", "The DNS name of the expander service.") - expanderURL = flag.String("expanderURL", "", "The URL for the expander service.") - deployerName = flag.String("deployer", "resourcifier-service", "The DNS name of the deployer service.") - deployerURL = flag.String("deployerURL", "", "The URL for the deployer service.") - credentialFile = flag.String("credentialFile", "", "Local file to use for credentials.") - credentialSecrets = flag.Bool("credentialSecrets", true, "Use secrets for credentials.") - mongoName = flag.String("mongoName", "mongodb", "The DNS name of the mongodb service.") - mongoPort = flag.String("mongoPort", "27017", "The port of the mongodb service.") - mongoAddress = flag.String("mongoAddress", "mongodb:27017", "The address of the mongodb service.") -) - +// Deprecated. Use Context.Manager instead. var backend manager.Manager -func init() { - if !flag.Parsed() { - flag.Parse() +// Route defines a routing table entry to be registered with gorilla/mux. +// +// Route is deprecated. Use router.Routes instead. +type Route struct { + Name string + Path string + Methods string + HandlerFunc http.HandlerFunc + Type string +} + +func registerRoutes(c *router.Context) router.Routes { + re := regexp.MustCompile("{[a-z]+}") + + r := router.NewRoutes() + r.Add("GET /healthz", healthz) + + // TODO: Replace these routes with updated ones. + for _, d := range deployments { + path := fmt.Sprintf("%s %s", d.Methods, re.ReplaceAllString(d.Path, "*")) + fmt.Printf("\t%s\n", path) + r.Add(path, func(w http.ResponseWriter, r *http.Request, c *router.Context) error { + d.HandlerFunc(w, r) + return nil + }) } - routes = append(routes, deployments...) + return r +} + +func healthz(w http.ResponseWriter, r *http.Request, c *router.Context) error { + c.Log("manager: healthz checkpoint") + // TODO: This should check the availability of the repository, and fail if it + // cannot connect. + fmt.Fprintln(w, "OK") + return nil +} + +func setupDependencies(c *router.Context) error { var credentialProvider common.CredentialProvider - if *credentialFile != "" { - if *credentialSecrets { - panic(fmt.Errorf("Both credentialFile and credentialSecrets are set")) + if c.Config.CredentialFile != "" { + if c.Config.CredentialSecrets { + return errors.New("Both credentialFile and credentialSecrets are set") } var err error - credentialProvider, err = registry.NewFilebasedCredentialProvider(*credentialFile) + credentialProvider, err = registry.NewFilebasedCredentialProvider(c.Config.CredentialFile) if err != nil { - panic(fmt.Errorf("cannot create credential provider %s: %s", *credentialFile, err)) + return fmt.Errorf("cannot create credential provider %s: %s", c.Config.CredentialFile, err) } } else if *credentialSecrets { credentialProvider = registry.NewSecretsCredentialProvider() } else { credentialProvider = registry.NewInmemCredentialProvider() } - backend = newManager(credentialProvider) + c.CredentialProvider = credentialProvider + c.Manager = newManager(c) + + // FIXME: As soon as we can, we need to get rid of this. + backend = c.Manager + return nil } const expanderPort = "8080" const deployerPort = "8080" -func newManager(cp common.CredentialProvider) manager.Manager { +func newManager(c *router.Context) manager.Manager { + cfg := c.Config service := registry.NewInmemRegistryService() - registryProvider := registry.NewDefaultRegistryProvider(cp, service) + registryProvider := registry.NewDefaultRegistryProvider(c.CredentialProvider, service) resolver := manager.NewTypeResolver(registryProvider, util.DefaultHTTPClient()) - expander := manager.NewExpander(getServiceURL(*expanderURL, *expanderName, expanderPort), resolver) - deployer := manager.NewDeployer(getServiceURL(*deployerURL, *deployerName, deployerPort)) - address := strings.TrimPrefix(getServiceURL(*mongoAddress, *mongoName, *mongoPort), "http://") + expander := manager.NewExpander(getServiceURL(cfg.ExpanderURL, cfg.ExpanderName, expanderPort), resolver) + deployer := manager.NewDeployer(getServiceURL(cfg.DeployerURL, cfg.DeployerName, deployerPort)) + address := strings.TrimPrefix(getServiceURL(cfg.MongoAddress, cfg.MongoName, cfg.MongoPort), "http://") repository := createRepository(address) - return manager.NewManager(expander, deployer, repository, registryProvider, service, cp) + return manager.NewManager(expander, deployer, repository, registryProvider, service, c.CredentialProvider) } func createRepository(address string) repository.Repository { diff --git a/cmd/manager/main.go b/cmd/manager/main.go index a4c5df85a..7aca7e80a 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -17,75 +17,64 @@ limitations under the License. package main import ( - "github.com/kubernetes/deployment-manager/pkg/util" + "github.com/kubernetes/deployment-manager/cmd/manager/router" "github.com/kubernetes/deployment-manager/pkg/version" "flag" "fmt" - "log" "net/http" "os" - - "github.com/gorilla/handlers" - "github.com/gorilla/mux" ) -// Route defines a routing table entry to be registered with gorilla/mux. -type Route struct { - Name string - Path string - Methods string - HandlerFunc http.HandlerFunc - Type string -} - -var routes = []Route{ - {"HealthCheck", "/healthz", "GET", healthCheckHandlerFunc, ""}, -} - -// port to listen on -var port = flag.Int("port", 8080, "The port to listen on") +var ( + port = flag.Int("port", 8080, "The port to listen on") + maxLength = flag.Int64("maxLength", 1024, "The maximum length (KB) of a template.") + expanderName = flag.String("expander", "expandybird-service", "The DNS name of the expander service.") + expanderURL = flag.String("expanderURL", "", "The URL for the expander service.") + deployerName = flag.String("deployer", "resourcifier-service", "The DNS name of the deployer service.") + deployerURL = flag.String("deployerURL", "", "The URL for the deployer service.") + credentialFile = flag.String("credentialFile", "", "Local file to use for credentials.") + credentialSecrets = flag.Bool("credentialSecrets", true, "Use secrets for credentials.") + mongoName = flag.String("mongoName", "mongodb", "The DNS name of the mongodb service.") + mongoPort = flag.String("mongoPort", "27017", "The port of the mongodb service.") + mongoAddress = flag.String("mongoAddress", "mongodb:27017", "The address of the mongodb service.") +) -func init() { - if !flag.Parsed() { - flag.Parse() +func main() { + // Set up dependencies + c := &router.Context{ + Config: parseFlags(), } -} -func main() { - if !flag.Parsed() { - flag.Parse() + if err := setupDependencies(c); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) } - router := mux.NewRouter() - router.StrictSlash(true) - for _, route := range routes { - handler := http.Handler(http.HandlerFunc(route.HandlerFunc)) - switch route.Type { - case "JSON": - handler = handlers.ContentTypeHandler(handler, "application/json") - case "": - break - default: - log.Fatalf("invalid route type: %v", route.Type) - } + // Set up routes + routes := registerRoutes(c) - r := router.NewRoute() - r.Name(route.Name). - Path(route.Path). - Methods(route.Methods). - Handler(handler) + // Now create a server. + c.Log("Starting Manager %s on %s", version.Version, c.Config.Address) + if err := http.ListenAndServe(c.Config.Address, router.NewHandler(c, routes)); err != nil { + c.Err("Server exited with error %s", err) + os.Exit(1) } - - address := fmt.Sprintf(":%d", *port) - handler := handlers.CombinedLoggingHandler(os.Stderr, router) - log.Printf("Version: %s", version.Version) - log.Printf("Listening on port %d...", *port) - log.Fatal(http.ListenAndServe(address, handler)) } -func healthCheckHandlerFunc(w http.ResponseWriter, r *http.Request) { - handler := "manager: get health" - util.LogHandlerEntry(handler, r) - util.LogHandlerExitWithText(handler, w, "OK", http.StatusOK) +func parseFlags() *router.Config { + flag.Parse() + return &router.Config{ + Address: fmt.Sprintf(":%d", *port), + MaxTemplateLength: *maxLength, + ExpanderName: *expanderName, + ExpanderURL: *expanderURL, + DeployerName: *deployerName, + DeployerURL: *deployerURL, + CredentialFile: *credentialFile, + CredentialSecrets: *credentialSecrets, + MongoName: *mongoName, + MongoPort: *mongoPort, + MongoAddress: *mongoAddress, + } } diff --git a/cmd/manager/router/encoder.go b/cmd/manager/router/encoder.go new file mode 100644 index 000000000..fd401cb59 --- /dev/null +++ b/cmd/manager/router/encoder.go @@ -0,0 +1,121 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package router + +import ( + "encoding/json" + "errors" + "fmt" + "mime" + "net/http" + "reflect" + "strings" + + "github.com/ghodss/yaml" +) + +type Encoder interface { + // Encode encoders a given response + // + // When an encoder fails, it logs any necessary data and then responds to + // the client. + Encode(http.ResponseWriter, *http.Request, *Context, interface{}) +} + +// AcceptEncodder uses the accept headers on a request to determine the response type. +// +// It supports the following encodings: +// - application/json: passed to encoding/json.Marshal +// - text/yaml: passed to gopkg.in/yaml.v2.Marshal +// - text/plain: passed to fmt.Sprintf("%V") +type AcceptEncoder struct { + DefaultEncoding string +} + +// Encode encodeds the given interface to the first available type in the Accept header. +func (e *AcceptEncoder) Encode(w http.ResponseWriter, r *http.Request, c *Context, out interface{}) { + a := r.Header.Get("accept") + fn := encoders[e.DefaultEncoding] + mt := e.DefaultEncoding + if a != "" { + mt, fn = e.parseAccept(a) + } + + data, err := fn(out) + if err != nil { + Fatal(w, r, "Could not marshal data: %s", err) + return + } + w.Header().Add("content-type", mt) + w.Write(data) +} + +// parseAccept parses the value of an Accept: header and returns the best match. +// +// This returns the matched MIME type and the Marshal function. +func (e *AcceptEncoder) parseAccept(h string) (string, Marshaler) { + + keys := strings.Split(h, ",") + for _, k := range keys { + mt, _, err := mime.ParseMediaType(k) + if err != nil { + continue + } + if enc, ok := encoders[mt]; ok { + return mt, enc + } + } + return e.DefaultEncoding, encoders[e.DefaultEncoding] +} + +type Marshaler func(interface{}) ([]byte, error) + +var encoders = map[string]Marshaler{ + "application/json": json.Marshal, + "text/yaml": yaml.Marshal, + "application/x-yaml": yaml.Marshal, + "text/plain": textMarshal, +} + +var ErrUnsupportedKind = errors.New("unsupported kind") + +// textMarshal marshals v into a text representation ONLY IN NARROW CASES. +// +// An error will have its Error() method called. +// A fmt.Stringer will have its String() method called. +// Scalar types will be marshaled with fmt.Sprintf("%v"). +// +// This will only marshal scalar types for securoty reasons (namely, we don't +// want the possibility of forcing exposure of non-exported data or ptr +// addresses, etc.) +func textMarshal(v interface{}) ([]byte, error) { + switch s := v.(type) { + case error: + return []byte(s.Error()), nil + case fmt.Stringer: + return []byte(s.String()), nil + } + + // Error on kinds we don't support. + val := reflect.Indirect(reflect.ValueOf(v)) + switch val.Kind() { + case reflect.Invalid, reflect.Array, reflect.Chan, reflect.Func, reflect.Interface, + reflect.Map, reflect.Ptr, reflect.Slice, reflect.Struct, reflect.UnsafePointer: + return []byte{}, ErrUnsupportedKind + } + return []byte(fmt.Sprintf("%v", v)), nil +} diff --git a/cmd/manager/router/encoder_test.go b/cmd/manager/router/encoder_test.go new file mode 100644 index 000000000..283581b3e --- /dev/null +++ b/cmd/manager/router/encoder_test.go @@ -0,0 +1,110 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package router + +import ( + "encoding/json" + "errors" + "io/ioutil" + "net/http" + "testing" +) + +var _ Encoder = &AcceptEncoder{} + +func TestParseAccept(t *testing.T) { + e := &AcceptEncoder{ + DefaultEncoding: "application/json", + } + tests := map[string]string{ + "": e.DefaultEncoding, + "*/*": e.DefaultEncoding, + // To stay true to spec, this _should_ be an error. But our thought + // on this case is that we'd rather send a default format. + "audio/*; q=0.2, audio/basic": e.DefaultEncoding, + "text/html; q=0.8, text/yaml,application/json": "text/yaml", + "application/x-yaml; foo=bar": "application/x-yaml", + "text/monkey, TEXT/YAML ; zoom=zoom ": "text/yaml", + } + + for in, expects := range tests { + mt, enc := e.parseAccept(in) + if mt != expects { + t.Errorf("Expected %q, got %q", expects, mt) + continue + } + _, err := enc([]string{"hello", "world"}) + if err != nil { + t.Fatalf("Failed to marshal: %s", err) + } + } +} + +func TestTextMarshal(t *testing.T) { + tests := map[string]interface{}{ + "foo": "foo", + "5": 5, + "stinky cheese": errors.New("stinky cheese"), + } + for expect, in := range tests { + if o, err := textMarshal(in); err != nil || string(o) != expect { + t.Errorf("Expected %q, got %q", expect, o) + } + } + + if _, err := textMarshal(struct{ foo int }{5}); err != ErrUnsupportedKind { + t.Fatalf("Expected unsupported kind, got %v", err) + } +} + +func TestAcceptEncoder(t *testing.T) { + c := &Context{ + Encoder: &AcceptEncoder{DefaultEncoding: "application/json"}, + } + fn := func(w http.ResponseWriter, r *http.Request, c *Context) error { + c.Encoder.Encode(w, r, c, []string{"hello", "world"}) + return nil + } + s := httpHarness(c, "GET /", fn) + defer s.Close() + + res, err := http.Get(s.URL) + if err != nil { + t.Fatal(err) + } + if res.StatusCode != 200 { + t.Fatalf("Unexpected response code %d", res.StatusCode) + } + if mt := res.Header.Get("content-type"); mt != "application/json" { + t.Errorf("Unexpected content type: %q", mt) + } + + data, err := ioutil.ReadAll(res.Body) + res.Body.Close() + if err != nil { + t.Fatalf("Failed to read response body: %s", err) + } + + out := []string{} + if err := json.Unmarshal(data, &out); err != nil { + t.Fatalf("Failed to unmarshal JSON: %s", err) + } + + if out[0] != "hello" { + t.Fatalf("Unexpected JSON data in slot 0: %s", out[0]) + } +} diff --git a/cmd/manager/router/router.go b/cmd/manager/router/router.go new file mode 100644 index 000000000..6f15292cd --- /dev/null +++ b/cmd/manager/router/router.go @@ -0,0 +1,203 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/* Package router is an HTTP router. + +This router provides appropriate dependency injection/encapsulation for the +HTTP routing layer. This removes the requirement to set global variables for +resources like database handles. + +This library does not replace the default HTTP mux because there is no need. +Instead, it implements an HTTP handler. + +It then defines a handler function that is given a context as well as a +request and response. +*/ +package router + +import ( + "fmt" + "net/http" + "os" + + "github.com/Masterminds/httputil" + "github.com/kubernetes/deployment-manager/cmd/manager/manager" + "github.com/kubernetes/deployment-manager/pkg/common" +) + +const LogAccess = "Access: %s %s" + +// Config holds the global configuration parameters passed into the router. +// +// Config is used concurrently. Once a config is created, it should be treated +// as immutable. +type Config struct { + // Address is the host and port (:8080) + Address string + // MaxTemplateLength is the maximum length of a template. + MaxTemplateLength int64 + // ExpanderName is the DNS name of the expansion service. + ExpanderName string + // ExpanderURL is the expander service's URL. + ExpanderURL string + // DeployerName is the deployer's DNS name + DeployerName string + // DeployerURL is the deployer's URL + DeployerURL string + // CredentialFile is the file to the credentials. + CredentialFile string + // CredentialSecrets tells the service to use a secrets file instead. + CredentialSecrets bool + // MongoName is the DNS name of the mongo server. + MongoName string + // MongoPort is the port for the MongoDB protocol on the mongo server. + // It is a string for historical reasons. + MongoPort string + // MongoAddress is the name and port. + MongoAddress string +} + +// Context contains dependencies that are passed to each handler function. +// +// Context carries typed information, often scoped to interfaces, so that the +// caller's contract with the service is known at compile time. +// +// Members of the context must be concurrency safe. +type Context struct { + Config *Config + // Manager is a deployment-manager/manager/manager.Manager + Manager manager.Manager + Encoder Encoder + CredentialProvider common.CredentialProvider +} + +func (c *Context) Log(msg string, v ...interface{}) { + // FIXME: This should be configurable via the context. + fmt.Fprintf(os.Stdout, msg+"\n", v...) +} + +func (c *Context) Err(msg string, v ...interface{}) { + // FIXME: This should be configurable via the context. + fmt.Fprintf(os.Stderr, msg+"\n", v...) +} + +// NotFound writes a 404 error to the client and logs an error. +func NotFound(w http.ResponseWriter, r *http.Request) { + // TODO: Log this. + w.WriteHeader(http.StatusNotFound) + fmt.Fprintln(w, "File Not Found") +} + +// Fatal writes a 500 response to the client and logs the message. +// +// Additional arguments are past into the the formatter as params to msg. +func Fatal(w http.ResponseWriter, r *http.Request, msg string, v ...interface{}) { + // TODO: Log this. + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintln(w, "Internal Server Error") +} + +// HandlerFunc responds to an individual HTTP request. +// +// Returned errors will be captured, logged, and returned as HTTP 500 errors. +type HandlerFunc func(w http.ResponseWriter, r *http.Request, c *Context) error + +// Handler implements an http.Handler. +// +// This is the top level route handler. +type Handler struct { + c *Context + resolver *httputil.Resolver + routes Routes +} + +// Create a new Handler. +// +// Routes cannot be modified after construction. The order that the route +// names are returned by Routes.Paths() determines the lookup order. +func NewHandler(c *Context, r Routes) *Handler { + paths := make([]string, r.Len()) + i := 0 + for _, k := range r.Paths() { + paths[i] = k + i++ + } + + return &Handler{ + c: c, + resolver: httputil.NewResolver(paths), + routes: r, + } +} + +// ServeHTTP serves an HTTP request. +func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + h.c.Log(LogAccess, r.Method, r.URL) + route, err := h.resolver.Resolve(r) + if err != nil { + NotFound(w, r) + return + } + + fn, ok := h.routes.Get(route) + if !ok { + Fatal(w, r, "route %s missing", route) + } + + if err := fn(w, r, h.c); err != nil { + Fatal(w, r, err.Error()) + } +} + +// Routes defines a container for route-to-function mapping. +type Routes interface { + Add(string, HandlerFunc) + Get(string) (HandlerFunc, bool) + Len() int + Paths() []string +} + +// NewRoutes creates a default implementation of a Routes. +// +// The ordering of routes is nonderministic. +func NewRoutes() Routes { + return routeMap{} +} + +type routeMap map[string]HandlerFunc + +func (r routeMap) Add(name string, fn HandlerFunc) { + r[name] = fn +} + +func (r routeMap) Get(name string) (HandlerFunc, bool) { + f, ok := r[name] + return f, ok +} + +func (r routeMap) Len() int { + return len(r) +} + +func (r routeMap) Paths() []string { + b := make([]string, len(r)) + i := 0 + for k := range r { + b[i] = k + i++ + } + return b +} diff --git a/cmd/manager/router/router_test.go b/cmd/manager/router/router_test.go new file mode 100644 index 000000000..df673ad04 --- /dev/null +++ b/cmd/manager/router/router_test.go @@ -0,0 +1,72 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package router + +import ( + "fmt" + "io/ioutil" + "net/http" + "net/http/httptest" + "testing" +) + +// Canary +var v Routes = routeMap{} + +func TestHandler(t *testing.T) { + c := &Context{} + r := NewRoutes() + + r.Add("GET /", func(w http.ResponseWriter, r *http.Request, c *Context) error { + fmt.Fprintln(w, "hello") + return nil + }) + r.Add("POST /", func(w http.ResponseWriter, r *http.Request, c *Context) error { + fmt.Fprintln(w, "goodbye") + return nil + }) + + h := NewHandler(c, r) + + s := httptest.NewServer(h) + defer s.Close() + + res, err := http.Get(s.URL) + if err != nil { + t.Fatal(err) + } + data, err := ioutil.ReadAll(res.Body) + res.Body.Close() + if err != nil { + t.Fatal(err) + } + + if "hello\n" != string(data) { + t.Errorf("Expected 'hello', got %q", data) + } +} + +// httpHarness is a simple test server fixture. +// Simple fixture for standing up a test server with a single route. +// +// You must Close() the returned server. +func httpHarness(c *Context, route string, fn HandlerFunc) *httptest.Server { + r := NewRoutes() + r.Add(route, fn) + h := NewHandler(c, r) + return httptest.NewServer(h) +} diff --git a/glide.lock b/glide.lock index 2a683ff75..67961e905 100644 --- a/glide.lock +++ b/glide.lock @@ -1,5 +1,5 @@ -hash: d9beab9a799ac8dd0d76c4f7a3a32753d44833dd3527b3caa8e786865ea26816 -updated: 2016-03-04T09:54:13.155442463-07:00 +hash: f34e830b237fcba5202f84c55b21764226b7e8c201f7622e9639c7a628e33b0b +updated: 2016-03-10T15:03:34.650809572-07:00 imports: - name: github.com/aokoli/goutils version: 9c37978a95bd5c709a15883b6242714ea6709e64 @@ -25,10 +25,12 @@ imports: version: 8f2758070a82adb7a3ad6b223a0b91878f32d400 - name: github.com/gorilla/mux version: 26a6070f849969ba72b72256e9f14cf519751690 +- name: github.com/Masterminds/httputil + version: e9b977e9cf16f9d339573e18f0f1f7ce5d3f419a - name: github.com/Masterminds/semver version: c4f7ef0702f269161a60489ccbbc9f1241ad1265 - name: github.com/Masterminds/sprig - version: fd057ca403105755181f84645696d705a58852dd + version: 0199893f008a87287bf2b4e3e390e66bb074c659 - name: golang.org/x/net version: 04b9de9b512f58addf28c9853d50ebef61c3953e subpackages: diff --git a/glide.yaml b/glide.yaml index 0ef3eafea..79237d249 100644 --- a/glide.yaml +++ b/glide.yaml @@ -14,3 +14,4 @@ import: - package: github.com/gorilla/mux - package: gopkg.in/yaml.v2 - package: github.com/Masterminds/sprig +- package: github.com/Masterminds/httputil From 05a3965602d11a109f4043211aa27835a6882e37 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Fri, 11 Mar 2016 17:40:59 -0700 Subject: [PATCH 2/4] fix(manager): add API test for healthz. --- cmd/manager/deployments.go | 2 +- cmd/manager/deployments_test.go | 40 +++++++++++++++ cmd/manager/main.go | 5 +- cmd/manager/router/router.go | 46 ++++------------- glide.lock | 2 +- pkg/httputil/doc.go | 21 ++++++++ .../router => pkg/httputil}/encoder.go | 19 +++++-- .../router => pkg/httputil}/encoder_test.go | 14 +++--- pkg/httputil/httperrors.go | 48 ++++++++++++++++++ pkg/httputil/httperrors_test.go | 50 +++++++++++++++++++ 10 files changed, 195 insertions(+), 52 deletions(-) create mode 100644 cmd/manager/deployments_test.go create mode 100644 pkg/httputil/doc.go rename {cmd/manager/router => pkg/httputil}/encoder.go (82%) rename {cmd/manager/router => pkg/httputil}/encoder_test.go (90%) create mode 100644 pkg/httputil/httperrors.go create mode 100644 pkg/httputil/httperrors_test.go diff --git a/cmd/manager/deployments.go b/cmd/manager/deployments.go index e71a2418e..6068dae46 100644 --- a/cmd/manager/deployments.go +++ b/cmd/manager/deployments.go @@ -99,7 +99,7 @@ func registerRoutes(c *router.Context) router.Routes { } func healthz(w http.ResponseWriter, r *http.Request, c *router.Context) error { - c.Log("manager: healthz checkpoint") + log.Println("manager: healthz checkpoint") // TODO: This should check the availability of the repository, and fail if it // cannot connect. fmt.Fprintln(w, "OK") diff --git a/cmd/manager/deployments_test.go b/cmd/manager/deployments_test.go new file mode 100644 index 000000000..58a5ad3f6 --- /dev/null +++ b/cmd/manager/deployments_test.go @@ -0,0 +1,40 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/kubernetes/deployment-manager/cmd/manager/router" +) + +func TestHealthz(t *testing.T) { + c := mockContext() + s := httpHarness(c, "GET /", healthz) + defer s.Close() + + res, err := http.Get(s.URL) + if err != nil { + t.Fatalf("Failed to GET healthz: %s", err) + } else if res.StatusCode != 200 { + t.Fatalf("Unexpected status: %d", res.StatusCode) + } + + // TODO: Get the body and check on the content type and the body. +} + +// httpHarness is a simple test server fixture. +// Simple fixture for standing up a test server with a single route. +// +// You must Close() the returned server. +func httpHarness(c *router.Context, route string, fn router.HandlerFunc) *httptest.Server { + r := router.NewRoutes() + r.Add(route, fn) + h := router.NewHandler(c, r) + return httptest.NewServer(h) +} + +func mockContext() *router.Context { + // TODO: We need mocks for credentials and manager. + return &router.Context{} +} diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 7aca7e80a..4c26d599f 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -22,6 +22,7 @@ import ( "flag" "fmt" + "log" "net/http" "os" ) @@ -55,9 +56,9 @@ func main() { routes := registerRoutes(c) // Now create a server. - c.Log("Starting Manager %s on %s", version.Version, c.Config.Address) + log.Printf("Starting Manager %s on %s", version.Version, c.Config.Address) if err := http.ListenAndServe(c.Config.Address, router.NewHandler(c, routes)); err != nil { - c.Err("Server exited with error %s", err) + log.Printf("Server exited with error %s", err) os.Exit(1) } } diff --git a/cmd/manager/router/router.go b/cmd/manager/router/router.go index 6f15292cd..3a6bf5a42 100644 --- a/cmd/manager/router/router.go +++ b/cmd/manager/router/router.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -/* Package router is an HTTP router. +/*Package router is an HTTP router. This router provides appropriate dependency injection/encapsulation for the HTTP routing layer. This removes the requirement to set global variables for @@ -29,17 +29,15 @@ request and response. package router import ( - "fmt" + "log" "net/http" - "os" "github.com/Masterminds/httputil" "github.com/kubernetes/deployment-manager/cmd/manager/manager" "github.com/kubernetes/deployment-manager/pkg/common" + helmhttp "github.com/kubernetes/deployment-manager/pkg/httputil" ) -const LogAccess = "Access: %s %s" - // Config holds the global configuration parameters passed into the router. // // Config is used concurrently. Once a config is created, it should be treated @@ -80,36 +78,10 @@ type Context struct { Config *Config // Manager is a deployment-manager/manager/manager.Manager Manager manager.Manager - Encoder Encoder + Encoder helmhttp.Encoder CredentialProvider common.CredentialProvider } -func (c *Context) Log(msg string, v ...interface{}) { - // FIXME: This should be configurable via the context. - fmt.Fprintf(os.Stdout, msg+"\n", v...) -} - -func (c *Context) Err(msg string, v ...interface{}) { - // FIXME: This should be configurable via the context. - fmt.Fprintf(os.Stderr, msg+"\n", v...) -} - -// NotFound writes a 404 error to the client and logs an error. -func NotFound(w http.ResponseWriter, r *http.Request) { - // TODO: Log this. - w.WriteHeader(http.StatusNotFound) - fmt.Fprintln(w, "File Not Found") -} - -// Fatal writes a 500 response to the client and logs the message. -// -// Additional arguments are past into the the formatter as params to msg. -func Fatal(w http.ResponseWriter, r *http.Request, msg string, v ...interface{}) { - // TODO: Log this. - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprintln(w, "Internal Server Error") -} - // HandlerFunc responds to an individual HTTP request. // // Returned errors will be captured, logged, and returned as HTTP 500 errors. @@ -124,7 +96,7 @@ type Handler struct { routes Routes } -// Create a new Handler. +// NewHandler creates a new Handler. // // Routes cannot be modified after construction. The order that the route // names are returned by Routes.Paths() determines the lookup order. @@ -145,20 +117,20 @@ func NewHandler(c *Context, r Routes) *Handler { // ServeHTTP serves an HTTP request. func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - h.c.Log(LogAccess, r.Method, r.URL) + log.Printf(helmhttp.LogAccess, r.Method, r.URL) route, err := h.resolver.Resolve(r) if err != nil { - NotFound(w, r) + helmhttp.NotFound(w, r) return } fn, ok := h.routes.Get(route) if !ok { - Fatal(w, r, "route %s missing", route) + helmhttp.Fatal(w, r, "route %s missing", route) } if err := fn(w, r, h.c); err != nil { - Fatal(w, r, err.Error()) + helmhttp.Fatal(w, r, err.Error()) } } diff --git a/glide.lock b/glide.lock index 67961e905..9c507ef66 100644 --- a/glide.lock +++ b/glide.lock @@ -1,5 +1,5 @@ hash: f34e830b237fcba5202f84c55b21764226b7e8c201f7622e9639c7a628e33b0b -updated: 2016-03-10T15:03:34.650809572-07:00 +updated: 2016-03-14T10:53:18.091793732-06:00 imports: - name: github.com/aokoli/goutils version: 9c37978a95bd5c709a15883b6242714ea6709e64 diff --git a/pkg/httputil/doc.go b/pkg/httputil/doc.go new file mode 100644 index 000000000..2c33cf5b2 --- /dev/null +++ b/pkg/httputil/doc.go @@ -0,0 +1,21 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/*Package httputil provides common HTTP tools. + +This package provides tools for working with HTTP requests and responses. +*/ +package httputil diff --git a/cmd/manager/router/encoder.go b/pkg/httputil/encoder.go similarity index 82% rename from cmd/manager/router/encoder.go rename to pkg/httputil/encoder.go index fd401cb59..beefd20c0 100644 --- a/cmd/manager/router/encoder.go +++ b/pkg/httputil/encoder.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package router +package httputil import ( "encoding/json" @@ -28,26 +28,35 @@ import ( "github.com/ghodss/yaml" ) +// Encoder takes input and translate it to an expected encoded output. +// +// Implementations of encoders may use details of the HTTP request and response +// to correctly encode an object for return to the client. +// +// Encoders are expected to produce output, even if that output is an error +// message. type Encoder interface { // Encode encoders a given response // // When an encoder fails, it logs any necessary data and then responds to // the client. - Encode(http.ResponseWriter, *http.Request, *Context, interface{}) + Encode(http.ResponseWriter, *http.Request, interface{}) } -// AcceptEncodder uses the accept headers on a request to determine the response type. +// AcceptEncoder uses the accept headers on a request to determine the response type. // // It supports the following encodings: // - application/json: passed to encoding/json.Marshal // - text/yaml: passed to gopkg.in/yaml.v2.Marshal // - text/plain: passed to fmt.Sprintf("%V") +// +// It treats `application/x-yaml` as `text/yaml`. type AcceptEncoder struct { DefaultEncoding string } // Encode encodeds the given interface to the first available type in the Accept header. -func (e *AcceptEncoder) Encode(w http.ResponseWriter, r *http.Request, c *Context, out interface{}) { +func (e *AcceptEncoder) Encode(w http.ResponseWriter, r *http.Request, out interface{}) { a := r.Header.Get("accept") fn := encoders[e.DefaultEncoding] mt := e.DefaultEncoding @@ -82,6 +91,7 @@ func (e *AcceptEncoder) parseAccept(h string) (string, Marshaler) { return e.DefaultEncoding, encoders[e.DefaultEncoding] } +// Marshaler marshals an interface{} into a []byte. type Marshaler func(interface{}) ([]byte, error) var encoders = map[string]Marshaler{ @@ -91,6 +101,7 @@ var encoders = map[string]Marshaler{ "text/plain": textMarshal, } +// ErrUnsupportedKind indicates that the marshal cannot marshal a particular Go Kind (e.g. struct or chan). var ErrUnsupportedKind = errors.New("unsupported kind") // textMarshal marshals v into a text representation ONLY IN NARROW CASES. diff --git a/cmd/manager/router/encoder_test.go b/pkg/httputil/encoder_test.go similarity index 90% rename from cmd/manager/router/encoder_test.go rename to pkg/httputil/encoder_test.go index 283581b3e..115c961d5 100644 --- a/cmd/manager/router/encoder_test.go +++ b/pkg/httputil/encoder_test.go @@ -14,13 +14,14 @@ See the License for the specific language governing permissions and limitations under the License. */ -package router +package httputil import ( "encoding/json" "errors" "io/ioutil" "net/http" + "net/http/httptest" "testing" ) @@ -72,14 +73,13 @@ func TestTextMarshal(t *testing.T) { } func TestAcceptEncoder(t *testing.T) { - c := &Context{ - Encoder: &AcceptEncoder{DefaultEncoding: "application/json"}, + enc := &AcceptEncoder{ + DefaultEncoding: "application/json", } - fn := func(w http.ResponseWriter, r *http.Request, c *Context) error { - c.Encoder.Encode(w, r, c, []string{"hello", "world"}) - return nil + fn := func(w http.ResponseWriter, r *http.Request) { + enc.Encode(w, r, []string{"hello", "world"}) } - s := httpHarness(c, "GET /", fn) + s := httptest.NewServer(http.HandlerFunc(fn)) defer s.Close() res, err := http.Get(s.URL) diff --git a/pkg/httputil/httperrors.go b/pkg/httputil/httperrors.go new file mode 100644 index 000000000..1d9f0d56e --- /dev/null +++ b/pkg/httputil/httperrors.go @@ -0,0 +1,48 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package httputil + +import ( + "fmt" + "log" + "net/http" +) + +const ( + // LogAccess is for logging access messages. Form: Access r.Method, r.URL + LogAccess = "Access: %s %s" + // LogNotFound is for logging 404 errors. Form: Not Found r.Method, r.URL + LogNotFound = "Not Found: %s %s" + // LogFatal is for logging 500 errors. Form: Internal Server Error r.Method r.URL message + LogFatal = "Internal Server Error: %s %s %s" +) + +// NotFound writes a 404 error to the client and logs an error. +func NotFound(w http.ResponseWriter, r *http.Request) { + log.Printf(LogNotFound, r.Method, r.URL) + w.WriteHeader(http.StatusNotFound) + fmt.Fprintln(w, "File Not Found") +} + +// Fatal writes a 500 response to the client and logs the message. +// +// Additional arguments are past into the the formatter as params to msg. +func Fatal(w http.ResponseWriter, r *http.Request, msg string, v ...interface{}) { + log.Printf(LogFatal, r.Method, r.URL, fmt.Sprintf(msg, v...)) + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintln(w, "Internal Server Error") +} diff --git a/pkg/httputil/httperrors_test.go b/pkg/httputil/httperrors_test.go new file mode 100644 index 000000000..ea1fecad9 --- /dev/null +++ b/pkg/httputil/httperrors_test.go @@ -0,0 +1,50 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package httputil + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func TestNotFound(t *testing.T) { + fn := func(w http.ResponseWriter, r *http.Request) { + NotFound(w, r) + } + testStatusCode(http.HandlerFunc(fn), 404, t) +} + +func TestFatal(t *testing.T) { + fn := func(w http.ResponseWriter, r *http.Request) { + Fatal(w, r, "fatal %s", "foo") + } + testStatusCode(http.HandlerFunc(fn), 500, t) +} + +func testStatusCode(fn http.HandlerFunc, expect int, t *testing.T) { + s := httptest.NewServer(fn) + defer s.Close() + + res, err := http.Get(s.URL) + if err != nil { + t.Fatal(err) + } + if res.StatusCode != expect { + t.Errorf("Expected %d, got %d", expect, res.StatusCode) + } +} From ce32351bec1fbde402c8a1315852ae9c2a6a5e00 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Mon, 14 Mar 2016 12:41:59 -0600 Subject: [PATCH 3/4] fix(manager): simplify adding routes This removes the Routes interface and routesMap struct in favor of just maintaining the routes inside of the handler. --- cmd/manager/deployments.go | 9 +-- cmd/manager/deployments_test.go | 5 +- cmd/manager/main.go | 5 +- cmd/manager/router/context.go | 51 +++++++++++++ cmd/manager/router/router.go | 114 +++++------------------------- cmd/manager/router/router_test.go | 22 +----- 6 files changed, 78 insertions(+), 128 deletions(-) create mode 100644 cmd/manager/router/context.go diff --git a/cmd/manager/deployments.go b/cmd/manager/deployments.go index 6068dae46..39764121c 100644 --- a/cmd/manager/deployments.go +++ b/cmd/manager/deployments.go @@ -79,23 +79,20 @@ type Route struct { Type string } -func registerRoutes(c *router.Context) router.Routes { +func registerRoutes(c *router.Context, h *router.Handler) { re := regexp.MustCompile("{[a-z]+}") - r := router.NewRoutes() - r.Add("GET /healthz", healthz) + h.Add("GET /healthz", healthz) // TODO: Replace these routes with updated ones. for _, d := range deployments { path := fmt.Sprintf("%s %s", d.Methods, re.ReplaceAllString(d.Path, "*")) fmt.Printf("\t%s\n", path) - r.Add(path, func(w http.ResponseWriter, r *http.Request, c *router.Context) error { + h.Add(path, func(w http.ResponseWriter, r *http.Request, c *router.Context) error { d.HandlerFunc(w, r) return nil }) } - - return r } func healthz(w http.ResponseWriter, r *http.Request, c *router.Context) error { diff --git a/cmd/manager/deployments_test.go b/cmd/manager/deployments_test.go index 58a5ad3f6..b4560f794 100644 --- a/cmd/manager/deployments_test.go +++ b/cmd/manager/deployments_test.go @@ -28,9 +28,8 @@ func TestHealthz(t *testing.T) { // // You must Close() the returned server. func httpHarness(c *router.Context, route string, fn router.HandlerFunc) *httptest.Server { - r := router.NewRoutes() - r.Add(route, fn) - h := router.NewHandler(c, r) + h := router.NewHandler(c) + h.Add(route, fn) return httptest.NewServer(h) } diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 4c26d599f..297f53706 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -53,11 +53,12 @@ func main() { } // Set up routes - routes := registerRoutes(c) + handler := router.NewHandler(c) + registerRoutes(c, handler) // Now create a server. log.Printf("Starting Manager %s on %s", version.Version, c.Config.Address) - if err := http.ListenAndServe(c.Config.Address, router.NewHandler(c, routes)); err != nil { + if err := http.ListenAndServe(c.Config.Address, handler); err != nil { log.Printf("Server exited with error %s", err) os.Exit(1) } diff --git a/cmd/manager/router/context.go b/cmd/manager/router/context.go new file mode 100644 index 000000000..bca74ddfc --- /dev/null +++ b/cmd/manager/router/context.go @@ -0,0 +1,51 @@ +package router + +import ( + "github.com/kubernetes/deployment-manager/cmd/manager/manager" + "github.com/kubernetes/deployment-manager/pkg/common" + helmhttp "github.com/kubernetes/deployment-manager/pkg/httputil" +) + +// Config holds the global configuration parameters passed into the router. +// +// Config is used concurrently. Once a config is created, it should be treated +// as immutable. +type Config struct { + // Address is the host and port (:8080) + Address string + // MaxTemplateLength is the maximum length of a template. + MaxTemplateLength int64 + // ExpanderName is the DNS name of the expansion service. + ExpanderName string + // ExpanderURL is the expander service's URL. + ExpanderURL string + // DeployerName is the deployer's DNS name + DeployerName string + // DeployerURL is the deployer's URL + DeployerURL string + // CredentialFile is the file to the credentials. + CredentialFile string + // CredentialSecrets tells the service to use a secrets file instead. + CredentialSecrets bool + // MongoName is the DNS name of the mongo server. + MongoName string + // MongoPort is the port for the MongoDB protocol on the mongo server. + // It is a string for historical reasons. + MongoPort string + // MongoAddress is the name and port. + MongoAddress string +} + +// Context contains dependencies that are passed to each handler function. +// +// Context carries typed information, often scoped to interfaces, so that the +// caller's contract with the service is known at compile time. +// +// Members of the context must be concurrency safe. +type Context struct { + Config *Config + // Manager is a deployment-manager/manager/manager.Manager + Manager manager.Manager + Encoder helmhttp.Encoder + CredentialProvider common.CredentialProvider +} diff --git a/cmd/manager/router/router.go b/cmd/manager/router/router.go index 3a6bf5a42..69ecc1e72 100644 --- a/cmd/manager/router/router.go +++ b/cmd/manager/router/router.go @@ -33,55 +33,9 @@ import ( "net/http" "github.com/Masterminds/httputil" - "github.com/kubernetes/deployment-manager/cmd/manager/manager" - "github.com/kubernetes/deployment-manager/pkg/common" helmhttp "github.com/kubernetes/deployment-manager/pkg/httputil" ) -// Config holds the global configuration parameters passed into the router. -// -// Config is used concurrently. Once a config is created, it should be treated -// as immutable. -type Config struct { - // Address is the host and port (:8080) - Address string - // MaxTemplateLength is the maximum length of a template. - MaxTemplateLength int64 - // ExpanderName is the DNS name of the expansion service. - ExpanderName string - // ExpanderURL is the expander service's URL. - ExpanderURL string - // DeployerName is the deployer's DNS name - DeployerName string - // DeployerURL is the deployer's URL - DeployerURL string - // CredentialFile is the file to the credentials. - CredentialFile string - // CredentialSecrets tells the service to use a secrets file instead. - CredentialSecrets bool - // MongoName is the DNS name of the mongo server. - MongoName string - // MongoPort is the port for the MongoDB protocol on the mongo server. - // It is a string for historical reasons. - MongoPort string - // MongoAddress is the name and port. - MongoAddress string -} - -// Context contains dependencies that are passed to each handler function. -// -// Context carries typed information, often scoped to interfaces, so that the -// caller's contract with the service is known at compile time. -// -// Members of the context must be concurrency safe. -type Context struct { - Config *Config - // Manager is a deployment-manager/manager/manager.Manager - Manager manager.Manager - Encoder helmhttp.Encoder - CredentialProvider common.CredentialProvider -} - // HandlerFunc responds to an individual HTTP request. // // Returned errors will be captured, logged, and returned as HTTP 500 errors. @@ -93,28 +47,32 @@ type HandlerFunc func(w http.ResponseWriter, r *http.Request, c *Context) error type Handler struct { c *Context resolver *httputil.Resolver - routes Routes + routes map[string]HandlerFunc + paths []string } // NewHandler creates a new Handler. // // Routes cannot be modified after construction. The order that the route // names are returned by Routes.Paths() determines the lookup order. -func NewHandler(c *Context, r Routes) *Handler { - paths := make([]string, r.Len()) - i := 0 - for _, k := range r.Paths() { - paths[i] = k - i++ - } - +func NewHandler(c *Context) *Handler { return &Handler{ c: c, - resolver: httputil.NewResolver(paths), - routes: r, + resolver: httputil.NewResolver([]string{}), + routes: map[string]HandlerFunc{}, + paths: []string{}, } } +// Add a route to a handler. +// +// The route name is "VERB /ENPOINT/PATH", e.g. "GET /foo". +func (h *Handler) Add(route string, fn HandlerFunc) { + h.routes[route] = fn + h.paths = append(h.paths, route) + h.resolver = httputil.NewResolver(h.paths) +} + // ServeHTTP serves an HTTP request. func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { log.Printf(helmhttp.LogAccess, r.Method, r.URL) @@ -124,7 +82,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - fn, ok := h.routes.Get(route) + fn, ok := h.routes[route] if !ok { helmhttp.Fatal(w, r, "route %s missing", route) } @@ -133,43 +91,3 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { helmhttp.Fatal(w, r, err.Error()) } } - -// Routes defines a container for route-to-function mapping. -type Routes interface { - Add(string, HandlerFunc) - Get(string) (HandlerFunc, bool) - Len() int - Paths() []string -} - -// NewRoutes creates a default implementation of a Routes. -// -// The ordering of routes is nonderministic. -func NewRoutes() Routes { - return routeMap{} -} - -type routeMap map[string]HandlerFunc - -func (r routeMap) Add(name string, fn HandlerFunc) { - r[name] = fn -} - -func (r routeMap) Get(name string) (HandlerFunc, bool) { - f, ok := r[name] - return f, ok -} - -func (r routeMap) Len() int { - return len(r) -} - -func (r routeMap) Paths() []string { - b := make([]string, len(r)) - i := 0 - for k := range r { - b[i] = k - i++ - } - return b -} diff --git a/cmd/manager/router/router_test.go b/cmd/manager/router/router_test.go index df673ad04..d34057919 100644 --- a/cmd/manager/router/router_test.go +++ b/cmd/manager/router/router_test.go @@ -24,24 +24,19 @@ import ( "testing" ) -// Canary -var v Routes = routeMap{} - func TestHandler(t *testing.T) { c := &Context{} - r := NewRoutes() - r.Add("GET /", func(w http.ResponseWriter, r *http.Request, c *Context) error { + h := NewHandler(c) + h.Add("GET /", func(w http.ResponseWriter, r *http.Request, c *Context) error { fmt.Fprintln(w, "hello") return nil }) - r.Add("POST /", func(w http.ResponseWriter, r *http.Request, c *Context) error { + h.Add("POST /", func(w http.ResponseWriter, r *http.Request, c *Context) error { fmt.Fprintln(w, "goodbye") return nil }) - h := NewHandler(c, r) - s := httptest.NewServer(h) defer s.Close() @@ -59,14 +54,3 @@ func TestHandler(t *testing.T) { t.Errorf("Expected 'hello', got %q", data) } } - -// httpHarness is a simple test server fixture. -// Simple fixture for standing up a test server with a single route. -// -// You must Close() the returned server. -func httpHarness(c *Context, route string, fn HandlerFunc) *httptest.Server { - r := NewRoutes() - r.Add(route, fn) - h := NewHandler(c, r) - return httptest.NewServer(h) -} From 527a4f9c9d94baea29315c8252315d6c3ad412ee Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Tue, 15 Mar 2016 14:41:03 -0600 Subject: [PATCH 4/4] fix(manager): rewrite URL param grabbing This updates path grabbing to match the new URI matcher. --- cmd/manager/deployments.go | 60 +++++++++++++++++++++++--------------- pkg/httputil/httperrors.go | 8 +++++ 2 files changed, 44 insertions(+), 24 deletions(-) diff --git a/cmd/manager/deployments.go b/cmd/manager/deployments.go index 39764121c..e3537fc0e 100644 --- a/cmd/manager/deployments.go +++ b/cmd/manager/deployments.go @@ -38,15 +38,14 @@ import ( "github.com/kubernetes/deployment-manager/cmd/manager/repository/transient" "github.com/kubernetes/deployment-manager/cmd/manager/router" "github.com/kubernetes/deployment-manager/pkg/common" + "github.com/kubernetes/deployment-manager/pkg/httputil" "github.com/kubernetes/deployment-manager/pkg/registry" "github.com/kubernetes/deployment-manager/pkg/util" ) var deployments = []Route{ - {"ListDeployments", "/deployments", "GET", listDeploymentsHandlerFunc, ""}, - {"GetDeployment", "/deployments/{deployment}", "GET", getDeploymentHandlerFunc, ""}, {"CreateDeployment", "/deployments", "POST", createDeploymentHandlerFunc, "JSON"}, - {"DeleteDeployment", "/deployments/{deployment}", "DELETE", deleteDeploymentHandlerFunc, ""}, + {"DeleteDeplyment", "/deployments/{deployment}", "DELETE", deleteDeploymentHandlerFunc, ""}, {"PutDeployment", "/deployments/{deployment}", "PUT", putDeploymentHandlerFunc, "JSON"}, {"ListManifests", "/deployments/{deployment}/manifests", "GET", listManifestsHandlerFunc, ""}, {"GetManifest", "/deployments/{deployment}/manifests/{manifest}", "GET", getManifestHandlerFunc, ""}, @@ -83,6 +82,8 @@ func registerRoutes(c *router.Context, h *router.Handler) { re := regexp.MustCompile("{[a-z]+}") h.Add("GET /healthz", healthz) + h.Add("GET /deployments", listDeploymentsHandlerFunc) + h.Add("GET /deployments/*", getDeploymentHandlerFunc) // TODO: Replace these routes with updated ones. for _, d := range deployments { @@ -186,13 +187,13 @@ func makeEnvVariableName(str string) string { return strings.ToUpper(strings.Replace(str, "-", "_", -1)) } -func listDeploymentsHandlerFunc(w http.ResponseWriter, r *http.Request) { +func listDeploymentsHandlerFunc(w http.ResponseWriter, r *http.Request, c *router.Context) error { handler := "manager: list deployments" util.LogHandlerEntry(handler, r) l, err := backend.ListDeployments() if err != nil { util.LogAndReturnError(handler, http.StatusInternalServerError, err, w) - return + return nil } var names []string for _, d := range l { @@ -200,23 +201,25 @@ func listDeploymentsHandlerFunc(w http.ResponseWriter, r *http.Request) { } util.LogHandlerExitWithJSON(handler, w, names, http.StatusOK) + return nil } -func getDeploymentHandlerFunc(w http.ResponseWriter, r *http.Request) { +func getDeploymentHandlerFunc(w http.ResponseWriter, r *http.Request, c *router.Context) error { handler := "manager: get deployment" util.LogHandlerEntry(handler, r) name, err := getPathVariable(w, r, "deployment", handler) if err != nil { - return + return nil } d, err := backend.GetDeployment(name) if err != nil { util.LogAndReturnError(handler, http.StatusBadRequest, err, w) - return + return nil } util.LogHandlerExitWithJSON(handler, w, d, http.StatusOK) + return nil } func createDeploymentHandlerFunc(w http.ResponseWriter, r *http.Request) { @@ -240,7 +243,7 @@ func deleteDeploymentHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: delete deployment" util.LogHandlerEntry(handler, r) defer r.Body.Close() - name, err := getPathVariable(w, r, "deployment", handler) + name, err := pos(w, r, 2) //getPathVariable(w, r, "deployment", handler) if err != nil { return } @@ -258,7 +261,7 @@ func putDeploymentHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: update deployment" util.LogHandlerEntry(handler, r) defer r.Body.Close() - name, err := getPathVariable(w, r, "deployment", handler) + name, err := pos(w, r, 2) //getPathVariable(w, r, "deployment", handler) if err != nil { return } @@ -275,6 +278,15 @@ func putDeploymentHandlerFunc(w http.ResponseWriter, r *http.Request) { } } +func pos(w http.ResponseWriter, r *http.Request, i int) (string, error) { + parts := strings.Split(r.URL.Path, "/") + if len(parts) < i-1 { + httputil.BadRequest(w, r) + return "", fmt.Errorf("No index for %d", i) + } + return parts[i], nil +} + func getPathVariable(w http.ResponseWriter, r *http.Request, variable, handler string) (string, error) { vars := mux.Vars(r) escaped, ok := vars[variable] @@ -315,7 +327,7 @@ func getTemplate(w http.ResponseWriter, r *http.Request, handler string) *common func listManifestsHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: list manifests" util.LogHandlerEntry(handler, r) - deploymentName, err := getPathVariable(w, r, "deployment", handler) + deploymentName, err := pos(w, r, 2) if err != nil { return } @@ -337,12 +349,12 @@ func listManifestsHandlerFunc(w http.ResponseWriter, r *http.Request) { func getManifestHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: get manifest" util.LogHandlerEntry(handler, r) - deploymentName, err := getPathVariable(w, r, "deployment", handler) + deploymentName, err := pos(w, r, 2) if err != nil { return } - manifestName, err := getPathVariable(w, r, "manifest", handler) + manifestName, err := pos(w, r, 4) if err != nil { return } @@ -390,7 +402,7 @@ func listTypesHandlerFunc(w http.ResponseWriter, r *http.Request) { func listTypeInstancesHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: list instances" util.LogHandlerEntry(handler, r) - typeName, err := getPathVariable(w, r, "type", handler) + typeName, err := pos(w, r, 2) if err != nil { return } @@ -407,7 +419,7 @@ func listTypeInstancesHandlerFunc(w http.ResponseWriter, r *http.Request) { func getRegistryForTypeHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: get type registry" util.LogHandlerEntry(handler, r) - typeName, err := getPathVariable(w, r, "type", handler) + typeName, err := pos(w, r, 2) if err != nil { return } @@ -424,7 +436,7 @@ func getRegistryForTypeHandlerFunc(w http.ResponseWriter, r *http.Request) { func getMetadataForTypeHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: get type metadata" util.LogHandlerEntry(handler, r) - typeName, err := getPathVariable(w, r, "type", handler) + typeName, err := pos(w, r, 2) if err != nil { return } @@ -454,7 +466,7 @@ func listRegistriesHandlerFunc(w http.ResponseWriter, r *http.Request) { func getRegistryHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: get registry" util.LogHandlerEntry(handler, r) - registryName, err := getPathVariable(w, r, "registry", handler) + registryName, err := pos(w, r, 2) if err != nil { return } @@ -489,7 +501,7 @@ func createRegistryHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: create registry" util.LogHandlerEntry(handler, r) defer r.Body.Close() - registryName, err := getPathVariable(w, r, "registry", handler) + registryName, err := pos(w, r, 2) if err != nil { return } @@ -514,7 +526,7 @@ func createRegistryHandlerFunc(w http.ResponseWriter, r *http.Request) { func listRegistryTypesHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: list registry types" util.LogHandlerEntry(handler, r) - registryName, err := getPathVariable(w, r, "registry", handler) + registryName, err := pos(w, r, 2) if err != nil { return } @@ -547,12 +559,12 @@ func listRegistryTypesHandlerFunc(w http.ResponseWriter, r *http.Request) { func getDownloadURLsHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: get download URLs" util.LogHandlerEntry(handler, r) - registryName, err := getPathVariable(w, r, "registry", handler) + registryName, err := pos(w, r, 2) if err != nil { return } - typeName, err := getPathVariable(w, r, "type", handler) + typeName, err := pos(w, r, 4) if err != nil { return } @@ -579,7 +591,7 @@ func getDownloadURLsHandlerFunc(w http.ResponseWriter, r *http.Request) { func getFileHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: get file" util.LogHandlerEntry(handler, r) - registryName, err := getPathVariable(w, r, "registry", handler) + registryName, err := pos(w, r, 2) if err != nil { return } @@ -619,7 +631,7 @@ func createCredentialHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: create credential" util.LogHandlerEntry(handler, r) defer r.Body.Close() - credentialName, err := getPathVariable(w, r, "credential", handler) + credentialName, err := pos(w, r, 2) if err != nil { return } @@ -639,7 +651,7 @@ func createCredentialHandlerFunc(w http.ResponseWriter, r *http.Request) { func getCredentialHandlerFunc(w http.ResponseWriter, r *http.Request) { handler := "manager: get credential" util.LogHandlerEntry(handler, r) - credentialName, err := getPathVariable(w, r, "credential", handler) + credentialName, err := pos(w, r, 2) if err != nil { return } diff --git a/pkg/httputil/httperrors.go b/pkg/httputil/httperrors.go index 1d9f0d56e..c13a2a6bf 100644 --- a/pkg/httputil/httperrors.go +++ b/pkg/httputil/httperrors.go @@ -29,6 +29,8 @@ const ( LogNotFound = "Not Found: %s %s" // LogFatal is for logging 500 errors. Form: Internal Server Error r.Method r.URL message LogFatal = "Internal Server Error: %s %s %s" + // LogBadRequest logs 400 errors. + LogBadRequest = "Bad Request: %s %s" ) // NotFound writes a 404 error to the client and logs an error. @@ -38,6 +40,12 @@ func NotFound(w http.ResponseWriter, r *http.Request) { fmt.Fprintln(w, "File Not Found") } +func BadRequest(w http.ResponseWriter, r *http.Request) { + log.Printf(LogNotFound, r.Method, r.URL) + w.WriteHeader(http.StatusBadRequest) + fmt.Fprintln(w, "Bad Request") +} + // Fatal writes a 500 response to the client and logs the message. // // Additional arguments are past into the the formatter as params to msg.