feat(plan): disable KnownFields for YAML unmarshal

[flotter]

Disable YAML KnownFields during plan unmarshal, so that we align with the general Pebble JSON unmarshal strategy (DisallowUnknownFields is false)

Pebble currently sets KnownFields=true when loading the configuration layers, or when parsing a layer supplied over the daemon server API.

The result of this is strict attribute checking. An unmarshal error will be reported if any attribute present in the YAML does not exist in the backing code structure.

This option trades forward/backwards compatibility for user input error detection. This may have been the best choice up until this point in time.

However, Pebble is getting used in derivative projects where the ability for the derivative binary to cope with an earlier or later plan revision YAML data will make it more robust against upgrades and downgrades.

Since this relaxes the unmarshal rules, this will be backwards compatible in all existing non-error use-cases. Any existing tests expecting an error from Pebble will have to be updated/removed.

This would also align with how JSON decoding is done in Pebble (since we never set DisallowUnknownFields).

[flotter]

Following a discussion with @niemeyer on supporting KnownFields for yaml.Node (https://github.com/go-yaml/yaml/issues/460), as would have been needed to support strict unmarshalling for the plan extension system we are working on, we identified the fact that this is actually not wanted behaviour.

KnownFields=true would make Pebble unable to cope with a plan generated by an earlier or later version of Pebble (or derivative), which may have added/removed either a existing schema child attribute, or a complete plan section extension (as we add new overlord managers).

I also noticed that our JSON use in Pebble follows the proposed change (e.g. the overlord/state backend), by not setting DisallowUnknownFields.

[benhoyt]

I'm pretty wary of removing this: it's definitely a good check now that when you add a layer and mistype "services: ..." as (say) "serviced: ...", you get a clear error. With this change it'll silently think you've got no services in the new layer.

As on option, we could think of a way for derivative projects to disable this.

But is there some other way to achieve what we want here? For upgrades, we should still error in such cases (because without breaking compatibility Pebble can't remove existing fields). For downgrades I can see where something like this is needed -- but I'm wondering if that's needed, or if so, if there's a way to solve the downgrade issue specifically without turning this error checking off for all plans/layers.

[flotter]

@benhoyt , thanks for the feedback. Yeah it makes perfect sense what you are saying. In fact, I would not be surprised if Gustavo did not mean for me to change the behaviour in Pebble. This reason this came up was that it turns out the missing feature above makes the code that still allows the pebble side to use struct unmarshal (ParseLayer) pretty nasty, when combining the plan section support.

Let me check with gustavo today, and I will reach out to you to sync up on this one.

Update: @benhoyt : Had a quick chat with @niemeyer and it sounds like a quick 5min discussion would be the best way to move this forward (this is not longer urgent for me - I will reach out to you to see when would be a good time to chat). However, in the meantime, I will proceed with the plan section working keeping the behaviour of strict marshalling on the pebble side (with the slightly hacky code).

[benhoyt]

@flotter Any update here? Should we go ahead and close this?

[benhoyt]

Oops, sorry! Didn't mean to actually click close! Oh well, I'll leave it now -- we can always re-open if you'd like to pursue this approach.