validate: allow unset "type" fields in resource devices whitelist #491

cyphar · 2017-09-30T06:20:38Z

According to the spec, an unset .Type field in the
.Linux.Resources.Devices list is permitted (and it maps to "all").
This was broken recently, making our own default profile (as well as
umoci's and runc's) not pass validation anymore.

Signed-off-by: Aleksa Sarai asarai@suse.de

cyphar · 2017-09-30T06:24:12Z

CI failure is caused by a build failure I cannot reproduce on my local machine.

wking · 2017-10-01T01:52:21Z

Build failures were fixed by #490.

The check you're patching is from #374, and I think we can drop at least the Type check in favor of JSON Schema (with opencontainers/runtime-spec#690 hopefully landing soon ;). But if we want local code until that lands, this change looks fine to me.

cyphar · 2017-10-01T22:09:10Z

I've commented on opencontainers/runtime-spec#690, which currently has the same bug as the one I'm fixing here. But since it might take a while to land in runtime-spec, we should fix this bug as currently our validation isn't even consistent with our own defaults (which really shouldn't have happened in the first place -- don't we test that our own default configuration is compliant?).

wking · 2017-10-01T22:38:42Z

... don't we test that our own default configuration is compliant?

+1 to unit tests for this code, runtime-tools is currently sparsely-tested.

I'm fine with either this PR or dropping the local Type check with the expection that opencontainers/runtime-spec#690 will land soon. I don't think anyone is in favor of keeping the overly-strict local test we have in master now.

cyphar · 2017-10-02T12:42:10Z

Rebased, added the smoke test, and corrected another bug in our default configuration.

wking · 2017-10-02T16:32:44Z

validate/validate.go

@@ -206,11 +206,6 @@ func (v *Validator) CheckRoot() (errs error) {
 		return
 	}

-	if filepath.Base(v.spec.Root.Path) != "rootfs" {
-		errs = multierror.Append(errs,
-			specerror.NewError(specerror.PathName, fmt.Errorf("path name should be the conventional 'rootfs'"), rspec.Version))


This is a spec SHOULD, and removing it is why your Travis tests fail. I think we need to keep this check, and users that don't care about SHOULD-level violations can ignore those errors like this.

I know it's a SHOULD. But CheckAll doesn't appear to understand that SHOULD doesn't mean MUST. The following error happens:

=== RUN TestGenerateValid --- FAIL: TestGenerateValid (0.52s) generate_test.go:42: unexpected error(s) validating default: 1 error occurred: * path name should be the conventional 'rootfs' Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#root

And this means that oci-runtime-tool validate returns a non-zero error code.

But CheckAll doesn't appear to understand that SHOULD doesn't mean MUST.

CheckAll shouldn't care; it's up to the caller to decide what is fatal or not. Using rootfs in the default config should be enough for your TestGenerateValid. cmd/oci-runtime-tool/validate.go needs a --compliance-level like runtimetest (which may be in flight somewhere, I don't have my dev box handy to grep open PRs).

cmd/oci-runtime-tool/validate.go needs a --compliance-level like runtimetest (which may be in flight somewhere, I don't have my dev box handy to grep open PRs).

I couldn't find this in flight, so I filed it as #492.

cyphar · 2017-10-10T12:23:33Z

Rebased off #492.

wking · 2017-10-10T14:28:21Z

validation/generate_test.go

+		levelErrors, err2 := specerror.SplitLevel(err, rfc2119.Must)
+		if err2 != nil {
+			t.Errorf("unexpected error(s) filtering errors default: %+v", err2)
+			t.Errorf("unexpected error(s): %+v", err)


What you call err2 will only ever be non-nil if it exactly matches the input error. So I think you want to drop the previous line and keep only this one to avoid redundancy. Perhaps the SplitLevel comment can also be improved to make this more clear?

wking

One long-term concern, but I don't see anything that would hold back merging.

wking · 2017-10-14T04:07:42Z

validation/generate_test.go

+	}
+
+	// Validate the bundle.
+	v, err := validate.NewValidatorFromPath(bundle, true, runtime.GOOS)


I'm not excited about the asymmetry between unparmeterized generation and host-specific validation. This will cerainly die if runtime.GOOS is not Linux. It may die even on Linux if the host system is sufficiently ancient or the stock config becomes sufficiently demanding. But I'm ok crossing those bridges later.

Do you want me to set the OS explicitly?

Do you want me to set the OS explicitly?

I'm fine leaving it as it stands, as a reminder that New will need similar options when we get our first non-Linux user and/or stabilize the API.

According to the spec, an unset .Type field in the .Linux.Resources.Devices list is permitted (and it maps to "all"). This was broken recently, making our own default profile (as well as umoci's and runc's) not pass validation anymore. Signed-off-by: Aleksa Sarai <asarai@suse.de>

It doesn't make sense for us to not include a rootfs in our default configuration -- as it means that we're providing an invalid configuration. Signed-off-by: Aleksa Sarai <asarai@suse.de>

To avoid cases where our own validation code would consider our defaults unsafe (which has happened in the past several times), add a smoke-test to ensure that this won't happen. Our defaults should not be intentionally invalid, as that confuses downstreams like umoci which use runtime-tools for the default as well as for validation of the generated configuration. Signed-off-by: Aleksa Sarai <asarai@suse.de>

cyphar · 2017-10-23T22:35:18Z

/ping @opencontainers/runtime-tools-maintainers

Mashimiao · 2017-10-24T01:22:29Z

LGTM

hqhq · 2017-10-24T01:30:07Z

LGTM

cyphar mentioned this pull request Oct 2, 2017

ci: update Dockerfile to fix broken tests opencontainers/umoci#176

Merged

cyphar force-pushed the validate-fix-type-whitelist branch 2 times, most recently from fc0b7d8 to f9de4c6 Compare October 2, 2017 12:41

wking reviewed Oct 2, 2017

View reviewed changes

cyphar force-pushed the validate-fix-type-whitelist branch from f9de4c6 to 4d6860d Compare October 10, 2017 12:23

wking reviewed Oct 10, 2017

View reviewed changes

cyphar force-pushed the validate-fix-type-whitelist branch from 4d6860d to 27ad856 Compare October 14, 2017 03:34

wking approved these changes Oct 14, 2017

View reviewed changes

cyphar added 3 commits October 24, 2017 09:35

config: correct rootfs default

a6f475f

It doesn't make sense for us to not include a rootfs in our default configuration -- as it means that we're providing an invalid configuration. Signed-off-by: Aleksa Sarai <asarai@suse.de>

cyphar force-pushed the validate-fix-type-whitelist branch from 27ad856 to 6df06d9 Compare October 23, 2017 22:35

hqhq merged commit e0df0fd into opencontainers:master Oct 24, 2017

cyphar deleted the validate-fix-type-whitelist branch October 24, 2017 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validate: allow unset "type" fields in resource devices whitelist #491

validate: allow unset "type" fields in resource devices whitelist #491

cyphar commented Sep 30, 2017

cyphar commented Sep 30, 2017

wking commented Oct 1, 2017 •

edited

Loading

cyphar commented Oct 1, 2017

wking commented Oct 1, 2017 •

edited

Loading

cyphar commented Oct 2, 2017

wking Oct 2, 2017

cyphar Oct 2, 2017 •

edited

Loading

wking Oct 2, 2017

wking Oct 2, 2017

cyphar commented Oct 10, 2017

wking Oct 10, 2017

wking left a comment

wking Oct 14, 2017

cyphar Oct 15, 2017

wking Oct 15, 2017

cyphar commented Oct 23, 2017

Mashimiao commented Oct 24, 2017 •

edited by caniszczyk

Loading

hqhq commented Oct 24, 2017 •

edited by caniszczyk

Loading

validate: allow unset "type" fields in resource devices whitelist #491

validate: allow unset "type" fields in resource devices whitelist #491

Conversation

cyphar commented Sep 30, 2017

cyphar commented Sep 30, 2017

wking commented Oct 1, 2017 • edited Loading

cyphar commented Oct 1, 2017

wking commented Oct 1, 2017 • edited Loading

cyphar commented Oct 2, 2017

wking Oct 2, 2017

Choose a reason for hiding this comment

cyphar Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

wking Oct 2, 2017

Choose a reason for hiding this comment

wking Oct 2, 2017

Choose a reason for hiding this comment

cyphar commented Oct 10, 2017

wking Oct 10, 2017

Choose a reason for hiding this comment

wking left a comment

Choose a reason for hiding this comment

wking Oct 14, 2017

Choose a reason for hiding this comment

cyphar Oct 15, 2017

Choose a reason for hiding this comment

wking Oct 15, 2017

Choose a reason for hiding this comment

cyphar commented Oct 23, 2017

Mashimiao commented Oct 24, 2017 • edited by caniszczyk Loading

hqhq commented Oct 24, 2017 • edited by caniszczyk Loading

wking commented Oct 1, 2017 •

edited

Loading

wking commented Oct 1, 2017 •

edited

Loading

cyphar Oct 2, 2017 •

edited

Loading

Mashimiao commented Oct 24, 2017 •

edited by caniszczyk

Loading

hqhq commented Oct 24, 2017 •

edited by caniszczyk

Loading