validate: optimize capabilites check #220

Mashimiao · 2016-09-23T08:15:41Z

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

By now, if we generate with option --priviliged, the validation of bundle will fail.
capabilities need to be more than original defaultCap has.

wking · 2016-09-28T04:10:45Z

cmd/oci-runtime-tool/validate.go

-		if !capValid(capability) {
-			msgs = append(msgs, fmt.Sprintf("capability %q is not valid, man capabilities(7)", process.Capabilities[index]))
+		if !capValid(capability, hostCheck) {
+			msgs = append(msgs, fmt.Sprintf("capability %q is not valid, man capabilities(7)", capability))


Complaining by capability (and not by index) makes sense to me. But now that we're doing that, I think we should replace the current for loop with:

for _, capability := range process.Capabilities {

fixed, thanks.

wking · 2016-09-28T04:23:04Z

cmd/oci-runtime-tool/validate.go

+
+    for _, cap := range capability.List() {
+        if cp == fmt.Sprintf("CAP_%s", strings.ToUpper(cap.String())) {
+            if hostSpecific && cap > lastCap() {


I'm not sure List is the right logic for host-agnostic checks. The docs say:

List returns list of all supported capabilities

What we want here is a list of all possible capabilities. But maybe I'm just misreading the docs. Does someone have an old kernel around to test this on?

wking · 2016-09-28T04:25:35Z

cmd/oci-runtime-tool/validate.go

 	return false
 }

+func lastCap() capability.Cap {


I don't like duplicating code between the generate and validate libraries. Validation seems more fundamental to me, so can we make a public LastCap function here and import that into the generate library?

It looks like your new capValid code is also coming over from generate. Can we move all of that to a public validate function and import that into the generate library as well?

I also don't like duplicating code. @liangchenye is working on a new validate module at #188 .
Maybe we can made a cleanup after that merged or wait until it merged?

I think #188 is ready to land, so I'd rather idle here.

Mashimiao · 2016-10-20T05:12:05Z

@wking @mrunalp @liangchenye rebased.

wking · 2016-10-20T07:53:53Z

generate/generate.go

@@ -679,12 +679,12 @@ func lastCap() capability.Cap {
 	return last
 }

-func checkCap(c string, hostSpecific bool) error {
+func ValidCap(c string, hostSpecific bool) error {
 	isValid := false
 	cp := strings.ToUpper(c)


I think we can drop this. The spec says nothing about case-insensitivity, and the values listed in the referenced man page are uppercase.

wking · 2016-10-20T07:55:07Z

generate/generate.go

@@ -694,7 +694,7 @@ func checkCap(c string, hostSpecific bool) error {
 	}

 	if !isValid {
-		return fmt.Errorf("Invalid value passed for adding capability")
+		return fmt.Errorf("Invalid capability value: %s", cp)


Do we need “value”? I'd rather have Invalid capability: %q.

wking · 2016-10-20T07:56:44Z

generate/generate.go

@@ -709,12 +709,11 @@ func (g *Generator) ClearProcessCapabilities() {

 // AddProcessCapability adds a process capability into g.spec.Process.Capabilities.
 func (g *Generator) AddProcessCapability(c string) error {
-	if err := checkCap(c, g.HostSpecific); err != nil {
+	cp := fmt.Sprintf("CAP_%s", strings.ToUpper(c))


Why are we adding the CAP_ prefix here? I'd rather require AddProcessCapability callers give us a valid string without massaging it internally.

wking · 2016-10-20T07:59:12Z

generate/generate.go

@@ -679,12 +679,12 @@ func lastCap() capability.Cap {
 	return last
 }

-func checkCap(c string, hostSpecific bool) error {
+func ValidCap(c string, hostSpecific bool) error {


I think ValidCap should live in validate/ and generate.go should import the validate package.

Fine, fixed.

wking · 2016-10-20T08:02:01Z

validate/validate.go

-		if process.Rlimits[index].Hard < process.Rlimits[index].Soft {
-			msgs = append(msgs, fmt.Sprintf("hard limit of rlimit %s should not be less than soft limit.", process.Rlimits[index].Type))
+		if rlimit.Hard < rlimit.Soft {
+			msgs = append(msgs, fmt.Sprintf("hard limit of rlimit %s should not be less than soft limit.", rlimit.Type))


rlimitValid is an overly-generic name if it only validates the type. I'd recommend renaming it to validRlimitType or moving the hard/soft limit checks inside validRlimit.

move hard/soft limit check into rlimitValid

wking

Looking pretty good, just a few remaining nits.

wking · 2016-10-22T07:16:30Z

validate/validate.go

@@ -123,6 +129,8 @@ func (v *Validator) CheckRootfsPath() (msgs []string) {
 	return

 }
+
+// CheckSemVer checks v.sepc.Version


“sepc” → “spec”

wking · 2016-10-22T07:17:05Z

validate/validate.go

@@ -203,6 +213,7 @@ func checkEventHooks(hookType string, hooks []rspec.Hook, hostSpecific bool) (ms
 	return
 }

+// CheckProcess checks v.spec.process


“process” → “Process”.

wking · 2016-10-22T07:22:48Z

validate/validate.go

+		if val == rlimit.Type {
+			if rlimit.Hard < rlimit.Soft {
+				return fmt.Errorf("hard limit of rlimit %s should not be less than soft limit", rlimit.Type)
+			}


I'd rather not bury the hard/soft comparison inside the type check. I'm fine checking this before the type, because “hard < soft” is always wrong, but “type not in defaultRlimits” could be “runtime-tools maintainers haven't taught runtime-tools about the new rlimit”. If you really want the type check first, you could do something like:

found := false for … { if val == rlimit.Type { found = true break } } if !found { return fmt.Errorf("rlimit type %q is invalid", rlimit.Type) }

Mashimiao · 2016-10-25T18:21:27Z

On 10/22/2016 03:23 PM, W. Trevor King wrote:

I'd rather not bury the hard/soft comparison inside the type check. I'm fine checking this before the type, because “hard < soft” is /always/ wrong, but “type not in |defaultRlimits|” could be “runtime-tools maintainers haven't taught runtime-tools about the new rlimit”. If you really want the type check first, you could do something like:

I'd like do type check first.
I think if type was invalid, hard/soft check doesn't make any sense.

|found := false for … { if val == rlimit.Type { found = true break } } if !found { return fmt.Errorf("rlimit type %q is invalid", rlimit.Type) } |

Maybe I didn't understand your means.
I think the above code has no difference with current code except lack of hard/soft check.

wking · 2016-10-25T18:41:29Z

On Tue, Oct 25, 2016 at 11:21:28AM -0700, Ma Shimiao wrote:

I'd like do type check first. I think if type was invalid,
hard/soft check doesn't make any sense.

But runtime-tools may think the type is invalid when it really isn't
1.

I think the above code has no difference with current code except
lack of hard/soft check.

Exactly. That lets you handle the hard/soft check independently after
passing through the type-checking loop. Separating the checks inside
rlimitValid makes it more clear that the type and hard/soft checks
are completely orthogonal.

Mashimiao · 2016-10-26T08:53:13Z

Fixed. @wking PTAL

wking · 2016-10-26T16:35:43Z

a73a5e8 looks good to me, although it would be better to have squashed that change into the “optimize capabilites check” commit instead of putting it in the “add comments for exported functions” commit.

Mashimiao · 2016-11-08T03:30:29Z

ping @opencontainers/runtime-tools-maintainers

liangchenye · 2017-01-03T18:47:01Z

LGTM

Mashimiao · 2017-02-03T08:32:36Z

@mrunalp @hqhq PTAL

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

Mashimiao · 2017-02-14T01:54:26Z

Rebased. @opencontainers/runtime-tools-maintainers PTAL

Mashimiao · 2017-02-23T02:48:02Z

@hqhq @mrunalp @liangchenye PTAL

hqhq · 2017-03-06T13:44:05Z

LGTM

hqhq · 2017-03-06T13:44:46Z

ping @liangchenye needs re-LGTM.

liangchenye · 2017-03-06T14:24:19Z

LGTM

wking reviewed Sep 28, 2016

View reviewed changes

Mashimiao force-pushed the validate-optimize-cap-check branch from 9311515 to 409b189 Compare September 28, 2016 05:37

Mashimiao force-pushed the validate-optimize-cap-check branch from 409b189 to 6a29a11 Compare October 20, 2016 03:30

wking reviewed Oct 20, 2016

View reviewed changes

Mashimiao force-pushed the validate-optimize-cap-check branch 2 times, most recently from dd8124d to d2b72fc Compare October 20, 2016 09:10

wking reviewed Oct 22, 2016

View reviewed changes

Mashimiao force-pushed the validate-optimize-cap-check branch from d2b72fc to 9e28f93 Compare October 24, 2016 02:40

Mashimiao force-pushed the validate-optimize-cap-check branch from 9e28f93 to a73a5e8 Compare October 26, 2016 08:53

Ma Shimiao added 2 commits February 14, 2017 09:51

validate: optimize capabilites check

1a899a6

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

add comments for exported functions in validate

73680b2

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

Mashimiao force-pushed the validate-optimize-cap-check branch from a73a5e8 to 73680b2 Compare February 14, 2017 01:53

Mashimiao mentioned this pull request Feb 22, 2017

validate: completion capabilities #328

Closed

Mashimiao merged commit 9539bef into opencontainers:master Mar 6, 2017

zhouhao3 mentioned this pull request Apr 13, 2017

validate: delete the extra defaultCaps #363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate: optimize capabilites check #220

validate: optimize capabilites check #220

Mashimiao commented Sep 23, 2016 •

edited

Loading

wking Sep 28, 2016

Mashimiao Sep 28, 2016

wking Sep 28, 2016

wking Sep 28, 2016

wking Sep 28, 2016

Mashimiao Sep 28, 2016

wking Sep 28, 2016

Mashimiao commented Oct 20, 2016

wking Oct 20, 2016

wking Oct 20, 2016

Mashimiao Oct 20, 2016

wking Oct 20, 2016

Mashimiao Oct 20, 2016

wking Oct 20, 2016

Mashimiao Oct 20, 2016

wking Oct 20, 2016

Mashimiao Oct 20, 2016

wking left a comment

wking Oct 22, 2016

wking Oct 22, 2016

wking Oct 22, 2016

Mashimiao commented Oct 25, 2016

wking commented Oct 25, 2016

Mashimiao commented Oct 26, 2016

wking commented Oct 26, 2016 via email

Mashimiao commented Nov 8, 2016

liangchenye commented Jan 3, 2017 •

edited by caniszczyk

Loading

Mashimiao commented Feb 3, 2017

Mashimiao commented Feb 14, 2017

Mashimiao commented Feb 23, 2017

hqhq commented Mar 6, 2017 •

edited by caniszczyk

Loading

hqhq commented Mar 6, 2017

liangchenye commented Mar 6, 2017 •

edited by caniszczyk

Loading

validate: optimize capabilites check #220

validate: optimize capabilites check #220

Conversation

Mashimiao commented Sep 23, 2016 • edited Loading

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

Mashimiao commented Oct 20, 2016

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

Choose a reason for hiding this comment

wking left a 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

Mashimiao commented Oct 25, 2016

wking commented Oct 25, 2016

Mashimiao commented Oct 26, 2016

wking commented Oct 26, 2016 via email

Mashimiao commented Nov 8, 2016

liangchenye commented Jan 3, 2017 • edited by caniszczyk Loading

Mashimiao commented Feb 3, 2017

Mashimiao commented Feb 14, 2017

Mashimiao commented Feb 23, 2017

hqhq commented Mar 6, 2017 • edited by caniszczyk Loading

hqhq commented Mar 6, 2017

liangchenye commented Mar 6, 2017 • edited by caniszczyk Loading

Mashimiao commented Sep 23, 2016 •

edited

Loading

liangchenye commented Jan 3, 2017 •

edited by caniszczyk

Loading

hqhq commented Mar 6, 2017 •

edited by caniszczyk

Loading

liangchenye commented Mar 6, 2017 •

edited by caniszczyk

Loading