[NO MERGE] generate: Adjust command-line generator to use .Config directly

[wking]

This series has two (new) commits on top of #266. The first removes a lot of trivial generator methods and exposes the pointer-init methods. The second transitions the command-line generator to use .Config directly. I don't actually care if we remove the trivial generator methods or not (I think the API is cleaner without them, but if maintainers like them enough to want to maintain them, I don't have a problem with that).

The point of this PR is that the second commit allows maintainers to assess the effect of a public-Config approach. @Mashimiao had expressed concerns in #266 (where I'm floating the public-Config change), and I thought looking at the second commit here would help show that:

• In most cases, there's basically no difference in usability.
• In a few cases, (e.g. process.env and other array appends, and hook/mount type appends) the direct access is more convenient than the old methods.

I didn't see anywhere where the direct access was less convenient, but that's because I kept the wrapping methods that held non-trivial logic. And I am certainly in favor of continuing to provide those wrappers where we find the wrapper more convenient than direct access.

[wking]

I've added a NO MERGE tag to the PR subject, because (as explained in the topic post), this PR is intended to help evaluate #266. I don't think this PR is polished enough to land on its own (although if #266 lands and any parts of this PR look interesting, I'm happy to polish them up).

[Mashimiao]

From the code, part of them uses Config directly, part of them still use set/add function(which needs loop logic or value checks).
I can't deny currently most of set/add functions seem valueless, but some of them(which needs loop logic or value checks) I think is really helpful to keep code of command generate concise.
And I think currently accept values from them and applying to config is not very suitable, we should add some values checks before applying them and that's also what I plan to do.
And If there was no big difference in usability, I can't see any strong requirements to change them except spending time in maintaining set/add functions.
any opinions from others?

[wking]

On Mon, Nov 07, 2016 at 11:21:25PM -0800, Ma Shimiao wrote:

From the code, part of them uses Config directly, part of them still
use set/add function(which needs loop logic or value checks). I
can't deny currently most of set/add functions seem valueless, but
some of them(which needs loop logic or value checks) I think is
really helpful to keep code of command generate concise.

I agree, and I've kept those methods here. Having a public Config
doesn't mean we have to remove all the wrapper methods. It just means
we can focus wrappers on the tricky parts.

And I think currently accept values from them and applying to config
is not very suitable, we should add some values checks before
applying them and that's also what I plan to do.

Value checks are nice, but some validation involves comparing multiple
settings (e.g. you could error out if someone tries to set a
hostname but doesn't create a new UTS namespace). I'd rather do
everything the caller asked without much validation, and leave it to
them to call ‘oci-runtime-tool validate …’ when they feel their config
is complete.

And If there was no big difference in usability, I can't see any
strong requirements to change them except spending time in
maintaining set/add functions.

Besides less maintainer time on existing wrappers, it also lifts the
need for additional wrappers (e.g. AddMount 1) with one stroke. Of
course, there may be additional wrappers to be written that actually
add convenience, but I expect that number to be smaller than the
number of trivial wrappers we still need to cover the still-missing
runtime-spec properties (we haven't even started on Windows/Solaris
yet).

[Mashimiao]

On 11/08/2016 03:32 PM, W. Trevor King wrote:

everything the caller asked without much validation, and leave it to
them to call ‘oci-runtime-tool validate …’ when they feel their config
is complete.

I am also considering about call 'oci-runtime-tool validate'.
But now part of validation has close link with bundle, we need to modify this kind of validation.
and check in generate may not need so detailed validation.
So, I haven't decided to choose which. adding checking in wrapper function in generator or modify
validate and call it.

[wking]

On Mon, Nov 07, 2016 at 11:47:08PM -0800, Ma Shimiao wrote:

But now part of validation has close link with bundle, we need to
modify this kind of validation. and check in generate may not need
so detailed validation. So, I haven't decided to choose
which. adding checking in wrapper function in generator or modify
validate and call it.

I think there could be a few useful levels here:

1. Validating the whole bundle (“is there a directory at root.path?”,
   etc.).
2. Validating the config in isolation (potentially still host-specific
   checks for kernel support, etc., but no filesystem checks).
3. Validating sub-sections of the config (e.g. just
   Linux.RootfsPropagation).

The only validation I noticed myself removing in this PR was the
switch in SetLinuxRootPropagation (which falls under (3)). If it's
important enough to keep that validation in ‘oci-runtime-tools
genarate …’ (and I'm fine either way), I recommend we move it to
validate/validate.go as:

func (v *Validator) CheckLinuxRootfsPropagation() (msgs []string)

I don't think the lack of a real Validator.bundlePath would be a
problem for that check.

It would be nice to provide a way to conveniently check all such
bundlePath-agnostic properties, but then we might want to give callers
a way to disable the validation (maybe they were planning on fixing a
broken value in post-generate processing?). But instead of:

$ oci-runtime-tools generate --validate …

and:

$ oci-runtime-tools generate --no-validate …

it seems like it would be easier to just suggest folks call:

$ oci-runtime-tools generate … && oci-runtime-tools validate …

if they felt like they were ready for validation. But I'm fine with
whatever maintainers want here. And even if they think the best way
to handle this validation is with a generate wrapper like
SetLinuxRootPropagation, that's fine with me. I still think we want a
public Config ;).

[cyphar]

NACK. I could not disagree more with this proposal.

The whole benefit of generate is that it's an interface which doesn't requiring knowing exactly what the name of a particular attribute is. In particular, I'm hoping that once the spec starts evolving even more we will keep the current interface constant (only adding new methods) but we can change the implementation to modify different fields of the spec.

By removing all of the "useless" .Set*s you're removing the whole goddamn point of this library.

[wking]

On Wed, Nov 09, 2016 at 06:41:05AM -0800, Aleksa Sarai wrote:

The whole benefit of generate is that it's an interface which
doesn't requiring knowing exactly what the name of a particular
attribute is.

I have no problem with the runtime-tools maintainers continuing to
support wrappers like SetProcessNoNewPrivileges if they like. Landing
#266 (whose new public .Config this no-merge PR just demostrates)
means that users who want direct access to the config (because they
find it more convenient or because the generate package doesn't wrap
what they need to do) can use it.

More broadly, I see a few points to the current generate package:

a. It makes it easy to generate a default spec 1.
b. It makes it easy to perform high-level operations, like granting
privileges 2.
c. It makes it easy to interact with Go arrays whose semantics make
them more like sets or objects (e.g. process.capabilities 3 and
linux.namespaces 4. process.rlimits will fall under this too
once we wrap them, unless opencontainers/runtime-spec#583 lands
upstream).
d. It might buffer you from future Go-type shuffling in runtime-spec,
depending on maintainer policy.

My personal position is that the spec will be stable enough that (d)
is more trouble than it's worth. But maintainers and users who want
(d) can certainly continue to maintain and use those wrapping methods.

Keeping the config private, on the other hand, means that users who
need direct access need to jump through a hoop like:

config := gen.Spec()
config.Process.Env = append(config.Process.Env, context.StringSlice("env")...)
config.Process.Rlimits = []rspec.Rlimit{
{
Type: "RLIMIT_NOFILE",
Hard: uint64(4096),
Soft: uint64(4096),
},
}
config.Hooks = platformDefaultHooks()
gen2 := generate.NewFromSpec(config)
gen2.HostSpecific = gen.HostSpecific
gen = gen2

I don't see a point in keeping that hoop in place.

[rhatdan]

Can someone close this PR.