Skip to content

Commit

Permalink
MGMT-18292: removing usage of installconfig.Load function as it set
Browse files Browse the repository at this point in the history
install config default values that are breaking image based config image
In image based config we are using only small amount of install config
values and there is no need to set defaults for it
  • Loading branch information
tsorya committed Oct 1, 2024
1 parent 7072cb0 commit 4cdbfd8
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 19 deletions.
4 changes: 3 additions & 1 deletion pkg/asset/imagebased/configimage/clusterconfiguration.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,9 @@ func (cc *ClusterConfiguration) Generate(_ context.Context, dependencies asset.P
}

// validation for the length of the MachineNetwork is performed in the InstallConfig
cc.Config.MachineNetwork = installConfig.Config.Networking.MachineNetwork[0].CIDR.String()
if len(installConfig.Config.Networking.MachineNetwork) > 0 {
cc.Config.MachineNetwork = installConfig.Config.Networking.MachineNetwork[0].CIDR.String()
}

clusterConfigurationData, err := json.Marshal(cc.Config)
if err != nil {
Expand Down
69 changes: 63 additions & 6 deletions pkg/asset/imagebased/configimage/installconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,22 @@ package configimage

import (
"context"
"encoding/json"
"fmt"
"os"
"strings"

configv1 "github.com/openshift/api/config/v1"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
"k8s.io/apimachinery/pkg/util/validation/field"
"sigs.k8s.io/yaml"

configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/installconfig"
"github.com/openshift/installer/pkg/types"
"github.com/openshift/installer/pkg/types/conversion"
"github.com/openshift/installer/pkg/types/defaults"
"github.com/openshift/installer/pkg/types/none"
"github.com/openshift/installer/pkg/types/validation"
)
Expand Down Expand Up @@ -43,11 +50,49 @@ func (i *InstallConfig) Generate(_ context.Context, parents asset.Parents) error
return nil
}

// LoadFromFile returns the installconfig from disk.
func (i *InstallConfig) loadFromFile(f asset.FileFetcher) (found bool, err error) {
file, err := f.FetchByName(InstallConfigFilename)
if err != nil {
if os.IsNotExist(err) {
return false, nil
}
return false, errors.Wrap(err, asset.InstallConfigError)
}

config := &types.InstallConfig{}
if err := yaml.UnmarshalStrict(file.Data, config, yaml.DisallowUnknownFields); err != nil {
err = errors.Wrapf(err, "failed to unmarshal %s", InstallConfigFilename)
if !strings.Contains(err.Error(), "unknown field") {
return false, errors.Wrap(err, asset.InstallConfigError)
}
err = errors.Wrapf(err, "failed to parse first occurrence of unknown field")
logrus.Warnf(err.Error())
logrus.Info("Attempting to unmarshal while ignoring unknown keys because strict unmarshaling failed")
if err = yaml.Unmarshal(file.Data, config); err != nil {
err = errors.Wrapf(err, "failed to unmarshal %s", InstallConfigFilename)
return false, errors.Wrap(err, asset.InstallConfigError)
}
}
i.Config = config

// Upconvert any deprecated fields
if err := conversion.ConvertInstallConfig(i.Config); err != nil {
return false, errors.Wrap(errors.Wrap(err, "failed to upconvert install config"), asset.InstallConfigError)
}

return true, nil
}

// Load returns the installconfig from disk.
func (i *InstallConfig) Load(f asset.FileFetcher) (bool, error) {
found, err := i.LoadFromFile(f)
found, err := i.loadFromFile(f)
if found && err == nil {
if err := i.validateInstallConfig(i.Config).ToAggregate(); err != nil {
installConfig := &types.InstallConfig{}
if err := deepCopy(i.Config, installConfig); err != nil {
return false, fmt.Errorf("invalid install-config configuration: %w", err)
}
if err := i.validateInstallConfig(installConfig).ToAggregate(); err != nil {
return false, fmt.Errorf("invalid install-config configuration: %w", err)
}
if err := i.RecordFile(); err != nil {
Expand All @@ -57,9 +102,14 @@ func (i *InstallConfig) Load(f asset.FileFetcher) (bool, error) {
return found, err
}

// in order to avoid the validation errors, we need to set the defaults and validate the configuration
// though those defaults are not used in the image-based install config
// we provide installConfig by value to avoid modifying the original installConfig
func (i *InstallConfig) validateInstallConfig(installConfig *types.InstallConfig) field.ErrorList {
var allErrs field.ErrorList
if err := validation.ValidateInstallConfig(i.Config, true); err != nil {

defaults.SetInstallConfigDefaults(installConfig)
if err := validation.ValidateInstallConfig(installConfig, true); err != nil {
allErrs = append(allErrs, err...)
}

Expand All @@ -76,7 +126,6 @@ func (i *InstallConfig) validateInstallConfig(installConfig *types.InstallConfig
if err := i.validateSNOConfiguration(installConfig); err != nil {
allErrs = append(allErrs, err...)
}

return allErrs
}

Expand Down Expand Up @@ -117,7 +166,7 @@ func (i *InstallConfig) validateSNOConfiguration(installConfig *types.InstallCon
}

machineNetworksCount := len(installConfig.Networking.MachineNetwork)
if machineNetworksCount != 1 {
if machineNetworksCount > 1 {
fieldPath = field.NewPath("Networking", "MachineNetwork")
allErrs = append(allErrs, field.TooMany(fieldPath, machineNetworksCount, 1))
}
Expand Down Expand Up @@ -171,3 +220,11 @@ func warnUnusedConfig(installConfig *types.InstallConfig) {
logrus.Warnf(fmt.Sprintf("%s is ignored", fieldPath))
}
}

func deepCopy(src, dst interface{}) error {
bytes, err := json.Marshal(src)
if err != nil {
return err
}
return json.Unmarshal(bytes, dst)
}
72 changes: 60 additions & 12 deletions pkg/asset/imagebased/configimage/installconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,19 +167,68 @@ pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"c3VwZXItc2VjcmV0Cg==\"}}}"
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
},
AdditionalTrustBundlePolicy: types.PolicyProxyOnly,
BaseDomain: "test-domain",
BaseDomain: "test-domain",
Networking: &types.Networking{
MachineNetwork: []types.MachineNetworkEntry{
{CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")},
NetworkType: "OVNKubernetes",
},
ControlPlane: &types.MachinePool{
Name: "master",
Replicas: ptr.To[int64](1),
Hyperthreading: types.HyperthreadingEnabled,
Architecture: types.ArchitectureAMD64,
},
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: ptr.To[int64](0),
Hyperthreading: types.HyperthreadingEnabled,
Architecture: types.ArchitectureAMD64,
},
NetworkType: "OVNKubernetes",
ServiceNetwork: []ipnet.IPNet{*ipnet.MustParseCIDR("172.30.0.0/16")},
ClusterNetwork: []types.ClusterNetworkEntry{
{
CIDR: *ipnet.MustParseCIDR("10.128.0.0/14"),
HostPrefix: 23,
},
},
Platform: types.Platform{None: &none.Platform{}},
PullSecret: `{"auths":{"example.com":{"auth":"c3VwZXItc2VjcmV0Cg=="}}}`,
},
},
{
name: "valid configuration for none platform for sno with machine network",
data: `
apiVersion: v1
metadata:
name: test-cluster
baseDomain: test-domain
networking:
networkType: OVNKubernetes
machineNetwork:
- cidr: 10.0.0.0/24
compute:
- architecture: amd64
hyperthreading: Enabled
name: worker
platform: {}
replicas: 0
controlPlane:
architecture: amd64
hyperthreading: Enabled
name: master
platform: {}
replicas: 1
platform:
none : {}
pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"c3VwZXItc2VjcmV0Cg==\"}}}"
`,
expectedFound: true,
expectedConfig: &types.InstallConfig{
TypeMeta: metav1.TypeMeta{
APIVersion: types.InstallConfigVersion,
},
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
},
BaseDomain: "test-domain",
Networking: &types.Networking{
NetworkType: "OVNKubernetes",
MachineNetwork: []types.MachineNetworkEntry{
{CIDR: *ipnet.MustParseCIDR("10.0.0.0/24")},
},
},
ControlPlane: &types.MachinePool{
Expand All @@ -198,7 +247,6 @@ pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"c3VwZXItc2VjcmV0Cg==\"}}}"
},
Platform: types.Platform{None: &none.Platform{}},
PullSecret: `{"auths":{"example.com":{"auth":"c3VwZXItc2VjcmV0Cg=="}}}`,
Publish: types.ExternalPublishingStrategy,
},
},
}
Expand Down

0 comments on commit 4cdbfd8

Please sign in to comment.