Merge pull request #12617 from steven-zou/fix/preheat_npe_issues

fix(preheat): fix npe issues

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 {
+	if schema == nil {
-		now := time.Now()
+		return 0, errors.New("nil policy schema provided")
-		schema.CreatedAt = now
+	}
-		schema.UpdatedTime = now
+
+	// 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
+		if err = decodeSchema(schema); err == nil {
-		err = c.pManager.Update(ctx, schema, "trigger")
+			err = c.pManager.Update(ctx, schema, "trigger")
+		}
+
 		if err != nil {
-			errUnsch := c.scheduler.UnSchedule(ctx, schema.Trigger.Settings.JobID)
+			if e := c.scheduler.UnSchedule(ctx, schema.Trigger.Settings.JobID); e != nil {
-			if errUnsch != nil {
+				return 0, errors.Wrap(e, err.Error())
-				return 0, errUnsch
 			}
+
 			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 {
+	if schema == nil {
-		schema.UpdatedTime = time.Now()
+		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
+}

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 := &policy.Schema{
-	policy.Trigger.Settings.Cron = "* * * * */1"
+		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 := &policy.Schema{
-	p2.Trigger.Settings.Cron = "* * * * */2"
+		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)

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.
+// ParsePolicy parses persisting data to policy model.
-func parsePolicy(schema *policy.Schema) (*policy.Schema, error) {
+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{}

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)
+	m.dao.On("GetByName").Return(&policy.Schema{
-	_, err := m.mgr.Get(nil, 1)
+		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)

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