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: support validate volume create config #452

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 VolumeCreateConfig field restriction in swagger.yml;
  • generate structs in apis/types, in this struct file, there are validation funcs which takes restriction into consideration:

for example the name in VolumeCreateConfig validation:

func (m *VolumeCreateConfig) validateName(formats strfmt.Registry) error {

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

	if err := validate.MinLength("Name", "body", string(m.Name), 1); err != nil {
		return err
	}

	if err := validate.MaxLength("Name", "body", string(m.Name), 64); err != nil {
		return err
	}

	if err := validate.Pattern("Name", "body", string(m.Name), `[a-zA-Z0-9][a-zA-Z0-9-_.]{0,63}`); 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/volume_bridge.go#L23 will work.

	config := &types.VolumeCreateConfig{}
	// 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 /volumes/create test.
/cc @Letty5411

5.Special notes for reviews
NONE

@codecov-io
Copy link

codecov-io commented Dec 28, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #452   +/-   ##
=========================================
  Coverage          ?   18.04%           
=========================================
  Files             ?       35           
  Lines             ?     1790           
  Branches          ?        0           
=========================================
  Hits              ?      323           
  Misses            ?     1432           
  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 e080984...d911800. Read the comment docs.

@allencloud allencloud added this to the v0.1-Milestone milestone Dec 28, 2017
type: "object"
additionalProperties:
type: "string"
minLength: 1
maxLength: 128
pattern: "[a-zA-Z0-9][a-zA-Z0-9-_. ]{0,127}"
Copy link
Collaborator Author

@allencloud allencloud Dec 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is correct. Since in some environment, there are labels like "removeMounts": "/etc/entrypoint.sh" and "ContainerHnFormat": "aserver{{IpPad .ContainerIp}}.center.et2" . So we need to add /, {} and some others.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, / is necessary, need to add it.

@allencloud
Copy link
Collaborator Author

Moving it to milestone 0.3.

Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
@allencloud allencloud force-pushed the volume-create-body-validate branch from 30b207d to d911800 Compare March 29, 2018 09:19
@pouchrobot
Copy link
Collaborator

ping @allencloud

CI fails according integration system.
Please refer to the CI failure Details button to corresponding test, and update your PR to pass CI.

If this is flaky test, welcome to track this with profiling an issue.

build url: https://travis-ci.org/alibaba/pouch/builds/359753465
build duration: 158s

@pouchrobot
Copy link
Collaborator

ping @allencloud
We found that this PR is 737 commits, which is more than 10 commits, behind master.
Please rebase the branch against master and push it back again. Thanks a lot.

@allencloud allencloud closed this Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants