Skip to content

assets/installconfig: apply defaults to install config #902

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
staebler wants to merge 2 commits into from
Closed

assets/installconfig: apply defaults to install config #902

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b72a736
assets/installconfig: apply defaults to install config
staebler Dec 13, 2018
6c8f95c
asset/installconfig: include defaults in install-config.yaml
staebler Jan 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/asset/ignition/machine/master_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
4 changes: 2 additions & 2 deletions pkg/asset/ignition/machine/worker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
29 changes: 0 additions & 29 deletions pkg/asset/installconfig/clusterid.go
View file
Open in desktop

This file was deleted.

86 changes: 32 additions & 54 deletions pkg/asset/installconfig/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ import (
"github.com/sirupsen/logrus"
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"
)
Expand All @@ -22,13 +21,6 @@ const (
deprecatedInstallConfigFilename = "install-config.yml"
)

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"`
Expand All @@ -41,7 +33,6 @@ var _ asset.WritableAsset = (*InstallConfig)(nil)
// InstallConfig asset.
func (a *InstallConfig) Dependencies() []asset.Asset {
return []asset.Asset{
&clusterID{},
&sshPublicKey{},
&baseDomain{},
&clusterName{},
Expand All @@ -52,14 +43,12 @@ func (a *InstallConfig) Dependencies() []asset.Asset {

// Generate generates the install-config.yaml file.
func (a *InstallConfig) Generate(parents asset.Parents) error {
clusterID := &clusterID{}
sshPublicKey := &sshPublicKey{}
baseDomain := &baseDomain{}
clusterName := &clusterName{}
pullSecret := &pullSecret{}
platform := &platform{}
parents.Get(
clusterID,
sshPublicKey,
baseDomain,
clusterName,
Expand All @@ -71,51 +60,15 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {
ObjectMeta: metav1.ObjectMeta{
Name: clusterName.ClusterName,
},
ClusterID: clusterID.ClusterID,
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.Machines = []types.MachinePool{
{
Name: "master",
Replicas: func(x int64) *int64 { return &x }(numberOfMasters),
},
{
Name: "worker",
Replicas: func(x int64) *int64 { return &x }(numberOfWorkers),
},
}
a.Config.AWS = platform.AWS
a.Config.Libvirt = platform.Libvirt
a.Config.None = platform.None
a.Config.OpenStack = platform.OpenStack

data, err := yaml.Marshal(a.Config)
if err != nil {
Expand All @@ -126,6 +79,16 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {
Data: data,
}

// Apply the defaults *after* marshaling into yaml so that the defaults
Copy link
Contributor

@abhinavdahiya abhinavdahiya Dec 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little concerned. So the asset has 2 values of the install-config. one in File and other in Config.
And in the past people have tried to use Files() on an asset and decode rather than using the type.

Also, another thing which is more of a personal preference.
getting the install-config with values defaults is a good representation of what the installer has chosen for you and what you can modify for yourself.

Copy link
Contributor

@abhinavdahiya abhinavdahiya Dec 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ thoughts @crawford @wking @cgwalters @staebler

Copy link
Contributor

@crawford crawford Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmmmmmmmmmmm. I think I'd prefer if the install-config contained the defaults. Can we do any tricks to comment out the defaults but still include them?

Copy link
Member

@wking wking Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do any tricks to comment out the defaults but still include them?

This sounds really complicated.

As long as the installer handles default-injection after loading an install-config, I don't really care whether we include defaults in configs we write to disk or not.

Copy link
Contributor

@crawford crawford Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this so that the defaults are included? We can provide documentation to show which fields can be safely omitted for users who want to slim down.

Copy link
Contributor Author

@staebler staebler Jan 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll make that change right after I put the kids to bed.

// are omitted from the on-disk representation of the asset.
if err := a.setDefaults(); err != nil {
return errors.Wrapf(err, "failed to set defaults for install config")
}

if err := validation.ValidateInstallConfig(a.Config, openstackvalidation.NewValidValuesFetcher()).ToAggregate(); err != nil {
return errors.Wrap(err, "invalid install config")
}

return nil
}

Expand All @@ -148,20 +111,35 @@ func (a *InstallConfig) Load(f asset.FileFetcher) (found bool, err error) {
if file == nil {
return false, err
}
a.File = file

config := &types.InstallConfig{}
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
return true, nil
}

func (a *InstallConfig) setDefaults() error {
defaults.SetInstallConfigDefaults(a.Config)
if a.Config.Platform.Libvirt != nil {
if err := libvirt.SetLatestImage(a.Config.Platform.Libvirt); err != nil {
return err
}
}
return nil
}

func fetchInstallConfigFile(f asset.FileFetcher) (*asset.File, error) {
names := []string{installConfigFilename, deprecatedInstallConfigFilename}
for i, name := range names {
Expand Down
78 changes: 47 additions & 31 deletions pkg/asset/installconfig/installconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ 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"
)
Expand All @@ -22,27 +22,7 @@ func validInstallConfig() *types.InstallConfig {
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
},
ClusterID: "test-cluster-id",
BaseDomain: "test-domain",
Networking: types.Networking{
Type: "OpenshiftSDN",
MachineCIDR: *defaultMachineCIDR,
ServiceCIDR: *defaultServiceCIDR,
ClusterNetworks: []netopv1.ClusterNetwork{
{
CIDR: "192.168.1.0/24",
HostSubnetLength: 4,
},
},
},
Machines: []types.MachinePool{
{
Name: "master",
},
{
Name: "worker",
},
},
Platform: types.Platform{
AWS: &aws.Platform{
Region: "us-east-1",
Expand All @@ -63,13 +43,51 @@ 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
clusterID: test-cluster-id
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",
},
ClusterID: "test-cluster-id",
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",
Expand Down Expand Up @@ -125,12 +143,10 @@ metadata:
assert.NoError(t, err, "unexpected error from Load")
}
if tc.expectedFound {
assert.Equal(t, tc.expectedConfig, ic.Config, "unexpected Config in InstallConfig")
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")
})
}
}
Expand Down
Loading