From 507d792655ab9914cb27f1845a07690d3a3a3351 Mon Sep 17 00:00:00 2001 From: Steven Zou Date: Wed, 29 Jul 2020 18:16:30 +0800 Subject: [PATCH] fix(preheat): fix npe issues - fix npe issue in create/update policy - fix issue of missing schedule job id in the preheat policy Signed-off-by: Steven Zou - increase the client timeout --- src/controller/p2p/preheat/controller.go | 75 ++++++++++++++++--- src/controller/p2p/preheat/controllor_test.go | 33 ++++++-- src/pkg/p2p/preheat/policy/manager.go | 16 ++-- src/pkg/p2p/preheat/policy/manager_test.go | 24 ++++-- .../preheat/provider/client/http_client.go | 2 +- 5 files changed, 120 insertions(+), 30 deletions(-) diff --git a/src/controller/p2p/preheat/controller.go b/src/controller/p2p/preheat/controller.go index 12645813a..b8634e714 100644 --- a/src/controller/p2p/preheat/controller.go +++ b/src/controller/p2p/preheat/controller.go @@ -2,6 +2,7 @@ package preheat import ( "context" + "encoding/json" "time" "github.com/goharbor/harbor/src/lib/errors" @@ -212,10 +213,19 @@ type TriggerParam struct { // CreatePolicy creates the policy. func (c *controller) CreatePolicy(ctx context.Context, schema *policyModels.Schema) (id int64, err error) { - if schema != nil { - now := time.Now() - schema.CreatedAt = now - schema.UpdatedTime = now + if schema == nil { + return 0, errors.New("nil policy schema provided") + } + + // Update timestamps + now := time.Now() + schema.CreatedAt = now + schema.UpdatedTime = now + + // Get full model of policy schema + _, err = policy.ParsePolicy(schema) + if err != nil { + return 0, err } id, err = c.pManager.Create(ctx, schema) @@ -223,6 +233,8 @@ func (c *controller) CreatePolicy(ctx context.Context, schema *policyModels.Sche return } + schema.ID = id + if schema.Trigger != nil && schema.Trigger.Type == policyModels.TriggerTypeScheduled && len(schema.Trigger.Settings.Cron) > 0 { @@ -232,13 +244,15 @@ func (c *controller) CreatePolicy(ctx context.Context, schema *policyModels.Sche return 0, err } - schema.ID = id - err = c.pManager.Update(ctx, schema, "trigger") + if err = decodeSchema(schema); err == nil { + err = c.pManager.Update(ctx, schema, "trigger") + } + if err != nil { - errUnsch := c.scheduler.UnSchedule(ctx, schema.Trigger.Settings.JobID) - if errUnsch != nil { - return 0, errUnsch + if e := c.scheduler.UnSchedule(ctx, schema.Trigger.Settings.JobID); e != nil { + return 0, errors.Wrap(e, err.Error()) } + return 0, err } } @@ -258,13 +272,27 @@ func (c *controller) GetPolicyByName(ctx context.Context, projectID int64, name // UpdatePolicy updates the policy. func (c *controller) UpdatePolicy(ctx context.Context, schema *policyModels.Schema, props ...string) error { - if schema != nil { - schema.UpdatedTime = time.Now() + if schema == nil { + return errors.New("nil policy schema provided") } + + // Get policy cache s0, err := c.pManager.Get(ctx, schema.ID) if err != nil { return err } + + // Double check trigger + if s0.Trigger == nil { + return errors.Errorf("missing trigger settings in preheat policy %s", s0.Name) + } + + // Get full model of updating policy + _, err = policy.ParsePolicy(schema) + if err != nil { + return err + } + var cron = schema.Trigger.Settings.Cron var oldJobID = s0.Trigger.Settings.JobID var needUn bool @@ -309,8 +337,16 @@ func (c *controller) UpdatePolicy(ctx context.Context, schema *policyModels.Sche return err } schema.Trigger.Settings.JobID = jobid + if err := decodeSchema(schema); err != nil { + // Possible + // TODO: Refactor + return err // whether update or not has been not important as the jon reference will be lost + } } + // Update timestamp + schema.UpdatedTime = time.Now() + return c.pManager.Update(ctx, schema, props...) } @@ -369,3 +405,20 @@ func (c *controller) CheckHealth(ctx context.Context, instance *providerModels.I return nil } + +// decodeSchema decodes the trigger object to JSON string +func decodeSchema(schema *policyModels.Schema) error { + if schema.Trigger == nil { + // do nothing + return nil + } + + b, err := json.Marshal(schema.Trigger) + if err != nil { + return err + } + + schema.TriggerStr = string(b) + + return nil +} diff --git a/src/controller/p2p/preheat/controllor_test.go b/src/controller/p2p/preheat/controllor_test.go index 2fe4b3cb2..8751842eb 100644 --- a/src/controller/p2p/preheat/controllor_test.go +++ b/src/controller/p2p/preheat/controllor_test.go @@ -3,6 +3,7 @@ package preheat import ( "context" "errors" + "fmt" "net/http" "net/http/httptest" "testing" @@ -198,8 +199,11 @@ func (s *preheatSuite) TestCountPolicy() { } func (s *preheatSuite) TestCreatePolicy() { - policy := &policy.Schema{Name: "test", Trigger: &policy.Trigger{Type: policy.TriggerTypeScheduled}} - policy.Trigger.Settings.Cron = "* * * * */1" + policy := &policy.Schema{ + Name: "test", + FiltersStr: `[{"type":"repository","value":"harbor*"},{"type":"tag","value":"2*"}]`, + TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{"cron":"* * * * */1"}}`, policy.TriggerTypeScheduled), + } s.fakeScheduler.On("Schedule", s.ctx, mock.Anything, mock.Anything, mock.Anything).Return(int64(1), nil) s.fakePolicyMgr.On("Create", s.ctx, policy).Return(int64(1), nil) s.fakePolicyMgr.On("Update", s.ctx, mock.Anything, mock.Anything).Return(nil) @@ -229,18 +233,37 @@ func (s *preheatSuite) TestUpdatePolicy() { var p0 = &policy.Schema{Name: "test", Trigger: &policy.Trigger{Type: policy.TriggerTypeScheduled}} p0.Trigger.Settings.JobID = 1 p0.Trigger.Settings.Cron = "* * * * */1" + p0.Filters = []*policy.Filter{ + { + Type: policy.FilterTypeRepository, + Value: "harbor*", + }, + { + Type: policy.FilterTypeTag, + Value: "2*", + }, + } s.fakePolicyMgr.On("Get", s.ctx, int64(1)).Return(p0, nil) // need change to schedule - p1 := &policy.Schema{ID: 1, Name: "test", Trigger: &policy.Trigger{Type: policy.TriggerTypeManual}} + p1 := &policy.Schema{ + ID: 1, + Name: "test", + FiltersStr: `[{"type":"repository","value":"harbor*"},{"type":"tag","value":"2*"}]`, + TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{}}`, policy.TriggerTypeManual), + } s.fakePolicyMgr.On("Update", s.ctx, p1, mock.Anything).Return(nil) err := s.controller.UpdatePolicy(s.ctx, p1, "") s.NoError(err) s.False(p1.UpdatedTime.IsZero()) // need update schedule - p2 := &policy.Schema{ID: 1, Name: "test", Trigger: &policy.Trigger{Type: policy.TriggerTypeScheduled}} - p2.Trigger.Settings.Cron = "* * * * */2" + p2 := &policy.Schema{ + ID: 1, + Name: "test", + FiltersStr: `[{"type":"repository","value":"harbor*"},{"type":"tag","value":"2*"}]`, + TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{"cron":"* * * * */2"}}`, policy.TriggerTypeScheduled), + } s.fakePolicyMgr.On("Update", s.ctx, p2, mock.Anything).Return(nil) err = s.controller.UpdatePolicy(s.ctx, p2, "") s.NoError(err) diff --git a/src/pkg/p2p/preheat/policy/manager.go b/src/pkg/p2p/preheat/policy/manager.go index 530e54c37..def4c9d3e 100644 --- a/src/pkg/p2p/preheat/policy/manager.go +++ b/src/pkg/p2p/preheat/policy/manager.go @@ -81,7 +81,7 @@ func (m *manager) Get(ctx context.Context, id int64) (schema *policy.Schema, err return nil, err } - return parsePolicy(schema) + return ParsePolicy(schema) } // Get the policy schema by name @@ -91,7 +91,7 @@ func (m *manager) GetByName(ctx context.Context, projectID int64, name string) ( return nil, err } - return parsePolicy(schema) + return ParsePolicy(schema) } // Delete the policy schema by id @@ -107,7 +107,7 @@ func (m *manager) ListPolicies(ctx context.Context, query *q.Query) (schemas []* } for i := range schemas { - schema, err := parsePolicy(schemas[i]) + schema, err := ParsePolicy(schemas[i]) if err != nil { return nil, err } @@ -132,8 +132,8 @@ func (m *manager) ListPoliciesByProject(ctx context.Context, project int64, quer return m.ListPolicies(ctx, query) } -// parsePolicy parse policy model. -func parsePolicy(schema *policy.Schema) (*policy.Schema, error) { +// ParsePolicy parses persisting data to policy model. +func ParsePolicy(schema *policy.Schema) (*policy.Schema, error) { if schema == nil { return nil, errors.New("policy schema can not be nil") } @@ -157,8 +157,9 @@ func parsePolicy(schema *policy.Schema) (*policy.Schema, error) { // parseFilters parse filterStr to filter. func parseFilters(filterStr string) ([]*policy.Filter, error) { + // Filters are required if len(filterStr) == 0 { - return nil, nil + return nil, errors.New("missing filters in preheat policy schema") } var filters []*policy.Filter @@ -188,8 +189,9 @@ func parseFilters(filterStr string) ([]*policy.Filter, error) { // parseTrigger parse triggerStr to trigger. func parseTrigger(triggerStr string) (*policy.Trigger, error) { + // trigger must be existing, at least is a "manual" trigger. if len(triggerStr) == 0 { - return nil, nil + return nil, errors.New("missing trigger settings in preheat policy schema") } trigger := &policy.Trigger{} diff --git a/src/pkg/p2p/preheat/policy/manager_test.go b/src/pkg/p2p/preheat/policy/manager_test.go index 2d10f7244..6e0823a17 100644 --- a/src/pkg/p2p/preheat/policy/manager_test.go +++ b/src/pkg/p2p/preheat/policy/manager_test.go @@ -16,6 +16,7 @@ package policy import ( "context" + "fmt" "testing" "github.com/goharbor/harbor/src/lib/q" @@ -115,15 +116,26 @@ func (m *managerTestSuite) TestUpdate() { // TestGet tests Get method. func (m *managerTestSuite) TestGet() { - m.dao.On("Get").Return(&policy.Schema{}, nil) + m.dao.On("Get").Return(&policy.Schema{ + ID: 1, + Name: "mgr-policy", + FiltersStr: `[{"type":"repository","value":"harbor*"},{"type":"tag","value":"2*"}]`, + TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{"cron":"* * * * */1"}}`, policy.TriggerTypeScheduled), + }, nil) _, err := m.mgr.Get(nil, 1) m.Require().Nil(err) } // TestGetByName tests Get method. func (m *managerTestSuite) TestGetByName() { - m.dao.On("GetByName").Return(&policy.Schema{}, nil) - _, err := m.mgr.Get(nil, 1) + m.dao.On("GetByName").Return(&policy.Schema{ + ID: 1, + ProjectID: 1, + Name: "mgr-policy", + FiltersStr: `[{"type":"repository","value":"harbor*"},{"type":"tag","value":"2*"}]`, + TriggerStr: fmt.Sprintf(`{"type":"%s", "trigger_setting":{"cron":"* * * * */1"}}`, policy.TriggerTypeScheduled), + }, nil) + _, err := m.mgr.GetByName(nil, 1, "mgr-policy") m.Require().Nil(err) } @@ -151,11 +163,11 @@ func (m *managerTestSuite) TestListPoliciesByProject() { // TestParsePolicy tests parsePolicy. func (m *managerTestSuite) TestParsePolicy() { schema := &policy.Schema{FiltersStr: "invalid"} - _, err := parsePolicy(schema) + _, err := ParsePolicy(schema) m.Require().Error(err) schema = &policy.Schema{TriggerStr: "invalid"} - _, err = parsePolicy(schema) + _, err = ParsePolicy(schema) m.Require().Error(err) schema = &policy.Schema{ @@ -187,7 +199,7 @@ func (m *managerTestSuite) TestParsePolicy() { } }`, } - schema, err = parsePolicy(schema) + schema, err = ParsePolicy(schema) m.Require().NoError(err) m.Require().NotNil(schema.Trigger) m.Require().Equal("0 0 2 1 * ? *", schema.Trigger.Settings.Cron) diff --git a/src/pkg/p2p/preheat/provider/client/http_client.go b/src/pkg/p2p/preheat/provider/client/http_client.go index 789363c03..a08e9117f 100644 --- a/src/pkg/p2p/preheat/provider/client/http_client.go +++ b/src/pkg/p2p/preheat/provider/client/http_client.go @@ -16,7 +16,7 @@ import ( ) const ( - clientTimeout = 10 * time.Second + clientTimeout = 30 * time.Second maxIdleConnections = 20 idleConnectionTimeout = 30 * time.Second tlsHandshakeTimeout = 30 * time.Second