feature: add rules for type validation #2394

zhuangqh · 2018-10-31T08:32:13Z

Signed-off-by: zhuangqh zhuangqhc@gmail.com

Ⅰ. Describe what this PR did

Add rules to validate post request body.

Parameter	type	pattern
StopTimeout	*int64	>= 0

Parameter	type	pattern
ExecCreateConfig.Cmd	[]string	len > 0

Parameter	type	pattern
HostPort	string	^([1-9]\|[1-9]\d{1,3}\|[1-5]\d{4}\|6[0-4]\d{3}\|65[0-4]\d{2}\|655[0-2]\d\|6553[0-5])$ (0-65535)

Ⅱ. Does this pull request fix one issue?

fixes #443

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

added, but not complete. I would like to invite volunteers to complete test.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov · 2018-10-31T08:32:34Z

Codecov Report

Merging #2394 into master will increase coverage by 0.24%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #2394      +/-   ##
==========================================
+ Coverage   68.35%   68.59%   +0.24%     
==========================================
  Files         272      272              
  Lines       18206    18210       +4     
==========================================
+ Hits        12445    12492      +47     
+ Misses       4332     4302      -30     
+ Partials     1429     1416      -13

Flag	Coverage Δ
#criv1alpha1test	`31.39% <0%> (+0.11%)`	⬆️
#criv1alpha2test	`35.65% <0%> (+0.21%)`	⬆️
#integrationtest	`40.01% <33.33%> (+0.13%)`	⬆️
#nodee2etest	`32.95% <0%> (+0.22%)`	⬆️
#unittest	`26.44% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
apis/opts/labels.go	`100% <ø> (+12.5%)`	⬆️
apis/server/system_bridge.go	`47.77% <0%> (-2.23%)`	⬇️
apis/server/container_bridge.go	`87.42% <100%> (+0.07%)`	⬆️
daemon/mgr/container_validation.go	`42.5% <0%> (-2.5%)`	⬇️
pkg/streams/utils.go	`89.28% <0%> (-2.39%)`	⬇️
cri/v1alpha2/cri_wrapper.go	`63.6% <0%> (ø)`	⬆️
cri/v1alpha2/cri_utils.go	`91.21% <0%> (+0.29%)`	⬆️
cri/v1alpha1/cri.go	`60.59% <0%> (+0.33%)`	⬆️
daemon/mgr/container.go	`59% <0%> (+0.64%)`	⬆️
... and 9 more

allencloud · 2018-10-31T08:33:21Z

feels so good to see it! 😍

apis/swagger.yml

zhuangqh · 2018-10-31T11:06:21Z

@rudyfly please double check these rules.

fuweid · 2018-11-01T01:44:22Z

I think that there is no limit of Labels. Do we have any requirement for this kind of validation?

allencloud · 2018-11-02T06:04:41Z

apis/server/container_bridge.go

@@ -325,6 +325,10 @@ func (s *Server) updateContainer(ctx context.Context, rw http.ResponseWriter, re
 	if err := json.NewDecoder(reader).Decode(config); err != nil {
 		return httputils.NewHTTPError(err, http.StatusBadRequest)
 	}
+	// validate request body
+	if err := config.Validate(strfmt.NewFormats()); err != nil {


I think this validation should be before plugin point. WDYT?

Any feedback on this?

In my opinion, the validation should be immediately before ContainerMgr.Update to promise
valid types like a sentinel, that's enough. @allencloud

I got your point. Agree with you. @zhuangqh

allencloud · 2018-11-04T04:23:54Z

apis/swagger.yml

@@ -3172,11 +3173,13 @@ definitions:
        description: "The new volume's name. If not specified, Pouch generates a name."
        type: "string"
        x-nullable: false
+        pattern: ^[a-zA-Z0-9][a-zA-Z0-9_\-.]{0,63}$


Here we need @rudyfly to take a review of this to validate if the restriction rules is reasonable for volume management

allencloud · 2018-11-04T04:24:57Z

apis/swagger.yml

@@ -3909,8 +3913,9 @@ definitions:
        type: "string"
        example: "127.0.0.1"
      HostPort:
-        description: "Host port number that the container's port is mapped to."
+        description: "Host port number that the container's port is mapped to. range [0,65535]"


Is the port 0 is valid? I am it should be (0,65535].
WDYT

has updated

allencloud · 2018-11-04T04:27:39Z

apis/swagger.yml

@@ -3909,8 +3913,9 @@ definitions:
        type: "string"
        example: "127.0.0.1"
      HostPort:
-        description: "Host port number that the container's port is mapped to."
+        description: "Host port number that the container's port is mapped to. range [0,65535]"
        type: "string"


Although it is strange to use string type to describe port, it is compatible with Moby.
Just comment to record this.

Signed-off-by: zhuangqh <zhuangqhc@gmail.com>

zhuangqh · 2018-11-05T09:40:22Z

@allencloud after discussing with @rudyfly . I have removed all of the rules related to volume or label.

allencloud · 2018-11-05T13:58:56Z

LGTM

pouchrobot added kind/feature size/XL labels Oct 31, 2018

zhuangqh force-pushed the feature-type-validation branch from 3312a22 to 6136667 Compare October 31, 2018 08:38

starnop reviewed Oct 31, 2018

View reviewed changes

apis/swagger.yml Outdated Show resolved Hide resolved

zhuangqh force-pushed the feature-type-validation branch from 6136667 to 504e13b Compare October 31, 2018 09:45

zhuangqh force-pushed the feature-type-validation branch from 504e13b to b192d7a Compare October 31, 2018 13:15

zhuangqh force-pushed the feature-type-validation branch 3 times, most recently from 56e8d07 to afd8571 Compare November 1, 2018 08:33

allencloud reviewed Nov 2, 2018

View reviewed changes

zhuangqh mentioned this pull request Nov 2, 2018

[bug] volume name should be validated before Create #2416

Closed

allencloud reviewed Nov 4, 2018

View reviewed changes

pouchrobot added the conflict/needs-rebase label Nov 4, 2018

zhuangqh requested a review from rudyfly November 4, 2018 12:09

zhuangqh force-pushed the feature-type-validation branch from afd8571 to b037706 Compare November 4, 2018 12:17

pouchrobot removed the conflict/needs-rebase label Nov 4, 2018

feature: add rules for type validation

3a66d8d

Signed-off-by: zhuangqh <zhuangqhc@gmail.com>

zhuangqh force-pushed the feature-type-validation branch from b037706 to 3a66d8d Compare November 5, 2018 09:38

pouchrobot added size/L and removed size/XL labels Nov 5, 2018

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 5, 2018

allencloud merged commit 2b5fad7 into AliyunContainerService:master Nov 5, 2018

pouchrobot mentioned this pull request Nov 9, 2018

WeeklyReport of PouchContainer from 2018-11-02 to 2018-11-09 #2457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add rules for type validation #2394

feature: add rules for type validation #2394

zhuangqh commented Oct 31, 2018 •

edited

codecov bot commented Oct 31, 2018 •

edited

allencloud commented Oct 31, 2018

zhuangqh commented Oct 31, 2018

fuweid commented Nov 1, 2018

allencloud Nov 2, 2018

allencloud Nov 4, 2018

zhuangqh Nov 4, 2018

allencloud Nov 5, 2018

allencloud Nov 4, 2018

allencloud Nov 4, 2018

zhuangqh Nov 4, 2018

allencloud Nov 4, 2018

zhuangqh commented Nov 5, 2018

allencloud commented Nov 5, 2018

feature: add rules for type validation #2394

feature: add rules for type validation #2394

Conversation

zhuangqh commented Oct 31, 2018 • edited

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Oct 31, 2018 • edited

Codecov Report

allencloud commented Oct 31, 2018

zhuangqh commented Oct 31, 2018

fuweid commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuangqh commented Nov 5, 2018

allencloud commented Nov 5, 2018

zhuangqh commented Oct 31, 2018 •

edited

codecov bot commented Oct 31, 2018 •

edited