Move generate code into a separate library #131

haiyanmeng · 2016-07-09T02:56:08Z

Move generate code into a separate library, libgen.

To avoid using cli.Context inside libgen, the modify function responds to get the command options, and transfer them into the functions inside libgen.

The PR tries to fix #130 .

haiyanmeng · 2016-07-09T02:58:42Z

@mrunalp , PTAL.

haiyanmeng · 2016-07-09T03:02:50Z

@mrunalp , @wking , this PR only aims to move the generate code into a separate library.
We may do another PR to coordinate with #128 to make reviewing PRs easier.

wking · 2016-07-09T03:38:39Z

generate.go

@@ -9,9 +9,8 @@ import (
 	"strconv"
 	"strings"

-	"github.com/Sirupsen/logrus"
+	"github.com/opencontainers/ocitools/libgen"


There was a reasonable amount of “what should we call this?” discussion in #128, which resolved around seccomp-gen. If we're aiming for separate generate/validate/(test ?) commands, we probably want something like:

ocitools |-- cmd | `-- ocitools | |-- main.go | |-- generate.go | |-- validate.go | … |-- generate | |-- generate.go | |-- utils.go, etc. | `-- seccomp (#128) | |-- consts.go | … |-- validate | |-- validate.go | `-- utils.go …

Shifting the import paths around while we settle into whatever layout we choose isn't a big deal as long as we don't cut releases. But I expect that the libraries will start picking up external consumers shortly after they're born, and we'll make life easier on those downstream folks if we don't churn the layout too much ;).

@wking , @mrunalp , I think I will first get myself familiar with #54 , and #128 .
Then we can come back to this PR and decide how we want it to be.

On Mon, Jul 11, 2016 at 07:09:30AM -0700, hmeng-19 wrote:

@@ -9,9 +9,8 @@ import (
"strconv"
"strings"

"github.com/Sirupsen/logrus"

"github.com/opencontainers/ocitools/libgen"

@wking , this new package structure looks very different from the
current package structure…

Yeah, but if we're going to restructure things, I think we want to
take a big step to get to where we (think we) want to be, instead of
taking a bunch of short steps. The big step may not be perfect, but
it's got a better chance at being right than a small step after which
we already plan on making further changes.

@wking proposed layout scheme looks reasonable to me.

@wking , @mrunalp . Sounds good. I will modify the code structure as @wking suggested.
@wking , in your proposal, there are two generate.go - one is under cmd/ocitools, another is under generate. What is their difference? The same problem happens to validate.go.
I think all the files under generate and validate should be libraries, right?

On Mon, Jul 11, 2016 at 11:36:11AM -0700, hmeng-19 wrote:

@wking , in your proposal, there are two generate.go - one is
under cmd/ocitools, another is under generate. What is their
difference? The same problem happens to validate.go.

The cmd/ocitools stuff is just thin bindings between the library code
(under generate/, validate/, etc.) and urfave/cli.

I think all the files under generate and validate should be
libraries, right?

Yup.

wking · 2016-07-09T03:57:12Z

On Fri, Jul 08, 2016 at 08:02:51PM -0700, hmeng-19 wrote:

We may do another PR to coordinate with #128 to make reviewing PRs
easier.

This is also going to create some tedious conflicts with #54 (now
stable for almost two months ;). Can we get an up/down ruling on that
PR in the near future. Having these major PRs all touching the same
code and in flight at the same time is just going to create busywork.

wking · 2016-07-09T04:08:52Z

libgen/utils.go

+}
+
+// AddIDMappings add the mappings of uid and gid into spec.
+func AddIDMappings(spec *rspec.Spec, uidMaps, gidMaps []string) error {


This should probably be a single:

func ParseIDMappings(spec *rspec.Spec, idType string, maps []string) error {…}

which the command implementation can call with something like (untested, and I don't do much Go ;):

idTypes := ["uid", "gid"] for idType := range idTypes { maps := context.StringSlice(idType + "mappings") if err := libgen.AddIDMappings(spec, idType, maps) err != nil { return err } }

The approach your copy/pasting was acceptable for an internal helper, but it seems like we should aim for something DRYer for a library method. On the other hand, I'm fine cleaning this stuff up in follow-up PRs, as long as we don't wait long enough for API-breaking to become a concern.

mrunalp · 2016-07-11T17:55:34Z

I agree with @wking that we should combine this with #128 so we have a single cohesive change/library.

haiyanmeng · 2016-07-11T18:38:47Z

@mrunalp , I am okay with the plan.
Should I also try to modify the code structure of the current validate.go file? If so, we probabaly should rename the title of #130 to be more generic.

haiyanmeng · 2016-07-11T21:37:44Z

@mrunalp , @wking , the CI checks failed because this PR tries to introduce a new package cmd/ocitools:

The command "$HOME/gopath/bin/git-validation -run DCO,short-subject -v -range ${TRAVIS_COMMIT_RANGE}" exited with 0.
0.01s$ make
go build -tags "" -o ocitools ./cmd/ocitools
can't load package: package github.com/opencontainers/ocitools/cmd/ocitools: cannot find package "github.com/opencontainers/ocitools/cmd/ocitools" in any of:
    /home/travis/.gimme/versions/go1.5.3.linux.amd64/src/github.com/opencontainers/ocitools/cmd/ocitools (from $GOROOT)
    /home/travis/gopath/src/github.com/opencontainers/ocitools/Godeps/_workspace/src/github.com/opencontainers/ocitools/cmd/ocitools (from $GOPATH)
    /home/travis/gopath/src/github.com/opencontainers/ocitools/cmd/ocitools
make: *** [all] Error 1

haiyanmeng · 2016-07-11T21:40:58Z

@mrunalp , @wking , the new changes I made include:

rename libgen -> generate
move package main into cmd/ocitools
Add an empty generate/seccomp package

The new changes focus on the package structure, does not really polish the function API.
I think we can merge this PR to unblock #128. Then we can make a new PR to polish the new function API.

wking · 2016-07-11T21:46:44Z

On Mon, Jul 11, 2016 at 02:37:44PM -0700, hmeng-19 wrote:

@mrunalp , @wking , the CI checks failed because this PR tries to
introduce a new package cmd/ocitools:

…
can't load package: package github.com/opencontainers/ocitools/cmd/ocitools: cannot find package "github.com/opencontainers/ocitools/cmd/ocitools" in any of:
/home/travis/.gimme/versions/go1.5.3.linux.amd64/src/github.com/opencontainers/ocitools/cmd/ocitools (from $GOROOT)
/home/travis/gopath/src/github.com/opencontainers/ocitools/Godeps/_workspace/src/github.com/opencontainers/ocitools/cmd/ocitools (from $GOPATH)
/home/travis/gopath/src/github.com/opencontainers/ocitools/cmd/ocitools

Actually, I think that's failing because you haven't added
cmd/ocitools. Did you forget a ‘git add cmd/ocitools’ with
d2f10ed?

haiyanmeng · 2016-07-11T21:55:11Z

@wking , you are right. My .gitignore confused me and I thought I added those files.
Fix it. Thanks. PTAL.

wking · 2016-07-11T22:02:13Z

On Mon, Jul 11, 2016 at 02:55:12PM -0700, hmeng-19 wrote:

My .gitignore confused me and I thought I added those files.

Ah, the ‘ocitools’ entry in .gitignore should probably be ‘/ocitools’
to anchor the ignore (and not ignore cmd/ocitools). 9777ceb made a
similar fix for runtimetest.

I'm not sure what the ‘oci’ entry in .gitignore is for. It's from
7ac0686 (2015-09-15), and maybe is just stale?

haiyanmeng · 2016-07-11T22:36:24Z

@wking , I think the problem is the pattern ocitools in the .gitignore file, which automatically
ignore all the files or directories with the name of ocitools. In our case, cmd/ocitools, together with all the entries under it, gets ignored.
However, by adding a leading slash into a pattern, it would only match the beginning of the pathname. That is how Mrunal solved the problem in 9777ceb. /runtimetest only matches runtimetest directly under the git repo.

Please refer to here https://git-scm.com/docs/gitignore .

haiyanmeng · 2016-07-11T22:40:19Z

@wking , @mrunalp , fix the .gitignore file. PTAL.

wking · 2016-07-11T23:00:02Z

Through fed976f look fine to me if we want to shuffle things around
without changing APIs (much), although I'd rather it was squashed down
into a single commit.

But I don't see a need to split “restructure and reconsider the API”
into two separate PRs 1. And I think it would be nice if #54 and
#128 (which were both filed before this change was proposed in #130)
got reviewed first.

grantseltzer · 2016-07-12T14:13:43Z

@hmeng-19 and I are in agreement that this PR should get merged before #128, and i'll probably close #128 and make a new one that will add all of the seccomp stuff into the generate library that she is separating out.

wking · 2016-07-12T16:18:08Z

On Tue, Jul 12, 2016 at 07:13:44AM -0700, Grant Seltzer Richman wrote:

@hmeng-19 and I are in agreement that this PR should get merged
before #128, and i'll probably close #128 and make a new one that
will add all of the seccomp stuff into the generate library that she
is separating out.

Whatever's easier, but you can rebase your existing #128 branch once
this PR lands. And if the plan is to land this PR first, I'll survive
a rebase of #54 (but it would be nice if PRs were reviewed and
merged/rejected more quickly to avoid these parallel-work conflicts.

mrunalp · 2016-07-13T00:06:40Z

cmd/ocitools/generate.go

+	Flags:  generateFlags,
+	Before: before,
+	Action: func(context *cli.Context) error {
+		spec := generate.GetDefaultTemplate()


This could be generate.New()

Again, a rename suggestion for GetDefaultTemplate to New

mrunalp · 2016-07-13T00:09:26Z

commit history needs to be cleaned up or they should be squashed.

haiyanmeng · 2016-07-13T18:14:28Z

@mrunalp , @wking , renamed the two functions you suggested, and squashed all my changes into a single commit.
PTAL.

mrunalp · 2016-07-13T21:53:54Z

While this is a step in the right direction, I think API like the following will be easier to use:

type generator struct {
    spec *rspec.Spec
}

// Start from default template
specgen := generator.New()

// Make modifications
specgen.SetRootPath("/path/to/rootfs")
specgen.AddCapabilities(...)
specgen.AddTmpfsMount(...)
specgen.SetSelinuxLabel(...)
...
...
// Write to disk
specgen.Save("/path/to/file")

// Get back the spec object
spec := specgen.GetSpec()

// Load a template.json
specgen := generator.NewFromTemplate("/path/to/file")
specgen.AddMount(src, dest, options..)
specgen.Save("/some/other/path")

// Load existing spec object and modify it to generate a new spec
specgen := generator.NewFromSpec(someSpecObject)
specgen.SetNoNewPriviliges(true)
specgen.Save("/yet/another/path")

wking · 2016-07-13T21:57:38Z

On Wed, Jul 13, 2016 at 02:53:54PM -0700, Mrunal Patel wrote:

specgen := generator.NewFromTemplate("/path/to/file")

I'd load this from an io.Reader for more flexibility. Wrapping that
in something that opened a file, deferred a close, and passed the
writer in to generator.NewFromReader is acceptable noise for the
command-line binding package.

mrunalp · 2016-07-15T23:01:40Z

Please push your updates so the latest version could be reviewed.

Sent from my iPhone

On Jul 15, 2016, at 3:58 PM, hmeng-19 notifications@github.com wrote:

@mrunalp , @wking , I am finishing my change on my side for now, and also rebase and squash my commit.
Do you want to push my changes to this PR now? or you guys want to finish review the current version?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

haiyanmeng · 2016-07-15T23:04:31Z

@mrunalp , @wking , the latest PR is in.

mrunalp · 2016-07-15T23:22:10Z

generate/generate.go

+}
+
+// ClearLinuxUIDMapping clears g.spec.Linux.UIDMappings.
+func (g Generator) ClearLinuxUIDMapping() {


ClearLinuxUIDmappings

haiyanmeng · 2016-07-16T00:29:20Z

@mrunalp , @wking , fixed the problems you pointed out in a separate commit 0e6dfc0 . PTAL.

liangchenye · 2016-07-18T02:29:01Z

0e6dfc0 LGTM

mrunalp · 2016-07-18T18:40:26Z

generate/generate.go

+
+func checkCap(c string) error {
+	isValid := false
+	cp := fmt.Sprintf("CAP_%s", c)


This isn't required. The list you are checking against doesn't have CAP_

The list I am checking against is capability.List(), which includes CAP_CHOWN, CAP_DAC_OVERRIDE, and so on.

Ignore my comment. I got your point.

mrunalp · 2016-07-18T18:44:05Z

--seccomp-default doesn't get populated

[root@dhcp-16-129 busybox]# ocitools generate --tty --output=config.json --seccomp-default SCMP_ACT_ALLOW --seccomp-errno getcwd
[root@dhcp-16-129 busybox]# tail -n 20 config.json 
                        {
                                "type": "mount"
                        }
                ],
                "seccomp": {
                        "defaultAction": "",
                        "architectures": null,
                        "syscalls": [
                                {
                                        "name": "getcwd",
                                        "action": "SCMP_ACT_ERRNO"
                                }
                        ]
                }
        },
        "solaris": {
                "cappedCPU": {},
                "cappedMemory": {}
        }

mrunalp · 2016-07-18T18:44:53Z

Also trying to add a capability or dropping it fails
Try --cap-add SYS_ADMIN. I have pointed to the problem in earlier comment.

haiyanmeng · 2016-07-18T19:43:42Z

@mrunalp , fixed the problems you pointed out and also tested all the other options, PTAL.

mrunalp · 2016-07-18T19:51:49Z

Can you re-order or squash the commits?

haiyanmeng · 2016-07-18T19:53:18Z

@mrunalp , no problem. Are you done checking the latest changes?

Signed-off-by: Haiyan Meng <hmeng@redhat.com>

mrunalp · 2016-07-18T19:55:17Z

@hmeng-19 Yeah, the changes look good and am ready to merge.

haiyanmeng · 2016-07-18T19:55:59Z

@mrunalp , I sqashed the commits.

mrunalp · 2016-07-18T19:57:24Z

LGTM

wking reviewed Jul 9, 2016
View reviewed changes

haiyanmeng force-pushed the 130-libgen branch from 31848b5 to 795618a Compare July 11, 2016 21:53

mrunalp reviewed Jul 13, 2016
View reviewed changes

mrunalp mentioned this pull request Jul 13, 2016

translate: Stub in a translation framework #54

Closed

haiyanmeng force-pushed the 130-libgen branch from fed976f to b8b2673 Compare July 13, 2016 18:07

haiyanmeng force-pushed the 130-libgen branch from b8b2673 to 800afe4 Compare July 14, 2016 14:20

haiyanmeng force-pushed the 130-libgen branch from a230f83 to e2136ae Compare July 15, 2016 23:03

mrunalp reviewed Jul 15, 2016
View reviewed changes

mrunalp reviewed Jul 18, 2016
View reviewed changes

haiyanmeng force-pushed the 130-libgen branch 3 times, most recently from 2a0235e to 9e31200 Compare July 18, 2016 19:38

Move generate code into a separate library

80e63b3

Signed-off-by: Haiyan Meng <hmeng@redhat.com>

haiyanmeng force-pushed the 130-libgen branch from 9e31200 to 80e63b3 Compare July 18, 2016 19:54

mrunalp merged commit cc6b676 into opencontainers:master Jul 18, 2016

mrunalp mentioned this pull request Jul 18, 2016

Use oci-seccomp-gen library for adding seccomp configurations #128

Closed

haiyanmeng deleted the 130-libgen branch July 18, 2016 19:59

wking mentioned this pull request Jul 20, 2016

generate: Adjust to Spec.Linux being a pointer #112

Merged

wking mentioned this pull request Aug 20, 2016

Discuss ocitools moving into the runtime-spec repo #83

Closed

Move generate code into a separate library #131

Move generate code into a separate library #131

Conversation

haiyanmeng commented Jul 9, 2016

haiyanmeng commented Jul 9, 2016

haiyanmeng commented Jul 9, 2016

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

wking commented Jul 9, 2016

Choose a reason for hiding this comment

mrunalp commented Jul 11, 2016

haiyanmeng commented Jul 11, 2016

haiyanmeng commented Jul 11, 2016

haiyanmeng commented Jul 11, 2016

wking commented Jul 11, 2016 • edited Loading

haiyanmeng commented Jul 11, 2016

wking commented Jul 11, 2016

haiyanmeng commented Jul 11, 2016

haiyanmeng commented Jul 11, 2016

wking commented Jul 11, 2016

grantseltzer commented Jul 12, 2016

wking commented Jul 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Jul 13, 2016

haiyanmeng commented Jul 13, 2016

mrunalp commented Jul 13, 2016 • edited Loading

wking commented Jul 13, 2016

mrunalp commented Jul 15, 2016

haiyanmeng commented Jul 15, 2016

Choose a reason for hiding this comment

haiyanmeng commented Jul 16, 2016

liangchenye commented Jul 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Jul 18, 2016

mrunalp commented Jul 18, 2016

haiyanmeng commented Jul 18, 2016

mrunalp commented Jul 18, 2016

haiyanmeng commented Jul 18, 2016

mrunalp commented Jul 18, 2016

haiyanmeng commented Jul 18, 2016

mrunalp commented Jul 18, 2016

wking commented Jul 11, 2016 •

edited

Loading

mrunalp commented Jul 13, 2016 •

edited

Loading