Skip to content

Commit

Permalink
Fix comparison in pointer ignition customization asset
Browse files Browse the repository at this point in the history 
An issue was spotted after openshift#4413 landed, the config comparison
always evaluates to true, so we're generating empty machineconfig
objects even when no customization has happened.

Update to compare a json representation of the configs, and add
a test to confirm all works as expected.
  • Loading branch information
Steven Hardy committed Dec 4, 2020
1 parent 268a56a commit 1308fc7
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 54 deletions.
7 changes: 5 additions & 2 deletions pkg/asset/ignition/machine/master_ignition_customizations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/sirupsen/logrus"

"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/ignition"
"github.com/openshift/installer/pkg/asset/installconfig"
"github.com/openshift/installer/pkg/asset/tls"
mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
Expand Down Expand Up @@ -45,10 +46,12 @@ func (a *MasterIgnitionCustomizations) Generate(dependencies asset.Parents) erro
defaultPointerIgnition := pointerIgnitionConfig(installConfig.Config, rootCA.Cert(), "master")
savedPointerIgnition := master.Config

if savedPointerIgnition != defaultPointerIgnition {
savedPointerIgnitionJson, err := ignition.Marshal(savedPointerIgnition)
defaultPointerIgnitionJson, err := ignition.Marshal(defaultPointerIgnition)
if string(savedPointerIgnitionJson) != string(defaultPointerIgnitionJson) {
logrus.Infof("Master pointer ignition was modified. Saving contents to a machineconfig")
mc := &mcfgv1.MachineConfig{}
mc, err := generatePointerMachineConfig(*savedPointerIgnition, "master")
mc, err = generatePointerMachineConfig(*savedPointerIgnition, "master")
if err != nil {
return errors.Wrap(err, "failed to generate master installer machineconfig")
}
Expand Down
82 changes: 57 additions & 25 deletions pkg/asset/ignition/machine/master_ignition_customizations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/stretchr/testify/assert"

"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/ignition"
"github.com/openshift/installer/pkg/asset/installconfig"
"github.com/openshift/installer/pkg/asset/tls"
"github.com/openshift/installer/pkg/ipnet"
Expand All @@ -15,36 +16,67 @@ import (

// TestMasterIgnitionCustomizationsGenerate tests generating the master ignition check asset.
func TestMasterIgnitionCustomizationsGenerate(t *testing.T) {
installConfig := &installconfig.InstallConfig{
Config: &types.InstallConfig{
Networking: &types.Networking{
ServiceNetwork: []ipnet.IPNet{*ipnet.MustParseCIDR("10.0.1.0/24")},
},
Platform: types.Platform{
AWS: &aws.Platform{
Region: "us-east",
},
},
cases := []struct {
name string
customize bool
assetExpected bool
}{
{
name: "not customized",
customize: false,
assetExpected: false,
},
{
name: "pointer customized",
customize: true,
assetExpected: true,
},
}

rootCA := &tls.RootCA{}
err := rootCA.Generate(nil)
assert.NoError(t, err, "unexpected error generating root CA")
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
installConfig := &installconfig.InstallConfig{
Config: &types.InstallConfig{
Networking: &types.Networking{
ServiceNetwork: []ipnet.IPNet{*ipnet.MustParseCIDR("10.0.1.0/24")},
},
Platform: types.Platform{
AWS: &aws.Platform{
Region: "us-east",
},
},
},
}

rootCA := &tls.RootCA{}
err := rootCA.Generate(nil)
assert.NoError(t, err, "unexpected error generating root CA")

parents := asset.Parents{}
parents.Add(installConfig, rootCA)

parents := asset.Parents{}
parents.Add(installConfig, rootCA)
master := &Master{}
err = master.Generate(parents)
assert.NoError(t, err, "unexpected error generating master asset")

master := &Master{}
err = master.Generate(parents)
assert.NoError(t, err, "unexpected error generating master asset")
if tc.customize == true {
// Modify the master config, emulating a customization to the pointer
master.Config.Storage.Files = append(master.Config.Storage.Files,
ignition.FileFromString("/etc/foo", "root", 0644, "foo"))
}

parents.Add(master)
masterIgnCheck := &MasterIgnitionCustomizations{}
err = masterIgnCheck.Generate(parents)
assert.NoError(t, err, "unexpected error generating master ignition check asset")
parents.Add(master)
masterIgnCheck := &MasterIgnitionCustomizations{}
err = masterIgnCheck.Generate(parents)
assert.NoError(t, err, "unexpected error generating master ignition check asset")

actualFiles := masterIgnCheck.Files()
assert.Equal(t, 1, len(actualFiles), "unexpected number of files in master state")
assert.Equal(t, masterMachineConfigFileName, actualFiles[0].Filename, "unexpected name for master ignition config")
actualFiles := masterIgnCheck.Files()
if tc.assetExpected == true {
assert.Equal(t, 1, len(actualFiles), "unexpected number of files in master state")
assert.Equal(t, masterMachineConfigFileName, actualFiles[0].Filename, "unexpected name for master ignition config")
} else {
assert.Equal(t, 0, len(actualFiles), "unexpected number of files in master state")
}
})
}
}
7 changes: 5 additions & 2 deletions pkg/asset/ignition/machine/worker_ignition_customizations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/sirupsen/logrus"

"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/ignition"
"github.com/openshift/installer/pkg/asset/installconfig"
"github.com/openshift/installer/pkg/asset/tls"
mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
Expand Down Expand Up @@ -46,10 +47,12 @@ func (a *WorkerIgnitionCustomizations) Generate(dependencies asset.Parents) erro
savedPointerIgnition := worker.Config

// Create a machineconfig if the ignition has been modified
if savedPointerIgnition != defaultPointerIgnition {
savedPointerIgnitionJson, err := ignition.Marshal(savedPointerIgnition)
defaultPointerIgnitionJson, err := ignition.Marshal(defaultPointerIgnition)
if string(savedPointerIgnitionJson) != string(defaultPointerIgnitionJson) {
logrus.Infof("Worker pointer ignition was modified. Saving contents to a machineconfig")
mc := &mcfgv1.MachineConfig{}
mc, err := generatePointerMachineConfig(*savedPointerIgnition, "worker")
mc, err = generatePointerMachineConfig(*savedPointerIgnition, "worker")
if err != nil {
return errors.Wrap(err, "failed to generate worker installer machineconfig")
}
Expand Down
82 changes: 57 additions & 25 deletions pkg/asset/ignition/machine/worker_ignition_customizations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/stretchr/testify/assert"

"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/ignition"
"github.com/openshift/installer/pkg/asset/installconfig"
"github.com/openshift/installer/pkg/asset/tls"
"github.com/openshift/installer/pkg/ipnet"
Expand All @@ -15,36 +16,67 @@ import (

// TestWorkerIgnitionCustomizationsGenerate tests generating the worker ignition check asset.
func TestWorkerIgnitionCustomizationsGenerate(t *testing.T) {
installConfig := &installconfig.InstallConfig{
Config: &types.InstallConfig{
Networking: &types.Networking{
ServiceNetwork: []ipnet.IPNet{*ipnet.MustParseCIDR("10.0.1.0/24")},
},
Platform: types.Platform{
AWS: &aws.Platform{
Region: "us-east",
},
},
cases := []struct {
name string
customize bool
assetExpected bool
}{
{
name: "not customized",
customize: false,
assetExpected: false,
},
{
name: "pointer customized",
customize: true,
assetExpected: true,
},
}

rootCA := &tls.RootCA{}
err := rootCA.Generate(nil)
assert.NoError(t, err, "unexpected error generating root CA")
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
installConfig := &installconfig.InstallConfig{
Config: &types.InstallConfig{
Networking: &types.Networking{
ServiceNetwork: []ipnet.IPNet{*ipnet.MustParseCIDR("10.0.1.0/24")},
},
Platform: types.Platform{
AWS: &aws.Platform{
Region: "us-east",
},
},
},
}

rootCA := &tls.RootCA{}
err := rootCA.Generate(nil)
assert.NoError(t, err, "unexpected error generating root CA")

parents := asset.Parents{}
parents.Add(installConfig, rootCA)

parents := asset.Parents{}
parents.Add(installConfig, rootCA)
worker := &Worker{}
err = worker.Generate(parents)
assert.NoError(t, err, "unexpected error generating worker asset")

worker := &Worker{}
err = worker.Generate(parents)
assert.NoError(t, err, "unexpected error generating worker asset")
if tc.customize == true {
// Modify the worker config, emulating a customization to the pointer
worker.Config.Storage.Files = append(worker.Config.Storage.Files,
ignition.FileFromString("/etc/foo", "root", 0644, "foo"))
}

parents.Add(worker)
workerIgnCheck := &WorkerIgnitionCustomizations{}
err = workerIgnCheck.Generate(parents)
assert.NoError(t, err, "unexpected error generating worker ignition check asset")
parents.Add(worker)
workerIgnCheck := &WorkerIgnitionCustomizations{}
err = workerIgnCheck.Generate(parents)
assert.NoError(t, err, "unexpected error generating worker ignition check asset")

actualFiles := workerIgnCheck.Files()
assert.Equal(t, 1, len(actualFiles), "unexpected number of files in worker state")
assert.Equal(t, workerMachineConfigFileName, actualFiles[0].Filename, "unexpected name for worker ignition config")
actualFiles := workerIgnCheck.Files()
if tc.assetExpected == true {
assert.Equal(t, 1, len(actualFiles), "unexpected number of files in worker state")
assert.Equal(t, workerMachineConfigFileName, actualFiles[0].Filename, "unexpected name for worker ignition config")
} else {
assert.Equal(t, 0, len(actualFiles), "unexpected number of files in worker state")
}
})
}
}

0 comments on commit 1308fc7

Please sign in to comment.