Repository: replicatedhq/kots
Pull request #226 (2019-12-28 01:26:24)

Prevent duplicates in the base
Some helm charts have duplicate resource definitions. This is fine in helm and kubectl, but kustomize doesn't like it. We can't pick one of these, because there is no guarantee that they are identical. So let's create a merge patch.


Patch:
diff --git a/pkg/base/base.go b/pkg/base/base.go
index 90bb0caea..f4fbe2fb3 100644
--- a/pkg/base/base.go
+++ b/pkg/base/base.go
@@ -1,6 +1,9 @@
 package base
 
 import (
+	"crypto/sha256"
+	"fmt"
+
 	kotsscheme "github.com/replicatedhq/kots/kotskinds/client/kotsclientset/scheme"
 	troubleshootscheme "github.com/replicatedhq/troubleshoot/pkg/client/troubleshootclientset/scheme"
 	"gopkg.in/yaml.v2"
@@ -21,45 +24,36 @@ type OverlySimpleGVK struct {
 	Kind       string `yaml:"kind"`
 }
 
+type OverlySimpleGVKWithName struct {
+	APIVersion string               `yaml:"apiVersion"`
+	Kind       string               `yaml:"kind"`
+	Metadata   OverlySimpleMetadata `yaml:"metadata"`
+}
+
+type OverlySimpleMetadata struct {
+	Name string `yaml:"name"`
+}
+
 func init() {
 	kotsscheme.AddToScheme(scheme.Scheme)
 	troubleshootscheme.AddToScheme(scheme.Scheme)
 }
 
-// ShouldBeIncludedInBaseKustomization attempts to determine if this is a valid Kubernetes manifest.
-// It accomplished this by trying to unmarshal the YAML and looking for a apiVersion and Kind
-func (f BaseFile) ShouldBeIncludedInBaseKustomization(excludeKotsKinds bool) bool {
-	o := OverlySimpleGVK{}
-
-	if err := yaml.Unmarshal(f.Content, &o); err != nil {
-		return false
-	}
-
-	if o.APIVersion == "" || o.Kind == "" {
-		return false
-	}
-
-	if excludeKotsKinds {
-		if o.APIVersion == "kots.io/v1beta1" {
-			return false
-		}
-
-		if o.APIVersion == "troubleshoot.replicated.com/v1beta1" {
-			return false
-		}
+func GetGVKWithNameHash(content []byte) []byte {
+	o := OverlySimpleGVKWithName{}
 
-		// In addition to kotskinds, we exclude the application crd for now
-		if o.APIVersion == "app.k8s.io/v1beta1" {
-			return false
-		}
+	if err := yaml.Unmarshal(content, &o); err != nil {
+		return nil
 	}
 
-	return true
+	h := sha256.New()
+	h.Write([]byte(fmt.Sprintf("%s-%s-%s", o.APIVersion, o.Kind, o.Metadata.Name)))
+	return h.Sum(nil)
 }
 
-// ShouldBeIncludedInBaseFilesystem attempts to determine if this is a valid Kubernetes manifest.
+// ShouldBeIncludedInBaseKustomization attempts to determine if this is a valid Kubernetes manifest.
 // It accomplished this by trying to unmarshal the YAML and looking for a apiVersion and Kind
-func (f BaseFile) ShouldBeIncludedInBaseFilesystem(excludeKotsKinds bool) bool {
+func (f BaseFile) ShouldBeIncludedInBaseKustomization(excludeKotsKinds bool) bool {
 	o := OverlySimpleGVK{}
 
 	if err := yaml.Unmarshal(f.Content, &o); err != nil {
@@ -79,6 +73,7 @@ func (f BaseFile) ShouldBeIncludedInBaseFilesystem(excludeKotsKinds bool) bool {
 			return false
 		}
 
+		// In addition to kotskinds, we exclude the application crd for now
 		if o.APIVersion == "app.k8s.io/v1beta1" {
 			return false
 		}
diff --git a/pkg/base/replicated.go b/pkg/base/replicated.go
index 8b55602dd..809ad4217 100644
--- a/pkg/base/replicated.go
+++ b/pkg/base/replicated.go
@@ -159,7 +159,7 @@ func renderReplicated(u *upstreamtypes.Upstream, renderOptions *RenderOptions) (
 
 		for _, helmBaseFile := range helmBase.Files {
 			// this is a little bit of an abuse of the next function
-			if !helmBaseFile.ShouldBeIncludedInBaseFilesystem(false) {
+			if !helmBaseFile.ShouldBeIncludedInBaseKustomization(false) {
 				continue
 			}
 
diff --git a/pkg/base/write.go b/pkg/base/write.go
index 9a11b7963..602cece29 100644
--- a/pkg/base/write.go
+++ b/pkg/base/write.go
@@ -1,6 +1,7 @@
 package base
 
 import (
+	"bytes"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -37,32 +38,41 @@ func (b *Base) WriteBase(options WriteOptions) error {
 		}
 	}
 
-	kustomizeResources := []string{}
-	for _, file := range b.Files {
-		writeToBase := file.ShouldBeIncludedInBaseFilesystem(options.ExcludeKotsKinds)
-		writeToKustomization := file.ShouldBeIncludedInBaseKustomization(options.ExcludeKotsKinds)
+	resources, patches := deduplicateOnContent(b.Files, options.ExcludeKotsKinds)
 
-		if !writeToBase && !writeToKustomization {
-			continue
+	kustomizeResources := []string{}
+	kustomizePatches := []kustomizetypes.PatchStrategicMerge{}
+
+	for _, file := range resources {
+		fileRenderPath := path.Join(renderDir, file.Path)
+		d, _ := path.Split(fileRenderPath)
+		if _, err := os.Stat(d); os.IsNotExist(err) {
+			if err := os.MkdirAll(d, 0744); err != nil {
+				return errors.Wrap(err, "failed to mkdir")
+			}
 		}
 
-		if writeToKustomization {
-			kustomizeResources = append(kustomizeResources, path.Join(".", file.Path))
+		if err := ioutil.WriteFile(fileRenderPath, file.Content, 0644); err != nil {
+			return errors.Wrap(err, "failed to write base file")
 		}
 
-		if writeToBase {
-			fileRenderPath := path.Join(renderDir, file.Path)
-			d, _ := path.Split(fileRenderPath)
-			if _, err := os.Stat(d); os.IsNotExist(err) {
-				if err := os.MkdirAll(d, 0744); err != nil {
-					return errors.Wrap(err, "failed to mkdir")
-				}
-			}
+		kustomizeResources = append(kustomizeResources, path.Join(".", file.Path))
+	}
 
-			if err := ioutil.WriteFile(fileRenderPath, file.Content, 0644); err != nil {
-				return errors.Wrap(err, "failed to write base file")
+	for _, file := range patches {
+		fileRenderPath := path.Join(renderDir, file.Path)
+		d, _ := path.Split(fileRenderPath)
+		if _, err := os.Stat(d); os.IsNotExist(err) {
+			if err := os.MkdirAll(d, 0744); err != nil {
+				return errors.Wrap(err, "failed to mkdir")
 			}
 		}
+
+		if err := ioutil.WriteFile(fileRenderPath, file.Content, 0644); err != nil {
+			return errors.Wrap(err, "failed to write base file")
+		}
+
+		kustomizePatches = append(kustomizePatches, kustomizetypes.PatchStrategicMerge(path.Join(".", file.Path)))
 	}
 
 	kustomization := kustomizetypes.Kustomization{
@@ -70,7 +80,8 @@ func (b *Base) WriteBase(options WriteOptions) error {
 			APIVersion: "kustomize.config.k8s.io/v1beta1",
 			Kind:       "Kustomization",
 		},
-		Resources: kustomizeResources,
+		Resources:             kustomizeResources,
+		PatchesStrategicMerge: kustomizePatches,
 	}
 
 	if err := k8sutil.WriteKustomizationToFile(&kustomization, path.Join(renderDir, "kustomization.yaml")); err != nil {
@@ -80,6 +91,41 @@ func (b *Base) WriteBase(options WriteOptions) error {
 	return nil
 }
 
+func deduplicateOnContent(files []BaseFile, excludeKotsKinds bool) ([]BaseFile, []BaseFile) {
+	resources := []BaseFile{}
+	patches := []BaseFile{}
+
+	foundGVKNames := [][]byte{}
+
+	for _, file := range files {
+		writeToKustomization := file.ShouldBeIncludedInBaseKustomization(excludeKotsKinds)
+
+		if !writeToKustomization {
+			continue
+		}
+
+		if writeToKustomization {
+			found := false
+			thisGVKName := GetGVKWithNameHash(file.Content)
+			for _, gvkName := range foundGVKNames {
+				if bytes.Compare(gvkName, thisGVKName) == 0 {
+					found = true
+				}
+			}
+
+			if !found || thisGVKName == nil {
+				resources = append(resources, file)
+				foundGVKNames = append(foundGVKNames, thisGVKName)
+			} else {
+				patches = append(patches, file)
+			}
+		}
+
+	}
+
+	return resources, patches
+}
+
 func (b *Base) GetOverlaysDir(options WriteOptions) string {
 	renderDir := options.BaseDir
 


Test patch:
diff --git a/pkg/base/write_test.go b/pkg/base/write_test.go
new file mode 100644
index 000000000..e45405489
--- /dev/null
+++ b/pkg/base/write_test.go
@@ -0,0 +1,141 @@
+package base
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+var (
+	TestServiceA = `apiVersion: v1
+kind: Service
+metadata:
+  name: service-a`
+
+	TestServiceB = `apiVersion: v1
+kind: Service
+metadata:
+  name: service-b`
+
+	TestPodA = `apiVersion: v1
+kind: Pod
+metadata:
+  name: pod-a`
+
+	TestPodNamedServiceA = `apiVersion: v1
+kind: Pod
+metadata:
+  name: service-a`
+)
+
+func Test_DeduplicateOnContent(t *testing.T) {
+	tests := []struct {
+		name              string
+		files             []BaseFile
+		excludeKotsKinds  bool
+		expectedResources []BaseFile
+		expectedPatches   []BaseFile
+	}{
+		{
+			name: "all unique",
+			files: []BaseFile{
+				{
+					Path:    "service-a",
+					Content: []byte(TestServiceA),
+				},
+				{
+					Path:    "service-b",
+					Content: []byte(TestServiceB),
+				},
+			},
+			excludeKotsKinds: true,
+			expectedResources: []BaseFile{
+				{
+					Path:    "service-a",
+					Content: []byte(TestServiceA),
+				},
+				{
+					Path:    "service-b",
+					Content: []byte(TestServiceB),
+				},
+			},
+			expectedPatches: []BaseFile{},
+		},
+		{
+			name: "duplicated service",
+			files: []BaseFile{
+				{
+					Path:    "service-a",
+					Content: []byte(TestServiceA),
+				},
+				{
+					Path:    "service-b",
+					Content: []byte(TestServiceB),
+				},
+				{
+					Path:    "service-b",
+					Content: []byte(TestServiceB),
+				},
+			},
+			excludeKotsKinds: true,
+			expectedResources: []BaseFile{
+				{
+					Path:    "service-a",
+					Content: []byte(TestServiceA),
+				},
+				{
+					Path:    "service-b",
+					Content: []byte(TestServiceB),
+				},
+			},
+			expectedPatches: []BaseFile{
+				{
+					Path:    "service-b",
+					Content: []byte(TestServiceB),
+				},
+			},
+		},
+		{
+			name: "same-name-different-gvk",
+			files: []BaseFile{
+				{
+					Path:    "service-a",
+					Content: []byte(TestServiceA),
+				},
+				{
+					Path:    "service-a",
+					Content: []byte(TestServiceB),
+				},
+				{
+					Path:    "service-a",
+					Content: []byte(TestPodNamedServiceA),
+				},
+			},
+			excludeKotsKinds: true,
+			expectedResources: []BaseFile{
+				{
+					Path:    "service-a",
+					Content: []byte(TestServiceA),
+				},
+				{
+					Path:    "service-a",
+					Content: []byte(TestServiceB),
+				},
+				{
+					Path:    "service-a",
+					Content: []byte(TestPodNamedServiceA),
+				},
+			},
+			expectedPatches: []BaseFile{},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			actualResources, actualPatches := deduplicateOnContent(test.files, test.excludeKotsKinds)
+			assert.ElementsMatch(t, test.expectedResources, actualResources)
+			assert.ElementsMatch(t, test.expectedPatches, actualPatches)
+		})
+	}
+
+}