Skip to content

Commit

Permalink
chore: disallow duplicate documents on decoder level
Browse files Browse the repository at this point in the history
Required for #9275

Signed-off-by: Dmitriy Matrenichev <dmitry.matrenichev@siderolabs.com>
  • Loading branch information
DmitriyMV committed Sep 6, 2024
1 parent bcaf636 commit cd7c682
Show file tree
Hide file tree
Showing 8 changed files with 93 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func ControlPlane(defaultAction nethelpers.DefaultAction, cidrs []netip.Prefix,
panic(err)
}

return configpatcher.StrategicMergePatch{Provider: provider}
return configpatcher.NewStrategicMergePatch(provider)
}

// Worker generates a default firewall for a worker node.
Expand Down Expand Up @@ -187,5 +187,5 @@ func Worker(defaultAction nethelpers.DefaultAction, cidrs []netip.Prefix, gatewa
panic(err)
}

return configpatcher.StrategicMergePatch{Provider: provider}
return configpatcher.NewStrategicMergePatch(provider)
}
42 changes: 42 additions & 0 deletions pkg/machinery/config/configloader/internal/decoder/decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package decoder

import (
"cmp"
"errors"
"fmt"
"io"
Expand Down Expand Up @@ -47,6 +48,12 @@ func NewDecoder() *Decoder {
return &Decoder{}
}

type documentID struct {
APIVersion string
Kind string
Name string
}

func parse(r io.Reader) (decoded []config.Document, err error) {
// Recover from yaml.v3 panics because we rely on machine configuration loading _a lot_.
defer func() {
Expand All @@ -61,6 +68,8 @@ func parse(r io.Reader) (decoded []config.Document, err error) {

dec.KnownFields(true)

knownDocuments := map[documentID]struct{}{}

// Iterate through all defined documents.
for {
var manifests yaml.Node
Expand All @@ -78,6 +87,18 @@ func parse(r io.Reader) (decoded []config.Document, err error) {
}

for _, manifest := range manifests.Content {
id := documentID{
APIVersion: findValue(manifest, ManifestAPIVersionKey, false),
Kind: cmp.Or(findValue(manifest, ManifestKindKey, false), "v1alpha1"),
Name: findValue(manifest, "name", false),
}

if _, ok := knownDocuments[id]; ok {
return nil, fmt.Errorf("duplicate document %s/%s/%s is not allowed", id.APIVersion, id.Kind, id.Name)
}

knownDocuments[id] = struct{}{}

var target config.Document

if target, err = decode(manifest); err != nil {
Expand Down Expand Up @@ -146,3 +167,24 @@ func decode(manifest *yaml.Node) (target config.Document, err error) {

return target, nil
}

func findValue(node *yaml.Node, key string, required bool) string {
if node.Kind != yaml.MappingNode {
panic(errors.New("expected a mapping node"))
}

for i := 0; i < len(node.Content)-1; i += 2 {
keyNode := node.Content[i]
val := node.Content[i+1]

if keyNode.Kind == yaml.ScalarNode && keyNode.Value == key {
return val.Value
}
}

if required {
panic(fmt.Errorf("missing '%s'", key))
}

return ""
}
14 changes: 14 additions & 0 deletions pkg/machinery/config/configloader/internal/decoder/decoder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ package decoder_test

import (
"bytes"
"io/fs"
"os"
"path/filepath"
"testing"

"github.com/siderolabs/gen/xtesting/must"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -345,6 +347,18 @@ func TestDecoderV1Alpha1Config(t *testing.T) {
}
}

func TestDoubleV1Alpha1(t *testing.T) {
t.Parallel()

files := os.DirFS("testdata/double").(fs.ReadFileFS) //nolint:errcheck
contents := must.Value(files.ReadFile("v1alpha1.yaml"))(t)

d := decoder.NewDecoder()
_, err := d.Decode(bytes.NewReader(contents))
require.Error(t, err)
require.ErrorContains(t, err, "not allowed")
}

func BenchmarkDecoderV1Alpha1Config(b *testing.B) {
b.ReportAllocs()

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
version: v1alpha1
machine:
type: controlplane
---
version: v1alpha1
machine:
type: worker
4 changes: 2 additions & 2 deletions pkg/machinery/config/configpatcher/load.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ type patch []map[string]any

// LoadPatch loads the strategic merge patch or JSON patch (JSON/YAML for JSON patch).
func LoadPatch(in []byte) (Patch, error) {
// try configloader first, it is more strict about config format
// Try configloader first, as it is more strict about the config format
cfg, strategicErr := configloader.NewFromBytes(in)
if strategicErr == nil {
return StrategicMergePatch{cfg}, nil
return NewStrategicMergePatch(cfg), nil
}

var (
Expand Down
4 changes: 2 additions & 2 deletions pkg/machinery/config/configpatcher/load_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestLoadStrategic(t *testing.T) {
p, ok := raw.(configpatcher.StrategicMergePatch)
require.True(t, ok)

assert.Equal(t, "foo.bar", p.Machine().Network().Hostname())
assert.Equal(t, "foo.bar", p.Provider().Machine().Network().Hostname())
}

func TestLoadJSONPatches(t *testing.T) {
Expand Down Expand Up @@ -106,6 +106,6 @@ func TestLoadMixedPatches(t *testing.T) {
require.Len(t, patchList, 3)

assert.IsType(t, jsonpatch.Patch{}, patchList[0])
assert.IsType(t, configpatcher.StrategicMergePatch{}, patchList[1])
assert.Implements(t, (*configpatcher.StrategicMergePatch)(nil), patchList[1])
assert.IsType(t, jsonpatch.Patch{}, patchList[2])
}
22 changes: 20 additions & 2 deletions pkg/machinery/config/configpatcher/strategic.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ import (
)

// StrategicMergePatch is a strategic merge config patch.
type StrategicMergePatch struct {
coreconfig.Provider
type StrategicMergePatch interface {
Documents() []config.Document
Provider() coreconfig.Provider
}

// StrategicMerge performs strategic merge config patching.
Expand Down Expand Up @@ -55,3 +56,20 @@ func StrategicMerge(cfg coreconfig.Provider, patch StrategicMergePatch) (corecon

return container.New(left...)
}

// NewStrategicMergePatch creates a new strategic merge patch. deleteSelectors is a list of delete selectors, can be empty.
func NewStrategicMergePatch(cfg coreconfig.Provider) StrategicMergePatch {
return strategicMergePatch{provider: cfg}
}

type strategicMergePatch struct {
provider coreconfig.Provider
}

func (s strategicMergePatch) Documents() []config.Document {
return s.provider.Documents()
}

func (s strategicMergePatch) Provider() coreconfig.Provider { return s.provider }

var _ StrategicMergePatch = strategicMergePatch{}
6 changes: 4 additions & 2 deletions pkg/machinery/config/container/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ func NewV1Alpha1(config *v1alpha1.Config) *Container {
// Clone the container.
//
// Cloned container is not readonly.
func (container *Container) Clone() coreconfig.Provider {
func (container *Container) Clone() coreconfig.Provider { return container.clone() }

func (container *Container) clone() *Container {
return &Container{
v1alpha1Config: container.v1alpha1Config.DeepCopy(),
documents: xslices.Map(container.documents, config.Document.Clone),
Expand Down Expand Up @@ -304,7 +306,7 @@ func (container *Container) Validate(mode validation.RuntimeMode, opt ...validat

// RedactSecrets returns a copy of the Provider with all secrets replaced with the given string.
func (container *Container) RedactSecrets(replacement string) coreconfig.Provider {
clone := container.Clone().(*Container) //nolint:forcetypeassert,errcheck
clone := container.clone()

if clone.v1alpha1Config != nil {
clone.v1alpha1Config.Redact(replacement)
Expand Down

0 comments on commit cd7c682

Please sign in to comment.