From 560aebcd21b0481c877bdf12c1e6b924d6fdfaab Mon Sep 17 00:00:00 2001 From: Adrien Gentil Date: Mon, 7 Aug 2023 17:42:10 +0200 Subject: [PATCH] MGMT-14933: Validate hosts running in OCI (#5413) * MGMT-14933: Validate hosts running in OCI Check the manufacturer to check if hosts are running in OCI when platform OCI is selected. * update unit-tests and fix lint --- internal/provider/external/base.go | 25 ++---- internal/provider/external/ignition.go | 4 +- internal/provider/external/installConfig.go | 2 +- internal/provider/external/inventory.go | 4 +- internal/provider/external/oci.go | 55 ++++++++++++ internal/provider/external/oci_test.go | 99 +++++++++++++++++++++ internal/provider/registry/registry.go | 2 +- internal/provider/registry/registry_test.go | 20 ++--- 8 files changed, 176 insertions(+), 35 deletions(-) create mode 100644 internal/provider/external/oci.go create mode 100644 internal/provider/external/oci_test.go diff --git a/internal/provider/external/base.go b/internal/provider/external/base.go index 075544c051..b0b46227e1 100644 --- a/internal/provider/external/base.go +++ b/internal/provider/external/base.go @@ -1,33 +1,20 @@ package external import ( - "github.com/openshift/assisted-service/internal/provider" "github.com/openshift/assisted-service/models" "github.com/sirupsen/logrus" ) -type externalProvider struct { - Log logrus.FieldLogger - platformType models.PlatformType +// baseExternalProvider provides a default implementation suitable for platforms relying on the external platform. +// Compose it and implement Name() to fullfil the Provider interface. +type baseExternalProvider struct { + Log logrus.FieldLogger } -// NewExternalProvider creates a new none platform provider. -func NewExternalProvider(log logrus.FieldLogger, platformType models.PlatformType) provider.Provider { - return &externalProvider{ - Log: log, - platformType: platformType, - } -} - -// Name returns the name of the provider -func (p *externalProvider) Name() models.PlatformType { - return p.platformType -} - -func (p *externalProvider) IsHostSupported(_ *models.Host) (bool, error) { +func (p *baseExternalProvider) IsHostSupported(_ *models.Host) (bool, error) { return true, nil } -func (p *externalProvider) AreHostsSupported(_ []*models.Host) (bool, error) { +func (p *baseExternalProvider) AreHostsSupported(hosts []*models.Host) (bool, error) { return true, nil } diff --git a/internal/provider/external/ignition.go b/internal/provider/external/ignition.go index ae322d0ed2..bff1e506a3 100644 --- a/internal/provider/external/ignition.go +++ b/internal/provider/external/ignition.go @@ -4,10 +4,10 @@ import ( "github.com/openshift/assisted-service/internal/common" ) -func (p externalProvider) PreCreateManifestsHook(cluster *common.Cluster, envVars *[]string, workDir string) error { +func (p baseExternalProvider) PreCreateManifestsHook(cluster *common.Cluster, envVars *[]string, workDir string) error { return nil } -func (p externalProvider) PostCreateManifestsHook(cluster *common.Cluster, envVars *[]string, workDir string) error { +func (p baseExternalProvider) PostCreateManifestsHook(cluster *common.Cluster, envVars *[]string, workDir string) error { return nil } diff --git a/internal/provider/external/installConfig.go b/internal/provider/external/installConfig.go index 3ba9675f44..9a2bf7693a 100644 --- a/internal/provider/external/installConfig.go +++ b/internal/provider/external/installConfig.go @@ -9,7 +9,7 @@ import ( "github.com/openshift/assisted-service/models" ) -func (p externalProvider) AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error { +func (p baseExternalProvider) AddPlatformToInstallConfig(cfg *installcfg.InstallerConfigBaremetal, cluster *common.Cluster) error { cfg.Platform = installcfg.Platform{ External: &installcfg.ExternalInstallConfigPlatform{ PlatformName: string(p.Name()), diff --git a/internal/provider/external/inventory.go b/internal/provider/external/inventory.go index 30a7374d7e..9272ae523d 100644 --- a/internal/provider/external/inventory.go +++ b/internal/provider/external/inventory.go @@ -5,11 +5,11 @@ import ( "github.com/openshift/assisted-service/models" ) -func (p *externalProvider) CleanPlatformValuesFromDBUpdates(_ map[string]interface{}) error { +func (p *baseExternalProvider) CleanPlatformValuesFromDBUpdates(_ map[string]interface{}) error { return nil } -func (p *externalProvider) SetPlatformUsages( +func (p *baseExternalProvider) SetPlatformUsages( usages map[string]models.Usage, usageApi usage.API) error { props := &map[string]interface{}{ diff --git a/internal/provider/external/oci.go b/internal/provider/external/oci.go new file mode 100644 index 0000000000..43d7595d48 --- /dev/null +++ b/internal/provider/external/oci.go @@ -0,0 +1,55 @@ +package external + +import ( + "fmt" + + "github.com/openshift/assisted-service/internal/common" + "github.com/openshift/assisted-service/internal/provider" + "github.com/openshift/assisted-service/models" + "github.com/sirupsen/logrus" +) + +const ( + OCIManufacturer string = "OracleCloud.com" +) + +type ociExternalProvider struct { + baseExternalProvider +} + +func NewOciExternalProvider(log logrus.FieldLogger) provider.Provider { + return &ociExternalProvider{ + baseExternalProvider: baseExternalProvider{ + Log: log, + }, + } +} + +func (p *baseExternalProvider) Name() models.PlatformType { + return models.PlatformTypeOci +} + +func (p *ociExternalProvider) IsHostSupported(host *models.Host) (bool, error) { + // during the discovery there is a short time that host didn't return its inventory to the service + if host.Inventory == "" { + return false, nil + } + hostInventory, err := common.UnmarshalInventory(host.Inventory) + if err != nil { + return false, fmt.Errorf("error marshaling host to inventory, error %w", err) + } + return hostInventory.SystemVendor.Manufacturer == OCIManufacturer, nil +} + +func (p *ociExternalProvider) AreHostsSupported(hosts []*models.Host) (bool, error) { + for _, h := range hosts { + supported, err := p.IsHostSupported(h) + if err != nil { + return false, fmt.Errorf("error while checking if host is supported, error is: %w", err) + } + if !supported { + return false, nil + } + } + return true, nil +} diff --git a/internal/provider/external/oci_test.go b/internal/provider/external/oci_test.go new file mode 100644 index 0000000000..b6c5675b99 --- /dev/null +++ b/internal/provider/external/oci_test.go @@ -0,0 +1,99 @@ +package external + +import ( + "encoding/json" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "github.com/openshift/assisted-service/internal/common" + "github.com/openshift/assisted-service/internal/provider" + "github.com/openshift/assisted-service/models" +) + +var _ = Describe("oci", func() { + var log = common.GetTestLog() + Context("host", func() { + var provider provider.Provider + var host *models.Host + BeforeEach(func() { + provider = NewOciExternalProvider(log) + host = &models.Host{} + }) + + setHostInventory := func(inventory *models.Inventory, host *models.Host) { + data, err := json.Marshal(inventory) + Expect(err).To(BeNil()) + host.Inventory = string(data) + } + + It("is supported", func() { + inventory := &models.Inventory{ + SystemVendor: &models.SystemVendor{ + Manufacturer: OCIManufacturer, + }, + } + setHostInventory(inventory, host) + supported, err := provider.IsHostSupported(host) + Expect(err).To(BeNil()) + Expect(supported).To(BeTrue()) + }) + + It("is not supported", func() { + inventory := &models.Inventory{ + SystemVendor: &models.SystemVendor{ + Manufacturer: "", + }, + } + setHostInventory(inventory, host) + supported, err := provider.IsHostSupported(host) + Expect(err).To(BeNil()) + Expect(supported).To(BeFalse()) + }) + + It("are supported", func() { + inventory := &models.Inventory{ + SystemVendor: &models.SystemVendor{ + Manufacturer: OCIManufacturer, + }, + } + setHostInventory(inventory, host) + supported, err := provider.AreHostsSupported([]*models.Host{host, host}) + Expect(err).To(BeNil()) + Expect(supported).To(BeTrue()) + }) + + It("are not supported", func() { + inventory := &models.Inventory{ + SystemVendor: &models.SystemVendor{ + Manufacturer: OCIManufacturer, + }, + } + setHostInventory(inventory, host) + + notOCIHost := &models.Host{} + notOCIinventory := &models.Inventory{ + SystemVendor: &models.SystemVendor{ + Manufacturer: "", + }, + } + setHostInventory(notOCIinventory, notOCIHost) + + supported, err := provider.AreHostsSupported([]*models.Host{host, notOCIHost}) + Expect(err).To(BeNil()) + Expect(supported).To(BeFalse()) + }) + + It("has no inventory", func() { + supported, err := provider.IsHostSupported(host) + Expect(err).To(BeNil()) + Expect(supported).To(BeFalse()) + }) + + It("has an invalid inventory", func() { + host.Inventory = "invalid-inventory" + supported, err := provider.IsHostSupported(host) + Expect(err).To(HaveOccurred()) + Expect(supported).To(BeFalse()) + }) + }) +}) diff --git a/internal/provider/registry/registry.go b/internal/provider/registry/registry.go index f20f568450..2f9b2c69c9 100644 --- a/internal/provider/registry/registry.go +++ b/internal/provider/registry/registry.go @@ -166,6 +166,6 @@ func InitProviderRegistry(log logrus.FieldLogger) ProviderRegistry { providerRegistry.Register(baremetal.NewBaremetalProvider(log)) providerRegistry.Register(none.NewNoneProvider(log)) providerRegistry.Register(nutanix.NewNutanixProvider(log)) - providerRegistry.Register(external.NewExternalProvider(log, models.PlatformTypeOci)) + providerRegistry.Register(external.NewOciExternalProvider(log)) return providerRegistry } diff --git a/internal/provider/registry/registry_test.go b/internal/provider/registry/registry_test.go index 9b96c1ad74..68a63e6b9f 100644 --- a/internal/provider/registry/registry_test.go +++ b/internal/provider/registry/registry_test.go @@ -85,16 +85,16 @@ var _ = Describe("Test GetSupportedProvidersByHosts", func() { hosts = append(hosts, createHost(false, models.HostStatusKnown, bmInventory)) platforms, err := providerRegistry.GetSupportedProvidersByHosts(hosts) Expect(err).To(BeNil()) - Expect(len(platforms)).Should(Equal(3)) - Expect(platforms).Should(ContainElements(models.PlatformTypeBaremetal, models.PlatformTypeNone, models.PlatformTypeOci)) + Expect(len(platforms)).Should(Equal(2)) + Expect(platforms).Should(ContainElements(models.PlatformTypeBaremetal, models.PlatformTypeNone)) }) It("single vsphere host", func() { hosts := make([]*models.Host, 0) hosts = append(hosts, createHost(true, models.HostStatusKnown, vsphereInventory)) platforms, err := providerRegistry.GetSupportedProvidersByHosts(hosts) Expect(err).To(BeNil()) - Expect(len(platforms)).Should(Equal(4)) - supportedPlatforms := []models.PlatformType{models.PlatformTypeBaremetal, models.PlatformTypeVsphere, models.PlatformTypeNone, models.PlatformTypeOci} + Expect(len(platforms)).Should(Equal(3)) + supportedPlatforms := []models.PlatformType{models.PlatformTypeBaremetal, models.PlatformTypeVsphere, models.PlatformTypeNone} Expect(platforms).Should(ContainElements(supportedPlatforms)) }) It("5 vsphere hosts - 3 masters, 2 workers", func() { @@ -106,8 +106,8 @@ var _ = Describe("Test GetSupportedProvidersByHosts", func() { hosts = append(hosts, createHost(false, models.HostStatusKnown, vsphereInventory)) platforms, err := providerRegistry.GetSupportedProvidersByHosts(hosts) Expect(err).To(BeNil()) - Expect(len(platforms)).Should(Equal(4)) - supportedPlatforms := []models.PlatformType{models.PlatformTypeBaremetal, models.PlatformTypeVsphere, models.PlatformTypeNone, models.PlatformTypeOci} + Expect(len(platforms)).Should(Equal(3)) + supportedPlatforms := []models.PlatformType{models.PlatformTypeBaremetal, models.PlatformTypeVsphere, models.PlatformTypeNone} Expect(platforms).Should(ContainElements(supportedPlatforms)) }) It("2 vsphere hosts 1 generic host", func() { @@ -117,8 +117,8 @@ var _ = Describe("Test GetSupportedProvidersByHosts", func() { hosts = append(hosts, createHost(true, models.HostStatusKnown, vsphereInventory)) platforms, err := providerRegistry.GetSupportedProvidersByHosts(hosts) Expect(err).To(BeNil()) - Expect(len(platforms)).Should(Equal(3)) - Expect(platforms).Should(ContainElements(models.PlatformTypeBaremetal, models.PlatformTypeNone, models.PlatformTypeOci)) + Expect(len(platforms)).Should(Equal(2)) + Expect(platforms).Should(ContainElements(models.PlatformTypeBaremetal, models.PlatformTypeNone)) }) It("3 vsphere masters 2 generic workers", func() { hosts := make([]*models.Host, 0) @@ -129,8 +129,8 @@ var _ = Describe("Test GetSupportedProvidersByHosts", func() { hosts = append(hosts, createHost(false, models.HostStatusKnown, bmInventory)) platforms, err := providerRegistry.GetSupportedProvidersByHosts(hosts) Expect(err).To(BeNil()) - Expect(len(platforms)).Should(Equal(3)) - Expect(platforms).Should(ContainElements(models.PlatformTypeBaremetal, models.PlatformTypeNone, models.PlatformTypeOci)) + Expect(len(platforms)).Should(Equal(2)) + Expect(platforms).Should(ContainElements(models.PlatformTypeBaremetal, models.PlatformTypeNone)) }) It("host with an invalid inventory", func() { hosts := make([]*models.Host, 0)