You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/04/23 00:06:00 UTC
[GitHub] [trafficcontrol] rawlinp commented on a diff in pull request #6754: Convert Traffic Ops into a Service Oriented architecture(SOA) product
rawlinp commented on code in PR #6754:
URL: https://github.com/apache/trafficcontrol/pull/6754#discussion_r856593738
##########
docs/source/admin/traffic_ops.rst:
##########
@@ -586,6 +590,29 @@ This file sets authentication options for connections to Riak when used as the T
.. impl-detail:: The name of this file is derived from the current database used in the implementation of Traffic Vault - `Riak KV <https://riak.com/products/riak-kv/index.html>`_.
+.. _backends.conf:
+
+backends.conf
+"""""""""""""
+This file deals with the configuration parameters of running Traffic Ops as a reverse proxy for certain endpoints that need to be served externally by other backend services. It is a JSON-format set of options and their respective values. `traffic_ops_golang`_ will use whatever file is specified (if any) by its :option:`--backendcfg` option. The keys of the file are described below.
+
+:routes: This is an array of options to configure Traffic Ops to forward requests of specified types to the appropriate backends.
+
+ :path: The endpoint that will be served by the backend, for example, `api/4.0/foo`.
+ :method: The HTTP method for the above mentioned path, for example, `GET` or `PUT`.
+ :routeId: The integral identifier for the new route being added.
+ :hosts: An array of the hosts and ports where the request (if matched) needs to be forwarded to, for example, `cdn-foo-backend-service-host:9090`.
+ :insecure: A boolean specifying whether or not to enable `InsecureSkipVerify`. This is an optional parameter, defaulting to `false` when not present.
+ :permissions: An array of permissions(strings) specifying the permissions required by the user to use this API route.
Review Comment:
space needed between `permissions` and `(strings)`
If there are no permissions specified, can the route be used by any user?
##########
docs/source/admin/traffic_ops.rst:
##########
@@ -586,6 +590,29 @@ This file sets authentication options for connections to Riak when used as the T
.. impl-detail:: The name of this file is derived from the current database used in the implementation of Traffic Vault - `Riak KV <https://riak.com/products/riak-kv/index.html>`_.
+.. _backends.conf:
+
+backends.conf
+"""""""""""""
+This file deals with the configuration parameters of running Traffic Ops as a reverse proxy for certain endpoints that need to be served externally by other backend services. It is a JSON-format set of options and their respective values. `traffic_ops_golang`_ will use whatever file is specified (if any) by its :option:`--backendcfg` option. The keys of the file are described below.
+
+:routes: This is an array of options to configure Traffic Ops to forward requests of specified types to the appropriate backends.
+
+ :path: The endpoint that will be served by the backend, for example, `api/4.0/foo`.
+ :method: The HTTP method for the above mentioned path, for example, `GET` or `PUT`.
+ :routeId: The integral identifier for the new route being added.
+ :hosts: An array of the hosts and ports where the request (if matched) needs to be forwarded to, for example, `cdn-foo-backend-service-host:9090`.
+ :insecure: A boolean specifying whether or not to enable `InsecureSkipVerify`. This is an optional parameter, defaulting to `false` when not present.
Review Comment:
Rather than saying this is for the `InsecureSkipVerify` struct field, we should elaborate on what that actually means (controls whether TO verifies the backend server's certificate chain and host name -- not recommended for production use).
##########
traffic_ops/traffic_ops_golang/routing/routing.go:
##########
@@ -280,8 +307,88 @@ func Handler(
h.ServeHTTP(w, r)
return
}
+ var backendRouteHandled bool
+ backendConfig := GetBackendConfig()
+ for i, backendRoute := range backendConfig.Routes {
+ if backendRoute.Path == r.URL.Path && backendRoute.Method == r.Method {
+ if backendRoute.Opts.Algorithm == "" || backendRoute.Opts.Algorithm == "roundrobin" {
+ index := backendRoute.Index % len(backendRoute.Hosts)
+ host := backendRoute.Hosts[index]
+ backendRoute.Index++
+ backendConfig.Routes[i] = backendRoute
+ backendRouteHandled = true
+ rp := httputil.NewSingleHostReverseProxy(&url.URL{
+ Host: host,
+ Scheme: cfg.URL.Scheme,
+ })
+ if backendRoute.Insecure {
+ rp.Transport = &http.Transport{
+ TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+ }
+ } else {
+ rp.Transport = &http.Transport{
+ TLSClientConfig: &tls.Config{},
+ }
+ }
+ rp.ErrorHandler = func(w http.ResponseWriter, r *http.Request, err error) {
+ api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, err)
+ return
+ }
+ routeCtx := context.WithValue(ctx, api.DBContextKey, db)
+ routeCtx = context.WithValue(routeCtx, api.PathParamsKey, map[string]string{})
+ r = r.WithContext(routeCtx)
+ r.Header.Add(middleware.RouteID, strconv.Itoa(backendRoute.ID))
Review Comment:
why do we need to add the routeID as a request header?
##########
traffic_ops/traffic_ops_golang/config/config.go:
##########
@@ -286,6 +305,20 @@ func (c Config) EventLog() log.LogLocation {
const BlockStartup = true
const AllowStartup = false
+func LoadBackendConfig(backendConfigPath string) (BackendConfig, error) {
+ confBytes, err := ioutil.ReadFile(backendConfigPath)
+ if err != nil {
+ return BackendConfig{}, fmt.Errorf("reading backend conf '%s': %v", backendConfigPath, err)
+ }
+
+ cfg := BackendConfig{}
+ err = json.Unmarshal(confBytes, &cfg)
+ if err != nil {
+ return BackendConfig{}, fmt.Errorf("unmarshalling '%s': %v", backendConfigPath, err)
+ }
+ return cfg, nil
Review Comment:
after unmarshalling, we should validate the provided config options (e.g. valid paths, hostnames, ports, algorithm, etc)
##########
CHANGELOG.md:
##########
@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- Replaces all Traffic Portal Tenant select boxes with a novel tree select box [#6427](https://github.com/apache/trafficcontrol/issues/6427).
- Traffic Monitor: Add support for `access.log` to TM.
- Added functionality for login to provide a Bearer token and for that token to be later used for authorization.
+- [Traffic Ops] Added support for backend configurations so that Traffic Ops can be run as a Service Oriented architecture(SOA) product [#6754](https://github.com/apache/trafficcontrol/pull/6754).
Review Comment:
> can be run as a Service Oriented architecture(SOA) product
maybe we should say "can act as a reverse proxy for other backend services" -- it's a bit more specific without prescribing SOA
##########
docs/source/admin/traffic_ops.rst:
##########
@@ -586,6 +590,29 @@ This file sets authentication options for connections to Riak when used as the T
.. impl-detail:: The name of this file is derived from the current database used in the implementation of Traffic Vault - `Riak KV <https://riak.com/products/riak-kv/index.html>`_.
+.. _backends.conf:
+
+backends.conf
+"""""""""""""
+This file deals with the configuration parameters of running Traffic Ops as a reverse proxy for certain endpoints that need to be served externally by other backend services. It is a JSON-format set of options and their respective values. `traffic_ops_golang`_ will use whatever file is specified (if any) by its :option:`--backendcfg` option. The keys of the file are described below.
+
+:routes: This is an array of options to configure Traffic Ops to forward requests of specified types to the appropriate backends.
+
+ :path: The endpoint that will be served by the backend, for example, `api/4.0/foo`.
+ :method: The HTTP method for the above mentioned path, for example, `GET` or `PUT`.
+ :routeId: The integral identifier for the new route being added.
+ :hosts: An array of the hosts and ports where the request (if matched) needs to be forwarded to, for example, `cdn-foo-backend-service-host:9090`.
Review Comment:
Rather than this being a `host:port` string combo, it might be better to make it an object:
```
"hosts": [
"protocol": "https",
"hostname": "cdn-foo-backend-service-host",
"port": 9090
]
```
This way, we could add more attributes to hosts in the future, such as weights, filters, etc.
##########
traffic_ops/traffic_ops_golang/routing/routing.go:
##########
@@ -46,6 +51,26 @@ import (
// RoutePrefix is a prefix that all API routes must match.
const RoutePrefix = "^api" // TODO config?
+// BackendConfig stores the current backend config supplied to traffic ops.
+var BackendConfig config.BackendConfig
+
+// Mutex is a mutex for safely reading/ writing to BackendConfig.
+var Mutex = sync.RWMutex{}
Review Comment:
I think it's a good practice to embed the mutex into a struct that contains the locked data. For example:
```
type backendConfigSynced struct {
cfg config.BackendConfig
*sync.RWMutex // note that this is a pointer to reduce the chances of accidentally copying the lock value (which is invalid)
}
var backendCfg backendConfigSynced{RWMutex: &sync.RWMutex{}} // note that it's lowercase so that it's private to this package
func GetBackendConfig() config.BackendConfig {
backendCfg.RLock()
defer backendCfg.RUnlock()
return backendCfg.cfg
}
```
##########
traffic_ops/traffic_ops_golang/traffic_ops_golang.go:
##########
@@ -214,7 +226,7 @@ func main() {
file.Close()
}
- if err := server.ListenAndServeTLS(cfg.CertPath, cfg.KeyPath); err != nil {
+ if err := http.ListenAndServeTLS(server.Addr, cfg.CertPath, cfg.KeyPath, mux); err != nil {
Review Comment:
So, if we're no longer using the `server` struct that was created above, does this still configure the TLSConfig, timeouts, etc?
##########
traffic_ops/traffic_ops_golang/traffic_ops_golang.go:
##########
@@ -350,6 +381,12 @@ func reloadProfilingInfo(configFileName string) (bool, string, error) {
return cfg.ProfilingEnabled, profilingLocation, nil
}
+func reloadBackendConfig(backendConfigFileName string) (config.BackendConfig, error) {
Review Comment:
this function doesn't seem like it's doing enough to deserve its own function -- should we just add the logging to `setNewBackendConfig` and get rid of this function?
##########
traffic_ops/app/conf/backends.conf:
##########
@@ -0,0 +1,38 @@
+{
Review Comment:
Typically the default config file should be usable (essentially, empty/all defaults). We wouldn't want fresh, default installs to include these routes, but this is a useful example. We should copy this into the documentation as an example, then make `production/backends.conf` basically empty.
##########
traffic_ops/app/conf/backends.conf:
##########
@@ -0,0 +1,38 @@
+{
+ "routes": [
+ {
+ "path": "/api/4.0/foo",
+ "method": "GET",
+ "hosts": [
+ "localhost:8444",
+ "localhost:8445"
+ ],
+ "insecure": true,
+ "permissions": [
+ "CDN:READ",
+ "CDN:WRITE"
+ ],
+ "routeId": 123456,
+ "opts": {
+ "alg": "roundrobin"
+ }
+ },
+ {
+ "path": "/api/4.0/foos",
+ "method": "GET",
+ "hosts": [
+ "localhost:8444",
+ "localhost:8445"
+ ],
+ "insecure": true,
+ "permissions": [
+ "CDN:READ",
+ "CDN:WRITE"
+ ],
+ "routeId": 123457,
+ "opts": {
+ "alg": "roundrobin"
+ }
+ }
+ ]
+}
Review Comment:
missing newline
##########
traffic_ops/traffic_ops_golang/routing/routing.go:
##########
@@ -280,8 +307,88 @@ func Handler(
h.ServeHTTP(w, r)
return
}
+ var backendRouteHandled bool
+ backendConfig := GetBackendConfig()
+ for i, backendRoute := range backendConfig.Routes {
+ if backendRoute.Path == r.URL.Path && backendRoute.Method == r.Method {
+ if backendRoute.Opts.Algorithm == "" || backendRoute.Opts.Algorithm == "roundrobin" {
+ index := backendRoute.Index % len(backendRoute.Hosts)
+ host := backendRoute.Hosts[index]
+ backendRoute.Index++
+ backendConfig.Routes[i] = backendRoute
+ backendRouteHandled = true
+ rp := httputil.NewSingleHostReverseProxy(&url.URL{
+ Host: host,
+ Scheme: cfg.URL.Scheme,
+ })
+ if backendRoute.Insecure {
+ rp.Transport = &http.Transport{
+ TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+ }
+ } else {
+ rp.Transport = &http.Transport{
+ TLSClientConfig: &tls.Config{},
+ }
+ }
+ rp.ErrorHandler = func(w http.ResponseWriter, r *http.Request, err error) {
+ api.HandleErr(w, r, nil, http.StatusInternalServerError, nil, err)
+ return
+ }
+ routeCtx := context.WithValue(ctx, api.DBContextKey, db)
+ routeCtx = context.WithValue(routeCtx, api.PathParamsKey, map[string]string{})
+ r = r.WithContext(routeCtx)
+ r.Header.Add(middleware.RouteID, strconv.Itoa(backendRoute.ID))
+ userErr, sysErr, code := HandleBackendRoute(cfg, backendRoute, w, r)
+ if userErr != nil || sysErr != nil {
+ h2 := middleware.WrapAccessLog(cfg.Secrets[0], middleware.BackendErrorHandler(code, userErr, sysErr))
+ h2.ServeHTTP(w, r)
+ return
+ }
+ backendHandler := middleware.WrapAccessLog(cfg.Secrets[0], rp)
+ backendHandler.ServeHTTP(w, r)
+ return
+ } else {
+ h2 := middleware.WrapAccessLog(cfg.Secrets[0], middleware.BackendErrorHandler(http.StatusBadRequest, errors.New("only an algorithm of roundrobin is supported by the backend options currently"), nil))
+ h2.ServeHTTP(w, r)
+ return
+ }
+ }
+ }
+ if !backendRouteHandled {
+ catchall.ServeHTTP(w, r)
+ }
+}
+
+// HandleBackendRoute does all the pre processing for the backend routes.
+func HandleBackendRoute(cfg *config.Config, route config.BackendRoute, w http.ResponseWriter, r *http.Request) (error, error, int) {
+ var userErr, sysErr error
+ var errCode int
+ var user auth.CurrentUser
+ var inf *api.APIInfo
- catchall.ServeHTTP(w, r)
+ user, userErr, sysErr, errCode = api.GetUserFromReq(w, r, cfg.Secrets[0])
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, errCode
+ }
+ v := api.GetRequestedAPIVersion(r.URL.Path)
Review Comment:
Why do we need to check the API version?
##########
traffic_ops/traffic_ops_golang/routing/routing.go:
##########
@@ -280,8 +307,88 @@ func Handler(
h.ServeHTTP(w, r)
return
}
+ var backendRouteHandled bool
+ backendConfig := GetBackendConfig()
+ for i, backendRoute := range backendConfig.Routes {
+ if backendRoute.Path == r.URL.Path && backendRoute.Method == r.Method {
Review Comment:
I think we'll probably want more than just exact string and method match -- what if the route is `/foo/{id}/bar`?
##########
traffic_ops/traffic_ops_golang/traffic_ops_golang.go:
##########
@@ -234,6 +246,13 @@ func main() {
reloadProfilingConfig := func() {
setNewProfilingInfo(*configFileName, &profiling, &profilingLocation, cfg.Version)
+ backendConfig, err = setNewBackendConfig(backendConfigFileName)
+ if err != nil {
+ log.Errorf("could not reload backend config: %v", err)
+ } else {
+ d.BackendConfig = backendConfig
Review Comment:
There is another thread that would be reading `d.BackendConfig`, right? Does this assignment need to be synchronized? Going through the rest of the PR, I guess I can't tell why `d.BackendConfig` is even needed or if it's even used? Wouldn't readers just call `routing.GetBackendConfig()`?
##########
traffic_ops/traffic_ops_golang/traffic_ops_golang.go:
##########
@@ -234,6 +246,13 @@ func main() {
reloadProfilingConfig := func() {
Review Comment:
this name no longer makes sense since it's reloading more than just the profiling config now
##########
traffic_ops/traffic_ops_golang/traffic_ops_golang.go:
##########
@@ -293,6 +312,18 @@ func setupTrafficVault(riakConfigFileName string, cfg *config.Config) trafficvau
return &disabled.Disabled{}
}
+func setNewBackendConfig(backendConfigFileName *string) (config.BackendConfig, error) {
Review Comment:
this doesn't seem to `set`, so maybe `get` would be a better?
##########
traffic_ops/traffic_ops_golang/routing/routing.go:
##########
@@ -280,8 +307,88 @@ func Handler(
h.ServeHTTP(w, r)
return
}
+ var backendRouteHandled bool
+ backendConfig := GetBackendConfig()
+ for i, backendRoute := range backendConfig.Routes {
+ if backendRoute.Path == r.URL.Path && backendRoute.Method == r.Method {
+ if backendRoute.Opts.Algorithm == "" || backendRoute.Opts.Algorithm == "roundrobin" {
+ index := backendRoute.Index % len(backendRoute.Hosts)
+ host := backendRoute.Hosts[index]
+ backendRoute.Index++
+ backendConfig.Routes[i] = backendRoute
+ backendRouteHandled = true
+ rp := httputil.NewSingleHostReverseProxy(&url.URL{
+ Host: host,
+ Scheme: cfg.URL.Scheme,
+ })
+ if backendRoute.Insecure {
+ rp.Transport = &http.Transport{
+ TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+ }
+ } else {
+ rp.Transport = &http.Transport{
+ TLSClientConfig: &tls.Config{},
+ }
+ }
Review Comment:
This can be simplified to just
```
rp.Transport = &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: insecure}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org