Avoid (second) copy when combining layers

[flotter]

By my evaluation of the code, the plan library is implemented with very specific handling of the layers. The content of layers come from the original layers directory load, and can then be further modified through HTTP API append and combine calls.

However, when performing any combine of layers, deep copies of layer contents must be used to prevent reference / pointer types backed by memory inside the layer struct from getting mutated during the lifetime of the process. Allowing this will corrupt future combine attempts of the original layers stack.

At the top level we seem to also copy the contents that may originate from the combined layer, which already can only contain copied content, as it always starts empty.

Although this does not hurt, with plan extensions support, it may be a good idea to have the code as accurate as possible, as this will typically be copy/pasted as reference code when implementing an extension.

In the code snippet below, do we really want to copy the already combined service, which itself is a copy?

CombineLayers:

    combined := &Layer{
        Services:   make(map[string]*Service),
        Checks:     make(map[string]*Check),
        LogTargets: make(map[string]*LogTarget),
        Sections:   make(map[string]Section),
    }
    :
    combined.Summary = last.Summary
    combined.Description = last.Description
    for _, layer := range layers {
        for name, service := range layer.Services {
            switch service.Override {
            case MergeOverride:
                if old, ok := combined.Services[name]; ok {
                    copied := old.Copy()    <<<============================== ?
                    copied.Merge(service)
                    combined.Services[name] = copied
                    break
                }
                fallthrough
            case ReplaceOverride:
                combined.Services[name] = service.Copy()
            case UnknownOverride:
                return nil, &FormatError{
                    Message: fmt.Sprintf(`layer %q must define "override" for service %q`,
                        layer.Label, service.Name),
                }
            default:
                return nil, &FormatError{
                    Message: fmt.Sprintf(`layer %q has invalid "override" value for service %q`,
                        layer.Label, service.Name),
                }
            }
        }
        :    

[flotter]

@benhoyt @hpidcock If you could share some wisdom here that would be very much appreciated! This is not urgent.

[benhoyt]

Yeah, I think you're right. Feel free to put up a PR to fix (there's probably already test coverage of these cases -- but worth double-checking). I've also never liked that fallthrough, as I just find it confusing. What about this:

			switch service.Override {
			case MergeOverride:
				if previous, ok := combined.Services[name]; ok {
					previous.Merge(service)
				} else {
					combined.Services[name] = service.Copy()
				}
			case ReplaceOverride:
				combined.Services[name] = service.Copy()
			...
			}

[benhoyt]

Hiya @flotter, any update on this?