Skip to content

CORS-2835: use build tags to produce installer with alternate infrastructure providers #7656

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

Merged
openshift-merge-bot merged 3 commits into from
Nov 7, 2023
Merged

CORS-2835: use build tags to produce installer with alternate infrastructure providers #7656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eb1d8ef
pkg/infrastructure: errorr on unsupported platform
patrickdillon Nov 1, 2023
7224fdb
Add Alternate Infrastructure Build Tag
patrickdillon Nov 1, 2023
7014647
hack/build.sh: skip terraform for altinfra or aro
patrickdillon Oct 31, 2023
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
5 changes: 4 additions & 1 deletion cmd/openshift-install/gather.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ func runGatherBootstrapCmd(directory string) (string, error) {
return "", errors.Wrapf(err, "failed to fetch %s", config.Name())
}

provider := infra.ProviderForPlatform(config.Config.Platform.Name())
provider, err := infra.ProviderForPlatform(config.Config.Platform.Name())
if err != nil {
return "", fmt.Errorf("error getting infrastructure provider: %w", err)
}
if err = provider.ExtractHostAddresses(directory, config.Config, ha); err != nil {
logrus.Warnf("Failed to extract host addresses: %s", err.Error())
}
Expand Down
2 changes: 1 addition & 1 deletion hack/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export CGO_ENABLED=0
MODE="${MODE:-release}"

# Build terraform binaries before setting environment variables since it messes up make
if test "${SKIP_TERRAFORM}" != y
if test "${SKIP_TERRAFORM}" != y && ! (echo "${TAGS}" | grep -q -e 'aro' -e 'altinfra')
Copy link
Contributor

@andfasano andfasano Nov 2, 2023

Choose a reason for hiding this comment

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

I wasn't aware about the specific build tag aro, must be skipped as well?

Copy link
Contributor Author

@patrickdillon patrickdillon Nov 3, 2023

Choose a reason for hiding this comment

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

Yes, so I have also added the aro tag here:
https://github.com/openshift/installer/pull/7656/files#diff-a4f0f6b61545d617e8db370db204ea8d9fbd62f380527ef49351bf617c781e32R1-R2

ARO has a fork of the installer and does not use Terraform (using ARO-RP instead). I believe they use this ARO build tag when building their fork, so the result should be that the ARO version of the installer is built without Terraform dependencies.

then
make -j8 -C terraform all
copy_terraform_to_mirror # Copy terraform parts to embedded mirror.
Expand Down
5 changes: 4 additions & 1 deletion pkg/asset/cluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ func (c *Cluster) Generate(parents asset.Parents) (err error) {
tfvarsFiles = append(tfvarsFiles, file)
}

provider := infra.ProviderForPlatform(platform)
provider, err := infra.ProviderForPlatform(platform)
if err != nil {
return fmt.Errorf("error getting infrastructure provider: %w", err)
}
files, err := provider.Provision(InstallDir, tfvarsFiles)
c.FileList = append(c.FileList, files...) // append state files even in case of failure
if err != nil {
Expand Down
6 changes: 5 additions & 1 deletion pkg/destroy/bootstrap/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ func Destroy(dir string) (err error) {
platform = typesazure.StackTerraformName
}

provider := infra.ProviderForPlatform(platform)
provider, err := infra.ProviderForPlatform(platform)
if err != nil {
return fmt.Errorf("error getting infrastructure provider: %w", err)
}

if err := provider.DestroyBootstrap(dir); err != nil {
return fmt.Errorf("error destroying bootstrap resources %w", err)
}
Expand Down
37 changes: 20 additions & 17 deletions pkg/infrastructure/platform/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
//go:build !(altinfra || aro)
// +build !altinfra,!aro

package platform

import (
Expand Down Expand Up @@ -34,40 +37,40 @@ import (
)

// ProviderForPlatform returns the stages to run to provision the infrastructure for the specified platform.
func ProviderForPlatform(platform string) infrastructure.Provider {
func ProviderForPlatform(platform string) (infrastructure.Provider, error) {
switch platform {
case alibabacloudtypes.Name:
return terraform.InitializeProvider(alibabacloud.PlatformStages)
return terraform.InitializeProvider(alibabacloud.PlatformStages), nil
case awstypes.Name:
return terraform.InitializeProvider(aws.PlatformStages)
return terraform.InitializeProvider(aws.PlatformStages), nil
case azuretypes.Name:
return terraform.InitializeProvider(azure.PlatformStages)
return terraform.InitializeProvider(azure.PlatformStages), nil
case azuretypes.StackTerraformName:
return terraform.InitializeProvider(azure.StackPlatformStages)
return terraform.InitializeProvider(azure.StackPlatformStages), nil
case baremetaltypes.Name:
return terraform.InitializeProvider(baremetal.PlatformStages)
return terraform.InitializeProvider(baremetal.PlatformStages), nil
case gcptypes.Name:
return terraform.InitializeProvider(gcp.PlatformStages)
return terraform.InitializeProvider(gcp.PlatformStages), nil
case ibmcloudtypes.Name:
return terraform.InitializeProvider(ibmcloud.PlatformStages)
return terraform.InitializeProvider(ibmcloud.PlatformStages), nil
case libvirttypes.Name:
return terraform.InitializeProvider(libvirt.PlatformStages)
return terraform.InitializeProvider(libvirt.PlatformStages), nil
case nutanixtypes.Name:
return terraform.InitializeProvider(nutanix.PlatformStages)
return terraform.InitializeProvider(nutanix.PlatformStages), nil
case powervstypes.Name:
return terraform.InitializeProvider(powervs.PlatformStages)
return terraform.InitializeProvider(powervs.PlatformStages), nil
case openstacktypes.Name:
return terraform.InitializeProvider(openstack.PlatformStages)
return terraform.InitializeProvider(openstack.PlatformStages), nil
case ovirttypes.Name:
return terraform.InitializeProvider(ovirt.PlatformStages)
return terraform.InitializeProvider(ovirt.PlatformStages), nil
case vspheretypes.Name:
return terraform.InitializeProvider(vsphere.PlatformStages)
return terraform.InitializeProvider(vsphere.PlatformStages), nil
case nonetypes.Name:
// terraform is not used when the platform is "none"
return terraform.InitializeProvider([]terraform.Stage{})
return terraform.InitializeProvider([]terraform.Stage{}), nil
case externaltypes.Name:
// terraform is not used when the platform is "external"
return terraform.InitializeProvider([]terraform.Stage{})
return terraform.InitializeProvider([]terraform.Stage{}), nil
}
panic(fmt.Sprintf("unsupported platform %q", platform))
return nil, fmt.Errorf("unsupported platform %q", platform)
Copy link
Contributor

@andfasano andfasano Nov 2, 2023

Choose a reason for hiding this comment

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

nit: since essentially the terraform.InitializeProvider is always invoked for each platform stage type, code could be slightly simplified by storing the relationship in a map, ie:

	platformStages := map[string][]terraform.Stage{
		alibabacloudtypes.Name: alibabacloud.PlatformStages,
		awstypes.Name:          aws.PlatformStages,
                // ... other types
	}

	s, ok := platformStages[platform]
	if !ok {
		return nil, fmt.Errorf("unsupported platform %q", platform)
	}

	return terraform.InitializeProvider(s)

Copy link
Contributor Author

@patrickdillon patrickdillon Nov 3, 2023

Choose a reason for hiding this comment

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

This is a good suggestion. I would like to keep it this way, because once #7413 merges, we can introduce alternate providers with feature gates in these case statements, so we would have something like:

case awstypes.Name:
                 if fg.Enabled(configv1.FeatureGateInstallAWSSDK) {
                     return aws.InitializeProvider("")
                 }
		return terraform.InitializeProvider(aws.PlatformStages), nil

Copy link
Contributor

@andfasano andfasano Nov 6, 2023

Choose a reason for hiding this comment

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

Ok, we could eventually review it in future

}
29 changes: 29 additions & 0 deletions pkg/infrastructure/platform/platform_altinfra.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//go:build altinfra || aro
// +build altinfra aro

package platform

import (
"fmt"

"github.com/openshift/installer/pkg/infrastructure"
awstypes "github.com/openshift/installer/pkg/types/aws"
azuretypes "github.com/openshift/installer/pkg/types/azure"
vspheretypes "github.com/openshift/installer/pkg/types/vsphere"
)

// ProviderForPlatform returns the stages to run to provision the infrastructure for the specified platform.
func ProviderForPlatform(platform string) (infrastructure.Provider, error) {
switch platform {
case awstypes.Name:
panic("not implemented")
return nil, nil
case azuretypes.Name:
panic("not implemented")
return nil, nil
case vspheretypes.Name:
panic("not implemented")
return nil, nil
}
return nil, fmt.Errorf("platform %q is not supported in the altinfra Installer build", platform)
}