Fix a few bugs of replication (#7619)

1. handle the public/private property when creating the projects
2. extend the length of access_secret
3. update the task status by using orm functions

Signed-off-by: Wenkai Yin <yinw@vmware.com>

make/migrations/postgresql/0004_1.8.0_schema.up.sql

@@ -59,7 +59,7 @@ ALTER TABLE registry ALTER COLUMN url TYPE varchar(256);
 ALTER TABLE registry ADD COLUMN credential_type varchar(16);
 ALTER TABLE registry RENAME COLUMN username TO access_key;
 ALTER TABLE registry RENAME COLUMN password TO access_secret;
-ALTER TABLE registry ALTER COLUMN access_secret TYPE varchar(1024);
+ALTER TABLE registry ALTER COLUMN access_secret TYPE varchar(4096);
 ALTER TABLE registry ADD COLUMN type varchar(32);
 ALTER TABLE registry DROP COLUMN target_type;
 ALTER TABLE registry ADD COLUMN description text;

src/core/api/replication_policy.go

@@ -95,6 +95,7 @@ func (r *ReplicationPolicyAPI) Create() {
 		return
 	}
 
+	policy.Creator = r.SecurityCtx.GetUsername()
 	id, err := replication.PolicyCtl.Create(policy)
 	if err != nil {
 		r.SendInternalServerError(fmt.Errorf("failed to create the policy: %v", err))

src/replication/adapter/harbor/adapter.go

@@ -18,6 +18,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"strconv"
 	"strings"
 
 	common_http "github.com/goharbor/harbor/src/common/http"
@@ -135,11 +136,18 @@ func (a *adapter) PrepareForPush(resources []*model.Resource) error {
 		if len(resource.Metadata.Repository.Name) == 0 {
 			return errors.New("the name of the repository cannot be null")
 		}
+
 		paths := strings.Split(resource.Metadata.Repository.Name, "/")
 		projectName := paths[0]
-		// TODO handle the public
+		// handle the public properties
+		metadata := resource.Metadata.Repository.Metadata
+		pro, exist := projects[projectName]
+		if exist {
+			metadata = mergeMetadata(pro.Metadata, metadata)
+		}
 		projects[projectName] = &project{
-			Name: projectName,
+			Name:     projectName,
+			Metadata: metadata,
 		}
 	}
 	for _, project := range projects {
@@ -161,28 +169,38 @@ func (a *adapter) PrepareForPush(resources []*model.Resource) error {
 		log.Debugf("project %s created", project.Name)
 	}
 	return nil
+}
 
-	// TODO
-	/*
-		// handle the public of the project
-		if meta, exist := namespace.Metadata["public"]; exist {
-			public := true
-			// if one of them is "private", the set the public as false
-			for _, value := range meta.(map[string]interface{}) {
-				b, err := strconv.ParseBool(value.(string))
-				if err != nil {
-					return err
-				}
-				if !b {
-					public = false
-					break
-				}
-			}
-			project.Metadata = map[string]interface{}{
-				"public": public,
-			}
+// currently, mergeMetadata only handles the public metadata
+func mergeMetadata(metadata1, metadata2 map[string]interface{}) map[string]interface{} {
+	public := parsePublic(metadata1) && parsePublic(metadata2)
+	return map[string]interface{}{
+		"public": strconv.FormatBool(public),
+	}
+}
+
+func parsePublic(metadata map[string]interface{}) bool {
+	if metadata == nil {
+		return false
+	}
+	pub, exist := metadata["public"]
+	if !exist {
+		return false
+	}
+	public, ok := pub.(bool)
+	if ok {
+		return public
+	}
+	pubstr, ok := pub.(string)
+	if ok {
+		public, err := strconv.ParseBool(pubstr)
+		if err != nil {
+			log.Errorf("failed to parse %s to bool: %v", pubstr, err)
+			return false
 		}
-	*/
+		return public
+	}
+	return false
 }
 
 type project struct {

src/replication/adapter/harbor/adapter_test.go

@@ -16,6 +16,7 @@ package harbor
 
 import (
 	"net/http"
+	"strconv"
 	"testing"
 
 	"github.com/goharbor/harbor/src/common/utils/test"
@@ -152,3 +153,60 @@ func TestPrepareForPush(t *testing.T) {
 		})
 	require.Nil(t, err)
 }
+
+func TestParsePublic(t *testing.T) {
+	cases := []struct {
+		metadata map[string]interface{}
+		result   bool
+	}{
+		{nil, false},
+		{map[string]interface{}{}, false},
+		{map[string]interface{}{"public": true}, true},
+		{map[string]interface{}{"public": "not_bool"}, false},
+		{map[string]interface{}{"public": "true"}, true},
+		{map[string]interface{}{"public": struct{}{}}, false},
+	}
+	for _, c := range cases {
+		assert.Equal(t, c.result, parsePublic(c.metadata))
+	}
+}
+
+func TestMergeMetadata(t *testing.T) {
+	cases := []struct {
+		m1     map[string]interface{}
+		m2     map[string]interface{}
+		public bool
+	}{
+		{
+			m1: map[string]interface{}{
+				"public": "true",
+			},
+			m2: map[string]interface{}{
+				"public": "true",
+			},
+			public: true,
+		},
+		{
+			m1: map[string]interface{}{
+				"public": "false",
+			},
+			m2: map[string]interface{}{
+				"public": "true",
+			},
+			public: false,
+		},
+		{
+			m1: map[string]interface{}{
+				"public": "false",
+			},
+			m2: map[string]interface{}{
+				"public": "false",
+			},
+			public: false,
+		},
+	}
+	for _, c := range cases {
+		m := mergeMetadata(c.m1, c.m2)
+		assert.Equal(t, strconv.FormatBool(c.public), m["public"].(string))
+	}
+}

src/replication/adapter/harbor/chart_registry.go

@@ -27,8 +27,6 @@ import (
 	"github.com/goharbor/harbor/src/replication/model"
 )
 
-// TODO review the logic in this file
-
 type chart struct {
 	Name    string `json:"name"`
 	Project string
@@ -110,8 +108,8 @@ func (a *adapter) FetchCharts(filters []*model.Filter) ([]*model.Resource, error
 					Registry: a.registry,
 					Metadata: &model.ResourceMetadata{
 						Repository: &model.Repository{
-							Name: fmt.Sprintf("%s/%s", project.Name, chart.Name),
-							// TODO handle the metadata
+							Name:     fmt.Sprintf("%s/%s", project.Name, chart.Name),
+							Metadata: project.Metadata,
 						},
 						Vtags: []string{version.Version},
 					},

src/replication/adapter/harbor/image_registry.go

@@ -95,8 +95,8 @@ func (a *adapter) FetchImages(filters []*model.Filter) ([]*model.Resource, error
 				Registry: a.registry,
 				Metadata: &model.ResourceMetadata{
 					Repository: &model.Repository{
-						Name: repository.Name,
-						// TODO handle the metadata
+						Name:     repository.Name,
+						Metadata: project.Metadata,
 					},
 					Vtags: vtags,
 				},

src/replication/dao/execution.go

@@ -125,7 +125,7 @@ func fillExecution(execution *models.Execution) error {
 	}
 
 	if execution.Total != total {
-		log.Errorf("execution task count inconsistent and fixed, executionID=%d, execution.total=%d, tasks.count=%d",
+		log.Debugf("execution task count inconsistent and fixed, executionID=%d, execution.total=%d, tasks.count=%d",
 			execution.ID, execution.Total, total)
 		execution.Total = total
 	}
@@ -324,38 +324,24 @@ func UpdateTask(task *models.Task, props ...string) (int64, error) {
 
 // UpdateTaskStatus ...
 func UpdateTaskStatus(id int64, status string, statusCondition ...string) (int64, error) {
-	o := dao.GetOrmer()
-
-	// update status
-	params := []interface{}{}
-	sql := `update replication_task set status = ?`
-	params = append(params, status)
-	if taskFinished(status) { // should update endTime
-		sql += ` ,end_time = ?`
-		params = append(params, time.Now())
-	}
-	sql += ` where id = ?`
-	params = append(params, id)
+	qs := dao.GetOrmer().QueryTable(&models.Task{}).
+		Filter("id", id)
 	if len(statusCondition) > 0 {
-		sql += ` and status in (`
-		for _, stCondition := range statusCondition {
-			sql += ` ?,`
-			params = append(params, stCondition)
-		}
-		sql = sql[0 : len(sql)-1]
-		sql += `)`
+		qs = qs.Filter("status", statusCondition[0])
 	}
-
-	log.Infof("Update task %d: -> %s", id, status)
-	res, err := o.Raw(sql, params).Exec()
+	params := orm.Params{
+		"status": status,
+	}
+	if taskFinished(status) {
+		// should update endTime
+		params["end_time"] = time.Now()
+	}
+	n, err := qs.Update(params)
 	if err != nil {
 		return 0, err
 	}
-	count, err := res.RowsAffected()
-	if err != nil {
-		return 0, err
-	}
-	return count, nil
+	log.Debugf("update task status %d: -> %s", id, status)
+	return n, err
 }
 
 func taskFinished(status string) bool {

src/replication/model/policy.go

@@ -40,18 +40,15 @@ type Policy struct {
 	ID          int64  `json:"id"`
 	Name        string `json:"name"`
 	Description string `json:"description"`
-	// TODO consider to remove this property?
-	Creator string `json:"creator"`
+	Creator     string `json:"creator"`
 	// source
 	SrcRegistry *Registry `json:"src_registry"`
 	// destination
-	// TODO rename to DstRegistry
 	DestRegistry *Registry `json:"dest_registry"`
 	// Only support two dest namespace modes:
 	// Put all the src resources to the one single dest namespace
 	// or keep namespaces same with the source ones (under this case,
 	// the DestNamespace should be set to empty)
-	// TODO rename to DstNamespace
 	DestNamespace string `json:"dest_namespace"`
 	// Filters
 	Filters []*Filter `json:"filters"`