[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.