Skip to content

Commit

Permalink
Fix declared.Resources to be consistently ordered
Browse files Browse the repository at this point in the history
- Resources' DeclaredUnstructureds, DeclaredObjects, DeclaredGVKs,
  and getObjectSet were previously iterating over a Go map, causing the
  output to be randomly ordered. This change replaces the map with an
  OrderedMap that can be iterated in insert order. This makes it
  easier to test classes that use declared.Resources, including
  Applier.Apply and parse.parseAndUpdate.
- Add fakes for Applier, Remediator, and ConfigParser
- Add test validation for Apply, Remediate, and Parse inputs and outputs
- Fix fake.RootSync/RepoSync to avoid populating the status. These
  return a FileObject, which never have a status when created by the
  parser.
- Fix filesystem.AsCoreObjects to return nil when empty
- Replace kpt.dev/configsync/pkg/util/ordered with
  github.com/elliotchance/orderedmap/v2 which supports generics and
  actually has unit tests.
  • Loading branch information
karlkfi committed May 3, 2024
1 parent a701d09 commit ee8d669
Show file tree
Hide file tree
Showing 29 changed files with 902 additions and 982 deletions.
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ require (
github.com/Masterminds/semver v1.5.0
github.com/Masterminds/semver/v3 v3.2.1
github.com/davecgh/go-spew v1.1.1
github.com/elliotchance/orderedmap/v2 v2.2.0
github.com/ettle/strcase v0.1.1
github.com/evanphx/json-patch v4.12.0+incompatible
github.com/go-logr/logr v1.2.3
Expand All @@ -29,7 +30,6 @@ require (
github.com/spf13/cobra v1.6.1
github.com/spyzhov/ajson v0.7.2
github.com/stretchr/testify v1.8.1
github.com/wk8/go-ordered-map v1.0.0
go.opencensus.io v0.24.0
go.uber.org/multierr v1.10.0
go.uber.org/zap v1.26.0
Expand Down
5 changes: 2 additions & 3 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ github.com/docker/docker-credential-helpers v0.7.0/go.mod h1:rETQfLdHNT3foU5kuNk
github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3ebgob9U8Nd0kOddGdZWjyMGR8Wziv+TBNwSE=
github.com/elazarl/goproxy v0.0.0-20180725130230-947c36da3153 h1:yUdfgN0XgIJw7foRItutHYUIhlcKzcSf5vDpdhQAKTc=
github.com/elazarl/goproxy v0.0.0-20180725130230-947c36da3153/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc=
github.com/elliotchance/orderedmap/v2 v2.2.0 h1:7/2iwO98kYT4XkOjA9mBEIwvi4KpGB4cyHeOFOnj4Vk=
github.com/elliotchance/orderedmap/v2 v2.2.0/go.mod h1:85lZyVbpGaGvHvnKa7Qhx7zncAdBIBq6u56Hb1PRU5Q=
github.com/emicklei/go-restful/v3 v3.9.0 h1:XwGDlfxEnQZzuopoqxwSEllNcCOM9DhhFyhFIIGKwxE=
github.com/emicklei/go-restful/v3 v3.9.0/go.mod h1:6n3XBCmQQb25CM2LCACGz8ukIrRry+4bhvbpWn3mrbc=
github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
Expand Down Expand Up @@ -362,7 +364,6 @@ github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXf
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
Expand All @@ -371,8 +372,6 @@ github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o
github.com/urfave/cli v1.22.4/go.mod h1:Gos4lmkARVdJ6EkW0WaNv/tZAAMe9V7XWyB60NtXRu0=
github.com/vbatts/tar-split v0.11.2 h1:Via6XqJr0hceW4wff3QRzD5gAk/tatMw/4ZA7cTlIME=
github.com/vbatts/tar-split v0.11.2/go.mod h1:vV3ZuO2yWSVsz+pfFzDG/upWH1JhjOiEaWq6kXyQ3VI=
github.com/wk8/go-ordered-map v1.0.0 h1:BV7z+2PaK8LTSd/mWgY12HyMAo5CEgkHqbkVq2thqr8=
github.com/wk8/go-ordered-map v1.0.0/go.mod h1:9ZIbRunKbuvfPKyBP1SIKLcXNlv74YCOZ3t3VTS6gRk=
github.com/xeipuuv/gojsonpointer v0.0.0-20180127040702-4e3ac2762d5f/go.mod h1:N2zxlSyiKSe5eX1tZViRH5QA0qijqEDrYZiPEAiq3wU=
github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415/go.mod h1:GwrjFmJcFw6At/Gs6z4yjiIwzuJ1/+UwLxMQDVQXShQ=
github.com/xeipuuv/gojsonschema v1.2.0/go.mod h1:anYRn/JVcOK2ZgGU+IjEV4nwlhoK5sQluxsYJ78Id3Y=
Expand Down
2 changes: 1 addition & 1 deletion pkg/applier/applier.go
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ func (a *supervisor) addError(err error) {
defer a.errorMux.Unlock()

if _, ok := err.(status.Error); !ok {
// Wrap as an applier.Error to indidate the source of the error
// Wrap as an applier.Error to indicate the source of the error
err = Error(err)
}

Expand Down
68 changes: 68 additions & 0 deletions pkg/applier/fake/applier.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package fake

import (
"context"
"fmt"

"kpt.dev/configsync/pkg/applier"
"kpt.dev/configsync/pkg/status"
"sigs.k8s.io/controller-runtime/pkg/client"
)

// Applier fakes applier.Applier.
//
// This is not in kpt.dev/configsync/pkg/testing/fake because that would cause
// a import loop (applier -> fake -> applier).
type Applier struct {
ApplyInputs []ApplierInputs
ApplyOutputs []ApplierOutputs

ApplyCalls, ErrorsCalls int

currentErrors status.MultiError
}

// ApplierInputs stores inputs for fake.Applier.Apply()
type ApplierInputs struct {
Objects []client.Object
}

// ApplierOutputs stores outputs for fake.Applier.Apply()
type ApplierOutputs struct {
Errors status.MultiError
}

// Apply fakes applier.Applier.Apply()
func (a *Applier) Apply(_ context.Context, objects []client.Object) status.MultiError {
a.ApplyInputs = append(a.ApplyInputs, ApplierInputs{
Objects: objects,
})
if a.ApplyCalls >= len(a.ApplyOutputs) {
panic(fmt.Sprintf("Expected only %d calls to Applier.Apply, but got more. Update Applier.ApplyOutputs if this is expected.", len(a.ApplyOutputs)))
}
outputs := a.ApplyOutputs[a.ApplyCalls]
a.ApplyCalls++
a.currentErrors = outputs.Errors
return outputs.Errors
}

// Errors fakes applier.Applier.Errors()
func (a *Applier) Errors() status.MultiError {
return a.currentErrors
}

var _ applier.Applier = &Applier{}
19 changes: 12 additions & 7 deletions pkg/declared/namespace_safeguard.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package declared

import (
"github.com/elliotchance/orderedmap/v2"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"kpt.dev/configsync/pkg/core"
"kpt.dev/configsync/pkg/kinds"
Expand All @@ -29,18 +30,22 @@ import (
// 2) current contains zero Namespaces.
//
// Otherwise returns nil.
func deletesAllNamespaces(previous, current map[core.ID]*unstructured.Unstructured) status.Error {
func deletesAllNamespaces(previous, current *orderedmap.OrderedMap[core.ID, *unstructured.Unstructured]) status.Error {
var previousNamespaces []string
var currentNamespaces []string

for p := range previous {
if p.GroupKind == kinds.Namespace().GroupKind() {
previousNamespaces = append(previousNamespaces, p.Name)
if previous != nil {
for pair := previous.Front(); pair != nil; pair = pair.Next() {
if pair.Key.GroupKind == kinds.Namespace().GroupKind() {
previousNamespaces = append(previousNamespaces, pair.Key.Name)
}
}
}
for c := range current {
if c.GroupKind == kinds.Namespace().GroupKind() {
currentNamespaces = append(currentNamespaces, c.Name)
if current != nil {
for pair := current.Front(); pair != nil; pair = pair.Next() {
if pair.Key.GroupKind == kinds.Namespace().GroupKind() {
currentNamespaces = append(currentNamespaces, pair.Key.Name)
}
}
}

Expand Down
9 changes: 5 additions & 4 deletions pkg/declared/namespace_safeguard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"errors"
"testing"

"github.com/elliotchance/orderedmap/v2"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"kpt.dev/configsync/pkg/core"
"kpt.dev/configsync/pkg/kinds"
Expand Down Expand Up @@ -82,15 +83,15 @@ func TestDontDeleteAllNamespaces(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
previous := make(map[core.ID]*unstructured.Unstructured)
previous := orderedmap.NewOrderedMap[core.ID, *unstructured.Unstructured]()
for _, p := range tc.previous {
u := fake.UnstructuredObject(kinds.Namespace(), core.Name(p))
previous[core.IDOf(u)] = u
previous.Set(core.IDOf(u), u)
}
current := make(map[core.ID]*unstructured.Unstructured)
current := orderedmap.NewOrderedMap[core.ID, *unstructured.Unstructured]()
for _, c := range tc.current {
u := fake.UnstructuredObject(kinds.Namespace(), core.Name(c))
current[core.IDOf(u)] = u
current.Set(core.IDOf(u), u)
}

got := deletesAllNamespaces(previous, current)
Expand Down
61 changes: 37 additions & 24 deletions pkg/declared/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"sync"

"github.com/elliotchance/orderedmap/v2"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/klog/v2"
Expand All @@ -33,20 +34,20 @@ import (
// repo.
type Resources struct {
mutex sync.RWMutex
// objectSet is a map of object IDs to the unstructured format of those
// objectMap is a map of object IDs to the unstructured format of those
// objects. Note that the pointer to this map is threadsafe but the map itself
// is not threadsafe. This map should never be returned from a function
// directly. The map should never be written to once it has been assigned to
// this reference; it should be treated as read-only from then on.
objectSet map[core.ID]*unstructured.Unstructured
objectMap *orderedmap.OrderedMap[core.ID, *unstructured.Unstructured]
// commit of the source in which the resources were declared
commit string
}

// Update performs an atomic update on the resource declaration set.
func (r *Resources) Update(ctx context.Context, objects []client.Object, commit string) ([]client.Object, status.Error) {
// First build up the new map using a local pointer/reference.
newSet := make(map[core.ID]*unstructured.Unstructured)
newSet := orderedmap.NewOrderedMap[core.ID, *unstructured.Unstructured]()
newObjects := []client.Object{}
for _, obj := range objects {
if obj == nil {
Expand All @@ -61,31 +62,34 @@ func (r *Resources) Update(ctx context.Context, objects []client.Object, commit
return nil, status.InternalErrorBuilder.Wrap(err).
Sprintf("converting %v to unstructured.Unstructured", id).Build()
}
newSet[id] = u
newSet.Set(id, u)
newObjects = append(newObjects, obj)
}

// Record the declared_resources metric, after parsing but before validation.
metrics.RecordDeclaredResources(ctx, commit, len(newObjects))

previousSet, _ := r.getObjectSet()
previousSet, _ := r.getObjectMap()
if err := deletesAllNamespaces(previousSet, newSet); err != nil {
return nil, err
}

// Now assign the pointer for the new map to the struct reference in a
// threadsafe context. From now on, this map is read-only.
r.setObjectSet(newSet, commit)
r.setObjectMap(newSet, commit)
return newObjects, nil
}

// Get returns a copy of the resource declaration as read from Git
func (r *Resources) Get(id core.ID) (*unstructured.Unstructured, string, bool) {
objSet, commit := r.getObjectSet()
objSet, commit := r.getObjectMap()
if objSet == nil || objSet.Len() == 0 {
return nil, commit, false
}

// A local reference to the map is threadsafe since only the struct reference
// is replaced on update.
u, found := objSet[id]
u, found := objSet.Get(id)
// We return a copy of the Unstructured, as
// 1) client.Client methods mutate the objects passed into them.
// 2) We don't want to persist any changes made to an object we retrieved
Expand All @@ -96,54 +100,63 @@ func (r *Resources) Get(id core.ID) (*unstructured.Unstructured, string, bool) {
// DeclaredUnstructureds returns all resource objects declared in the source,
// along with the source commit.
func (r *Resources) DeclaredUnstructureds() ([]*unstructured.Unstructured, string) {
var objects []*unstructured.Unstructured
objSet, commit := r.getObjectSet()
objSet, commit := r.getObjectMap()
if objSet == nil || objSet.Len() == 0 {
return nil, commit
}

// A local reference to the map is threadsafe since only the struct reference
// is replaced on update.
for _, obj := range objSet {
objects = append(objects, obj)
var objects []*unstructured.Unstructured
for pair := objSet.Front(); pair != nil; pair = pair.Next() {
objects = append(objects, pair.Value)
}
return objects, commit
}

// DeclaredObjects returns all resource objects declared in the source, along
// with the source commit.
func (r *Resources) DeclaredObjects() ([]client.Object, string) {
var objects []client.Object
objSet, commit := r.getObjectSet()
objSet, commit := r.getObjectMap()
if objSet == nil || objSet.Len() == 0 {
return nil, commit
}

// A local reference to the map is threadsafe since only the struct reference
// is replaced on update.
for _, obj := range objSet {
objects = append(objects, obj)
var objects []client.Object
for pair := objSet.Front(); pair != nil; pair = pair.Next() {
objects = append(objects, pair.Value)
}
return objects, commit
}

// DeclaredGVKs returns the set of all GroupVersionKind found in the source,
// along with the source commit.
func (r *Resources) DeclaredGVKs() (map[schema.GroupVersionKind]struct{}, string) {
gvkSet := make(map[schema.GroupVersionKind]struct{})
objSet, commit := r.getObjectSet()
objSet, commit := r.getObjectMap()
if objSet == nil || objSet.Len() == 0 {
return nil, commit
}

// A local reference to the objSet map is threadsafe since only the pointer to
// the map is replaced on update.
for _, obj := range objSet {
gvkSet[obj.GroupVersionKind()] = struct{}{}
gvkSet := make(map[schema.GroupVersionKind]struct{})
for pair := objSet.Front(); pair != nil; pair = pair.Next() {
gvkSet[pair.Value.GroupVersionKind()] = struct{}{}
}
return gvkSet, commit
}

func (r *Resources) getObjectSet() (map[core.ID]*unstructured.Unstructured, string) {
func (r *Resources) getObjectMap() (*orderedmap.OrderedMap[core.ID, *unstructured.Unstructured], string) {
r.mutex.RLock()
defer r.mutex.RUnlock()
return r.objectSet, r.commit
return r.objectMap, r.commit
}

func (r *Resources) setObjectSet(objectSet map[core.ID]*unstructured.Unstructured, commit string) {
func (r *Resources) setObjectMap(objectMap *orderedmap.OrderedMap[core.ID, *unstructured.Unstructured], commit string) {
r.mutex.Lock()
defer r.mutex.Unlock()
r.objectSet = objectSet
r.objectMap = objectMap
r.commit = commit
}
2 changes: 1 addition & 1 deletion pkg/declared/resources_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func TestUpdate(t *testing.T) {
}

for _, id := range expectedIDs {
if _, ok := dr.objectSet[id]; !ok {
if _, ok := dr.objectMap.Get(id); !ok {
t.Errorf("ID %v not found in the declared resource", id)
}
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/importer/filesystem/config_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ type ConfigParser interface {

// AsCoreObjects converts a slice of FileObjects to a slice of client.Objects.
func AsCoreObjects(fos []ast.FileObject) []client.Object {
if len(fos) == 0 {
return nil
}
result := make([]client.Object, len(fos))
for i, fo := range fos {
result[i] = fo.Unstructured
Expand Down
Loading

0 comments on commit ee8d669

Please sign in to comment.