-
Notifications
You must be signed in to change notification settings - Fork 143
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
validate: add Hooks validation #49
validate: add Hooks validation #49
Conversation
@@ -106,6 +107,29 @@ func checkMounts(mounts []rspec.Mount, rootfs string) { | |||
} | |||
} | |||
|
|||
func checkHooks(process rspec.Hooks, rootfs string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, …).
This sort of validation would be easier if we could recycle the
|
0daf264
to
1910466
Compare
Thanks @Mashimiao,
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.) |
On Tue, May 03, 2016 at 02:36:04AM -0700, 梁辰晔 (Liang Chenye) wrote:
We can check “Does the hook command exist in the host's filesystem?”, |
4b3b29e
to
6ed1bd2
Compare
@wking @liangchenye |
The hook configuration is a little tricky, it is not portable. |
6ed1bd2
to
29a4d8e
Compare
@wking @liangchenye PR updated |
@@ -20,6 +21,9 @@ Validate an OCI bundle | |||
**--path="PATH" | |||
Path to bundle | |||
|
|||
**--hooks-check** | |||
Specify check hooks on host, default is false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?”).
52bc38a
to
028d996
Compare
ping @liangchenye |
@@ -6,6 +6,9 @@ ocitools-validate - Validate a OCI bundle | |||
|
|||
# SYNOPSIS | |||
**ocitools validate** *[OPTIONS]* | |||
[**--help**] | |||
[**--path**[=*PATH*] | |||
[**--hooks-check**[=*true*|*false*] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ;).
c71d294
to
f54700d
Compare
Thanks @Mashimiao LGTM |
Ah, I'd missed the responses to my earlier comments. f54700d looks
good to me too.
|
ping @mrunalp |
Hi @Mashimiao, |
@liangchenye OK, I can implement it in this PR. |
f54700d
to
51d1cc6
Compare
51d1cc6 still looks good to me.
|
51d1cc6
to
ce49f78
Compare
@wking Ah, yes. I did not read @liangchenye 's words carefully. How about now? |
On Fri, May 20, 2016 at 01:30:20AM -0700, Ma Shimiao wrote:
ce49f78 looks great :). |
@@ -18,6 +18,9 @@ Validate an OCI bundle | |||
**--path=PATH | |||
Path to bundle | |||
|
|||
**--hooks** | |||
Specify check hooks exists and executable on host, default is false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Check specified hooks exist and are executable on the host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrunalp fixed.
ce49f78
to
f11bccf
Compare
@@ -19,6 +20,7 @@ import ( | |||
|
|||
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"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change it here as well? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrunalp sorry, miss it. fixed.
f11bccf
to
8f62787
Compare
Needs rebase |
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
8f62787
to
d5f4bb2
Compare
@mrunalp rebased. |
LGTM |
Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com