From b2ec543dd3028f224416e9e440cb78d57c2b8944 Mon Sep 17 00:00:00 2001 From: "xu.zhu" Date: Mon, 18 Dec 2023 14:23:44 +0800 Subject: [PATCH] fix: lint and ut Signed-off-by: xu.zhu --- core/controller/application/admission.go | 1 - core/controller/cluster/admission.go | 73 ------------ core/middleware/admission/admission.go | 17 ++- pkg/admission/http.go | 91 +++++++++------ pkg/admission/models/models.go | 5 +- pkg/admission/webhook.go | 46 ++------ pkg/admission/webhook_test.go | 139 +++++++++++++++++++++++ 7 files changed, 219 insertions(+), 153 deletions(-) delete mode 100644 core/controller/application/admission.go delete mode 100644 core/controller/cluster/admission.go create mode 100644 pkg/admission/webhook_test.go diff --git a/core/controller/application/admission.go b/core/controller/application/admission.go deleted file mode 100644 index b584a8a4..00000000 --- a/core/controller/application/admission.go +++ /dev/null @@ -1 +0,0 @@ -package application diff --git a/core/controller/cluster/admission.go b/core/controller/cluster/admission.go deleted file mode 100644 index 506e53b8..00000000 --- a/core/controller/cluster/admission.go +++ /dev/null @@ -1,73 +0,0 @@ -package cluster - -import ( - "github.com/horizoncd/horizon/core/common" - appmodels "github.com/horizoncd/horizon/pkg/application/models" - "github.com/horizoncd/horizon/pkg/cluster/code" - "github.com/horizoncd/horizon/pkg/cluster/models" - tagmodels "github.com/horizoncd/horizon/pkg/tag/models" -) - -type Cluster struct { - *models.Cluster `json:",inline"` - *TemplateInput `json:"templateInput,omitempty"` - Tags tagmodels.TagsBasic `json:"tags,omitempty"` -} - -type Clusterv2 struct { - *models.Cluster `json:",inline"` - *TemplateInput `json:"templateInput,omitempty"` - TemplateConfig map[string]interface{} `json:"templateConfig,omitempty"` - TemplateInfo *code.TemplateInfo `json:"-"` - MergePatch bool `json:"mergePatch"` - BuildConfig map[string]interface{} `json:"buildConfig"` - Tags tagmodels.TagsBasic `json:"tags,omitempty"` -} - -func (c *Clusterv2) toClusterModel(application *appmodels.Application) *models.Cluster { - cluster := &models.Cluster{ - ApplicationID: c.ApplicationID, - Name: c.Name, - EnvironmentName: c.EnvironmentName, - RegionName: c.RegionName, - Description: c.Description, - ExpireSeconds: c.ExpireSeconds, - Template: c.Template, - TemplateRelease: c.TemplateRelease, - Status: common.ClusterStatusCreating, - } - if cluster.Template == application.Template { - cluster.GitURL = func() string { - if c.GitURL == "" && application.GitURL != "" { - return application.GitURL - } - // if URL is empty string, this means this cluster not depends on build from git - return c.GitURL - }() - cluster.GitSubfolder = func() string { - if c.GitSubfolder == "" { - return application.GitSubfolder - } - return c.GitSubfolder - }() - cluster.GitRef = func() string { - if c.GitRef == "" { - return application.GitRef - } - return c.GitRef - }() - cluster.GitRefType = func() string { - if c.GitRefType == "" { - return application.GitRefType - } - return c.GitRefType - }() - cluster.Image = func() string { - if c.Image == "" { - return application.Image - } - return c.Image - }() - } - return cluster -} diff --git a/core/middleware/admission/admission.go b/core/middleware/admission/admission.go index 5d951568..64dbf35c 100644 --- a/core/middleware/admission/admission.go +++ b/core/middleware/admission/admission.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "github.com/gin-gonic/gin" + "github.com/gin-gonic/gin/binding" "github.com/horizoncd/horizon/core/common" "github.com/horizoncd/horizon/core/middleware" admissionwebhook "github.com/horizoncd/horizon/pkg/admission" @@ -14,6 +15,7 @@ import ( "github.com/horizoncd/horizon/pkg/auth" "github.com/horizoncd/horizon/pkg/server/response" "github.com/horizoncd/horizon/pkg/server/rpcerror" + "github.com/horizoncd/horizon/pkg/util/log" ) // Middleware to validate and mutate admission request @@ -41,10 +43,17 @@ func Middleware(skippers ...middleware.Skipper) gin.HandlerFunc { return } c.Request.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes)) - if err := json.Unmarshal(bodyBytes, &object); err != nil { - response.AbortWithRPCError(c, - rpcerror.ParamError.WithErrMsg(fmt.Sprintf("unmarshal request body failed, err: %v", err))) - return + if len(bodyBytes) > 0 { + contentType := c.ContentType() + if contentType == binding.MIMEJSON { + if err := json.Unmarshal(bodyBytes, &object); err != nil { + response.AbortWithRPCError(c, + rpcerror.ParamError.WithErrMsg(fmt.Sprintf("unmarshal request body failed, err: %v", err))) + return + } + } else { + log.Warningf(c, "unsupported content type: %s", contentType) + } } // fill in the request url query into admission request options queries := c.Request.URL.Query() diff --git a/pkg/admission/http.go b/pkg/admission/http.go index 6de02892..fbec07d0 100644 --- a/pkg/admission/http.go +++ b/pkg/admission/http.go @@ -17,6 +17,7 @@ import ( "github.com/horizoncd/horizon/pkg/admission/models" config "github.com/horizoncd/horizon/pkg/config/admission" perror "github.com/horizoncd/horizon/pkg/errors" + "github.com/horizoncd/horizon/pkg/util/common" ) type HTTPAdmissionClient struct { @@ -24,6 +25,7 @@ type HTTPAdmissionClient struct { http.Client } +// NewHTTPAdmissionClient creates a new HTTPAdmissionClient func NewHTTPAdmissionClient(config config.ClientConfig, timeout time.Duration) *HTTPAdmissionClient { var transport = &http.Transport{} if config.CABundle != "" { @@ -52,6 +54,7 @@ func NewHTTPAdmissionClient(config config.ClientConfig, timeout time.Duration) * } } +// Get sends the admission request to the webhook server and returns the response func (c *HTTPAdmissionClient) Get(ctx context.Context, admitData *Request) (*Response, error) { body, err := json.Marshal(admitData) if err != nil { @@ -87,6 +90,7 @@ type ResourceMatcher struct { versions map[string]struct{} } +// NewResourceMatcher creates a new ResourceMatcher func NewResourceMatcher(rule config.Rule) *ResourceMatcher { matcher := &ResourceMatcher{ resources: make(map[string]struct{}), @@ -117,6 +121,7 @@ func NewResourceMatcher(rule config.Rule) *ResourceMatcher { return matcher } +// Match returns true if the request matches the matcher func (m *ResourceMatcher) Match(req *Request) bool { if m.resources != nil { resource := req.Resource @@ -142,6 +147,7 @@ func (m *ResourceMatcher) Match(req *Request) bool { type ResourceMatchers []*ResourceMatcher +// NewResourceMatchers creates a new ResourceMatchers func NewResourceMatchers(rules []config.Rule) ResourceMatchers { matchers := make(ResourceMatchers, len(rules)) for i, rule := range rules { @@ -150,6 +156,7 @@ func NewResourceMatchers(rules []config.Rule) ResourceMatchers { return matchers } +// Match returns true if any matcher matches the request func (m ResourceMatchers) Match(req *Request) bool { for _, matcher := range m { if matcher.Match(req) { @@ -165,6 +172,7 @@ type HTTPAdmissionWebhook struct { matchers ResourceMatchers } +// NewHTTPWebhooks registers the webhooks func NewHTTPWebhooks(config config.Admission) { for _, webhook := range config.Webhooks { switch webhook.Kind { @@ -184,6 +192,7 @@ func NewHTTPWebhook(config config.Webhook) Webhook { } } +// Handle handles the admission request and returns the response func (m *HTTPAdmissionWebhook) Handle(ctx context.Context, req *Request) (*Response, error) { resp, err := m.httpclient.Get(ctx, req) if err != nil { @@ -192,20 +201,23 @@ func (m *HTTPAdmissionWebhook) Handle(ctx context.Context, req *Request) (*Respo return resp, nil } +// IgnoreError returns true if the webhook is allowed to ignore the error func (m *HTTPAdmissionWebhook) IgnoreError() bool { return m.config.FailurePolicy.Eq(config.FailurePolicyIgnore) } +// Interest returns true if the request matches the webhook func (m *HTTPAdmissionWebhook) Interest(req *Request) bool { return m.matchers.Match(req) } -type DummyMutatingWebhookServer struct { +type DummyValidatingWebhookServer struct { server *httptest.Server } -func NewDummyWebhookServer() *DummyMutatingWebhookServer { - webhook := &DummyMutatingWebhookServer{} +// NewDummyWebhookServer creates a dummy validating webhook server for testing +func NewDummyWebhookServer() *DummyValidatingWebhookServer { + webhook := &DummyValidatingWebhookServer{} mux := http.NewServeMux() mux.HandleFunc("/validate", webhook.Validating) @@ -215,7 +227,7 @@ func NewDummyWebhookServer() *DummyMutatingWebhookServer { return webhook } -func (*DummyMutatingWebhookServer) ReadAndResponse(resp http.ResponseWriter, +func (*DummyValidatingWebhookServer) ReadAndResponse(resp http.ResponseWriter, req *http.Request, fn func(Request, *Response)) { bodyBytes, _ := ioutil.ReadAll(req.Body) @@ -230,55 +242,60 @@ func (*DummyMutatingWebhookServer) ReadAndResponse(resp http.ResponseWriter, _, _ = resp.Write(respBytes) } -type Tag struct { - Key string `json:"key"` - Value string `json:"value"` -} - -func (w *DummyMutatingWebhookServer) Validating(resp http.ResponseWriter, req *http.Request) { +func (w *DummyValidatingWebhookServer) Validating(resp http.ResponseWriter, req *http.Request) { w.ReadAndResponse(resp, req, w.validating) } -func (w *DummyMutatingWebhookServer) validating(req Request, resp *Response) { +func (w *DummyValidatingWebhookServer) validating(req Request, resp *Response) { obj := req.Object.(map[string]interface{}) - allow := true - - name, ok := obj["name"].(string) - if !ok { - allow = false - resp.Result = "no name found" + if req.Operation.Eq(models.OperationCreate) { + // check name + name, ok := obj["name"].(string) + if !ok { + resp.Allowed = common.BoolPtr(false) + resp.Result = "no name found" + return + } + if strings.Contains(name, "invalid") { + resp.Allowed = common.BoolPtr(false) + resp.Result = fmt.Sprintf("name contains invalid: %s", name) + return + } } - if strings.Contains(name, "invalid") { - allow = false - resp.Result = fmt.Sprintf("name contains invalid: %s", name) + // check tags + tagsMap, ok := obj["tags"].([]interface{}) + if !ok { + // skip tag validation if no tags found + resp.Allowed = common.BoolPtr(true) + return } - - if obj["tags"] != nil { - tags := obj["tags"].([]interface{}) - for _, tag := range tags { - tag := tag.(map[string]interface{}) - tagKey := tag["key"].(string) - if strings.Contains(tagKey, "invalid") { - allow = false - resp.Result = fmt.Sprintf("tag key contains invalid: %s", tagKey) - break - } + targetKey := "scope" + exist := false + for _, tag := range tagsMap { + t, ok := tag.(map[string]interface{}) + if !ok { + continue + } + if t["key"] == targetKey { + exist = true + break } } - if !allow { - allow = false - resp.Allowed = &allow + if !exist { + resp.Allowed = common.BoolPtr(false) + resp.Result = fmt.Sprintf("no tag with key: %s", targetKey) return } - resp.Allowed = &allow + + resp.Allowed = common.BoolPtr(true) } -func (w *DummyMutatingWebhookServer) ValidatingURL() string { +func (w *DummyValidatingWebhookServer) ValidatingURL() string { return w.server.URL + "/validate" } -func (w *DummyMutatingWebhookServer) Stop() { +func (w *DummyValidatingWebhookServer) Stop() { w.server.Close() } diff --git a/pkg/admission/models/models.go b/pkg/admission/models/models.go index 0cb3e526..5498d499 100644 --- a/pkg/admission/models/models.go +++ b/pkg/admission/models/models.go @@ -19,7 +19,10 @@ func (o Operation) Eq(other Operation) bool { } const ( + KindValidating Kind = "validating" + MatchAll string = "*" - KindValidating Kind = "validating" + OperationCreate Operation = "create" + OperationUpdate Operation = "update" ) diff --git a/pkg/admission/webhook.go b/pkg/admission/webhook.go index c56b2fe0..21fa4c02 100644 --- a/pkg/admission/webhook.go +++ b/pkg/admission/webhook.go @@ -2,10 +2,8 @@ package admission import ( "context" - "encoding/json" "time" - jsonpatch "gopkg.in/evanphx/json-patch.v5" "k8s.io/apimachinery/pkg/util/runtime" herrors "github.com/horizoncd/horizon/core/errors" @@ -48,7 +46,7 @@ type Request struct { } type Response struct { - Allowed *bool `json:"allowed,omitempty"` + Allowed *bool `json:"allowed"` Result string `json:"result,omitempty"` Patch []byte `json:"patch,omitempty"` PatchType string `json:"patchType,omitempty"` @@ -82,10 +80,16 @@ func Validating(ctx context.Context, request *Request) error { resCh <- validateResult{err, nil} return } - if response.Allowed != nil { - resCh <- validateResult{nil, response} + if response == nil || response.Allowed == nil { + if webhook.IgnoreError() { + log.Errorf(ctx, "failed to admit request: response is nil or allowed is nil") + resCh <- validateResult{nil, nil} + return + } + resCh <- validateResult{perror.New("response is nil or allowed is nil"), nil} return } + resCh <- validateResult{nil, response} }(webhook) } @@ -106,35 +110,3 @@ func Validating(ctx context.Context, request *Request) error { return nil } - -func jsonPatch(obj interface{}, patchJSON []byte) (interface{}, error) { - objJSON, err := json.Marshal(obj) - if err != nil { - return nil, err - } - patch, err := jsonpatch.DecodePatch(patchJSON) - if err != nil { - return nil, err - } - - objPatched, err := patch.Apply(objJSON) - if err != nil { - return nil, err - } - err = json.Unmarshal(objPatched, &obj) - if err != nil { - return nil, err - } - return obj, nil -} - -func loggingError(ctx context.Context, err error, webhook Webhook) error { - if err != nil { - if webhook.IgnoreError() { - log.Errorf(ctx, "failed to admit request: %v", err) - return nil - } - return err - } - return err -} diff --git a/pkg/admission/webhook_test.go b/pkg/admission/webhook_test.go new file mode 100644 index 00000000..5022d909 --- /dev/null +++ b/pkg/admission/webhook_test.go @@ -0,0 +1,139 @@ +package admission + +import ( + "context" + "testing" + + clusterctrl "github.com/horizoncd/horizon/core/controller/cluster" + "github.com/horizoncd/horizon/pkg/admission/models" + codemodels "github.com/horizoncd/horizon/pkg/cluster/code" + admissionconfig "github.com/horizoncd/horizon/pkg/config/admission" + tagmodels "github.com/horizoncd/horizon/pkg/tag/models" + "github.com/stretchr/testify/assert" +) + +func TestWebhook(t *testing.T) { + ctx := context.Background() + + server := NewDummyWebhookServer() + defer server.Stop() + validatingURL := server.ValidatingURL() + + config := admissionconfig.Admission{ + Webhooks: []admissionconfig.Webhook{ + { + Name: "test1", + Kind: models.KindValidating, + FailurePolicy: admissionconfig.FailurePolicyFail, + TimeoutSeconds: 5, + Rules: []admissionconfig.Rule{ + { + Resources: []string{ + "applications/clusters", + }, + Operations: []models.Operation{ + models.OperationCreate, + }, + Versions: []string{"v2"}, + }, + }, + ClientConfig: admissionconfig.ClientConfig{ + URL: validatingURL, + }, + }, + { + Name: "test2", + Kind: models.KindValidating, + FailurePolicy: admissionconfig.FailurePolicyIgnore, + TimeoutSeconds: 5, + Rules: []admissionconfig.Rule{ + { + Resources: []string{ + "clusters", + }, + Operations: []models.Operation{ + models.OperationUpdate, + }, + Versions: []string{"v2"}, + }, + }, + ClientConfig: admissionconfig.ClientConfig{ + URL: validatingURL, + }, + }, + }, + } + NewHTTPWebhooks(config) + + createBody := clusterctrl.CreateClusterRequestV2{ + Name: "cluster-1", + Description: "xxx", + Priority: "P0", + Git: &codemodels.Git{ + URL: "https://github.com/horizoncd/horizon.git", + Branch: "main", + }, + TemplateInfo: &codemodels.TemplateInfo{ + Name: "javaapp", + Release: "v1.0.0", + }, + } + + createRequest := &Request{ + Operation: models.OperationCreate, + Resource: "applications", + ResourceName: "1", + SubResource: "clusters", + Version: "v2", + Object: createBody, + OldObject: nil, + Options: map[string]interface{}{ + "scope": []string{"online/hz1"}, + }, + } + err := Validating(ctx, createRequest) + assert.NoError(t, err) + + createBody.Name = "cluster-invalid" + createRequest.Object = createBody + err = Validating(ctx, createRequest) + assert.Error(t, err) + t.Logf("error: %v", err) + + updateBody := clusterctrl.UpdateClusterRequestV2{ + Description: "yyy", + Tags: tagmodels.TagsBasic{ + { + Key: "k1", + Value: "v1", + }, + { + Key: "scope", + Value: "online/hz1", + }, + }, + } + updateRequest := &Request{ + Operation: models.OperationUpdate, + Resource: "clusters", + ResourceName: "1", + SubResource: "", + Version: "v2", + Object: updateBody, + OldObject: nil, + Options: nil, + } + err = Validating(ctx, updateRequest) + assert.NoError(t, err) + + updateBody.Tags = tagmodels.TagsBasic{ + { + Key: "k1", + Value: "v1", + }, + } + updateRequest.Object = updateBody + err = Validating(ctx, updateRequest) + assert.Error(t, err) + t.Logf("error: %v", err) +}