validate: add Hooks validation by Mashimiao · Pull Request #49 · opencontainers/runtime-tools

Mashimiao · 2016-04-28T08:33:34Z

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

wking · 2016-04-28T14:40:07Z

 	}
 }

+func checkHooks(process rspec.Hooks, rootfs string) {


process is a strange name for this variable. How about hooks? And hooks are in the host mount namespace, so I don't think we need rootfs.

@wking change process to hooks is fine.
hooks are in the host mount namespace I'm not sure what it means.
Do you mean hooks is not in OCI bundle?

On Thu, Apr 28, 2016 at 07:54:26PM -0700, Ma Shimiao wrote:
hooks are in the host mount namespace I'm not sure what it means.
Do you mean hooks is not in OCI bundle?
The may be in the bundle, but if they are, the path will be
/path/to/bundle/and/hook, not /and/hook. Specs are 1:
“Hook paths are absolute and are executed from the host's
filesystem.”
Although I'd be happier if the wording used “runtime namespace” 2 so
it was explicit about other namespaces as well (e.g. hooks are also in
the host network namepace, the host ipc namespace, …).

wking · 2016-04-28T16:32:54Z

This sort of validation would be easier if we could recycle the
process validator 1.

 Subject: Unify container-process and hook-process structures
Date: Mon, 4 Jan 2016 21:56:37 -0800
Message-ID: <20160105055637.GG6362@odin.tremily.us>

liangchenye · 2016-05-03T09:36:03Z

Thanks @Mashimiao,
if we want to check hook, we can just check hook.Path directly, there is no need to join the rootfs.

Hook paths are absolute and are executed from the host's filesystem.

Also, since the hook command exists in the host's filesystem, we can not check that in bundle validation.go. (But we reuse this code in runtime test.)

wking · 2016-05-03T16:29:05Z

On Tue, May 03, 2016 at 02:36:04AM -0700, 梁辰晔 (Liang Chenye) wrote:

Also, since the hook command exists in the host's filesystem, we can
not check that in bundle validation.go.

We can check “Does the hook command exist in the host's filesystem?”,
it's just that the check will not be portable. I think it's still
worth performing the check (e.g. see opencontainers/runtime-spec#418),
but it's probably worth putting that sort of thing behind a flag
(--host-checks?). I don't really have a preference for the flag name
or whether it's opt-in or opt-out.

Mashimiao · 2016-05-09T06:35:33Z

@wking @liangchenye
This validation, as @wking said we can check “Does the hook command exist in the host's filesystem?”.
config.json belongs to OCI bundle. I think we check hook command exist or not is to check whether config.json is configured correct or not, is to check OCI bundle.
@wking I'm not sure what portability problem exists. If there was no hooks configured, the hook validation would not be executed.

liangchenye · 2016-05-09T07:26:58Z

The hook configuration is a little tricky, it is not portable.
If it was verified on hostA, it could still fail on hostB.
If it fails on hostA, it is hard to say this is an invalid bundle.
But it is helpful to a user who tries to use it, so agree with WKing, we can add a flag to bundle validate.

Mashimiao · 2016-05-10T03:47:22Z

@wking @liangchenye PR updated

wking · 2016-05-10T04:29:53Z

 Path to bundle

+**--hooks-check**
+ Specify check hooks on host, default is false.


I'd have gone with --hook-paths or some such, but I expect folks will get the idea regardless. It's probably worth detailing what the hook checks are and why this is optional. The only check that needs to be toggle-able is “Does the path exist (and point to an executable)?”, and there is other validation that you can do beyond that (e.g. “Are there equals signs in all the env values?”).

Mashimiao · 2016-05-16T01:14:34Z

ping @liangchenye

wking · 2016-05-16T04:33:35Z

 **ocitools validate** *[OPTIONS]*
+[**--help**]
+[**--path**[=*PATH*]
+[**--hooks-check**[=*true*|*false*]


Listing specific options here rolls back part of c1df0ce (Fixup man pages for consistency, 2016-05-07, #65). I like the shorter [OPTIONS] for generate (see #42) because it has lots of options. I don't care either way for validate, but we probably want to pick one approach and not flip back and forth without motivation ;).

liangchenye · 2016-05-17T06:09:47Z

Thanks @Mashimiao

LGTM

wking · 2016-05-17T06:54:05Z

Ah, I'd missed the responses to my earlier comments. f54700d looks good to me too.

Mashimiao · 2016-05-19T07:57:09Z

ping @mrunalp

liangchenye · 2016-05-20T03:59:21Z

Hi @Mashimiao,
I added another task to #66
'Hooks paths are absolute'.
It is different but related to this PR. We need to check if a path is absolute by default (without hooksCheck set to true). If you like to implement it (in this PR maybe), I'll add a WIP link.

Mashimiao · 2016-05-20T06:21:05Z

@liangchenye OK, I can implement it in this PR.
implemented, please review.

wking · 2016-05-20T07:09:56Z

51d1cc6 still looks good to me.

wking · 2016-05-20T07:11:54Z

Ah, actually 51d1cc6 does not look good to me ;). The absolute-path
check needs to happen regardless of whether folks have set the
host-specific switch 1. Maybe we want to lump all of the
host-specific checks behind a --host-specific option?

Mashimiao · 2016-05-20T08:30:19Z

@wking Ah, yes. I did not read @liangchenye 's words carefully. How about now?

wking · 2016-05-20T18:28:32Z

On Fri, May 20, 2016 at 01:30:20AM -0700, Ma Shimiao wrote:

How about now?

ce49f78 looks great :).

mrunalp · 2016-05-20T22:37:21Z

 Path to bundle

+**--hooks**
+ Specify check hooks exists and executable on host, default is false.


nit: Check specified hooks exist and are executable on the host.

@mrunalp fixed.

mrunalp · 2016-05-23T16:26:58Z


 var bundleValidateFlags = []cli.Flag{
 	cli.StringFlag{Name: "path", Usage: "path to a bundle"},
+	cli.BoolFlag{Name: "hooks", Usage: "Specify check hooks on host, default is false"},


Can you change it here as well? Thanks!

@mrunalp sorry, miss it. fixed.

mrunalp · 2016-05-24T01:38:14Z

Needs rebase

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

Mashimiao · 2016-05-24T01:42:11Z

@mrunalp rebased.

mrunalp · 2016-05-24T16:30:09Z

LGTM

wking reviewed Apr 28, 2016
View reviewed changes

Mashimiao force-pushed the add-hooks-bundle-validation branch from 0daf264 to 1910466Compare April 29, 2016 05:08

Mashimiao force-pushed the add-hooks-bundle-validation branch 2 times, most recently from 4b3b29e to 6ed1bd2Compare May 9, 2016 06:28

Mashimiao force-pushed the add-hooks-bundle-validation branch from 6ed1bd2 to 29a4d8eCompare May 10, 2016 03:47

wking reviewed May 10, 2016
View reviewed changes

Mashimiao force-pushed the add-hooks-bundle-validation branch 2 times, most recently from 52bc38a to 028d996Compare May 16, 2016 01:14

wking reviewed May 16, 2016
View reviewed changes

Mashimiao force-pushed the add-hooks-bundle-validation branch 3 times, most recently from c71d294 to f54700dCompare May 16, 2016 08:38

Mashimiao force-pushed the add-hooks-bundle-validation branch from f54700d to 51d1cc6Compare May 20, 2016 06:32

liangchenye mentioned this pull request May 20, 2016
runtimespec VS ocitools #66
Closed
76 tasks

Mashimiao force-pushed the add-hooks-bundle-validation branch from 51d1cc6 to ce49f78Compare May 20, 2016 08:29

mrunalp reviewed May 20, 2016
View reviewed changes

Mashimiao force-pushed the add-hooks-bundle-validation branch from ce49f78 to f11bccfCompare May 23, 2016 02:45

mrunalp reviewed May 23, 2016
View reviewed changes

Mashimiao force-pushed the add-hooks-bundle-validation branch from f11bccf to 8f62787Compare May 24, 2016 01:14

validate: add Hooks validation
d5f4bb2
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

Mashimiao force-pushed the add-hooks-bundle-validation branch from 8f62787 to d5f4bb2Compare May 24, 2016 01:41

mrunalp merged commit 7543087 into opencontainers:masterMay 24, 2016

wking mentioned this pull request Jun 1, 2016
validation: add env validation for hooks #84
Merged

wking mentioned this pull request Jun 23, 2016
validate: add mount type check #119
Merged

Mashimiao deleted the add-hooks-bundle-validation branch November 14, 2016 09:34

Uh oh!

Conversation

Mashimiao commented Apr 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Apr 28, 2016

Uh oh!

liangchenye commented May 3, 2016

Uh oh!

wking commented May 3, 2016

Uh oh!

Mashimiao commented May 9, 2016

Uh oh!

liangchenye commented May 9, 2016

Uh oh!

Mashimiao commented May 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mashimiao commented May 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liangchenye commented May 17, 2016

Uh oh!

wking commented May 17, 2016 via email

Uh oh!

Mashimiao commented May 19, 2016

Uh oh!

liangchenye commented May 20, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mashimiao commented May 20, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wking commented May 20, 2016 via email

Uh oh!

wking commented May 20, 2016

Uh oh!

Mashimiao commented May 20, 2016

Uh oh!

wking commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MashimiaoMay 23, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp commented May 24, 2016

Uh oh!

Mashimiao commented May 24, 2016

Uh oh!

mrunalp commented May 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

liangchenye commented May 20, 2016•
edited
Loading

Mashimiao commented May 20, 2016•
edited
Loading

MashimiaoMay 23, 2016•
edited
Loading