From 396c5553ca712a3a241909c7d6e6a916ecca9842 Mon Sep 17 00:00:00 2001 From: Mauro Morales Date: Tue, 1 Aug 2023 10:08:21 +0200 Subject: [PATCH] :bug: Wrong deep merge when array contains maps (#38) * Do not try to smart merge arrays, but just append them Signed-off-by: Mauro Morales * Fix tests Signed-off-by: Mauro Morales * Rewording for tests Signed-off-by: Mauro Morales * Rename function to match what is doing after refactoring Signed-off-by: Mauro Morales --------- Signed-off-by: Mauro Morales --- collector/collector.go | 42 ++------ collector/collector_test.go | 195 +++++++++++++++++++++++++++++++++--- 2 files changed, 192 insertions(+), 45 deletions(-) diff --git a/collector/collector.go b/collector/collector.go index 38fc569..28858b5 100644 --- a/collector/collector.go +++ b/collector/collector.go @@ -108,50 +108,26 @@ func (c *Config) MergeConfig(newConfig *Config) error { return c.applyMap(cMap) } -func deepMergeSlices(sliceA, sliceB []interface{}) ([]interface{}, error) { +func mergeSlices(sliceA, sliceB []interface{}) ([]interface{}, error) { // We use the first item in the slice to determine if there are maps present. - // Do we need to do the same for other types? firstItem := sliceA[0] + // If the first item is a map, we concatenate both slices if reflect.ValueOf(firstItem).Kind() == reflect.Map { - temp := make(map[string]interface{}) + union := append(sliceA, sliceB...) - // first we put in temp all the keys present in a, and assign them their existing values - for _, item := range sliceA { - for k, v := range item.(map[string]interface{}) { - temp[k] = v - } - } - - // then we go through b to merge each of its keys - for _, item := range sliceB { - for k, v := range item.(map[string]interface{}) { - current, ok := temp[k] - if ok { - // if the key exists, we deep merge it - dm, err := DeepMerge(current, v) - if err != nil { - return []interface{}{}, fmt.Errorf("cannot merge %s with %s", current, v) - } - temp[k] = dm - } else { - // otherwise we just set it - temp[k] = v - } - } - } - - return []interface{}{temp}, nil + return union, nil } - // This implementation is needed because Go 1.19 does not implement compare for {}interface. Once - // FIPS can be upgraded to 1.20, we should be able to use this other code: - // // for simple slices + // For any other type, we check if the every item in sliceB is already present in sliceA and if not, we add it. + // Implementation for 1.20: // for _, v := range sliceB { // i := slices.Index(sliceA, v) // if i < 0 { // sliceA = append(sliceA, v) // } // } + // This implementation is needed because Go 1.19 does not implement compare for {}interface. Once + // FIPS can be upgraded to 1.20, we should be able to use the code above instead. for _, vB := range sliceB { found := false for _, vA := range sliceA { @@ -204,7 +180,7 @@ func DeepMerge(a, b interface{}) (interface{}, error) { } if typeA.Kind() == reflect.Slice { - return deepMergeSlices(a.([]interface{}), b.([]interface{})) + return mergeSlices(a.([]interface{}), b.([]interface{})) } if typeA.Kind() == reflect.Map { diff --git a/collector/collector_test.go b/collector/collector_test.go index 808fa07..cfc3d96 100644 --- a/collector/collector_test.go +++ b/collector/collector_test.go @@ -242,15 +242,24 @@ info: It("merges", func() { c, err := DeepMerge(a, b) Expect(err).ToNot(HaveOccurred()) - users := c.([]interface{})[0].(map[string]interface{})["users"] - Expect(users).To(HaveLen(1)) - Expect(users).To(Equal([]interface{}{ + Expect(c).To(HaveLen(2)) + Expect(c).To(Equal([]interface{}{ map[string]interface{}{ - "kairos": map[string]interface{}{ - "passwd": "kairos", + "users": []interface{}{ + map[string]interface{}{ + "kairos": map[string]interface{}{ + "passwd": "kairos", + }, + }, }, - "foo": map[string]interface{}{ - "passwd": "bar", + }, + map[string]interface{}{ + "users": []interface{}{ + map[string]interface{}{ + "foo": map[string]interface{}{ + "passwd": "bar", + }, + }, }, }, })) @@ -300,6 +309,158 @@ info: }) Describe("Scan", func() { + Context("When users are created for the same stage on different files (issue kairos-io/kairos#1341)", func() { + var cmdLinePath, tmpDir1 string + var err error + + BeforeEach(func() { + tmpDir1, err = os.MkdirTemp("", "config1") + Expect(err).ToNot(HaveOccurred()) + err := os.WriteFile(path.Join(tmpDir1, "local_config_1.yaml"), []byte(`#cloud-config +install: + auto: true + reboot: false + poweroff: false + grub_options: + extra_cmdline: "console=tty0" +options: + device: /dev/sda +stages: + initramfs: + - users: + kairos: + groups: + - sudo + passwd: kairos +`), os.ModePerm) + Expect(err).ToNot(HaveOccurred()) + err = os.WriteFile(path.Join(tmpDir1, "local_config_2.yaml"), []byte(`#cloud-config +stages: + initramfs: + - users: + foo: + groups: + - sudo + passwd: bar +`), os.ModePerm) + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + err = os.RemoveAll(tmpDir1) + Expect(err).ToNot(HaveOccurred()) + }) + + It("keeps the two users", func() { + o := &Options{} + err := o.Apply( + MergeBootLine, + WithBootCMDLineFile(cmdLinePath), + Directories(tmpDir1), + ) + Expect(err).ToNot(HaveOccurred()) + + c, err := Scan(o, FilterKeysTestMerge) + Expect(err).ToNot(HaveOccurred()) + + fmt.Println(c.String()) + Expect(c.String()).To(Equal(`#cloud-config + +install: + auto: true + grub_options: + extra_cmdline: console=tty0 + poweroff: false + reboot: false +options: + device: /dev/sda +stages: + initramfs: + - users: + kairos: + groups: + - sudo + passwd: kairos + - users: + foo: + groups: + - sudo + passwd: bar +`)) + }) + }) + + Context("When a YIP if expression is contained", func() { + var cmdLinePath, tmpDir1 string + var err error + + BeforeEach(func() { + tmpDir1, err = os.MkdirTemp("", "config1") + Expect(err).ToNot(HaveOccurred()) + err := os.WriteFile(path.Join(tmpDir1, "local_config_1.yaml"), []byte(`#cloud-config +stages: + initramfs: + - users: + kairos: + passwd: kairos + - name: set_inotify_max_values + if: '[ ! -f /oem/80_stylus.yaml ]' + sysctl: + fs.inotify.max_user_instances: "8192" +`), os.ModePerm) + Expect(err).ToNot(HaveOccurred()) + err = os.WriteFile(path.Join(tmpDir1, "local_config_2.yaml"), []byte(`#cloud-config +stages: + initramfs: + - commands: + - ln -s /etc/kubernetes/admin.conf /run/kubeconfig + sysctl: + kernel.panic: "10" + kernel.panic_on_oops: "1" + vm.overcommit_memory: "1" +`), os.ModePerm) + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + err = os.RemoveAll(tmpDir1) + Expect(err).ToNot(HaveOccurred()) + }) + + It("it remains within its scope after merging", func() { + o := &Options{} + err := o.Apply( + MergeBootLine, + WithBootCMDLineFile(cmdLinePath), + Directories(tmpDir1), + ) + Expect(err).ToNot(HaveOccurred()) + + c, err := Scan(o, FilterKeysTestMerge) + Expect(err).ToNot(HaveOccurred()) + + fmt.Println(c.String()) + Expect(c.String()).To(Equal(`#cloud-config + +stages: + initramfs: + - users: + kairos: + passwd: kairos + - if: '[ ! -f /oem/80_stylus.yaml ]' + name: set_inotify_max_values + sysctl: + fs.inotify.max_user_instances: "8192" + - commands: + - ln -s /etc/kubernetes/admin.conf /run/kubeconfig + sysctl: + kernel.panic: "10" + kernel.panic_on_oops: "1" + vm.overcommit_memory: "1" +`)) + }) + }) + Context("duplicated configs", func() { var cmdLinePath, tmpDir1 string var err error @@ -358,7 +519,7 @@ install: Expect(err).ToNot(HaveOccurred()) }) - It("should be the same as just one of them", func() { + It("remain duplicated, and are the responsibility of the user", func() { o := &Options{} err := o.Apply( MergeBootLine, @@ -379,6 +540,9 @@ install: - rootfs_path: /usr/local/lib/extensions/kubo targets: - container://ttl.sh/97d4530c-df80-4eb4-9ae7-39f8f90c26e5:8h + - rootfs_path: /usr/local/lib/extensions/kubo + targets: + - container://ttl.sh/97d4530c-df80-4eb4-9ae7-39f8f90c26e5:8h device: auto grub_options: extra_cmdline: foobarzz @@ -390,9 +554,15 @@ stages: users: kairos: passwd: kairos + - hostname: kairos-{{ trunc 4 .Random }} + name: Set user and password + users: + kairos: + passwd: kairos `)) }) }) + Context("Deep merge maps within arrays", func() { var cmdLinePath, tmpDir1 string var err error @@ -460,14 +630,15 @@ options: stages: initramfs: - users: - foo: - groups: - - sudo - passwd: bar kairos: groups: - sudo passwd: kairos + - users: + foo: + groups: + - sudo + passwd: bar `)) }) })