cmd: generate: add --linux-namespace-* family of flags #288

cyphar · 2016-12-12T11:13:24Z

This allows users to modify what namespaces are used by a container.

Signed-off-by: Aleksa Sarai [email protected]

mrunalp · 2016-12-12T17:54:20Z

LGTM

mrunalp · 2016-12-12T17:54:30Z

Mashimiao · 2016-12-13T02:27:30Z

we already have --user --memory --ipc --mount --network --pid --uts to add namespaces, I think --linux-namespace-add conflicts with them.

cyphar · 2016-12-13T06:01:52Z

@Mashimiao Yeah, you're right. However, we should probably have something like --linux-namespace-remove and --linux-namespace-remove-all.

Mashimiao · 2016-12-13T06:33:18Z

If the value of current namespace options is host, we will remove existing namespace. For example,
if we set --uts=host, we will remove existing uts namespace from config.
But I think that --linux-namespace-* is clear for users to understand and use.
How about removing namespace options like --uts, --user, --pid, etc? @mrunalp @liangchenye @wking

wking · 2016-12-13T16:48:37Z

On Mon, Dec 12, 2016 at 10:33:19PM -0800, Ma Shimiao wrote: If the value of current namespace options is `host`, we will remove existing namespace.

Ah, right, I'd forgotten about that.

But I think that --linux-namespace-* is clear for users to understand and use. How about removing namespace options like --uts, --user, --pid, etc? @mrunalp @liangchenye @wking

Sounds good to me (it has better namespacing). This PR still needs the associated man-page changes.

cyphar · 2016-12-21T11:37:54Z

I've updated this PR so that it removes the old namespace flags, and added documentation changes for them.

/cc @Mashimiao @mrunalp

Mashimiao · 2016-12-21T13:34:32Z

@cyphar please also update completions/bash file

cyphar · 2016-12-21T14:15:57Z

@Mashimiao Done. There were a few other flags not included or incorrectly classified, so I've fixed it up.

wking

One nit and one caveat, but otherwise looks good to me :).

wking · 2016-12-21T21:46:29Z

cmd/oci-runtime-tool/generate.go

@@ -608,6 +613,18 @@ func parseRlimit(rlimit string) (string, uint64, uint64, error) {
 	return parts[0], uint64(hard), uint64(soft), nil
 }

+func parseNamespace(ns string) (string, string, error) {
+	parts := strings.Split(ns, ":")


Split → SplitN? Linux allows paths to contain colons.

wking · 2016-12-21T21:52:39Z

cmd/oci-runtime-tool/generate.go

 	if len(uidMaps) > 0 || len(gidMaps) > 0 {
-		needsNewUser = true
+		g.AddOrReplaceLinuxNamespace("user", "")


This will silently create an invalid config with --uidmappings=… --linux-namespace-add=user:foo, because the resulting config with call for tweaking a joined namespace. I'm pro-tweaking though, and we're already doing something silent about this case, so I'm ok with that ;). Folks who want to ensure validity can pass their final config through to validate as a separate step.

I don't think it'll generate an invalid config, because the --linux-namespace-add handling is done afterwards and so the user entry will be replaced.

Well, sorry, it will generate an invalid config, but the user asked us to generate it...

wking · 2016-12-22T17:13:55Z

a0f56f2 looks good to me.

Mashimiao · 2016-12-23T01:39:42Z

cmd/oci-runtime-tool/generate.go

@@ -608,6 +613,18 @@ func parseRlimit(rlimit string) (string, uint64, uint64, error) {
 	return parts[0], uint64(hard), uint64(soft), nil
 }

+func parseNamespace(ns string) (string, string, error) {
+	parts := strings.SplitN(ns, ":", 2)


If we don't check parts[0], then :testpat will also be a valid value. Though user set it, I think we should warn user it's invalid, as I did in #292

You can validate it against the spec's whitelist. But I'm fine punting validation to a post-generate call (for callers who care). Maybe they intend to fix the values in post-processing.

I've fixed it to check parts[0]. The whitelist checking should be done in the generate library (IMO), and I'll punt it for now. I mean, we don't actually have tests for generate at all -- so maybe we should do that first? 😉

what kind of tests?

There aren't any unit tests or integration tests for oci-runtime-tool. We should probably add some. Also, this tool isn't versioned which makes packaging annoying.

More importantly, as far as I can tell there's no tests of the validation -- which is quite worrying.

Yes, we should add unit tests. unit tests for validation already in #273

I'd floated some sharness stubs for testing in #180. Happy to rebase if there is renewed interest.

Mashimiao · 2016-12-24T01:11:36Z

cmd/oci-runtime-tool/generate.go

+		return parts[0], parts[1], nil
+	}
+
+	panic("should not be reached")


Actually, I don't like this. the panic error message does not tell user what's wrong clear.
I think we should give user clear error message and they don't have to hack code to find out what's wrong

It's a panic because you literally cannot reach that part of the code. len(parts) can only have a length of 0, 1, and 2 and all cases have been handled. If I remove the panic you get errors about no return statement, even though you cannot reach that line.

I can make the function longer to make you happy if you really want.

All cases of len(parts) are handled, so we will never reach panic. In my opinion, panic here make no sense.

So you want to return an error even though the actual line will never be hit? From my experience, people use panic in these situations because it's more clear that if you ever hit that line it's a programming error (not something that should happen normally).

No, I mean do not need panic or return here. Just remove this panic, because it will never be reached

Go won't compile that program, which is why I added panic... Go isn't clever enough to know that the line won't be hit.

I mention this above

If I remove the panic you get errors about no return statement, even though you cannot reach that line.

go build -tags "" -o oci-runtime-tool ./cmd/oci-runtime-tool # runtime-tools/cmd/oci-runtime-tool ../../.local/src/runtime-tools/cmd/oci-runtime-tool/generate.go:629: missing return at end of function

How about this?
func parseNamespace(ns string) (string, string, error) {
var nsType, nsPath string
parts := strings.SplitN(ns, ":", 2)
if len(parts) == 0 || parts[0] == "" {
return "", "", fmt.Errorf("invalid namespace value: %s", ns)
}

switch len(parts) { case 1: nsType, nsPath = parts[0], "" case 2: nsType, nsPath = parts[0], parts[1] } return nsType, nsPath, nil

}

Removed the panic. PTAL.

Mashimiao · 2016-12-24T05:58:20Z

LGTM

Mashimiao · 2016-12-30T07:28:19Z

cmd/oci-runtime-tool/generate.go

+		}
+	}
+
+	if context.IsSet("linux-namespace-remove-all") {


This should be context.Bool("linux-namespace-remove-all"). If not, when linux-namespace-remove-all is false, ClearLinuxNamespaces function will still be called.

I was copying what --rlimits-remove-all and --seccomp-remove-all (the only other flags of this form) do. I don't disagree, it's just that the problem exists with other flags too.

Yes, I just pushed a PR to fix it

This allows users to modify what namespaces are used by a container. Signed-off-by: Aleksa Sarai <[email protected]>

--uts, --user and the other flags are not properly namespaced or extensible, so remove them in favour of the new --linux-namespace-* family of flags. Signed-off-by: Aleksa Sarai <[email protected]>

Signed-off-by: Aleksa Sarai <[email protected]>

This is generated from "oci-runtime-tool generate -h". Signed-off-by: Aleksa Sarai <[email protected]>

Mashimiao · 2016-12-30T07:39:09Z

LGTM

cyphar · 2017-02-01T15:27:29Z

/ping @opencontainers/runtime-tools-maintainers

dqminh · 2017-02-01T16:30:52Z

LGTM

cyphar force-pushed the namespace-flags branch 2 times, most recently from 870cde8 to 081b841 Compare December 12, 2016 11:20

wking mentioned this pull request Dec 21, 2016

Use correct flag for host networking opencontainers/umoci#66

Closed

cyphar force-pushed the namespace-flags branch from db15cb8 to 247bfaf Compare December 21, 2016 11:37

wking reviewed Dec 21, 2016

View reviewed changes

cyphar force-pushed the namespace-flags branch from cb10076 to a0f56f2 Compare December 22, 2016 06:58

Mashimiao reviewed Dec 23, 2016

View reviewed changes

cyphar force-pushed the namespace-flags branch from a0f56f2 to 737aa11 Compare December 23, 2016 02:40

Mashimiao reviewed Dec 24, 2016

View reviewed changes

cyphar force-pushed the namespace-flags branch from 737aa11 to 6d7ec82 Compare December 24, 2016 03:09

Mashimiao reviewed Dec 30, 2016

View reviewed changes

cyphar added 4 commits December 30, 2016 18:34

cmd: generate: add --linux-namespace-* family of flags

d0697f1

This allows users to modify what namespaces are used by a container. Signed-off-by: Aleksa Sarai <[email protected]>

cmd: generate: remove old namespace flags

fac26fa

--uts, --user and the other flags are not properly namespaced or extensible, so remove them in favour of the new --linux-namespace-* family of flags. Signed-off-by: Aleksa Sarai <[email protected]>

man: update docs to use new --linux-namespace-* flags

3807b60

Signed-off-by: Aleksa Sarai <[email protected]>

completions: bash: update flags

eb189d5

This is generated from "oci-runtime-tool generate -h". Signed-off-by: Aleksa Sarai <[email protected]>

cyphar force-pushed the namespace-flags branch from 6d7ec82 to eb189d5 Compare December 30, 2016 07:34

dqminh merged commit 2d92f65 into opencontainers:master Feb 1, 2017

cyphar deleted the namespace-flags branch February 1, 2017 17:01

cmd: generate: add --linux-namespace-* family of flags #288

cmd: generate: add --linux-namespace-* family of flags #288

Conversation

cyphar commented Dec 12, 2016

mrunalp commented Dec 12, 2016 • edited by caniszczyk Loading

mrunalp commented Dec 12, 2016

Mashimiao commented Dec 13, 2016

cyphar commented Dec 13, 2016

Mashimiao commented Dec 13, 2016

wking commented Dec 13, 2016 via email

cyphar commented Dec 21, 2016 • edited Loading

Mashimiao commented Dec 21, 2016

cyphar commented Dec 21, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Dec 22, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar Dec 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

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

cyphar Dec 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Dec 24, 2016 • edited by caniszczyk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Dec 30, 2016 • edited by caniszczyk Loading

cyphar commented Feb 1, 2017

dqminh commented Feb 1, 2017 • edited by caniszczyk Loading

mrunalp commented Dec 12, 2016 •

edited by caniszczyk

Loading

cyphar commented Dec 21, 2016 •

edited

Loading

cyphar Dec 23, 2016 •

edited

Loading

cyphar Dec 24, 2016 •

edited

Loading

Mashimiao commented Dec 24, 2016 •

edited by caniszczyk

Loading

Mashimiao commented Dec 30, 2016 •

edited by caniszczyk

Loading

dqminh commented Feb 1, 2017 •

edited by caniszczyk

Loading