Merge pull request #1058 from wking/defaults-for-installconfig

assets/installconfig: apply defaults to install config

pkg/asset/ignition/machine/master_test.go

@@ -22,8 +22,8 @@ func TestMasterGenerate(t *testing.T) {
 				Name: "test-cluster",
 			},
 			BaseDomain: "test-domain",
-			Networking: types.Networking{
-				ServiceCIDR: *ipnet.MustParseCIDR("10.0.1.0/24"),
+			Networking: &types.Networking{
+				ServiceCIDR: ipnet.MustParseCIDR("10.0.1.0/24"),
 			},
 			Platform: types.Platform{
 				AWS: &aws.Platform{

pkg/asset/ignition/machine/worker_test.go

@@ -17,8 +17,8 @@ import (
 func TestWorkerGenerate(t *testing.T) {
 	installConfig := &installconfig.InstallConfig{
 		Config: &types.InstallConfig{
-			Networking: types.Networking{
-				ServiceCIDR: *ipnet.MustParseCIDR("10.0.1.0/24"),
+			Networking: &types.Networking{
+				ServiceCIDR: ipnet.MustParseCIDR("10.0.1.0/24"),
 			},
 			Platform: types.Platform{
 				AWS: &aws.Platform{

pkg/asset/installconfig/installconfig.go

@@ -7,11 +7,9 @@ import (
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	netopv1 "github.com/openshift/cluster-network-operator/pkg/apis/networkoperator/v1"
 	"github.com/openshift/installer/pkg/asset"
-	"github.com/openshift/installer/pkg/asset/installconfig/libvirt"
-	"github.com/openshift/installer/pkg/ipnet"
 	"github.com/openshift/installer/pkg/types"
+	"github.com/openshift/installer/pkg/types/defaults"
 	openstackvalidation "github.com/openshift/installer/pkg/types/openstack/validation"
 	"github.com/openshift/installer/pkg/types/validation"
 )
@@ -20,13 +18,6 @@ const (
 	installConfigFilename = "install-config.yaml"
 )
 
-var (
-	defaultMachineCIDR      = ipnet.MustParseCIDR("10.0.0.0/16")
-	defaultServiceCIDR      = ipnet.MustParseCIDR("172.30.0.0/16")
-	defaultClusterCIDR      = "10.128.0.0/14"
-	defaultHostSubnetLength = 9 // equivalent to a /23 per node
-)
-
 // InstallConfig generates the install-config.yaml file.
 type InstallConfig struct {
 	Config *types.InstallConfig `json:"config"`
@@ -68,47 +59,20 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {
 		},
 		SSHKey:     sshPublicKey.Key,
 		BaseDomain: baseDomain.BaseDomain,
-		Networking: types.Networking{
-			Type:        "OpenshiftSDN",
-			MachineCIDR: *defaultMachineCIDR,
-			ServiceCIDR: *defaultServiceCIDR,
-			ClusterNetworks: []netopv1.ClusterNetwork{
-				{
-					CIDR:             defaultClusterCIDR,
-					HostSubnetLength: uint32(defaultHostSubnetLength),
-				},
-			},
-		},
 		PullSecret: pullSecret.PullSecret,
 	}
 
-	numberOfMasters := int64(3)
-	numberOfWorkers := int64(3)
-	switch {
-	case platform.AWS != nil:
-		a.Config.AWS = platform.AWS
-	case platform.Libvirt != nil:
-		a.Config.Libvirt = platform.Libvirt
-		a.Config.Networking.MachineCIDR = *libvirt.DefaultMachineCIDR
-		numberOfMasters = 1
-		numberOfWorkers = 1
-	case platform.None != nil:
-		a.Config.None = platform.None
-	case platform.OpenStack != nil:
-		a.Config.OpenStack = platform.OpenStack
-	default:
-		panic("unknown platform type")
+	a.Config.AWS = platform.AWS
+	a.Config.Libvirt = platform.Libvirt
+	a.Config.None = platform.None
+	a.Config.OpenStack = platform.OpenStack
+
+	if err := a.setDefaults(); err != nil {
+		return errors.Wrapf(err, "failed to set defaults for install config")
 	}
 
-	a.Config.Machines = []types.MachinePool{
-		{
-			Name:     "master",
-			Replicas: func(x int64) *int64 { return &x }(numberOfMasters),
-		},
-		{
-			Name:     "worker",
-			Replicas: func(x int64) *int64 { return &x }(numberOfWorkers),
-		},
+	if err := validation.ValidateInstallConfig(a.Config, openstackvalidation.NewValidValuesFetcher()).ToAggregate(); err != nil {
+		return errors.Wrap(err, "invalid install config")
 	}
 
 	data, err := yaml.Marshal(a.Config)
@@ -119,7 +83,6 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {
 		Filename: installConfigFilename,
 		Data:     data,
 	}
-
 	return nil
 }
 
@@ -150,11 +113,29 @@ func (a *InstallConfig) Load(f asset.FileFetcher) (found bool, err error) {
 	if err := yaml.Unmarshal(file.Data, config); err != nil {
 		return false, errors.Wrapf(err, "failed to unmarshal")
 	}
+	a.Config = config
 
-	if err := validation.ValidateInstallConfig(config, openstackvalidation.NewValidValuesFetcher()).ToAggregate(); err != nil {
+	if err := a.setDefaults(); err != nil {
+		return false, errors.Wrapf(err, "failed to set defaults for install config")
+	}
+
+	if err := validation.ValidateInstallConfig(a.Config, openstackvalidation.NewValidValuesFetcher()).ToAggregate(); err != nil {
 		return false, errors.Wrapf(err, "invalid %q file", installConfigFilename)
 	}
 
-	a.File, a.Config = file, config
+	data, err := yaml.Marshal(a.Config)
+	if err != nil {
+		return false, errors.Wrap(err, "failed to Marshal InstallConfig")
+	}
+	a.File = &asset.File{
+		Filename: installConfigFilename,
+		Data:     data,
+	}
+
 	return true, nil
 }
+
+func (a *InstallConfig) setDefaults() error {
+	defaults.SetInstallConfigDefaults(a.Config)
+	return nil
+}

pkg/asset/installconfig/installconfig_test.go

@@ -5,16 +5,17 @@ import (
 	"os"
 	"testing"
 
-	"github.com/ghodss/yaml"
 	"github.com/golang/mock/gomock"
 	netopv1 "github.com/openshift/cluster-network-operator/pkg/apis/networkoperator/v1"
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/openshift/installer/pkg/asset"
 	"github.com/openshift/installer/pkg/asset/mock"
+	"github.com/openshift/installer/pkg/ipnet"
 	"github.com/openshift/installer/pkg/types"
 	"github.com/openshift/installer/pkg/types/aws"
+	"github.com/openshift/installer/pkg/types/none"
 )
 
 func validInstallConfig() *types.InstallConfig {
@@ -23,32 +24,67 @@ func validInstallConfig() *types.InstallConfig {
 			Name: "test-cluster",
 		},
 		BaseDomain: "test-domain",
-		Networking: types.Networking{
+		Platform: types.Platform{
+			AWS: &aws.Platform{
+				Region: "us-east-1",
+			},
+		},
+		PullSecret: `{"auths":{"example.com":{"auth":"authorization value"}}}`,
+	}
+}
+
+func TestInstallConfigGenerate_FillsInDefaults(t *testing.T) {
+	sshPublicKey := &sshPublicKey{}
+	baseDomain := &baseDomain{"test-domain"}
+	clusterName := &clusterName{"test-cluster"}
+	pullSecret := &pullSecret{`{"auths":{"example.com":{"auth":"authorization value"}}}`}
+	platform := &platform{
+		None: &none.Platform{},
+	}
+	installConfig := &InstallConfig{}
+	parents := asset.Parents{}
+	parents.Add(
+		sshPublicKey,
+		baseDomain,
+		clusterName,
+		pullSecret,
+		platform,
+	)
+	if err := installConfig.Generate(parents); err != nil {
+		t.Errorf("unexpected error generating install config: %v", err)
+	}
+	expected := &types.InstallConfig{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-cluster",
+		},
+		BaseDomain: "test-domain",
+		Networking: &types.Networking{
+			MachineCIDR: ipnet.MustParseCIDR("10.0.0.0/16"),
 			Type:        "OpenshiftSDN",
-			MachineCIDR: *defaultMachineCIDR,
-			ServiceCIDR: *defaultServiceCIDR,
+			ServiceCIDR: ipnet.MustParseCIDR("172.30.0.0/16"),
 			ClusterNetworks: []netopv1.ClusterNetwork{
 				{
-					CIDR:             "192.168.1.0/24",
-					HostSubnetLength: 4,
+					CIDR:             "10.128.0.0/14",
+					HostSubnetLength: 9,
 				},
 			},
 		},
 		Machines: []types.MachinePool{
 			{
-				Name: "master",
+				Name:     "master",
+				Replicas: func(x int64) *int64 { return &x }(3),
 			},
 			{
-				Name: "worker",
+				Name:     "worker",
+				Replicas: func(x int64) *int64 { return &x }(3),
 			},
 		},
 		Platform: types.Platform{
-			AWS: &aws.Platform{
-				Region: "us-east-1",
-			},
+			None: &none.Platform{},
 		},
 		PullSecret: `{"auths":{"example.com":{"auth":"authorization value"}}}`,
 	}
+	assert.Equal(t, expected, installConfig.Config, "unexpected config generated")
 }
 
 func TestInstallConfigLoad(t *testing.T) {
@@ -62,13 +98,49 @@ func TestInstallConfigLoad(t *testing.T) {
 	}{
 		{
 			name: "valid InstallConfig",
-			data: func() string {
-				ic := validInstallConfig()
-				data, _ := yaml.Marshal(ic)
-				return string(data)
-			}(),
-			expectedFound:  true,
-			expectedConfig: validInstallConfig(),
+			data: `
+metadata:
+  name: test-cluster
+baseDomain: test-domain
+platform:
+  aws:
+    region: us-east-1
+pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}"
+`,
+			expectedFound: true,
+			expectedConfig: &types.InstallConfig{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-cluster",
+				},
+				BaseDomain: "test-domain",
+				Networking: &types.Networking{
+					MachineCIDR: ipnet.MustParseCIDR("10.0.0.0/16"),
+					Type:        "OpenshiftSDN",
+					ServiceCIDR: ipnet.MustParseCIDR("172.30.0.0/16"),
+					ClusterNetworks: []netopv1.ClusterNetwork{
+						{
+							CIDR:             "10.128.0.0/14",
+							HostSubnetLength: 9,
+						},
+					},
+				},
+				Machines: []types.MachinePool{
+					{
+						Name:     "master",
+						Replicas: func(x int64) *int64 { return &x }(3),
+					},
+					{
+						Name:     "worker",
+						Replicas: func(x int64) *int64 { return &x }(3),
+					},
+				},
+				Platform: types.Platform{
+					AWS: &aws.Platform{
+						Region: "us-east-1",
+					},
+				},
+				PullSecret: `{"auths":{"example.com":{"auth":"authorization value"}}}`,
+			},
 		},
 		{
 			name: "invalid InstallConfig",
@@ -107,7 +179,7 @@ metadata:
 			fileFetcher.EXPECT().FetchByName(installConfigFilename).
 				Return(
 					&asset.File{
-						Filename: "test-filename",
+						Filename: installConfigFilename,
 						Data:     []byte(tc.data)},
 					tc.fetchError,
 				)
@@ -121,12 +193,8 @@ metadata:
 				assert.NoError(t, err, "unexpected error from Load")
 			}
 			if tc.expectedFound {
-				assert.Equal(t, "test-filename", ic.File.Filename, "unexpected File.Filename in InstallConfig")
-				assert.Equal(t, tc.data, string(ic.File.Data), "unexpected File.Data in InstallConfig")
-			} else {
-				assert.Nil(t, ic.File, "expected File in InstallConfig to be nil")
+				assert.Equal(t, tc.expectedConfig, ic.Config, "unexpected Config in InstallConfig")
 			}
-			assert.Equal(t, tc.expectedConfig, ic.Config, "unexpected Config in InstallConfig")
 		})
 	}
 }

pkg/asset/installconfig/libvirt/libvirt.go

@@ -4,21 +4,11 @@ package libvirt
 import (
 	survey "gopkg.in/AlecAivazis/survey.v1"
 
-	"github.com/openshift/installer/pkg/ipnet"
 	"github.com/openshift/installer/pkg/types/libvirt"
+	libvirtdefaults "github.com/openshift/installer/pkg/types/libvirt/defaults"
 	"github.com/openshift/installer/pkg/validate"
 )
 
-const (
-	defaultNetworkIfName = "tt0"
-)
-
-var (
-	// DefaultMachineCIDR is the libvirt default IP address space from
-	// which to assign machine IPs.
-	DefaultMachineCIDR = ipnet.MustParseCIDR("192.168.126.0/24")
-)
-
 // Platform collects libvirt-specific configuration.
 func Platform() (*libvirt.Platform, error) {
 	var uri string
@@ -27,7 +17,7 @@ func Platform() (*libvirt.Platform, error) {
 			Prompt: &survey.Input{
 				Message: "Libvirt Connection URI",
 				Help:    "The libvirt connection URI to be used. This must be accessible from the running cluster.",
-				Default: "qemu+tcp://192.168.122.1/system",
+				Default: libvirtdefaults.DefaultURI,
 			},
 			Validate: survey.ComposeValidators(survey.Required, uriValidator),
 		},
@@ -37,9 +27,6 @@ func Platform() (*libvirt.Platform, error) {
 	}
 
 	return &libvirt.Platform{
-		Network: libvirt.Network{
-			IfName: defaultNetworkIfName,
-		},
 		URI: uri,
 	}, nil
 }

pkg/types/aws/defaults/platform.go

@@ -0,0 +1,9 @@
+package defaults
+
+import (
+	"github.com/openshift/installer/pkg/types/aws"
+)
+
+// SetPlatformDefaults sets the defaults for the platform.
+func SetPlatformDefaults(p *aws.Platform) {
+}

pkg/types/aws/platform.go

@@ -7,10 +7,12 @@ type Platform struct {
 	Region string `json:"region"`
 
 	// UserTags specifies additional tags for AWS resources created for the cluster.
+	// +optional
 	UserTags map[string]string `json:"userTags,omitempty"`
 
 	// DefaultMachinePlatform is the default configuration used when
 	// installing on AWS for machine pools which do not define their own
 	// platform configuration.
+	// +optional
 	DefaultMachinePlatform *MachinePool `json:"defaultMachinePlatform,omitempty"`
 }