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

@@ -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{}

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
 }

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)
 }

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 {

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
+}

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 {

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

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
+}