generate: match spec conversion #120

vrothberg · 2017-05-10T12:06:17Z

Extend the config generation to match the conversion spec described in opencontainers/image-spec as requested in #119.

Signed-off-by: Valentin Rothberg [email protected]

cyphar · 2017-05-11T11:14:48Z

oci/config/convert/runtime.go

 	g.SetProcessTerminal(true)
 	g.SetRootPath(filepath.Base(rootfs))
 	g.SetRootReadonly(false)

-	g.SetProcessCwd("/")


The reason for this was to make sure we didn't have an empty workdir in the configuration. Do you think I need to clarify more in the spec that if a field is missing in the image that the converter can pick any default?

I removed the line, since it's already the default in the generator. Adding such information to the spec wouldn't hurt. I believe that any additional information helps to make the hard requirements and the corresponding degree of freedom more explicit.

cyphar

There are a couple of things mainly related to annotations. The key thing to note is that annotations are map[string]string in the spec, and that in general the keys must be namespaced (the conversion doc specifies what the fully namespaced name is in the notes for each conversion table).

cyphar · 2017-05-12T12:11:38Z

oci/config/convert/runtime.go

+	g.AddAnnotation("author", image.Author)
+	g.AddAnnotation("created", image.Created.Format(gspec.ISO8601))
+	// FIXME: currently not supported! Uncomment when moving to image-spec rc5.
+	// g.AddAnnotation("StopSignal", image.Config.StopSignal)


All of these annotations key names aren't right (you need to use the full org.opencontainers.<blah>.<blah> namespacing as per the conversion doc. From the doc:

The value of this field MUST be set as the value of org.opencontainers.image.author in annotations.

The value of this field MUST be set as the value of org.opencontainers.image.created in annotations.

The value of this field MUST be set as the value of org.opencontainers.image.stopSignal in annotations.

Thanks, I missed that entirely, but I believe it should be "*.image.StopSignal" (i.e., with a capital 'S'). That's at least what the JSON identifiers indicate.

cyphar · 2017-05-12T12:14:15Z

oci/config/convert/runtime.go

+	// Set optional fields
+	for exp := range image.Config.ExposedPorts {
+		g.AddAnnotation(exp, "")
+	}


As above this needs to be done as per the spec. The key name and format is in the conversion doc:

org.opencontainers.image.exposedPorts is the list of values that correspond to the keys defined for Config.ExposedPorts (string, comma-separated values).

Basically you need to set org.opencontainers.image.exposedPorts to the value of joining all of the keys in image.Config.ExposedPorts with commas.

Thanks, will fix.

cyphar · 2017-05-12T12:14:44Z

oci/config/convert/runtime.go

@@ -123,31 +126,23 @@ func MutateRuntimeSpec(g rgen.Generator, rootfs string, image ispec.Image, manif

 	g.SetProcessUID(uint32(execUser.Uid))
 	g.SetProcessGID(uint32(execUser.Gid))
-	g.ClearProcessAdditionalGids()


We almost certainly need this because otherwise if the default from runtime-spec changes for some reason you might end up with supplementary group IDs you don't expect.

Will fix. My intention of always using and relying on those defaults is that those should be meaningful (ideally). In case they would be changed in way that they break something, we have a specific case to point at.

The thing is, the defaults given by the runtime-tools/generate is not defined anywhere. While I appreciate wanting to reduce the amount of code in our generation I don't really like relying on some random defaults that could change in the future (and might be a pain to detect).

But with this especially it's not a matter of the defaults from the generator, Config.User has semantic meaning with regards to supplementary groups. If a user specifies 100:100 this code won't touch (clear) the supplementary groups (which could be wrong and result in something unfortunate with privileged groups).

vrothberg · 2017-05-15T09:37:32Z

Sorry for the noise. The superfluous and breaking test case has been removed now.

cyphar · 2017-05-15T11:53:36Z

oci/config/convert/runtime.go

+	g.AddAnnotation("org.opencontainers.image.author", image.Author)
+	g.AddAnnotation("org.opencontainers.image.created", image.Created.Format(gspec.ISO8601))
+	// FIXME: currently not supported! Uncomment when moving to image-spec rc5.
+	// g.AddAnnotation("org.opencontainers.image.StopSignal", image.Config.StopSignal)


Can you make these module-level constants? Something like authorAnnotation, createdAnnotation, stopSignalAnnotation?

cyphar · 2017-05-15T11:55:40Z

oci/config/convert/runtime.go

+	var ports []string
+	for port := range image.Config.ExposedPorts {
+		ports = append(ports, port)
+	}


As we discussed you'll need to sort ports after this because iteration over Go maps is random. You can use the sort package with sort.StringSlice (https://golang.org/pkg/sort/#StringSlice).

Though in future it would be nice if this was hidden behind the github.com/openSUSE/umoci/oci/config/generate API (we can do that after this is merged though).

func (g *Generator) ConfigExposedPortsToString() string { ... } ?

I'd prefer it to return the []string (the fact that internally it's implemented as a map is something that users shouldn't care about, but the fact it's a list of ports is important for implementations). In any case that's not important atm.

cyphar · 2017-05-15T11:58:22Z

test/config.bats

-	[ "$status" -eq 0 ]
-	[[ "$output" == "" ]]
+	# the ports are comma separated
+	[[ "${lines[0]}" == "1234/tcp,8080/tcp" ]]


If you want to implement this in a way that doesn't hardcode the ordering (not sure if it makes sense here) you could do something like $(echo "${lines[0]}" | tr ',' '\n' | grep <blah>) but it's a bit ugly for my taste.

I opted for splitting the string with jq so that we can identify mismatches easier in case something breaks in the future.

cyphar · 2017-05-15T14:07:02Z

third_party/user/user.go

@@ -378,7 +378,7 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (

 				// Okay, so it's numeric. We can just roll with this.
 			}
-		} else if len(groups) > 0 {
+		} else if len(groups) > 0 && uidErr != nil {


While I will merge this, note that this code comes from runc and I really don't want us to diverge from upstream (the reason I didn't just vendor it is because I needed to patch parts of it IIRC).

Can you elaborate why this change is necessary? Is it to avoid the supplementary groups stuff if the user is numeric? Maybe we should have this same patch put into runc?

Exactly, it's to apply to the convert spec. I figured this is a good place to put this logic.

Alright, cool. Do you want to open a sister PR in runc to add this change to libcontainer/user?

Sure, I'll prepare one tomorrow.

cyphar · 2017-05-16T09:48:09Z

Can you please rebase + squash and I'll merge this.

cyphar · 2017-05-16T12:07:40Z

Can you drop the merge commit into this branch? Those are usually unsightly -- just use git rebase origin/master.

vrothberg · 2017-05-16T12:12:00Z

Didn't work. Give me another try...

Extend the conversion mechanism to match the specification as described in: opencontainers/image-spec#492 Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2017-05-16T12:19:41Z

Done. I missed pulling from upstream before rebasing.

cyphar · 2017-05-17T10:11:54Z

LGTM

@cyphar

LGTMs: @cyphar Closes #120

cyphar reviewed May 11, 2017

View reviewed changes

cyphar requested changes May 12, 2017

View reviewed changes

cyphar reviewed May 15, 2017

View reviewed changes

generate: match conversion spec

11cb2c6

Extend the conversion mechanism to match the specification as described in: opencontainers/image-spec#492 Signed-off-by: Valentin Rothberg <[email protected]>

cyphar approved these changes May 17, 2017

View reviewed changes

cyphar merged commit 11cb2c6 into opencontainers:master May 17, 2017

cyphar added a commit that referenced this pull request May 17, 2017

merge branch 'pr-120'

cad9bbc

LGTMs: @cyphar Closes #120

This was referenced May 17, 2017

config generate: match spec conversion #119

Closed

CHANGELOG: add stuff from @vrothberg #123

Merged

vrothberg mentioned this pull request May 18, 2017

generate: get string array of exposed ports #125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generate: match spec conversion #120

generate: match spec conversion #120

vrothberg commented May 10, 2017

cyphar May 11, 2017

vrothberg May 11, 2017

cyphar left a comment

cyphar May 12, 2017

vrothberg May 12, 2017

cyphar May 12, 2017

vrothberg May 12, 2017

cyphar May 12, 2017

vrothberg May 12, 2017

cyphar May 12, 2017 •

edited

Loading

vrothberg commented May 15, 2017

cyphar May 15, 2017

vrothberg May 15, 2017

cyphar May 15, 2017 •

edited

Loading

vrothberg May 15, 2017

cyphar May 15, 2017

cyphar May 15, 2017

vrothberg May 15, 2017

cyphar May 15, 2017

vrothberg May 15, 2017

cyphar May 15, 2017

vrothberg May 15, 2017

cyphar commented May 16, 2017

cyphar commented May 16, 2017

vrothberg commented May 16, 2017

vrothberg commented May 16, 2017

cyphar commented May 17, 2017

generate: match spec conversion #120

generate: match spec conversion #120

Conversation

vrothberg commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar 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

Choose a reason for hiding this comment

cyphar May 12, 2017 • edited Loading

Choose a reason for hiding this comment

vrothberg commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar May 15, 2017 • 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

cyphar commented May 16, 2017

cyphar commented May 16, 2017

vrothberg commented May 16, 2017

vrothberg commented May 16, 2017

cyphar commented May 17, 2017

cyphar May 12, 2017 •

edited

Loading

cyphar May 15, 2017 •

edited

Loading