From 3f7884d9d2ba057696f88f99bb5c98047f1feca7 Mon Sep 17 00:00:00 2001 From: Wenkai Yin Date: Wed, 17 Apr 2019 13:03:11 +0800 Subject: [PATCH] Revert "Add new registry type: LocalHarbor" This reverts commit 94cacf762a77edc4655725b68d07cfb42cba1e32. Signed-off-by: Wenkai Yin --- src/core/api/registry.go | 41 +++++----- src/core/api/replication_execution_test.go | 5 +- src/core/api/replication_policy.go | 31 +++----- src/core/api/replication_policy_test.go | 92 ++-------------------- src/replication/adapter/harbor/adapter.go | 8 -- src/replication/dao/registry.go | 10 +-- src/replication/event/handler.go | 22 +++++- src/replication/event/handler_test.go | 32 +------- src/replication/model/policy.go | 17 +++- src/replication/model/policy_test.go | 23 +++--- src/replication/model/registry.go | 7 +- src/replication/registry/manager.go | 1 - 12 files changed, 87 insertions(+), 202 deletions(-) diff --git a/src/core/api/registry.go b/src/core/api/registry.go index 7ade7d87b..d9cc7cf1b 100644 --- a/src/core/api/registry.go +++ b/src/core/api/registry.go @@ -11,6 +11,7 @@ import ( "github.com/goharbor/harbor/src/core/api/models" "github.com/goharbor/harbor/src/replication" "github.com/goharbor/harbor/src/replication/adapter" + "github.com/goharbor/harbor/src/replication/event" "github.com/goharbor/harbor/src/replication/model" "github.com/goharbor/harbor/src/replication/policy" "github.com/goharbor/harbor/src/replication/registry" @@ -185,25 +186,12 @@ func (t *RegistryAPI) Post() { t.CustomAbort(http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)) return } + if reg != nil { t.HandleConflict(fmt.Sprintf("name '%s' is already used", r.Name)) return } - if r.Type == model.RegistryTypeLocalHarbor { - n, _, err := t.manager.List(&model.RegistryQuery{ - Type: string(model.RegistryTypeLocalHarbor), - }) - if err != nil { - t.HandleInternalServerError(fmt.Sprintf("failed to list registries: %v", err)) - return - } - if n > 0 { - t.HandleBadRequest(fmt.Sprintf("can only add one registry whose type is %s", model.RegistryTypeLocalHarbor)) - return - } - } - status, err := registry.CheckHealthStatus(r) if err != nil { t.HandleBadRequest(fmt.Sprintf("health check to registry %s failed: %v", r.URL, err)) @@ -362,19 +350,26 @@ func (t *RegistryAPI) Delete() { // GetInfo returns the base info and capability declarations of the registry func (t *RegistryAPI) GetInfo() { id, err := t.GetInt64FromPath(":id") - if err != nil || id <= 0 { + // "0" is used for the ID of the local Harbor registry + if err != nil || id < 0 { t.HandleBadRequest(fmt.Sprintf("invalid registry ID %s", t.GetString(":id"))) return } - registry, err := t.manager.Get(id) - if err != nil { - t.HandleInternalServerError(fmt.Sprintf("failed to get registry %d: %v", id, err)) - return - } - if registry == nil { - t.HandleNotFound(fmt.Sprintf("registry %d not found", id)) - return + var registry *model.Registry + if id == 0 { + registry = event.GetLocalRegistry() + } else { + registry, err = t.manager.Get(id) + if err != nil { + t.HandleInternalServerError(fmt.Sprintf("failed to get registry %d: %v", id, err)) + return + } + if registry == nil { + t.HandleNotFound(fmt.Sprintf("registry %d not found", id)) + return + } } + factory, err := adapter.GetFactory(registry.Type) if err != nil { t.HandleInternalServerError(fmt.Sprintf("failed to get the adapter factory for registry type %s: %v", registry.Type, err)) diff --git a/src/core/api/replication_execution_test.go b/src/core/api/replication_execution_test.go index 5044c2744..c6ed3fdfb 100644 --- a/src/core/api/replication_execution_test.go +++ b/src/core/api/replication_execution_test.go @@ -89,9 +89,6 @@ func (f *fakedPolicyManager) Get(id int64) (*model.Policy, error) { SrcRegistry: &model.Registry{ ID: 1, }, - DestRegistry: &model.Registry{ - ID: 2, - }, }, nil } if id == 2 { @@ -202,7 +199,7 @@ func TestCreateExecution(t *testing.T) { }, code: http.StatusNotFound, }, - // 400 the policy is disabled + // 400 { request: &testingRequest{ method: http.MethodPost, diff --git a/src/core/api/replication_policy.go b/src/core/api/replication_policy.go index 21cda491a..da4f5b64a 100644 --- a/src/core/api/replication_policy.go +++ b/src/core/api/replication_policy.go @@ -26,6 +26,8 @@ import ( "github.com/goharbor/harbor/src/replication/registry" ) +// TODO rename the file to "replication.go" + // ReplicationPolicyAPI handles the replication policy requests type ReplicationPolicyAPI struct { BaseController @@ -103,30 +105,21 @@ func (r *ReplicationPolicyAPI) validateName(policy *model.Policy) bool { // make sure the registry referred exists func (r *ReplicationPolicyAPI) validateRegistry(policy *model.Policy) bool { - srcRegistry, err := replication.RegistryMgr.Get(policy.SrcRegistry.ID) + var registryID int64 + if policy.SrcRegistry != nil && policy.SrcRegistry.ID > 0 { + registryID = policy.SrcRegistry.ID + } else { + registryID = policy.DestRegistry.ID + } + registry, err := replication.RegistryMgr.Get(registryID) if err != nil { - r.HandleInternalServerError(fmt.Sprintf("failed to get source registry %d: %v", policy.SrcRegistry.ID, err)) + r.HandleInternalServerError(fmt.Sprintf("failed to get registry %d: %v", registryID, err)) return false } - if srcRegistry == nil { - r.HandleNotFound(fmt.Sprintf("source registry %d not found", policy.SrcRegistry.ID)) + if registry == nil { + r.HandleNotFound(fmt.Sprintf("registry %d not found", registryID)) return false } - dstRegistry, err := replication.RegistryMgr.Get(policy.DestRegistry.ID) - if err != nil { - r.HandleInternalServerError(fmt.Sprintf("failed to get destination registry %d: %v", policy.DestRegistry.ID, err)) - return false - } - if dstRegistry == nil { - r.HandleNotFound(fmt.Sprintf("destination registry %d not found", policy.DestRegistry.ID)) - return false - } - // one of the source registry or destination registry must be local Harbor - if srcRegistry.Type != model.RegistryTypeLocalHarbor && dstRegistry.Type != model.RegistryTypeLocalHarbor { - r.HandleBadRequest(fmt.Sprintf("at least one of the registries' type is %s", model.RegistryTypeLocalHarbor)) - return false - } - return true } diff --git a/src/core/api/replication_policy_test.go b/src/core/api/replication_policy_test.go index 29d99e9ce..654eb909b 100644 --- a/src/core/api/replication_policy_test.go +++ b/src/core/api/replication_policy_test.go @@ -22,6 +22,8 @@ import ( "github.com/goharbor/harbor/src/replication/model" ) +// TODO rename the file to "replication.go" + type fakedRegistryManager struct{} func (f *fakedRegistryManager) Add(*model.Registry) (int64, error) { @@ -33,12 +35,7 @@ func (f *fakedRegistryManager) List(...*model.RegistryQuery) (int64, []*model.Re func (f *fakedRegistryManager) Get(id int64) (*model.Registry, error) { if id == 1 { return &model.Registry{ - Type: model.RegistryTypeLocalHarbor, - }, nil - } - if id == 2 { - return &model.Registry{ - Type: model.RegistryTypeHarbor, + Type: "faked_registry", }, nil } return nil, nil @@ -131,14 +128,11 @@ func TestReplicationPolicyAPICreate(t *testing.T) { SrcRegistry: &model.Registry{ ID: 1, }, - DestRegistry: &model.Registry{ - ID: 2, - }, }, }, code: http.StatusBadRequest, }, - // 400 empty source registry + // 400 empty registry { request: &testingRequest{ method: http.MethodPost, @@ -146,29 +140,10 @@ func TestReplicationPolicyAPICreate(t *testing.T) { credential: sysAdmin, bodyJSON: &model.Policy{ Name: "policy01", - DestRegistry: &model.Registry{ - ID: 1, - }, }, }, code: http.StatusBadRequest, }, - // 400 empty destination registry - { - request: &testingRequest{ - method: http.MethodPost, - url: "/api/replication/policies", - credential: sysAdmin, - bodyJSON: &model.Policy{ - Name: "policy01", - SrcRegistry: &model.Registry{ - ID: 1, - }, - }, - }, - code: http.StatusBadRequest, - }, - // 409, duplicate policy name { request: &testingRequest{ @@ -180,9 +155,6 @@ func TestReplicationPolicyAPICreate(t *testing.T) { SrcRegistry: &model.Registry{ ID: 1, }, - DestRegistry: &model.Registry{ - ID: 2, - }, }, }, code: http.StatusConflict, @@ -196,33 +168,12 @@ func TestReplicationPolicyAPICreate(t *testing.T) { bodyJSON: &model.Policy{ Name: "policy01", SrcRegistry: &model.Registry{ - ID: 1, - }, - DestRegistry: &model.Registry{ - ID: 3, + ID: 2, }, }, }, code: http.StatusNotFound, }, - // 400 both registry types are not local harbor - { - request: &testingRequest{ - method: http.MethodPost, - url: "/api/replication/policies", - credential: sysAdmin, - bodyJSON: &model.Policy{ - Name: "policy01", - SrcRegistry: &model.Registry{ - ID: 2, - }, - DestRegistry: &model.Registry{ - ID: 2, - }, - }, - }, - code: http.StatusBadRequest, - }, // 201 { request: &testingRequest{ @@ -234,9 +185,6 @@ func TestReplicationPolicyAPICreate(t *testing.T) { SrcRegistry: &model.Registry{ ID: 1, }, - DestRegistry: &model.Registry{ - ID: 2, - }, }, }, code: http.StatusCreated, @@ -343,9 +291,6 @@ func TestReplicationPolicyAPIUpdate(t *testing.T) { SrcRegistry: &model.Registry{ ID: 1, }, - DestRegistry: &model.Registry{ - ID: 2, - }, }, }, code: http.StatusBadRequest, @@ -361,9 +306,6 @@ func TestReplicationPolicyAPIUpdate(t *testing.T) { SrcRegistry: &model.Registry{ ID: 1, }, - DestRegistry: &model.Registry{ - ID: 2, - }, }, }, code: http.StatusConflict, @@ -377,33 +319,12 @@ func TestReplicationPolicyAPIUpdate(t *testing.T) { bodyJSON: &model.Policy{ Name: "policy01", SrcRegistry: &model.Registry{ - ID: 3, - }, - DestRegistry: &model.Registry{ ID: 2, }, }, }, code: http.StatusNotFound, }, - // 400 both registry types are not local harbor - { - request: &testingRequest{ - method: http.MethodPut, - url: "/api/replication/policies/1", - credential: sysAdmin, - bodyJSON: &model.Policy{ - Name: "policy01", - SrcRegistry: &model.Registry{ - ID: 2, - }, - DestRegistry: &model.Registry{ - ID: 2, - }, - }, - }, - code: http.StatusBadRequest, - }, // 200 { request: &testingRequest{ @@ -415,9 +336,6 @@ func TestReplicationPolicyAPIUpdate(t *testing.T) { SrcRegistry: &model.Registry{ ID: 1, }, - DestRegistry: &model.Registry{ - ID: 2, - }, }, }, code: http.StatusOK, diff --git a/src/replication/adapter/harbor/adapter.go b/src/replication/adapter/harbor/adapter.go index 9b7157903..fb7ae20f2 100644 --- a/src/replication/adapter/harbor/adapter.go +++ b/src/replication/adapter/harbor/adapter.go @@ -37,14 +37,6 @@ func init() { return } log.Infof("the factory for adapter %s registered", model.RegistryTypeHarbor) - - if err := adp.RegisterFactory(model.RegistryTypeLocalHarbor, func(registry *model.Registry) (adp.Adapter, error) { - return newAdapter(registry) - }); err != nil { - log.Errorf("failed to register factory for %s: %v", model.RegistryTypeLocalHarbor, err) - return - } - log.Infof("the factory for adapter %s registered", model.RegistryTypeLocalHarbor) } type adapter struct { diff --git a/src/replication/dao/registry.go b/src/replication/dao/registry.go index 9ad5b243a..b398b318c 100644 --- a/src/replication/dao/registry.go +++ b/src/replication/dao/registry.go @@ -10,7 +10,6 @@ import ( type ListRegistryQuery struct { // Query is name query Query string - Type string // Offset specifies the offset in the registry list to return Offset int64 // Limit specifies the maximum registries to return @@ -62,13 +61,8 @@ func ListRegistries(query ...*ListRegistryQuery) (int64, []*models.Registry, err o := dao.GetOrmer() q := o.QueryTable(&models.Registry{}) - if len(query) > 0 && query[0] != nil { - if len(query[0].Query) > 0 { - q = q.Filter("name__contains", query[0].Query) - } - if len(query[0].Type) > 0 { - q = q.Filter("type", query[0].Type) - } + if len(query) > 0 && len(query[0].Query) > 0 { + q = q.Filter("name__contains", query[0].Query) } total, err := q.Count() diff --git a/src/replication/event/handler.go b/src/replication/event/handler.go index ec5354a90..3d731a4b1 100644 --- a/src/replication/event/handler.go +++ b/src/replication/event/handler.go @@ -21,6 +21,7 @@ import ( "github.com/goharbor/harbor/src/replication/util" "github.com/goharbor/harbor/src/common/utils/log" + "github.com/goharbor/harbor/src/replication/config" "github.com/goharbor/harbor/src/replication/model" "github.com/goharbor/harbor/src/replication/operation" "github.com/goharbor/harbor/src/replication/policy" @@ -159,8 +160,8 @@ func PopulateRegistries(registryMgr registry.Manager, policy *model.Policy) erro } func getRegistry(registryMgr registry.Manager, registry *model.Registry) (*model.Registry, error) { - if registry == nil { - return nil, errors.New("empty registry") + if registry == nil || registry.ID == 0 { + return GetLocalRegistry(), nil } reg, err := registryMgr.Get(registry.ID) if err != nil { @@ -171,3 +172,20 @@ func getRegistry(registryMgr registry.Manager, registry *model.Registry) (*model } return reg, nil } + +// GetLocalRegistry returns the info of the local Harbor registry +func GetLocalRegistry() *model.Registry { + return &model.Registry{ + Type: model.RegistryTypeHarbor, + Name: "Local", + URL: config.Config.RegistryURL, + CoreURL: config.Config.CoreURL, + Status: "healthy", + Credential: &model.Credential{ + Type: model.CredentialTypeSecret, + // use secret to do the auth for the local Harbor + AccessSecret: config.Config.JobserviceSecret, + }, + Insecure: true, + } +} diff --git a/src/replication/event/handler_test.go b/src/replication/event/handler_test.go index 97e351bfa..be5c7efb2 100644 --- a/src/replication/event/handler_test.go +++ b/src/replication/event/handler_test.go @@ -62,12 +62,6 @@ func (f *fakedPolicyController) List(...*model.PolicyQuery) (int64, []*model.Pol ID: 1, Enabled: true, Deletion: true, - SrcRegistry: &model.Registry{ - ID: 1, - }, - DestRegistry: &model.Registry{ - ID: 1, - }, Trigger: &model.Trigger{ Type: model.TriggerTypeEventBased, }, @@ -83,13 +77,7 @@ func (f *fakedPolicyController) List(...*model.PolicyQuery) (int64, []*model.Pol ID: 2, Enabled: true, Deletion: true, - SrcRegistry: &model.Registry{ - ID: 1, - }, - DestRegistry: &model.Registry{ - ID: 1, - }, - Trigger: nil, + Trigger: nil, Filters: []*model.Filter{ { Type: model.FilterTypeName, @@ -102,12 +90,6 @@ func (f *fakedPolicyController) List(...*model.PolicyQuery) (int64, []*model.Pol ID: 3, Enabled: true, Deletion: false, - SrcRegistry: &model.Registry{ - ID: 1, - }, - DestRegistry: &model.Registry{ - ID: 1, - }, Trigger: &model.Trigger{ Type: model.TriggerTypeEventBased, }, @@ -123,12 +105,6 @@ func (f *fakedPolicyController) List(...*model.PolicyQuery) (int64, []*model.Pol ID: 4, Enabled: true, Deletion: true, - SrcRegistry: &model.Registry{ - ID: 1, - }, - DestRegistry: &model.Registry{ - ID: 1, - }, Trigger: &model.Trigger{ Type: model.TriggerTypeEventBased, }, @@ -144,12 +120,6 @@ func (f *fakedPolicyController) List(...*model.PolicyQuery) (int64, []*model.Pol ID: 5, Enabled: false, Deletion: true, - SrcRegistry: &model.Registry{ - ID: 1, - }, - DestRegistry: &model.Registry{ - ID: 1, - }, Trigger: &model.Trigger{ Type: model.TriggerTypeEventBased, }, diff --git a/src/replication/model/policy.go b/src/replication/model/policy.go index c2a856c89..3751d2c74 100644 --- a/src/replication/model/policy.go +++ b/src/replication/model/policy.go @@ -72,12 +72,20 @@ func (p *Policy) Valid(v *validation.Validation) { if len(p.Name) == 0 { v.SetError("name", "cannot be empty") } - if p.SrcRegistry == nil || p.SrcRegistry.ID == 0 { - v.SetError("src_registry", "cannot be empty") + var srcRegistryID, dstRegistryID int64 + if p.SrcRegistry != nil { + srcRegistryID = p.SrcRegistry.ID } - if p.DestRegistry == nil || p.DestRegistry.ID == 0 { - v.SetError("dest_registry", "cannot be empty") + if p.DestRegistry != nil { + dstRegistryID = p.DestRegistry.ID } + + // one of the source registry and destination registry must be Harbor itself + if srcRegistryID != 0 && dstRegistryID != 0 || + srcRegistryID == 0 && dstRegistryID == 0 { + v.SetError("src_registry, dest_registry", "one of them should be empty and the other one shouldn't be empty") + } + // valid the filters for _, filter := range p.Filters { if filter.Type != FilterTypeResource && @@ -88,6 +96,7 @@ func (p *Policy) Valid(v *validation.Validation) { break } } + // valid trigger if p.Trigger != nil { if p.Trigger.Type != TriggerTypeManual && diff --git a/src/replication/model/policy_test.go b/src/replication/model/policy_test.go index db35de65f..9b06a5f31 100644 --- a/src/replication/model/policy_test.go +++ b/src/replication/model/policy_test.go @@ -31,20 +31,23 @@ func TestValidOfPolicy(t *testing.T) { policy: &Policy{}, pass: false, }, - // empty source registry + // empty source registry and destination registry { policy: &Policy{ Name: "policy01", }, pass: false, }, - // empty destination registry + // source registry and destination registry both not empty { policy: &Policy{ Name: "policy01", SrcRegistry: &Registry{ ID: 1, }, + DestRegistry: &Registry{ + ID: 2, + }, }, pass: false, }, @@ -53,10 +56,10 @@ func TestValidOfPolicy(t *testing.T) { policy: &Policy{ Name: "policy01", SrcRegistry: &Registry{ - ID: 1, + ID: 0, }, DestRegistry: &Registry{ - ID: 2, + ID: 1, }, Filters: []*Filter{ { @@ -71,10 +74,10 @@ func TestValidOfPolicy(t *testing.T) { policy: &Policy{ Name: "policy01", SrcRegistry: &Registry{ - ID: 1, + ID: 0, }, DestRegistry: &Registry{ - ID: 2, + ID: 1, }, Filters: []*Filter{ { @@ -93,10 +96,10 @@ func TestValidOfPolicy(t *testing.T) { policy: &Policy{ Name: "policy01", SrcRegistry: &Registry{ - ID: 1, + ID: 0, }, DestRegistry: &Registry{ - ID: 2, + ID: 1, }, Filters: []*Filter{ { @@ -115,10 +118,10 @@ func TestValidOfPolicy(t *testing.T) { policy: &Policy{ Name: "policy01", SrcRegistry: &Registry{ - ID: 1, + ID: 0, }, DestRegistry: &Registry{ - ID: 2, + ID: 1, }, Filters: []*Filter{ { diff --git a/src/replication/model/registry.go b/src/replication/model/registry.go index 31976ecf3..0d9e10390 100644 --- a/src/replication/model/registry.go +++ b/src/replication/model/registry.go @@ -23,10 +23,8 @@ import ( // const definition const ( // RegistryTypeHarbor indicates registry type harbor - RegistryTypeHarbor RegistryType = "harbor" - // Local Harbor is the type of Harbor instance where the replication service is running on - RegistryTypeLocalHarbor RegistryType = "localHarbor" - RegistryTypeDockerHub RegistryType = "dockerHub" + RegistryTypeHarbor RegistryType = "harbor" + RegistryTypeDockerHub RegistryType = "dockerHub" FilterStyleTypeText = "input" FilterStyleTypeRadio = "radio" @@ -96,7 +94,6 @@ type Registry struct { type RegistryQuery struct { // Name is name of the registry to query Name string - Type string // Pagination specifies the pagination Pagination *models.Pagination } diff --git a/src/replication/registry/manager.go b/src/replication/registry/manager.go index 1f24ace05..c6d56289d 100644 --- a/src/replication/registry/manager.go +++ b/src/replication/registry/manager.go @@ -91,7 +91,6 @@ func (m *DefaultManager) List(query ...*model.RegistryQuery) (int64, []*model.Re // limit being -1 indicates no pagination specified, result in all registries matching name returned. listQuery := &dao.ListRegistryQuery{ Query: query[0].Name, - Type: query[0].Type, Limit: -1, } if query[0].Pagination != nil {