WIP: CAPI AWS implementation by patrickdillon · Pull Request #7879 · openshift/installer

patrickdillon · 2024-01-08T16:45:36Z

This is a WIP implementation of installing AWS using CAPI providers and the interface from #7824. The intent is to provide a reference for installing a complete, production-level cluster.

Depends on #7824
(the first four commits are from that PR)

Moves tfvars to its own package to decouple it from the cluster asset.

Updates the infrastructure provider interface to accept Parent assets-- rather than a list of files. This allows for easier handling of assets by the infrastructure provider.

Moves LoadMetadata to a separate package to allow implementers of the infrastructure provider interface to utilize the function. The cluster asset/package does not use LoadMetadata but does depend on the implementers of the interface. Moving to a separate package breaks the dependency loop.

Implements the infrastructure provider interface with the CAPI system. This encapsulates the CAPI implementation similar to Terraform. It also maintains pkg/infrastructure/platform.go (and build variants) as the canonical source of truth for choosing an infrastructure provider. This also adds an interface that cloud platforms utilizing the CAPI provisioning should implement to provision additional resources. Add installconfig to capi preprovision input infrastructure/clusterapi: execute control plane available hook SQUASH: CAPI bootstrap destroy

Adds an initial implementation of the CAPI infrastructure provider interface for AWS.

Create IAM roles to be used by control plane and compute nodes.

Add creation of dns records when control plane endpoint becomes available.

Adds filters to the machine api machinesets so they will recognize CAPI-created security groups and subnets. Adds these filters in addition to the Terraform-specific filters, which should be subsequently removed. fixup sg filter

Export load balancer function for use with CAPI provisioning workflow.

This can be merged if we refactor the function to allow target type to be set, but at this stage it is not worth refactoring.

openshift-ci · 2024-01-08T16:47:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from patrickdillon. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

r4f4 · 2024-01-08T17:00:30Z

pkg/asset/cluster/metadata/metadata.go

+	"path/filepath"
+
+	"github.com/openshift/installer/pkg/types"
+	"github.com/pkg/errors"


Let's not introduce this lib in new code.

r4f4 · 2024-01-08T17:00:59Z

pkg/asset/cluster/metadata/metadata.go

+
+	var metadata *types.ClusterMetadata
+	if err = json.Unmarshal(raw, &metadata); err != nil {
+		return nil, errors.Wrapf(err, "failed to Unmarshal data from %q to types.ClusterMetadata", path)


Suggested change

return nil, errors.Wrapf(err, "failed to Unmarshal data from %q to types.ClusterMetadata", path)

return nil, fmt.Errorf("failed to Unmarshal data from %q to types.ClusterMetadata: %w", path, err)

r4f4 · 2024-01-08T17:02:12Z

pkg/infrastructure/aws/clusterapi/clusterapi.go

+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/elbv2"
+	"github.com/aws/aws-sdk-go/service/route53"
+	"github.com/pkg/errors"


r4f4 · 2024-01-08T17:05:06Z

pkg/infrastructure/aws/clusterapi/iam.go

+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
+	"github.com/aws/aws-sdk-go/service/iam"
+	"github.com/pkg/errors"


r4f4 · 2024-01-08T17:05:42Z

pkg/infrastructure/aws/clusterapi/iam.go

+	// https://docs.aws.amazon.com/sdk-for-go/api/service/iam/#IAM.CreateRole
+	session, err := ic.AWS.Session(context.TODO())
+	if err != nil {
+		return errors.Wrap(err, "failed to load AWS session")


Suggested change

return errors.Wrap(err, "failed to load AWS session")

return fmt.Errorf("failed to load AWS session: %w", err)

r4f4 · 2024-01-08T17:09:56Z

pkg/infrastructure/aws/clusterapi/iam.go

+			}
+			time.Sleep(10 * time.Second)
+			if err := svc.WaitUntilInstanceProfileExists(&iam.GetInstanceProfileInput{InstanceProfileName: profileName}); err != nil {
+				return errors.Wrapf(err, "failed to wait for %s role to exist", role)


Suggested change

return errors.Wrapf(err, "failed to wait for %s role to exist", role)

return fmt.Errorf("failed to wait for %s role to exist: %w", role, err)

r4f4 · 2024-01-08T17:10:20Z

pkg/infrastructure/aws/clusterapi/iam.go

+				InstanceProfileName: profileName,
+				RoleName:            roleName,
+			}); err != nil {
+				return errors.Wrapf(err, "failed to add %s role to instance profile", role)


Suggested change

return errors.Wrapf(err, "failed to add %s role to instance profile", role)

return fmt.Errorf("failed to add %s role to instance profile: %w", role, err)

r4f4 · 2024-01-08T17:11:29Z

pkg/infrastructure/clusterapi/clusterapi.go

+	"fmt"
+	"time"
+
+	"github.com/pkg/errors"


r4f4 · 2024-01-08T17:11:57Z

pkg/infrastructure/clusterapi/clusterapi.go

+		masterIgn := string(masterIgnAsset.Files()[0].Data)
+		bootstrapIgn, err := injectInstallInfo(bootstrapIgnAsset.Files()[0].Data)
+		if err != nil {
+			return fileList, errors.Wrap(err, "unable to inject installation info")


Suggested change

return fileList, errors.Wrap(err, "unable to inject installation info")

return fileList, fmt.Errorf("unable to inject installation info: %w", err)

r4f4 · 2024-01-08T17:13:19Z

pkg/infrastructure/clusterapi/clusterapi.go

+			errMsg := fmt.Sprintf("failed to create infrastructure manifest %s from InstallConfig", fileName)
+			return fileList, errors.Wrapf(err, errMsg)


Suggested change

errMsg := fmt.Sprintf("failed to create infrastructure manifest %s from InstallConfig", fileName)

return fileList, errors.Wrapf(err, errMsg)

return fileList, fmt.Errorf("failed to create infrastructure manifest %s from InstallConfig: %w", fileName, err)

Switches the config so that CAPI creates the internal LB and the external LB is created with direct SDK calls. In the opposite configuration, one control plane would always be NOTREADY. This seems preferred anyway because we will ALWAYS need an internal LB, but an external LB is not needed with private clusters. Ultimately this logic will be moved to the CAPA provider.

patrickdillon · 2024-01-09T19:53:48Z

Switched the config so that CAPI creates the int LB (and use SDK to create ext) in ecb4551

Now installs are completing!

openshift-merge-robot · 2024-01-12T19:05:52Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Embeds the default CAPI provider.

Adds an ignition function to the CAPI provider interface and moves the default ignition secret generation to the default CAPI provider implementation.

bfournie · 2024-01-30T20:51:28Z

/cc @bfournie

openshift-ci · 2024-02-19T21:21:05Z

@patrickdillon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/altinfra-images	`6815b69`	link	true	`/test altinfra-images`
ci/prow/altinfra-e2e-aws-ovn-fips	`6815b69`	link	false	`/test altinfra-e2e-aws-ovn-fips`
ci/prow/e2e-aws-ovn	`6815b69`	link	true	`/test e2e-aws-ovn`
ci/prow/altinfra-e2e-aws-custom-security-groups	`6815b69`	link	false	`/test altinfra-e2e-aws-custom-security-groups`
ci/prow/golint	`6815b69`	link	true	`/test golint`
ci/prow/gofmt	`6815b69`	link	true	`/test gofmt`
ci/prow/altinfra-e2e-aws-ovn-single-node	`6815b69`	link	false	`/test altinfra-e2e-aws-ovn-single-node`
ci/prow/e2e-aws-ovn-edge-zones-manifest-validation	`6815b69`	link	true	`/test e2e-aws-ovn-edge-zones-manifest-validation`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

patrickdillon added 18 commits January 8, 2024 10:22

pkg/asset/cluster: move tfvars to its own package

c986b07

Moves tfvars to its own package to decouple it from the cluster asset.

provider interface: add asset.Parents arg

07dcab2

Updates the infrastructure provider interface to accept Parent assets-- rather than a list of files. This allows for easier handling of assets by the infrastructure provider.

pkg/infra/capi/aws: add initial implementation

36a26cf

Adds an initial implementation of the CAPI infrastructure provider interface for AWS.

AWS: Provision IAM roles for CAPI

8d06e47

Create IAM roles to be used by control plane and compute nodes.

SQUASH: use ErrorF in put IAM role

770888a

infrastructure/aws: create DNS records

f4edf2d

Add creation of dns records when control plane endpoint becomes available.

aws/machines: add CAPI sg, subnet filters

b8ed1a0

Adds filters to the machine api machinesets so they will recognize CAPI-created security groups and subnets. Adds these filters in addition to the Terraform-specific filters, which should be subsequently removed. fixup sg filter

fixup! aws/machines: add CAPI sg, subnet filters

90b0255

infrastructure/aws: export EnsureLoadBalancer

b581e2e

Export load balancer function for use with CAPI provisioning workflow.

WIP Add AWS supplemental resources

1365bfd

SQUASH: AWS CAPI fix LB -int suffix

f1cb627

Register control plane machines

f3b5a19

DNM: switch TG to instance id

d6a4a34

This can be merged if we refactor the function to allow target type to be set, but at this stage it is not worth refactoring.

WIP add tags to cluster

7827516

WIP AWS create internal LB out of band

28c09f7

Vendor: CAPA IAM v2

969118a

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2024

openshift-ci bot requested review from mtulio and rwsu January 8, 2024 16:47

patrickdillon mentioned this pull request Jan 8, 2024

CORS-3139: Move CAPI behind infrastructure provider interface #7824

Merged

r4f4 reviewed Jan 8, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2024

patrickdillon added 2 commits January 14, 2024 21:16

WIP: add base capi provider

77392f9

DefaultCAPIProvider implements Provider

bcd4cd7

patrickdillon added 2 commits January 15, 2024 15:09

infra/capi/aws: embed default capi provider

4eace3e

Embeds the default CAPI provider.

infra/capi: add Ignition() to provider interface

6815b69

Adds an ignition function to the CAPI provider interface and moves the default ignition secret generation to the default CAPI provider implementation.

openshift-ci bot requested a review from bfournie January 30, 2024 20:51

patrickdillon closed this Apr 6, 2024

	return nil, errors.Wrapf(err, "failed to Unmarshal data from %q to types.ClusterMetadata", path)
	return nil, fmt.Errorf("failed to Unmarshal data from %q to types.ClusterMetadata: %w", path, err)

	return errors.Wrap(err, "failed to load AWS session")
	return fmt.Errorf("failed to load AWS session: %w", err)

	return errors.Wrapf(err, "failed to wait for %s role to exist", role)
	return fmt.Errorf("failed to wait for %s role to exist: %w", role, err)

	return errors.Wrapf(err, "failed to add %s role to instance profile", role)
	return fmt.Errorf("failed to add %s role to instance profile: %w", role, err)

	return fileList, errors.Wrap(err, "unable to inject installation info")
	return fileList, fmt.Errorf("unable to inject installation info: %w", err)

		errMsg := fmt.Sprintf("failed to create infrastructure manifest %s from InstallConfig", fileName)
		return fileList, errors.Wrapf(err, errMsg)

	errMsg := fmt.Sprintf("failed to create infrastructure manifest %s from InstallConfig", fileName)
	return fileList, errors.Wrapf(err, errMsg)
	return fileList, fmt.Errorf("failed to create infrastructure manifest %s from InstallConfig: %w", fileName, err)

Conversation

patrickdillon commented Jan 8, 2024

Uh oh!

openshift-ci bot commented Jan 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Jan 9, 2024

Uh oh!

openshift-merge-robot commented Jan 12, 2024

Uh oh!

bfournie commented Jan 30, 2024

Uh oh!

openshift-ci bot commented Feb 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants