[Reviewing]Remove adminserver #6231

stonezdj · 2018-11-07T08:35:45Z

Remove adminserver

coveralls · 2018-11-21T03:15:46Z

Pull Request Test Coverage Report for Build 9667

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-31.9%) to 33.252%

Totals
Change from base Build 9644:	-31.9%
Covered Lines:	1915
Relevant Lines:	5759

💛 - Coveralls

Signed-off-by: stonezdj <stonezdj@gmail.com>

make/docker-compose.tpl

wy65701436 · 2018-11-26T06:52:39Z

make/docker-compose.tpl

  core:
    image: goharbor/harbor-core:__version__
    container_name: harbor-core
    env_file:
      - ./common/config/core/env
+      - ./common/config/adminserver/env


why not use the /core/env for single?

make/photon/db/Dockerfile

src/common/config/configmetadata.go

make/photon/db/Dockerfile

src/common/config/configlist.go

reasonerjt · 2018-11-26T09:13:24Z

src/common/config/client/db/configdriver.go

@@ -0,0 +1,105 @@
+package db


Why not call the file driver.go as the config is in the package?

reasonerjt · 2018-11-26T09:14:13Z

src/common/config/client/db/configdriver.go

+)
+
+// ConfigureDriver - Retrieve configurations from database
+type ConfigureDriver struct {


Similarly as the config is in the package path it should probably be called Driver

reasonerjt · 2018-11-26T09:16:18Z

src/common/config/config.go

@@ -32,6 +32,15 @@ type Manager struct {
 	key    string
 }

+// ManagerInterface api for configure


Could you add more comments for each func in this interface?

wy65701436 · 2018-11-26T09:16:12Z

src/core/api/configsetting.go

+)
+
+// ConfigSettingAPI ...
+type ConfigSettingAPI struct {


there is no function to set config, why it calls configsetting?

src/core/main.go

reasonerjt · 2018-11-26T09:27:57Z

src/common/config/configclient.go

+import "github.com/goharbor/harbor/src/common/models"
+
+// Client used to retrieve configuration
+type Client interface {


Why is it in the models package?

src/common/config/encrypt/encrypt.go

src/core/router.go

wy65701436 · 2018-11-26T09:55:32Z

src/core/config/config.go

@@ -401,12 +388,14 @@ func Database() (*models.Database, error) {
 		return nil, err
 	}

+	log.Infof("database connections = %+v", cfg)


the password gets compromised?

wy65701436 · 2018-11-26T10:42:09Z

src/core/api/configsetting.go

+		c.CustomAbort(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError))
+	}
+	configList, err := c.ConfigClient.GetSettingByGroup(group)
+	log.Errorf("Found items %v", len(configList))


why log error here?

wy65701436 · 2018-11-26T10:46:24Z

src/core/api/configsetting.go

+	configList, err := c.ConfigClient.GetSettingByGroup(group)
+	log.Errorf("Found items %v", len(configList))
+	if err != nil {
+		log.Error("failed to get setting by group %v", err)


return 501？

wy65701436 · 2018-11-26T10:48:50Z

src/common/config/encrypt/encrypt.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package encrypt


move it to src/common/util, it could be used by other components, not just config.

wy65701436 · 2018-11-26T10:50:29Z

src/common/config/configvalue.go

+func NewConfigureValue(key, value string) *ConfigureValue {
+	result := &ConfigureValue{}
+	if metaData, err := MetaData.GetConfigMetaData(key); err != nil {
+		if metaData.Type == PasswordType {


what's the difference on if/else?

wy65701436 · 2018-11-26T10:51:31Z

src/common/config/configvalue.go

+		}
+	}
+	log.Errorf("The current value can not convert to integer, %+v", c)
+	return 0


wy65701436 · 2018-11-26T10:51:41Z

src/common/config/configvalue.go

+		}
+	}
+	log.Errorf("The current value can not convert to integer, %+v", c)
+	return 0


wy65701436 · 2018-11-26T10:55:16Z

src/adminserver/client/client.go

 	return utils.TestTCPConn(addr, 60, 2)
 }

 // GetCfgs ...
 func (c *client) GetCfgs() (map[string]interface{}, error) {
 	url := c.baseURL + "/api/configs"
+	log.Infof("Create config from: url:%v", url)


log.Infof -> log.Debug

wy65701436 · 2018-11-26T10:57:41Z

src/common/config/client/db/configdriver.go

+
+// Load ...
+// FIXME: refactor
+func (cd *ConfigureDriver) Load() error {


agree with refactoring this function as hard to understand.

wy65701436 · 2018-11-26T11:25:56Z

src/common/config/client/remote/configdriver.go

@@ -0,0 +1,80 @@
+package remote
+


should we just leverage the core client?

wy65701436 · 2018-11-26T11:30:05Z

src/common/config/client/db/configmanager.go

+
+// NewCoreConfigManagerFromArray ...
+func NewCoreConfigManagerFromArray(items []config.Item) *CoreConfigManager {
+	return &CoreConfigManager{Driver: NewDBConfigureStoreFromArray(items)}


the DB driver is hardcoded here, how do we extend the driver? If the contributor wants to contribute a redis driver, the current structure seems not support well.

reasonerjt · 2018-11-27T02:35:16Z

src/common/config/configmetadata.go

+// ValidateFunc - function to validate configure items
+type ValidateFunc func(key, value string) error
+
+// ConfigureMetaData ...


Let's add more comment to this struct.

reasonerjt · 2018-11-27T02:35:25Z

src/common/config/configmetadata.go

+
+// ConfigureMetaData ...
+type ConfigureMetaData struct {
+	sync.RWMutex


remove the lock?

reasonerjt · 2018-11-27T02:36:53Z

src/common/config/configmetadata.go

+}
+
+// GetConfigMetaData - Get single configuration item
+func (cm *ConfigureMetaData) GetConfigMetaData(key string) (Item, error) {


Rename to Get

reasonerjt · 2018-11-27T02:38:01Z

src/common/config/configmetadata.go

+}
+
+// GetAllConfigureItems - Get All Configuration Items
+func (cm *ConfigureMetaData) GetAllConfigureItems() (items []Item) {


Rename to All

reasonerjt · 2018-11-27T02:39:15Z

src/common/config/configmetadata.go

+	ClairGroup     = "clair"
+
+	// Type
+	IntType      = "int"


how about create a type for the type of config item and set ValidateFunc as a func of this type?

reasonerjt · 2018-11-27T02:42:18Z

src/common/config/configstore.go

+var systemConfigEditable = false
+
+func init() {
+	if os.Getenv("UTTEST") == "true" {


Is it needed if we have SetSystemConfigEditable () in line 35?

reasonerjt · 2018-11-27T02:46:20Z

src/common/config/configvalue.go

+	GetStringToStringMap() map[string]string
+	// GetMap - return the map of current value
+	GetMap() map[string]interface{}
+	// Validator to validate configure items, if passed, return true, else return false and return error


Comment should be // Validate...

reasonerjt · 2018-11-27T02:46:43Z

src/common/config/configvalue.go

+	// Set this configure item to configure store
+	Set(key, value string) error
+
+	GetKey() string


Please comment the following functions.

reasonerjt · 2018-11-27T02:53:33Z

src/common/config/configvalue.go

+}
+
+// GetStringToStringMap - return the string to string map of current value
+func (c *ConfigureValue) GetStringToStringMap() map[string]string {


Call GetMap and tranform the result into map[string]string?

wy65701436 · 2018-11-27T02:53:55Z

src/common/config/configstore.go

+	MetaData.InitMetaData()
+	// Init Default Value
+	itemArray := MetaData.GetAllConfigureItems()
+	for _, item := range itemArray {


these two loops could be merged

reasonerjt · 2018-11-27T02:55:47Z

src/common/config/configvalue.go

+			} else if strings.EqualFold(c.Value, "off") || strings.EqualFold(c.Value, "false") {
+				c.Value = "false"
+			}
+			boolValue, err := strconv.ParseBool(c.Value)


else {
return ErrTypeNotMatch
}

wy65701436 · 2018-11-27T02:56:12Z

src/common/config/configlist.go

+	// 1. Add configure item in configlist.go
+	// 2. Get settings by config.Client
+	ConfigList = []Item{
+		{Scope: SystemScope, Group: BasicGroup, EnvironmentKey: "HARBOR_ADMIN_PASSWORD", DefaultValue: "", Name: "admin_initial_password", Type: PasswordType, Editable: true, Reloadable: false},


group them by scope

wy65701436 · 2018-11-27T02:57:34Z

src/common/config/configlist.go

+		{Scope: SystemScope, Group: BasicGroup, EnvironmentKey: "CFG_EXPIRATION", DefaultValue: "5", Name: "cfg_expiration", Type: StringType, Editable: false, Reloadable: true},
+		{Scope: SystemScope, Group: BasicGroup, EnvironmentKey: "CHART_REPOSITORY_URL", DefaultValue: "http://chartmuseum:9999", Name: "chart_repository_url", Type: StringType, Editable: false, Reloadable: true},
+
+		{Scope: SystemScope, Group: ClairGroup, EnvironmentKey: "CLAIR_DB", DefaultValue: "postgres", Name: "clair_db", Type: StringType, Editable: false, Reloadable: true},


editable should be false for systemscope item?

reasonerjt · 2018-11-27T03:00:18Z

src/common/config/configlist.go

+	Name string `json:"name,omitempty"`
+	// It can be integer, string, bool, password, map
+	Type string `json:"type,omitempty"`
+	// The validation function for this field.


If it's a func it should be a verb, so better rename to Validate

reasonerjt · 2018-11-27T03:02:02Z

src/common/config/configvalue.go

+}
+
+// GetInt - return the int value of current value
+func (c *ConfigureValue) GetInt() int {


Call GetInt64?

wy65701436 · 2018-11-27T03:02:35Z

src/common/config/configlist.go

+		{Scope: SystemScope, Group: ClairGroup, EnvironmentKey: "CLAIR_DB", DefaultValue: "postgres", Name: "clair_db", Type: StringType, Editable: false, Reloadable: true},
+		{Scope: SystemScope, Group: ClairGroup, EnvironmentKey: "CLAIR_DB_HOST", DefaultValue: "postgresql", Name: "clair_db_host", Type: StringType, Editable: false, Reloadable: true},
+		{Scope: SystemScope, Group: ClairGroup, EnvironmentKey: "CLAIR_DB_PASSWORD", DefaultValue: "root123", Name: "clair_db_password", Type: PasswordType, Editable: false, Reloadable: true},
+		{Scope: SystemScope, Group: ClairGroup, EnvironmentKey: "CLAIR_DB_PORT", DefaultValue: "5432", Name: "clair_db_port", Type: IntType, Editable: false, Reloadable: true},


put name first as you use the name as the key in the metadata map.

reasonerjt · 2018-11-27T03:04:11Z

src/common/config/configvalue.go

+}
+
+// SetPassword ...
+func (c *ConfigureValue) SetPassword(key, value string) error {


Is there a reason that we don't put encrypt and decrypt at the same level?

wy65701436 · 2018-11-27T03:32:53Z

src/common/config/configstore.go

+	s.configureValues.Range(func(key, value interface{}) bool {
+		if keyString, suc := key.(string); suc {
+			itemMataData, err := MetaData.GetConfigMetaData(keyString)
+			if err == nil {


what about err !=nil?

stonezdj force-pushed the remove_adminserver branch 3 times, most recently from ae02bcd to e716865 Compare November 20, 2018 09:15

stonezdj force-pushed the remove_adminserver branch 3 times, most recently from ac273f0 to 2f67c6d Compare November 22, 2018 03:48

wy65701436 mentioned this pull request Nov 22, 2018

Fix panic if ExtEndpoint is empty #6325

Closed

stonezdj force-pushed the remove_adminserver branch 6 times, most recently from bf79da1 to 094a111 Compare November 26, 2018 02:35

Remove admin server container

cfecbec

Signed-off-by: stonezdj <stonezdj@gmail.com>

stonezdj force-pushed the remove_adminserver branch from ccf7d58 to cfecbec Compare November 26, 2018 02:49

stonezdj requested review from wy65701436 and reasonerjt November 26, 2018 02:49

stonezdj assigned wy65701436 Nov 26, 2018

fix ldap default value issue

afe4cfa

wy65701436 reviewed Nov 26, 2018

View reviewed changes

reasonerjt reviewed Nov 26, 2018

View reviewed changes

make/photon/db/Dockerfile Show resolved Hide resolved

wy65701436 reviewed Nov 26, 2018

View reviewed changes

src/common/config/configlist.go Show resolved Hide resolved

reasonerjt reviewed Nov 26, 2018

View reviewed changes

with_chartmuseum should be system scope

451be9e

wy65701436 reviewed Nov 26, 2018

View reviewed changes

reasonerjt reviewed Nov 26, 2018

View reviewed changes

wy65701436 reviewed Nov 26, 2018

View reviewed changes

src/common/config/encrypt/encrypt.go Outdated Show resolved Hide resolved

wy65701436 reviewed Nov 26, 2018

View reviewed changes

src/core/router.go Show resolved Hide resolved

wy65701436 reviewed Nov 26, 2018

View reviewed changes

stonezdj added 2 commits November 26, 2018 18:17

Change order of init and dbdriver load

3416a98

Change order of init and dbdriver load

a91ab5a

wy65701436 reviewed Nov 26, 2018

View reviewed changes

stonezdj added 2 commits November 26, 2018 18:46

Change location of default keypath

f3075d6

Remove /etc/adminserver/key in compose file

fbcb182

wy65701436 reviewed Nov 26, 2018

View reviewed changes

reasonerjt reviewed Nov 27, 2018

View reviewed changes

src/common/config/configmetadata.go

// ConfigureMetaData ...

type ConfigureMetaData struct {

sync.RWMutex

Copy link

Contributor

reasonerjt Nov 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the lock?

reasonerjt reviewed Nov 27, 2018

View reviewed changes

stonezdj changed the title ~~[WIP]Remove adminserver~~ [Reviewing]Remove adminserver Nov 27, 2018

reasonerjt reviewed Nov 27, 2018

View reviewed changes

wy65701436 reviewed Nov 27, 2018

View reviewed changes

reasonerjt reviewed Nov 27, 2018

View reviewed changes

wy65701436 reviewed Nov 27, 2018

View reviewed changes

reasonerjt reviewed Nov 27, 2018

View reviewed changes

wy65701436 reviewed Nov 27, 2018

View reviewed changes

reasonerjt reviewed Nov 27, 2018

View reviewed changes

wy65701436 reviewed Nov 27, 2018

View reviewed changes

stonezdj closed this Nov 29, 2018

[Reviewing]Remove adminserver #6231

[Reviewing]Remove adminserver #6231

Conversation

stonezdj commented Nov 7, 2018

coveralls commented Nov 21, 2018 • edited

Pull Request Test Coverage Report for Build 9667

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reasonerjt Nov 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 21, 2018 •

edited

reasonerjt Nov 26, 2018 •

edited