Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: add ExecConfig validation rule in swagger #453

Conversation

allencloud
Copy link
Collaborator

Signed-off-by: Allen Sun allensun.shl@alibaba-inc.com

1.Describe what this PR did

This PR restricts the validate type Post request config body.

  • I add ExecCreateConfig field restriction in swagger.yml;
  • generate structs in apis/types, in this struct file, there are validation funcs which takes restriction into consideration:
func (m *ExecCreateConfig) validateCmd(formats strfmt.Registry) error {

	if swag.IsZero(m.Cmd) { // not required
		return nil
	}

	iCmdSize := int64(len(m.Cmd))

	if err := validate.MinItems("Cmd", "body", iCmdSize, 1); err != nil {
		return err
	}

	return nil
}

When the above two aspects has been done, codes in https://github.com/alibaba/pouch/blob/master/apis/server/container_bridge.go#L56 will work.

	config := &types.ExecCreateConfig{}
	// decode request body
	if err := json.NewDecoder(req.Body).Decode(config); err != nil {
		return httputils.NewHTTPError(err, http.StatusBadRequest)
	}
	// validate request body
	if err := config.Validate(strfmt.NewFormats()); err != nil {
		return httputils.NewHTTPError(err, http.StatusBadRequest)
	}

2.Does this pull request fix one issue?
none, this is related to issue #443.

3.Describe how you did it
NONE

4.Describe how to verify it
We need to add test cases according to this PR in API POST /containers/{id}/exec test.
/cc @Letty5411

5.Special notes for reviews

Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
@codecov-io
Copy link

codecov-io commented Dec 28, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@adb1071). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #453   +/-   ##
========================================
  Coverage          ?   18.4%           
========================================
  Files             ?      34           
  Lines             ?    1793           
  Branches          ?       0           
========================================
  Hits              ?     330           
  Misses            ?    1428           
  Partials          ?      35

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adb1071...d948265. Read the comment docs.

@allencloud allencloud added this to the v0.1-Milestone milestone Dec 28, 2017
@Letty5411
Copy link
Contributor

@allencloud I'll add a nil CMD check for exec API.

@skoowoo
Copy link
Contributor

skoowoo commented Jan 2, 2018

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 2, 2018
@allencloud allencloud merged commit 6a2992e into AliyunContainerService:master Jan 2, 2018
@allencloud allencloud deleted the exec-create-body-validate branch January 2, 2018 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants