coreos / ignition

First boot installer and configuration tool
https://coreos.github.io/ignition/
Apache License 2.0
837 stars 247 forks source link

Opt-in translation between config versions (vs. current required support) #602

Closed wking closed 6 years ago

wking commented 6 years ago

Currently config/v2_2 is importing config/v2_1 for translation (and backwards-compat). It would be nice for that to be optional (with the logic stored in a separate package), so consumers who knew they only needed to support v2.2 configs could prune the code that supports older config versions.

I'm sure there are many possible approaches to this sort of issue, but see here for an approach I've used previously. A generic version struct lets you extract the target version. You can then lookup the appropriate reader from a registry. The per-version readers register themselves on import. And consumers who want backwards compat opt in by importing the old versions they want to support.

If this is an issue you want to address, and the libpod hook approach sounds reasonable, I can work up an ignition PR for it.

ajeddeloh commented 6 years ago

I'm not opposed to reworking it, there's a lot of little gotchas that need special care however.

To make sure I understand your proposal correctly: you're suggesting determining the version ahead of time (btw that version struct is a little messy since v1 and v2.x configs have version in a different spots) and calling that Parse directly, then iteratively calling the translation function to bring it to the latest version. The translation code would move to a seperate package (perhaps a subpackage for each version). This would also have the side effect of not needing to duplicate version / is-this-an-ign-config detection in every config version.

Tricky bits:

It used to be upgrading the spec was a somewhat nightmarish operation. Derek fixed that in https://github.com/coreos/ignition/pull/519 (see also https://github.com/coreos/bugs/issues/2346). The two big things we gained were:

wking commented 6 years ago

To make sure I understand your proposal correctly...

Yup, all of this sounds like what I mean.

Ignition configs can be chained across versions. This works because internally everything is translated up to the same version before appending. We must not break that.

I'm suggesting we give consumers the option of breaking that. For example, maybe OpenShift's installer (which currently both generates and parses ignition configs), decides that parsing version 2.0 ignition configs no longer needs to be supported. I think they should be able to make that call independently of the compiled ignition tool deciding to drop 2.0 support.

Ignition needs to ignore anything that isn't an Ignition config.

Ah, I see that code here. Can that become optional instead of making it a required part of Parse? It seems like some package consumers might be interested in parsing errors vs. ignoring them. With a package API that exposed parsing errors, the consuming caller (e.g. the ignition executable) could still continue to ignore parsing errors, and in their case, they'd also be ignoring ignition configs that were old enough to have had support dropped.

Other tools (e.g. ct, matchbox) vendor Ignition. We need to be sure that we keep the same functionality exported and make sure the new API is at least as good as the previous one.

Absolutely. Do you know how they'll handle breaking API changes? Maybe breaking changes are ok, and they'll bump their usage when they bump their vendored copy around the change? Or maybe they need the ability to bump blindly without needing API updates, in which case we may need to add the new API in github.com/coreos/ignition/config2 or some such with the old github.com/coreos/ignition/config API as a shim wrapper around the new API.

Updating the spec version needs to be remain an easy, not-error-prone process.

I think this should be fine. The approach I'm proposing has most of the same parts as your current approach; they're just shuffled into packages slightly differently.

bgilbert commented 6 years ago

Ignition configs can be chained across versions. This works because internally everything is translated up to the same version before appending. We must not break that.

I'm suggesting we give consumers the option of breaking that. For example, maybe OpenShift's installer (which currently both generates and parses ignition configs), decides that parsing version 2.0 ignition configs no longer needs to be supported. I think they should be able to make that call independently of the compiled ignition tool deciding to drop 2.0 support.

I don't think we should encourage a fragmented ecosystem in which each tool decides which Ignition config versions to support. Backward compatibility should be the default.

Is the OpenShift case the only one at issue here, or do you have other cases in mind? Will it cause problems if OpenShift ships compatibility code for an Ignition spec version it doesn't intend to support?

With a package API that exposed parsing errors, the consuming caller (e.g. the ignition executable) could still continue to ignore parsing errors, and in their case, they'd also be ignoring ignition configs that were old enough to have had support dropped.

Ignition shouldn't ignore obsolete config versions; it should fail on them. What we need to ignore is things that are not Ignition configs at all, since that implies the userdata wasn't intended for us.

Maybe breaking changes are ok, and they'll bump their usage when they bump their vendored copy around the change?

Historically that's been the case. We're trying to break API less often, though.

wking commented 6 years ago

I don't think we should encourage a fragmented ecosystem in which each tool decides which Ignition config versions to support. Backward compatibility should be the default.

For all tools? It seems like whoever is in charge of generating the initial config should be in charge of validating it. If OpenShift generates v2.2 configs, it shouldn't need to care about other versions. If OpenShift accepts a user-supplied config as an override, I think we should leave validating that config to the user (although see here for some plans that don't agree ;). But with the emphasis on these configs being auto-generated, I don't think we have as much need for the long backwards-compat tails needed for human-written configs.

With the approach I proposed above, it's slightly harder to make it opt-out. Would you use Go build tags? With the opt-in approach I've proposed, if a given consumer forgets to import packages to support an old version, folks feeding that version will get a:

ignition config version 1.2.3 not supported

error (or similar) back, and can file an issue with their tool "you forgot to support version 1.2.3" to get the tool to add the missing import.

Is the OpenShift case the only one at issue here, or do you have other cases in mind?

I'm only interested in this for OpenShift, and it's not even a big deal there. I just thought it seemed like low-hanging fruit for reducing the OpenShift installer dependencies.

Will it cause problems if OpenShift ships compatibility code for an Ignition spec version it doesn't intend to support?

I don't think so. Personally, I'd prefer OpenShift just generate one version of ignition config (ideally the most recent stable version), and drop support for older ignition versions much sooner than ignition itself does. But if OpenShift decides to (continue to?) be responsible for also validating any user-supplied ignition configs, I'd expect it to translate them to the most recent ignition-config version it supports. So as far as ignition is concerned, OpenShift would only ever be supplying the most recent stable version (modulo vendor-bump lag).

It would cause problems if OpenShift supported user-supplied ignition configs for old versions of the config format and did not translate them. But I don't see an upside to that approach, so translating user-supplied configs (or just passing them through without looking at them) should be non-controversial.

Ignition shouldn't ignore obsolete config versions; it should fail on them. What we need to ignore is things that are not Ignition configs at all, since that implies the userdata wasn't intended for us.

Ah, that makes sense, and should be fairly straightforward since we can use the presence of ignition (or ignitionVersion) as markers.

bgilbert commented 6 years ago

I don't think we should encourage a fragmented ecosystem in which each tool decides which Ignition config versions to support. Backward compatibility should be the default.

For all tools? It seems like whoever is in charge of generating the initial config should be in charge of validating it. If OpenShift generates v2.2 configs, it shouldn't need to care about other versions.

For all tools. Otherwise there's a substantially increased risk that the provisioning pipeline, from Ignition config generator through OpenShift or matchbox or whatever to Ignition, will not have any supported spec versions in common.

Whether OpenShift chooses to validate user-provided configs seems orthogonal to me. But the whole point of translation is that OpenShift can consume a version 2.0 config without understanding anything about spec 2.0, because translation will give OpenShift the config version it wants.

But with the emphasis on these configs [being auto-generated][2], I don't think we have as much need for the long backwards-compat tails needed for human-written configs.

All Ignition configs should be auto-generated; humans should write Container Linux Configs or whatever equivalent we build for RHCOS and FCOS. The long tail consists of configs that were generated a long time ago or with obsolete tools but still need to be consumed by the downstream provisioning pipeline.

I just thought it seemed like low-hanging fruit for reducing the OpenShift installer dependencies.

How so? Even if we completely dropped old spec versions from Ignition, I don't think it would make the Ignition dependency graph any smaller.

Personally, I'd prefer OpenShift just generate one version of ignition config (ideally the most recent stable version), and drop support for older ignition versions much sooner than ignition itself does. But if OpenShift decides to (continue to?) be responsible for also validating any user-supplied ignition configs, I'd expect it to translate them to the most recent ignition-config version it supports. So as far as ignition is concerned, OpenShift would only ever be supplying the most recent stable version (modulo vendor-bump lag).

It would cause problems if OpenShift supported user-supplied ignition configs for old versions of the config format and did not translate them. But I don't see an upside to that approach, so translating user-supplied configs (or just passing them through without looking at them) should be non-controversial.

I agree with all of that, so I'm not sure I understand the problem this issue is trying to address.

wking commented 6 years ago

For all tools. Otherwise there's a substantially increased risk that the provisioning pipeline, from Ignition config generator through OpenShift or matchbox or whatever to Ignition, will not have any supported spec versions in common.

Whether OpenShift chooses to validate user-provided configs seems orthogonal to me. But the whole point of translation is that OpenShift can consume a version 2.0 config without understanding anything about spec 2.0, because translation will give OpenShift the config version it wants.

OpenShift is not really consuming Ignition configs (as far as I can tell; I'm pretty new ;). Let's assume it decides not to validate user-supplied Ignition configs, and it chooses to generate v2.2 configs. This is fairly close to the current installer. In that situation, I don't see why openshift/installer should be vendoring code for v2.1 and older versions.

All Ignition configs should be auto-generated; humans should write Container Linux Configs or whatever equivalent we build for RHCOS and FCOS.

So what about generating the configs in code (for example, the OpenShift installer injecting a user block)? That's somewhat auto-generated, but the code is not necessarily written by someone who understands all the intricacies of Ignition configs. Would you prefer that OpenShift operated on the Container Linux Config level internally, and then transpiled that using code vendored from the transpiler as a final step before pushing assets to S3 (or wherever)?

The long tail consists of configs that were generated a long time ago or with obsolete tools but still need to be consumed by the downstream provisioning pipeline.

This makes sense for Ignition itself. I'm not sure it makes sense for the OpenShift installer. The installer should certainly not be generating obsolete Ignition configs itself, but that seems orthogonal to whether the Ignition packages require you to support all past config versions. Users might provide obsolete Ignition configs as OpenShift installer input, but I'm fine leaving it to those users to modernize their config before feeding it in. That modernization could be via a separate tool you can go get ... from this repository, so I don't see a need to compile support into the OpenShift installer.

I just thought it seemed like low-hanging fruit for reducing the OpenShift installer dependencies.

How so? Even if we completely dropped old spec versions from Ignition, I don't think it would make the Ignition dependency graph any smaller.

It would make the OpenShift installer dependency graph slightly smaller, because we could prune the unused v1, v2.0, and v2.1 compatibility code we're currently vendoring (e.g. here).

I agree with all of that, so I'm not sure I understand the problem this issue is trying to address.

I'm just trying to save a few hundred KB from OpenShift installer's vendor/ and the associated compiled binary. Nothing complicated, or particularly important.

bgilbert commented 6 years ago

All Ignition configs should be auto-generated; humans should write Container Linux Configs or whatever equivalent we build for RHCOS and FCOS.

So what about generating the configs in code (for example, the OpenShift installer injecting a user block)? That's somewhat auto-generated, but the code is not necessarily written by someone who understands all the intricacies of Ignition configs. Would you prefer that OpenShift operated on the Container Linux Config level internally, and then transpiled that using code vendored from the transpiler as a final step before pushing assets to S3 (or wherever)?

I think the current consensus is that config generators should generate Ignition configs rather than CLCs. The Ignition config schema is mapped nearly one-to-one to the CLC schema; the difference is mostly that the CLC schema has some additional sugar for configuring particular services.

Users might provide obsolete Ignition configs as OpenShift installer input, but I'm fine leaving it to those users to modernize their config before feeding it in. That modernization could be via a separate tool you can go get ... from this repository, so I don't see a need to compile support into the OpenShift installer.

That's exactly the problem I'm concerned about. If upgrading OpenShift (or matchbox, or ...) requires a user to hand-convert all of their existing configs to a new version, that's not a great user experience.

I'm just trying to save a few hundred KB from OpenShift installer's vendor/ and the associated compiled binary. Nothing complicated, or particularly important.

Okay. Other Ignition folks are welcome to chime in here, but I'm inclined to say this isn't a priority at the moment.

wking commented 6 years ago

Users might provide obsolete Ignition configs as OpenShift installer input, but I'm fine leaving it to those users to modernize their config before feeding it in. That modernization could be via a separate tool you can go get ... from this repository, so I don't see a need to compile support into the OpenShift installer.

That's exactly the problem I'm concerned about. If upgrading OpenShift (or matchbox, or ...) requires a user to hand-convert all of their existing configs to a new version, that's not a great user experience.

With the opaque-pass-through approach, users would not need to upgrade their configs as long as Ignition itself continued to support the old versions.

Okay. Other Ignition folks are welcome to chime in here, but I'm inclined to say this isn't a priority at the moment.

Fair enough. I may circle back an reopen if I make progress on the opaque-pass-through approach ;).