OCPBUGS-44882: Add NTP sources to generated install-config.yaml

In OpenShift 4.18 a new `additionalNTPServers` field was added to the baremetal platform section of the `install-config.yaml` file . This needs to be populated with the NTP sources provided by the user in order to correctly set time before installation of new nodes using the baremetal provisioning flow. Related: https://issues.redhat.com/browse/OCPBUGS-44882 Signed-off-by: Juan Hernandez <[email protected]>
openshift · Nov 26, 2024 · 234490f · 234490f
1 parent 8dc67a1
commit 234490f
Show file tree

Hide file tree

Showing 15 changed files with 217 additions and 36 deletions.
diff --git a/internal/installcfg/builder/builder.go b/internal/installcfg/builder/builder.go
@@ -246,7 +246,7 @@ func (i *installConfigBuilder) getInstallConfig(cluster *common.Cluster, cluster
 		return nil, err
 	}
 
-	err = i.providerRegistry.AddPlatformToInstallConfig(cfg, cluster)
+	err = i.providerRegistry.AddPlatformToInstallConfig(cfg, cluster, clusterInfraenvs)
 	if err != nil {
 		return nil, fmt.Errorf(
 			"error while adding Platfom %s to install config, error is: %w", common.PlatformTypeValue(cluster.Platform.Type), err)

diff --git a/internal/installcfg/installcfg.go b/internal/installcfg/installcfg.go
@@ -42,6 +42,7 @@ type BareMetalInstallConfigPlatform struct {
 	ProvisioningNetworkInterface string   `json:"provisioningNetworkInterface,omitempty"`
 	ProvisioningNetworkCIDR      *string  `json:"provisioningNetworkCIDR,omitempty"`
 	ProvisioningDHCPRange        string   `json:"provisioningDHCPRange,omitempty"`
+	AdditionalNTPServers         []string `json:"additionalNTPServers,omitempty"`
 }
 
 type VsphereFailureDomainTopology struct {

diff --git a/internal/provider/baremetal/installConfig.go b/internal/provider/baremetal/installConfig.go
@@ -2,7 +2,9 @@ package baremetal
 
 import (
 	"encoding/json"
+	"slices"
 	"sort"
+	"strings"
 
 	"github.com/go-openapi/swag"
 	"github.com/openshift/assisted-service/internal/common"
@@ -14,8 +16,8 @@ import (
 	"github.com/pkg/errors"
 )
 
-func (p baremetalProvider) AddPlatformToInstallConfig(
-	cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error {
+func (p *baremetalProvider) AddPlatformToInstallConfig(
+	cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster, infraEnvs []*common.InfraEnv) error {
 	// set hosts
 	numMasters := cfg.ControlPlane.Replicas
 	// TODO: will we always have just one compute?
@@ -111,5 +113,36 @@ func (p baremetalProvider) AddPlatformToInstallConfig(
 			},
 		}
 	}
+
+	// We want to use the NTP sources specified in the cluster, and if that is empty, the ones specified in the
+	// infrastructure environment. Note that in some rare cases there may be multiple infrastructure environments,
+	// so we add the NTP sources of all of them.
+	ntpServers := p.splitNTPSources(cluster.AdditionalNtpSource)
+	if len(ntpServers) == 0 {
+		for _, infraEnv := range infraEnvs {
+			for _, ntpSource := range p.splitNTPSources(infraEnv.AdditionalNtpSources) {
+				if !slices.Contains(ntpServers, ntpSource) {
+					ntpServers = append(ntpServers, ntpSource)
+				}
+			}
+		}
+	}
+
+	// Note that the new `additionalNTPServers` field was added in OpenShift 4.18, but we add it to all versions
+	// here because older versions will just ignore it.
+	cfg.Platform.Baremetal.AdditionalNTPServers = ntpServers
+
 	return nil
 }
+
+func (p *baremetalProvider) splitNTPSources(sources string) []string {
+	split := strings.Split(sources, ",")
+	var result []string
+	for _, source := range split {
+		source = strings.TrimSpace(source)
+		if source != "" {
+			result = append(result, source)
+		}
+	}
+	return result
+}
diff --git a/internal/provider/baremetal/installConfig_test.go b/internal/provider/baremetal/installConfig_test.go
@@ -0,0 +1,129 @@
+package baremetal
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/openshift/assisted-service/internal/common"
+	"github.com/openshift/assisted-service/internal/installcfg"
+	"github.com/openshift/assisted-service/internal/provider"
+	"github.com/openshift/assisted-service/models"
+	"github.com/sirupsen/logrus"
+)
+
+var _ = Describe("Add NTP sources", func() {
+	var (
+		logger    logrus.FieldLogger
+		cluster   *common.Cluster
+		infraEnvs []*common.InfraEnv
+		cfg       *installcfg.InstallerConfigBaremetal
+		provider  provider.Provider
+	)
+
+	BeforeEach(func() {
+		logger = common.GetTestLog()
+		cluster = &common.Cluster{
+			Cluster: models.Cluster{
+				OpenshiftVersion: "4.18",
+			},
+		}
+		infraEnvs = []*common.InfraEnv{{
+			InfraEnv: models.InfraEnv{},
+		}}
+		cfg = &installcfg.InstallerConfigBaremetal{
+			ControlPlane: struct {
+				Hyperthreading string "json:\"hyperthreading,omitempty\""
+				Name           string "json:\"name\""
+				Replicas       int    "json:\"replicas\""
+			}{
+				Replicas: 1,
+			},
+			Compute: []struct {
+				Hyperthreading string "json:\"hyperthreading,omitempty\""
+				Name           string "json:\"name\""
+				Replicas       int    "json:\"replicas\""
+			}{{
+				Replicas: 1,
+			}},
+		}
+		provider = NewBaremetalProvider(logger)
+	})
+
+	It("Does nothing if there are no NTP sources", func() {
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(BeEmpty())
+	})
+
+	It("Adds one NTP source from cluster", func() {
+		cluster.AdditionalNtpSource = "1.1.1.1"
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(ConsistOf("1.1.1.1"))
+	})
+
+	It("Adds multiple NTP sources from cluster", func() {
+		cluster.AdditionalNtpSource = "1.1.1.1,2.2.2.2,3.3.3.3"
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(ConsistOf("1.1.1.1", "2.2.2.2", "3.3.3.3"))
+	})
+
+	It("Removes extra white space in NTP sources from cluster", func() {
+		cluster.AdditionalNtpSource = "  1.1.1.1,   \t  2.2.2.2 , 3.3.3.3  "
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(ConsistOf("1.1.1.1", "2.2.2.2", "3.3.3.3"))
+	})
+
+	It("Adds one source from one infrastructure environment", func() {
+		infraEnvs[0].AdditionalNtpSources = "1.1.1.1"
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(ConsistOf("1.1.1.1"))
+	})
+
+	It("Adds multiple sources from one infrastructure environment", func() {
+		infraEnvs[0].AdditionalNtpSources = "1.1.1.1,2.2.2.2,3.3.3.3"
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(ConsistOf("1.1.1.1", "2.2.2.2", "3.3.3.3"))
+	})
+
+	It("Removes extra white space in NTP sources from infrastructure environment", func() {
+		infraEnvs[0].AdditionalNtpSources = "  1.1.1.1,   \t  2.2.2.2 , 3.3.3.3  "
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(ConsistOf("1.1.1.1", "2.2.2.2", "3.3.3.3"))
+	})
+
+	It("Ignores sources from infrastructure environment if there are NTP sources in the cluster", func() {
+		cluster.AdditionalNtpSource = "1.1.1.1"
+		infraEnvs[0].AdditionalNtpSources = "2.2.2.2"
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(ConsistOf("1.1.1.1"))
+	})
+
+	It("Combines NTP sources from multiple infrastructure environments", func() {
+		infraEnvs := []*common.InfraEnv{
+			{
+				InfraEnv: models.InfraEnv{
+					AdditionalNtpSources: "1.1.1.1",
+				},
+			},
+			{
+				InfraEnv: models.InfraEnv{
+					AdditionalNtpSources: "2.2.2.2",
+				},
+			},
+			{
+				InfraEnv: models.InfraEnv{
+					AdditionalNtpSources: "3.3.3.3",
+				},
+			},
+		}
+		err := provider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(cfg.Platform.Baremetal.AdditionalNTPServers).To(ConsistOf("1.1.1.1", "2.2.2.2", "3.3.3.3"))
+	})
+})
diff --git a/internal/provider/baremetal/suite_test.go b/internal/provider/baremetal/suite_test.go
@@ -0,0 +1,13 @@
+package baremetal
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+func TestBaremetalProvider(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Baremetal provider")
+}
diff --git a/internal/provider/base.go b/internal/provider/base.go
@@ -26,7 +26,8 @@ type Provider interface {
 	IsProviderForPlatform(platform *models.Platform) bool
 	// AddPlatformToInstallConfig adds the provider platform to the installconfig platform field,
 	// sets platform fields from values within the cluster model.
-	AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error
+	AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster,
+		infraEnvs []*common.InfraEnv) error
 	// CleanPlatformValuesFromDBUpdates remove platform specific values from the `updates` data structure
 	CleanPlatformValuesFromDBUpdates(updates map[string]interface{}) error
 	// SetPlatformUsages uses the usageApi to update platform specific usages

diff --git a/internal/provider/external/base.go b/internal/provider/external/base.go
@@ -27,7 +27,8 @@ func (p *baseExternalProvider) AreHostsSupported(hosts []*models.Host) (bool, er
 	return true, nil
 }
 
-func (p *baseExternalProvider) AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error {
+func (p *baseExternalProvider) AddPlatformToInstallConfig(
+	cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster, infraEnvs []*common.InfraEnv) error {
 	cfg.Platform = installcfg.Platform{
 		External: &installcfg.ExternalInstallConfigPlatform{
 			PlatformName:           *cluster.Platform.External.PlatformName,

diff --git a/internal/provider/mock_base_provider.go b/internal/provider/mock_base_provider.go
diff --git a/internal/provider/none/installConfig.go b/internal/provider/none/installConfig.go
@@ -6,7 +6,8 @@ import (
 	"github.com/openshift/assisted-service/internal/provider"
 )
 
-func (p noneProvider) AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error {
+func (p noneProvider) AddPlatformToInstallConfig(
+	cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster, infraEnvs []*common.InfraEnv) error {
 	cfg.Platform = installcfg.Platform{
 		None: &installcfg.PlatformNone{},
 	}

diff --git a/internal/provider/nutanix/installConfig.go b/internal/provider/nutanix/installConfig.go
@@ -39,7 +39,7 @@ func setPlatformValues(platform *installcfg.NutanixInstallConfigPlatform) {
 }
 
 func (p nutanixProvider) AddPlatformToInstallConfig(
-	cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error {
+	cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster, infraEnvs []*common.InfraEnv) error {
 	nPlatform := &installcfg.NutanixInstallConfigPlatform{}
 	if !swag.BoolValue(cluster.UserManagedNetworking) {
 		if len(cluster.APIVips) == 0 {

diff --git a/internal/provider/registry/mock_providerregistry.go b/internal/provider/registry/mock_providerregistry.go
diff --git a/internal/provider/registry/registry.go b/internal/provider/registry/registry.go
@@ -31,7 +31,8 @@ type ProviderRegistry interface {
 	GetSupportedProvidersByHosts(hosts []*models.Host) ([]models.PlatformType, error)
 	// AddPlatformToInstallConfig adds the provider platform to the installconfig platform field,
 	// sets platform fields from values within the cluster model.
-	AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error
+	AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster,
+		infraEnvs []*common.InfraEnv) error
 	// SetPlatformUsages uses the usageApi to update platform specific usages
 	SetPlatformUsages(p *models.Platform, usages map[string]models.Usage, usageApi usage.API) error
 	// IsHostSupported checks if the provider supports the host
@@ -88,12 +89,13 @@ func (r *registry) Name() models.PlatformType {
 	return ""
 }
 
-func (r *registry) AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error {
+func (r *registry) AddPlatformToInstallConfig(
+	cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster, infraEnvs []*common.InfraEnv) error {
 	currentProvider, err := r.Get(cluster.Platform)
 	if err != nil {
 		return fmt.Errorf("error adding platform to install config, platform provider wasn't set: %w", err)
 	}
-	return currentProvider.AddPlatformToInstallConfig(cfg, cluster)
+	return currentProvider.AddPlatformToInstallConfig(cfg, cluster, infraEnvs)
 }
 
 func (r *registry) SetPlatformUsages(