From 6ea9291c090ead7d51510608e1616ff238cbc329 Mon Sep 17 00:00:00 2001 From: Steven Zou Date: Wed, 11 Jul 2018 17:31:34 +0800 Subject: [PATCH] Refactor code after PR reviewing pass cache config to the constructor of chart cache use break loop to instead of return in GetIndexYaml fix typo reids to redis pass credential to the reverse proxy and chart client via constructors --- src/chartserver/base_test.go | 2 +- src/chartserver/cache.go | 153 ++++++++++++------------------- src/chartserver/cache_test.go | 29 +++--- src/chartserver/client.go | 17 +++- src/chartserver/controller.go | 72 ++++++++++++++- src/chartserver/repo_handler.go | 18 +++- src/chartserver/reverse_proxy.go | 21 ++--- src/chartserver/utils.go | 53 +++++++++++ 8 files changed, 232 insertions(+), 133 deletions(-) diff --git a/src/chartserver/base_test.go b/src/chartserver/base_test.go index 571bb3b29..df57c7a41 100644 --- a/src/chartserver/base_test.go +++ b/src/chartserver/base_test.go @@ -140,7 +140,7 @@ var frontServer = httptest.NewUnstartedServer(http.HandlerFunc(func(w http.Respo })) //Http client -var httpClient = NewChartClient() +var httpClient = NewChartClient(nil) //Mock project manager type mockProjectManager struct{} diff --git a/src/chartserver/cache.go b/src/chartserver/cache.go index e176da2f2..470e815ba 100644 --- a/src/chartserver/cache.go +++ b/src/chartserver/cache.go @@ -3,10 +3,6 @@ package chartserver import ( "encoding/json" "errors" - "fmt" - "net/url" - "os" - "strings" "time" beego_cache "github.com/astaxie/beego/cache" @@ -28,43 +24,74 @@ const ( //ChartCache is designed to cache some processed data for repeated accessing //to improve the performance type ChartCache struct { - //cache driver + //Cache driver cache beego_cache.Cache - //Flag to indicate if cache driver is configured - isEnabled bool - - //Backend driver type + //Keep the driver type driverType string + + //To indicate if the chart cache is enabled + isEnabled bool +} + +//ChartCacheConfig keeps the configurations of ChartCache +type ChartCacheConfig struct { + //Only support 'in-memory' and 'redis' now + DriverType string + + //Align with config + Config string } //NewChartCache is constructor of ChartCache -func NewChartCache() *ChartCache { - driverType, isSet := parseCacheDriver() - +//If return nil, that means no cache is enabled for chart repository server +func NewChartCache(config *ChartCacheConfig) *ChartCache { + //Never return nil object chartCache := &ChartCache{ - isEnabled: isSet, + isEnabled: false, } - if !chartCache.isEnabled { - hlog.Info("No cache driver is configured, chart cache will be disabled") + + //Double check the configurations are what we want + if config == nil { return chartCache } - cache, enabledDriverType := initCacheDriver(driverType) + if config.DriverType != cacheDriverMem && config.DriverType != cacheDriverRedis { + return chartCache + } + + if config.DriverType == cacheDriverRedis { + if len(config.Config) == 0 { + return chartCache + } + } + + //Try to create the upstream cache + cache := initCacheDriver(config) + if cache == nil { + return chartCache + } + + //Cache enabled + chartCache.isEnabled = true + chartCache.driverType = config.DriverType chartCache.cache = cache - chartCache.driverType = enabledDriverType return chartCache } -//IsEnabled to indicate if the chart cache is configured +//IsEnabled to indicate if the chart cache is successfully enabled +//The cache may be disabled if +// user does not set +// wrong configurations func (chc *ChartCache) IsEnabled() bool { return chc.isEnabled } //PutChart caches the detailed data of chart version func (chc *ChartCache) PutChart(chart *ChartVersionDetails) { - if !chc.isEnabled { + //If cache is not enabled, do nothing + if !chc.IsEnabled() { return } @@ -85,7 +112,7 @@ func (chc *ChartCache) PutChart(chart *ChartVersionDetails) { } default: //Should not reach here, but still put guard code here - err = chc.cache.Put(chart.Metadata.Digest, chart, standardExpireTime) + err = errors.New("Meet invalid cache driver") } if err != nil { @@ -100,7 +127,8 @@ func (chc *ChartCache) PutChart(chart *ChartVersionDetails) { //If hit, return the cached item; //otherwise, nil object is returned func (chc *ChartCache) GetChart(chartDigest string) *ChartVersionDetails { - if !chc.isEnabled { + //If cache is not enabled, do nothing + if !chc.IsEnabled() { return nil } @@ -127,90 +155,27 @@ func (chc *ChartCache) GetChart(chartDigest string) *ChartVersionDetails { return nil } -//What's the cache driver if it is set -func parseCacheDriver() (string, bool) { - driver, ok := os.LookupEnv(cacheDriverENVKey) - return strings.ToLower(driver), ok -} - //Initialize the cache driver based on the config -func initCacheDriver(driverType string) (beego_cache.Cache, string) { - switch driverType { +func initCacheDriver(cacheConfig *ChartCacheConfig) beego_cache.Cache { + switch cacheConfig.DriverType { case cacheDriverMem: hlog.Info("Enable memory cache for chart caching") - return beego_cache.NewMemoryCache(), cacheDriverMem + return beego_cache.NewMemoryCache() case cacheDriverRedis: - redisConfig, err := parseRedisConfig() - if err != nil { - //Just logged - hlog.Errorf("Failed to read redis configurations with error: %s", err) - break - } - - redisCache, err := beego_cache.NewCache(cacheDriverRedis, redisConfig) + redisCache, err := beego_cache.NewCache(cacheDriverRedis, cacheConfig.Config) if err != nil { //Just logged hlog.Errorf("Failed to initialize redis cache: %s", err) - break + return nil } - hlog.Info("Enable reids cache for chart caching") - return redisCache, cacheDriverRedis + hlog.Info("Enable redis cache for chart caching") + return redisCache default: break } - hlog.Infof("Failed to config cache with driver '%s', enable memory cache by default for chart cache instead", driverType) - //Any other cases, use memory cache - return beego_cache.NewMemoryCache(), cacheDriverMem -} - -//Parse the redis configuration to the beego cache pattern -//Config pattern is "address:port[,weight,password,db_index]" -func parseRedisConfig() (string, error) { - redisConfigV := os.Getenv(redisENVKey) - if len(redisConfigV) == 0 { - return "", errors.New("empty redis config") - } - - redisConfig := make(map[string]string) - redisConfig["key"] = cacheCollectionName - - //The full pattern - if strings.Index(redisConfigV, ",") != -1 { - //Read only the previous 4 segments - configSegments := strings.SplitN(redisConfigV, ",", 4) - if len(configSegments) != 4 { - return "", errors.New("invalid redis config, it should be address:port[,weight,password,db_index]") - } - - redisConfig["conn"] = configSegments[0] - redisConfig["password"] = configSegments[2] - redisConfig["dbNum"] = configSegments[3] - } else { - //The short pattern - redisConfig["conn"] = redisConfigV - redisConfig["dbNum"] = "0" - redisConfig["password"] = "" - } - - //Try to validate the connection address - fullAddr := redisConfig["conn"] - if strings.Index(fullAddr, "://") == -1 { - //Append schema - fullAddr = fmt.Sprintf("redis://%s", fullAddr) - } - //Validate it by url - _, err := url.Parse(fullAddr) - if err != nil { - return "", err - } - - //Convert config map to string - cfgData, err := json.Marshal(redisConfig) - if err != nil { - return "", err - } - - return string(cfgData), nil + //Any other cases + hlog.Info("No cache is enabled for chart caching") + return nil } diff --git a/src/chartserver/cache_test.go b/src/chartserver/cache_test.go index 46927c71a..b3b9c4a4d 100644 --- a/src/chartserver/cache_test.go +++ b/src/chartserver/cache_test.go @@ -1,7 +1,7 @@ package chartserver import ( - "os" + "encoding/json" "testing" "k8s.io/helm/pkg/chartutil" @@ -24,7 +24,7 @@ var ( //Test the no cache set scenario func TestNoCache(t *testing.T) { - chartCache := NewChartCache() + chartCache := NewChartCache(nil) if chartCache == nil { t.Fatalf("cache instance should not be nil") } @@ -36,9 +36,9 @@ func TestNoCache(t *testing.T) { //Test the in memory cache func TestInMemoryCache(t *testing.T) { - os.Setenv(cacheDriverENVKey, cacheDriverMem) - - chartCache := NewChartCache() + chartCache := NewChartCache(&ChartCacheConfig{ + DriverType: cacheDriverMem, + }) if chartCache == nil { t.Fatalf("cache instance should not be nil") } @@ -56,17 +56,23 @@ func TestInMemoryCache(t *testing.T) { if theCachedChart == nil || theCachedChart.Metadata.Name != mockChart.Metadata.Name { t.Fatal("In memory cache does work") } - - os.Unsetenv(cacheDriverENVKey) } //Test redis cache //Failed to config redis cache and then use in memory instead func TestRedisCache(t *testing.T) { - os.Setenv(cacheDriverENVKey, cacheDriverRedis) - os.Setenv(redisENVKey, ":6379") + redisConfigV := make(map[string]string) + redisConfigV["key"] = cacheCollectionName + redisConfigV["conn"] = ":6379" + redisConfigV["dbNum"] = "0" + redisConfigV["password"] = "" - chartCache := NewChartCache() + redisConfig, _ := json.Marshal(redisConfigV) + + chartCache := NewChartCache(&ChartCacheConfig{ + DriverType: cacheDriverRedis, + Config: string(redisConfig), + }) if chartCache == nil { t.Fatalf("cache instance should not be nil") } @@ -84,7 +90,4 @@ func TestRedisCache(t *testing.T) { if theCachedChart == nil || theCachedChart.Metadata.Name != mockChart.Metadata.Name { t.Fatal("In memory cache does work") } - - os.Unsetenv(redisENVKey) - os.Unsetenv(cacheDriverENVKey) } diff --git a/src/chartserver/client.go b/src/chartserver/client.go index ec886fa98..0deda03e9 100644 --- a/src/chartserver/client.go +++ b/src/chartserver/client.go @@ -5,7 +5,6 @@ import ( "fmt" "io/ioutil" "net/http" - "os" "strings" "time" ) @@ -18,11 +17,16 @@ const ( //ChartClient is a http client to get the content from the external http server type ChartClient struct { + //HTTP client httpClient *http.Client + + //Auth info + credentail *Credential } //NewChartClient is constructor of ChartClient -func NewChartClient() *ChartClient { //Create http client with customized timeouts +//credentail can be nil +func NewChartClient(credentail *Credential) *ChartClient { //Create http client with customized timeouts client := &http.Client{ Timeout: clientTimeout, Transport: &http.Transport{ @@ -31,7 +35,10 @@ func NewChartClient() *ChartClient { //Create http client with customized timeou }, } - return &ChartClient{client} + return &ChartClient{ + httpClient: client, + credentail: credentail, + } } //GetContent get the bytes from the specified url @@ -46,7 +53,9 @@ func (cc *ChartClient) GetContent(url string) ([]byte, error) { } //Set basic auth - request.SetBasicAuth(userName, os.Getenv(passwordKey)) + if cc.credentail != nil { + request.SetBasicAuth(cc.credentail.Username, cc.credentail.Password) + } response, err := cc.httpClient.Do(request) if err != nil { diff --git a/src/chartserver/controller.go b/src/chartserver/controller.go index 561dda334..9f2824a16 100644 --- a/src/chartserver/controller.go +++ b/src/chartserver/controller.go @@ -2,9 +2,25 @@ package chartserver import ( "errors" + "fmt" "net/url" + "os" + "strings" + + hlog "github.com/vmware/harbor/src/common/utils/log" ) +const ( + userName = "chart_controller" + passwordKey = "UI_SECRET" +) + +//Credential keeps the username and password for the basic auth +type Credential struct { + Username string + Password string +} + //Controller is used to handle flows of related requests based on the corresponding handlers //A reverse proxy will be created and managed to proxy the related traffics between API and //backend chart server @@ -28,17 +44,32 @@ func NewController(backendServer *url.URL) (*Controller, error) { return nil, errors.New("failed to create chartserver.Controller: backend sever address is required") } + //Try to create credential + cred := &Credential{ + Username: userName, + Password: os.Getenv(passwordKey), + } + //Use customized reverse proxy - proxy := NewProxyEngine(backendServer) + proxy := NewProxyEngine(backendServer, cred) //Create http client with customized timeouts - client := NewChartClient() + client := NewChartClient(cred) //Initialize chart operator for use operator := &ChartOperator{} //Creat cache - cache := NewChartCache() + cacheCfg, err := getCacheConfig() + if err != nil { + //just log the error + //will not break the whole flow if failed to create cache + hlog.Errorf("failed to get cache configuration with error: %s", err) + } + cache := NewChartCache(cacheCfg) + if !cache.IsEnabled() { + hlog.Info("No cache is enabled for chart caching") + } return &Controller{ backendServerAddr: backendServer, @@ -72,3 +103,38 @@ func (c *Controller) GetRepositoryHandler() *RepositoryHandler { func (c *Controller) GetManipulationHandler() *ManipulationHandler { return c.manipulationHandler } + +//What's the cache driver if it is set +func parseCacheDriver() (string, bool) { + driver, ok := os.LookupEnv(cacheDriverENVKey) + return strings.ToLower(driver), ok +} + +//Get and parse the configuration for the chart cache +func getCacheConfig() (*ChartCacheConfig, error) { + driver, isSet := parseCacheDriver() + if !isSet { + return nil, nil + } + + if driver != cacheDriverMem && driver != cacheDriverRedis { + return nil, fmt.Errorf("cache driver '%s' is not supported, only support 'memory' and 'redis'", driver) + } + + if driver == cacheDriverMem { + return &ChartCacheConfig{ + DriverType: driver, + }, nil + } + + redisConfigV := os.Getenv(redisENVKey) + redisCfg, err := parseRedisConfig(redisConfigV) + if err != nil { + return nil, fmt.Errorf("failed to parse redis configurations from '%s' with error: %s", redisCfg, err) + } + + return &ChartCacheConfig{ + DriverType: driver, + Config: redisCfg, + }, nil +} diff --git a/src/chartserver/repo_handler.go b/src/chartserver/repo_handler.go index 39a572820..401cdaad1 100644 --- a/src/chartserver/repo_handler.go +++ b/src/chartserver/repo_handler.go @@ -155,12 +155,13 @@ LOOP: indexFileOfRepo: indexFile, } }(namespace) - case err := <-errorChan: - writeError(w, http.StatusInternalServerError, err) - return + case err = <-errorChan: + //Quit earlier + break LOOP case <-req.Context().Done(): - writeInternalError(w, errors.New("request aborted")) - return + //Quit earlier + err = errors.New("request of getting index yaml file is aborted") + break LOOP } } @@ -173,6 +174,13 @@ LOOP: //Wait until merging thread quit <-mergeDone + //All the threads are done + //Met an error + if err != nil { + writeInternalError(w, err) + return + } + //Remove duplicated keys in public key list hash := make(map[string]string) for _, key := range mergedIndexFile.PublicKeys { diff --git a/src/chartserver/reverse_proxy.go b/src/chartserver/reverse_proxy.go index be4bcba0e..4ee0bc290 100644 --- a/src/chartserver/reverse_proxy.go +++ b/src/chartserver/reverse_proxy.go @@ -14,10 +14,7 @@ import ( ) const ( - userName = "chart_controller" - passwordKey = "UI_SECRET" agentHarbor = "HARBOR" - authHeader = "Authorization" contentLengthHeader = "Content-Length" ) @@ -32,13 +29,13 @@ type ProxyEngine struct { } //NewProxyEngine is constructor of NewProxyEngine -func NewProxyEngine(target *url.URL) *ProxyEngine { +func NewProxyEngine(target *url.URL, cred *Credential) *ProxyEngine { return &ProxyEngine{ backend: target, engine: &httputil.ReverseProxy{ ErrorLog: log.New(os.Stdout, "", log.Ldate|log.Ltime|log.Lshortfile), Director: func(req *http.Request) { - director(target, req) + director(target, cred, req) }, ModifyResponse: modifyResponse, }, @@ -51,7 +48,8 @@ func (pe *ProxyEngine) ServeHTTP(w http.ResponseWriter, req *http.Request) { } //Overwrite the http requests -func director(target *url.URL, req *http.Request) { +func director(target *url.URL, cred *Credential, req *http.Request) { + //Closure targetQuery := target.RawQuery //Overwrite the request URL to the target path @@ -67,13 +65,10 @@ func director(target *url.URL, req *http.Request) { req.Header.Set("User-Agent", agentHarbor) } - //Get the password from the env - //Ignore the empty checking, the backend server should return the right status code - //with invalid credential - password := os.Getenv(passwordKey) - - //Add authentication header - req.SetBasicAuth(userName, password) + //Add authentication header if it is existing + if cred != nil { + req.SetBasicAuth(cred.Username, cred.Password) + } } //Modify the http response diff --git a/src/chartserver/utils.go b/src/chartserver/utils.go index fb69376d6..08a1e7e51 100644 --- a/src/chartserver/utils.go +++ b/src/chartserver/utils.go @@ -2,7 +2,11 @@ package chartserver import ( "encoding/json" + "errors" + "fmt" "net/http" + "net/url" + "strings" ) const ( @@ -31,3 +35,52 @@ func writeJSONData(w http.ResponseWriter, data []byte) { w.WriteHeader(http.StatusOK) w.Write(data) } + +//Parse the redis configuration to the beego cache pattern +//Config pattern is "address:port[,weight,password,db_index]" +func parseRedisConfig(redisConfigV string) (string, error) { + if len(redisConfigV) == 0 { + return "", errors.New("empty redis config") + } + + redisConfig := make(map[string]string) + redisConfig["key"] = cacheCollectionName + + //The full pattern + if strings.Index(redisConfigV, ",") != -1 { + //Read only the previous 4 segments + configSegments := strings.SplitN(redisConfigV, ",", 4) + if len(configSegments) != 4 { + return "", errors.New("invalid redis config, it should be address:port[,weight,password,db_index]") + } + + redisConfig["conn"] = configSegments[0] + redisConfig["password"] = configSegments[2] + redisConfig["dbNum"] = configSegments[3] + } else { + //The short pattern + redisConfig["conn"] = redisConfigV + redisConfig["dbNum"] = "0" + redisConfig["password"] = "" + } + + //Try to validate the connection address + fullAddr := redisConfig["conn"] + if strings.Index(fullAddr, "://") == -1 { + //Append schema + fullAddr = fmt.Sprintf("redis://%s", fullAddr) + } + //Validate it by url + _, err := url.Parse(fullAddr) + if err != nil { + return "", err + } + + //Convert config map to string + cfgData, err := json.Marshal(redisConfig) + if err != nil { + return "", err + } + + return string(cfgData), nil +}