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

bugfix: failed to validate namespace value #2393

Closed
wants to merge 2 commits into from
Closed

bugfix: failed to validate namespace value #2393

wants to merge 2 commits into from

Conversation

Ace-Tang
Copy link
Contributor

Ⅰ. Describe what this PR did

namespace related parameters not get validated, user should know
the valid value and invalid value when they specify the related
parameters.
remove repeated code in network namespace

Ⅱ. Does this pull request fix one issue?

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

add namespace related tests for run command

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Oct 31, 2018

Codecov Report

Merging #2393 into master will decrease coverage by 0.05%.
The diff coverage is 78%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2393      +/-   ##
=========================================
- Coverage   68.45%   68.4%   -0.06%     
=========================================
  Files         275     276       +1     
  Lines       18291   18283       -8     
=========================================
- Hits        12522   12507      -15     
- Misses       4340    4346       +6     
- Partials     1429    1430       +1
Flag Coverage Δ
#criv1alpha1test 31.95% <67%> (+0.13%) ⬆️
#criv1alpha2test 35.85% <67%> (+0.02%) ⬆️
#integrationtest 39.84% <60%> (+0.07%) ⬆️
#nodee2etest 33.29% <61%> (+0.01%) ⬆️
#unittest 25.55% <0%> (+0.02%) ⬆️
Impacted Files Coverage Δ
daemon/mgr/network_utils.go 17.85% <ø> (-19.36%) ⬇️
daemon/mgr/spec_linux.go 77.96% <100%> (+3.44%) ⬆️
daemon/mgr/container.go 59.37% <36.36%> (-0.22%) ⬇️
daemon/mgr/network.go 69.61% <50%> (ø) ⬆️
pkg/namespace/namespace.go 76% <76%> (ø)
daemon/mgr/container_validation.go 47.23% <83.33%> (+2.23%) ⬆️
daemon/mgr/spec_process.go 67.01% <0%> (-12.99%) ⬇️
daemon/containerio/cri_log_file.go 84.31% <0%> (-3.93%) ⬇️
apis/server/utils.go 71.15% <0%> (-3.85%) ⬇️
pkg/meta/store.go 62.5% <0%> (-3.66%) ⬇️
... and 26 more

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XL labels Oct 31, 2018
return fmt.Errorf("invalid uts namespace mode %s", hostConfig.UTSMode)
}

if !ns.Valid(specs.PIDNamespace, hostConfig.IpcMode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

PIDNamespace or IPCNamespace ? So do that with UTS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Big Bug, thanks!

@Ace-Tang
Copy link
Contributor Author

related to issue #2378

@Ace-Tang
Copy link
Contributor Author

@rudyfly , could you take a look at network part ?

namespace related parameters not get validated, user should know
the valid value and invalid value when they specify the related
parameters.
remove repeated code in network namespace

Signed-off-by: Ace-Tang <[email protected]>
@pouchrobot
Copy link
Collaborator

ping @Ace-Tang
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

@fuweid
Copy link
Contributor

fuweid commented Jun 2, 2019

please reopen it if you need

@fuweid fuweid closed this Jun 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict/needs-rebase kind/bug This is bug report for project size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants