Skip to content

Commit

Permalink
fix: review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: xu.zhu <[email protected]>
  • Loading branch information
@xuzhu-591
xuzhu-591 committed Jan 5, 2024
1 parent b2ec543 commit f8e5e61
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 66 deletions.
22 changes: 13 additions & 9 deletions core/middleware/admission/admission.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,21 @@ func Middleware(skippers ...middleware.Skipper) gin.HandlerFunc {
rpcerror.ParamError.WithErrMsg(fmt.Sprintf("request body is invalid, err: %v", err)))
return
}
// restore the request body
c.Request.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes))
if len(bodyBytes) > 0 {
contentType := c.ContentType()
if contentType == binding.MIMEJSON {
if contentType == binding.MIMEJSON || contentType == "" {
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)
log.Errorf(c, "unsupported content type: %s", contentType)
response.AbortWithRPCError(c,
rpcerror.ParamError.WithErrMsg(fmt.Sprintf("unsupported content type: %s", contentType)))
return
}
}
// fill in the request url query into admission request options
Expand All @@ -62,13 +66,13 @@ func Middleware(skippers ...middleware.Skipper) gin.HandlerFunc {
options[k] = v
}
admissionRequest := &admissionwebhook.Request{
Operation: admissionmodels.Operation(attr.GetVerb()),
Resource: attr.GetResource(),
ResourceName: attr.GetName(),
SubResource: attr.GetSubResource(),
Version: attr.GetAPIVersion(),
Object: object,
Options: options,
Operation: admissionmodels.Operation(attr.GetVerb()),
Resource: attr.GetResource(),
Name: attr.GetName(),
SubResource: attr.GetSubResource(),
Version: attr.GetAPIVersion(),
Object: object,
Options: options,
}
if err := admissionwebhook.Validating(c, admissionRequest); err != nil {
response.AbortWithRPCError(c,
Expand Down
9 changes: 7 additions & 2 deletions pkg/admission/http.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"github.com/horizoncd/horizon/pkg/util/common"
)

const DefaultTimeout = 5 * time.Second

type HTTPAdmissionClient struct {
config config.ClientConfig
http.Client
Expand All @@ -45,6 +47,9 @@ func NewHTTPAdmissionClient(config config.ClientConfig, timeout time.Duration) *
},
}
}
if timeout == 0 {
timeout = DefaultTimeout
}
return &HTTPAdmissionClient{
config: config,
Client: http.Client{
Expand All @@ -61,7 +66,7 @@ func (c *HTTPAdmissionClient) Get(ctx context.Context, admitData *Request) (*Res
return nil, err
}

req, err := http.NewRequest("POST", c.config.URL, bytes.NewReader(body))
req, err := http.NewRequestWithContext(ctx, "POST", c.config.URL, bytes.NewReader(body))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -183,7 +188,7 @@ func NewHTTPWebhooks(config config.Admission) {
}

func NewHTTPWebhook(config config.Webhook) Webhook {
client := NewHTTPAdmissionClient(config.ClientConfig, time.Duration(config.TimeoutSeconds)*time.Second)
client := NewHTTPAdmissionClient(config.ClientConfig, config.Timeout)
matchers := NewResourceMatchers(config.Rules)
return &HTTPAdmissionWebhook{
config: config,
Expand Down
47 changes: 21 additions & 26 deletions pkg/admission/webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package admission

import (
"context"
"time"

"k8s.io/apimachinery/pkg/util/runtime"

Expand All @@ -12,8 +11,6 @@ import (
"github.com/horizoncd/horizon/pkg/util/log"
)

const DefaultTimeout = 5 * time.Second

var (
validatingWebhooks []Webhook
)
Expand All @@ -25,31 +22,26 @@ func Register(kind models.Kind, webhook Webhook) {
}
}

func Clear() {
validatingWebhooks = nil
}

type validateResult struct {
req Request
err error
resp *Response
}

type Request struct {
Operation models.Operation `json:"operation"`
Resource string `json:"resource"`
ResourceName string `json:"resourceName"`
SubResource string `json:"subResource"`
Version string `json:"version"`
Object interface{} `json:"object"`
OldObject interface{} `json:"oldObject"`
Options map[string]interface{} `json:"options,omitempty"`
Operation models.Operation `json:"operation"`
Resource string `json:"resource"`
Name string `json:"name"`
SubResource string `json:"subResource"`
Version string `json:"version"`
Object interface{} `json:"object"`
OldObject interface{} `json:"oldObject"`
Options map[string]interface{} `json:"options,omitempty"`
}

type Response struct {
Allowed *bool `json:"allowed"`
Result string `json:"result,omitempty"`
Patch []byte `json:"patch,omitempty"`
PatchType string `json:"patchType,omitempty"`
Allowed *bool `json:"allowed"`
Result string `json:"result,omitempty"`
}

type Webhook interface {
Expand All @@ -67,29 +59,29 @@ func Validating(ctx context.Context, request *Request) error {
go func(webhook Webhook) {
defer runtime.HandleCrash()
if !webhook.Interest(request) {
resCh <- validateResult{nil, nil}
resCh <- validateResult{*request, nil, nil}
return
}
response, err := webhook.Handle(ctx, request)
if err != nil {
if webhook.IgnoreError() {
log.Errorf(ctx, "failed to admit request: %v", err)
resCh <- validateResult{nil, nil}
resCh <- validateResult{*request, nil, nil}
return
}
resCh <- validateResult{err, nil}
resCh <- validateResult{*request, err, nil}
return
}
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}
resCh <- validateResult{*request, nil, nil}
return
}
resCh <- validateResult{perror.New("response is nil or allowed is nil"), nil}
resCh <- validateResult{*request, perror.New("response is nil or allowed is nil"), nil}
return
}
resCh <- validateResult{nil, response}
resCh <- validateResult{*request, nil, response}
}(webhook)
}

Expand All @@ -99,7 +91,10 @@ func Validating(ctx context.Context, request *Request) error {
return res.err
}
if res.resp != nil && res.resp.Allowed != nil && !*res.resp.Allowed {
log.Infof(ctx, "request denied by webhook: %s", res.resp.Result)
log.Infof(ctx,
"request (resource: %s, resourceName: %s, subresource: %s, operation: %s) denied by webhook: %s",
res.req.Resource, res.req.Name, res.req.SubResource,
res.req.Operation, res.resp.Result)
return perror.Wrapf(herrors.ErrForbidden, "request denied by webhook: %s", res.resp.Result)
}
if finishedCount >= len(validatingWebhooks) {
Expand Down
45 changes: 22 additions & 23 deletions pkg/admission/webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package admission
import (
"context"
"testing"
"time"

clusterctrl "github.com/horizoncd/horizon/core/controller/cluster"
"github.com/horizoncd/horizon/pkg/admission/models"
Expand All @@ -22,10 +23,9 @@ func TestWebhook(t *testing.T) {
config := admissionconfig.Admission{
Webhooks: []admissionconfig.Webhook{
{
Name: "test1",
Kind: models.KindValidating,
FailurePolicy: admissionconfig.FailurePolicyFail,
TimeoutSeconds: 5,
Kind: models.KindValidating,
FailurePolicy: admissionconfig.FailurePolicyFail,
Timeout: 5 * time.Second,
Rules: []admissionconfig.Rule{
{
Resources: []string{
Expand All @@ -42,10 +42,9 @@ func TestWebhook(t *testing.T) {
},
},
{
Name: "test2",
Kind: models.KindValidating,
FailurePolicy: admissionconfig.FailurePolicyIgnore,
TimeoutSeconds: 5,
Kind: models.KindValidating,
FailurePolicy: admissionconfig.FailurePolicyIgnore,
Timeout: 5 * time.Second,
Rules: []admissionconfig.Rule{
{
Resources: []string{
Expand Down Expand Up @@ -80,13 +79,13 @@ func TestWebhook(t *testing.T) {
}

createRequest := &Request{
Operation: models.OperationCreate,
Resource: "applications",
ResourceName: "1",
SubResource: "clusters",
Version: "v2",
Object: createBody,
OldObject: nil,
Operation: models.OperationCreate,
Resource: "applications",
Name: "1",
SubResource: "clusters",
Version: "v2",
Object: createBody,
OldObject: nil,
Options: map[string]interface{}{
"scope": []string{"online/hz1"},
},
Expand Down Expand Up @@ -114,14 +113,14 @@ func TestWebhook(t *testing.T) {
},
}
updateRequest := &Request{
Operation: models.OperationUpdate,
Resource: "clusters",
ResourceName: "1",
SubResource: "",
Version: "v2",
Object: updateBody,
OldObject: nil,
Options: nil,
Operation: models.OperationUpdate,
Resource: "clusters",
Name: "1",
SubResource: "",
Version: "v2",
Object: updateBody,
OldObject: nil,
Options: nil,
}
err = Validating(ctx, updateRequest)
assert.NoError(t, err)
Expand Down
12 changes: 6 additions & 6 deletions pkg/config/admission/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package admission

import (
"strings"
"time"

"github.com/horizoncd/horizon/pkg/admission/models"
)
Expand Down Expand Up @@ -30,12 +31,11 @@ type Rule struct {
}

type Webhook struct {
Name string `yaml:"name"`
Kind models.Kind `yaml:"kind"`
FailurePolicy FailurePolicy `yaml:"failurePolicy"`
TimeoutSeconds int32 `yaml:"timeoutSeconds"`
Rules []Rule `yaml:"rules"`
ClientConfig ClientConfig `yaml:"clientConfig"`
Kind models.Kind `yaml:"kind"`
FailurePolicy FailurePolicy `yaml:"failurePolicy"`
Timeout time.Duration `yaml:"timeout"`
Rules []Rule `yaml:"rules"`
ClientConfig ClientConfig `yaml:"clientConfig"`
}

type Admission struct {
Expand Down

0 comments on commit f8e5e61

Please sign in to comment.