OCPBUGS-45218: aws: fix perm requirement for edge nodes #9256

r4f4 · 2024-12-02T14:39:19Z

If an edge machine pool is specified without an instance type, the installer needs the ec2:DescribeInstanceTypeOfferings permission to derive the correct instance type available according to the local/wavelength zones being used.

Before this change, the permission was optional and its absence would result in the installer picking a hard-coded non-edge pool instance type which can cause unsupported configuration issues in mapi's output:

     providerStatus:
      conditions:
      - lastTransitionTime: "2024-11-28T15:32:09Z"
        message: 'error launching instance: The requested configuration is currently
          not supported. Please check the documentation for supported configurations.'
        reason: MachineCreationFailed
        status: "False"
        type: MachineCreation

openshift-ci-robot · 2024-12-02T14:39:25Z

@r4f4: This pull request references Jira Issue OCPBUGS-45218, which is invalid:

expected the bug to target the "4.19.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

If an edge machine pool is specified without an instance type, the installer needs the ec2:DescribeInstanceTypeOfferings permission to derive the correct instance type available according to the local/wavelength zones being used.

Before this change, the permission was optional and its absence would result in the installer picking a hard-coded non-edge pool instance type which can cause unsupported configuration issues in mapi's output:
    providerStatus:
     conditions:
     - lastTransitionTime: "2024-11-28T15:32:09Z"
       message: 'error launching instance: The requested configuration is currently
         not supported. Please check the documentation for supported configurations.'
       reason: MachineCreationFailed
       status: "False"
       type: MachineCreation

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 openshift-eng/jira-lifecycle-plugin repository.

r4f4 · 2024-12-02T14:39:50Z

/uncc @andfasano
/cc @mtulio

r4f4 · 2024-12-02T14:44:41Z

/jira refresh

openshift-ci-robot · 2024-12-02T14:44:48Z

@r4f4: This pull request references Jira Issue OCPBUGS-45218, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.19.0) matches configured target version for branch (4.19.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @gpei

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

patrickdillon · 2024-12-05T11:47:56Z

I think requiring this perm is a good idea

/approve

IIRC the permission check is a dependency of the cluster asset and runs after the machine manifests have been generated. I am on my phone right now and can’t double check, but if that is the case it seems like the following could happen:

installer generates machineset with bad instance types (manifests target)
installer throws validation error for missing perms (cluster target)
User adds perms
User reruns install with same machinesets and install fails

If i am mistaken and permissions checks run earlier then this wouldn’t be possible. Also i dont think we need to hold up this pr with solving this problem but just wanted to point out this potential edge case for discussion

openshift-ci · 2024-12-05T11:48:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [patrickdillon]

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

patrickdillon · 2024-12-05T11:49:21Z

This LGTM but let’s get another reviewer to take a look

r4f4 · 2024-12-05T14:03:08Z

I think requiring this perm is a good idea

/approve

IIRC the permission check is a dependency of the cluster asset and runs after the machine manifests have been generated. I am on my phone right now and can’t double check, but if that is the case it seems like the following could happen:
* installer generates machineset with bad instance types (manifests target)

* installer throws validation error for missing perms (cluster target)

* User adds perms

* User reruns install with same machinesets and install fails

Indeed that's a problem. The Worker asset depends on the CredsCheck but not on the PermsCheck:

// Dependencies returns all of the dependencies directly needed by the
// Worker asset
func (w *Worker) Dependencies() []asset.Asset {
	return []asset.Asset{
		&installconfig.ClusterID{},
		// PlatformCredsCheck just checks the creds (and asks, if needed)
		// We do not actually use it in this asset directly, hence
		// it is put in the dependencies but not fetched in Generate
		&installconfig.PlatformCredsCheck{},
		&installconfig.InstallConfig{},

compared to the Cluster asset

// Dependencies returns the direct dependency for launching
// the cluster.
func (c *Cluster) Dependencies() []asset.Asset {
	return []asset.Asset{
		&installconfig.ClusterID{},
		&installconfig.InstallConfig{},
		// PlatformCredsCheck, PlatformPermsCheck, PlatformProvisionCheck, and VCenterContexts.
		// perform validations & check perms required to provision infrastructure.
		// We do not actually use them in this asset directly, hence
		// they are put in the dependencies but not fetched in Generate.
		&installconfig.PlatformCredsCheck{},
		&installconfig.PlatformPermsCheck{},

Isn't that an existing installer bug? Generating worker manifests involves SDK calls not only for AWS but also for other platforms. I'll open a separate bug to handle that.

r4f4 · 2024-12-05T14:13:30Z

I created https://issues.redhat.com/browse/OCPBUGS-45657 to address the point raised by Patrick.

barbacbd · 2024-12-06T16:03:18Z

pkg/asset/machines/worker.go

-						logrus.Warnf("failed to find preferred instance type for one or more zones in the %s pool, using default: %s", pool.Name, instanceTypes[0])
-						mpool.InstanceType = instanceTypes[0]
+					if !awsSetPreferredInstanceByEdgeZone(ctx, instanceTypes, installConfig.AWS, zones) {
+						// Using the default instance type from the non-edge pool often fails.


This seems to be the main part related to the bug. Is it ok to fail here (i.e. is this the intended behavior)?

My proposal is: it seems better to fail here instead of having an undetected node failing to come up with the error as in the PR's description. The non-edge node default might not be compatible with edge zones.

LGTM failing when any zone isn't found the respective type.

barbacbd

/lgtm

r4f4 · 2024-12-10T14:51:01Z

@mtulio can you review this one?

openshift-ci-robot · 2024-12-10T16:48:50Z

/retest-required

Remaining retests: 0 against base HEAD 78adb29 and 2 for PR HEAD 55d4f83 in total

mtulio · 2024-12-10T18:19:09Z

pkg/asset/installconfig/aws/permissions.go

+	PermissionEdgeDefaultInstance: {
+		// Needed to filter zones by instance type
+		"ec2:DescribeInstanceTypeOfferings",
+	},


@r4f4 @patrickdillon I would like to exercise a little bit more this comment. Isn't ec2:DescribeInstanceTypeOfferings called every installer execution to discover types from the pool, and fallback to static ones regardless of the pool type? My impression that this permissions must be in the base set of required permissions, instead of only when edge zones is added.

Holding until hear from you all. Feel free to drop.
/hold

Not every execution, only when the instanceType is not set and/or when zones are not specified. For the former, we fallback to a hardcoded type so the permission is optional. The problem with that logic for edge zones is that not all regions and instance types support them.

If you want to make it required for all cases, that should be addressed as a separate bug as it impacts managed services and their managed policies.

Not every execution, only when the instanceType is not set and/or when zones are not specified

Yeah, apologies. I mean default execution flow which those fields isn't set/required - and I am not sure if we have a CI scenario of that flow as a couple of fields were enforced in CI to, specially, save costs, preventing spreading the cluster across all zones discovered in the region.

as it impacts managed services and their managed policies.

If this change is only to support managed services flow for now, LGTM.

I agree @mtulio that ec2:DescribeInstanceTypeOfferings should be required in the default permission set for all installs.

If you want to make it required for all cases, that should be addressed as a separate bug as it impacts managed services and their managed policies.

@r4f4 should we open a new bug? I believe rosa supports edge zones (@mtulio probably knows better than I do) so I would think as it stands, this would affect the rosa policy as well

Right. So it looks like there are at least two regions that don't have M6i: eu-south-2 & ap-southeast-4. So IIUC the issue is that IFF you're installing to one of those regions & don't have this perm & don't specify an instance type, the install would fail (as it will default to the unavailable m6i instance).

@r4f4 I created https://issues.redhat.com/browse/OCPBUGS-46596

Thanks, Patrick.

@mtulio are you happy to proceede with this PR and make the perm always required as part of the new bug Patrick opened?

So IIUC the issue is that IFF you're installing to one of those regions & don't have this perm & don't specify an instance type, the install would fail (as it will default to the unavailable m6i instance).

Correct.

@r4f4 Yes. Patrick described the rationale behind my thoughts. When we introduced m6i we aimed better performance for the same price for control planes (in general new instances increases ~15%, and are cheaper, except 7th Gen). That time, m6i was available in a few regions, but the algorithm covered that edge cases.

@mtulio are you happy to proceede with this PR and make the perm always required as part of the new bug Patrick opened?

Yes.
/lgtm

FWIW some time ago I wrote an ~~ugly~~ script to collect and consolidate that information across zones around the globe, and mount the supported instance types on edge zones and in the region, if you are interested to explore, this could be a start point, and sample output.

r4f4 · 2024-12-16T19:15:36Z

Update: rebased on top of master to fix merge conflicts.

r4f4 · 2024-12-17T09:51:30Z

/retest-required

r4f4 · 2024-12-19T09:22:45Z

/retest-required

r4f4 · 2024-12-19T16:08:14Z

/hold cancel

r4f4 · 2024-12-19T16:11:25Z

/label acknowledge-critical-fixes-only

openshift-ci-robot · 2024-12-19T17:09:00Z

/retest-required

Remaining retests: 0 against base HEAD 27fa766 and 2 for PR HEAD 30724f9 in total

openshift-ci-robot · 2024-12-19T21:08:50Z

/retest-required

Remaining retests: 0 against base HEAD ec72ce6 and 1 for PR HEAD 30724f9 in total

r4f4 · 2024-12-19T21:11:06Z

/override ci/prow/e2e-azure-ovn-upi
Few e2e failures and not affected by this PR.

openshift-ci · 2024-12-19T21:11:36Z

@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-azure-ovn-upi

In response to this:

/override ci/prow/e2e-azure-ovn-upi
Few e2e failures and not affected by this PR.

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-sigs/prow repository.

openshift-ci-robot · 2024-12-20T04:27:14Z

/retest-required

Remaining retests: 0 against base HEAD ec72ce6 and 2 for PR HEAD 30724f9 in total

r4f4 · 2024-12-20T09:34:21Z

/retest-required

openshift-ci · 2024-12-20T14:01:27Z

@r4f4: 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/e2e-aws-ovn-shared-vpc-edge-zones	`30724f9`	link	false	`/test e2e-aws-ovn-shared-vpc-edge-zones`
ci/prow/e2e-aws-default-config	`30724f9`	link	false	`/test e2e-aws-default-config`
ci/prow/e2e-aws-ovn-single-node	`30724f9`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-aws-ovn-edge-zones	`30724f9`	link	false	`/test e2e-aws-ovn-edge-zones`
ci/prow/altinfra-e2e-aws-ovn	`30724f9`	link	false	`/test altinfra-e2e-aws-ovn`
ci/prow/e2e-external-aws-ccm	`30724f9`	link	false	`/test e2e-external-aws-ccm`
ci/prow/e2e-aws-ovn-heterogeneous	`30724f9`	link	false	`/test e2e-aws-ovn-heterogeneous`
ci/prow/e2e-aws-ovn-fips	`30724f9`	link	false	`/test e2e-aws-ovn-fips`
ci/prow/e2e-aws-ovn-imdsv2	`30724f9`	link	false	`/test e2e-aws-ovn-imdsv2`
ci/prow/okd-scos-e2e-aws-ovn	`30724f9`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-ovn-shared-vpc-custom-security-groups	`30724f9`	link	false	`/test e2e-aws-ovn-shared-vpc-custom-security-groups`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

r4f4 · 2024-12-20T14:39:33Z

/override ci/prow/e2e-azure-ovn-upi
Not affected by PR changes.

openshift-ci · 2024-12-20T14:40:06Z

@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-azure-ovn-upi

In response to this:

/override ci/prow/e2e-azure-ovn-upi
Not affected by PR changes.

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-sigs/prow repository.

openshift-ci-robot · 2024-12-20T14:43:41Z

@r4f4: Jira Issue OCPBUGS-45218: All pull requests linked via external trackers have merged:

openshift/installer#9256

Jira Issue OCPBUGS-45218 has been moved to the MODIFIED state.

In response to this:

If an edge machine pool is specified without an instance type, the installer needs the ec2:DescribeInstanceTypeOfferings permission to derive the correct instance type available according to the local/wavelength zones being used.

Before this change, the permission was optional and its absence would result in the installer picking a hard-coded non-edge pool instance type which can cause unsupported configuration issues in mapi's output:
    providerStatus:
     conditions:
     - lastTransitionTime: "2024-11-28T15:32:09Z"
       message: 'error launching instance: The requested configuration is currently
         not supported. Please check the documentation for supported configurations.'
       reason: MachineCreationFailed
       status: "False"
       type: MachineCreation

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2024-12-20T18:08:47Z

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-altinfra
This PR has been included in build ose-installer-altinfra-container-v4.19.0-202412201709.p0.gf7a8032.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2024-12-20T18:14:50Z

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-terraform-providers
This PR has been included in build ose-installer-terraform-providers-container-v4.19.0-202412201709.p0.gf7a8032.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2024-12-20T18:17:48Z

[ART PR BUILD NOTIFIER]

Distgit: ose-baremetal-installer
This PR has been included in build ose-baremetal-installer-container-v4.19.0-202412201709.p0.gf7a8032.assembly.stream.el9.
All builds following this will include this PR.

r4f4 · 2024-12-20T19:25:43Z

/cherry-pick release-4.18

openshift-cherrypick-robot · 2024-12-20T19:27:23Z

@r4f4: new pull request created: #9334

In response to this:

/cherry-pick release-4.18

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-sigs/prow repository.

openshift-bot · 2024-12-20T19:51:02Z

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-artifacts
This PR has been included in build ose-installer-artifacts-container-v4.19.0-202412201709.p0.gf7a8032.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 2, 2024

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Dec 2, 2024

openshift-ci bot requested review from andfasano and patrickdillon December 2, 2024 14:39

openshift-ci bot requested review from mtulio and removed request for andfasano December 2, 2024 14:39

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Dec 2, 2024

openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Dec 2, 2024

openshift-ci bot requested a review from gpei December 2, 2024 14:44

This was referenced Dec 2, 2024

OCPBUGS-44834: aws: include permissions for edge compute #9230

Merged

OCPBUGS-44925: aws: add ec2:AllocateAddress perm requirement. #9234

Merged

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2024

barbacbd reviewed Dec 6, 2024

View reviewed changes

openshift-ci bot assigned barbacbd Dec 6, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2024

mtulio reviewed Dec 10, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 10, 2024

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

r4f4 force-pushed the aws-edge-default-instance-fix branch from 55d4f83 to 30724f9 Compare December 16, 2024 19:15

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2024

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2024

openshift-ci bot assigned mtulio Dec 18, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 19, 2024

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Dec 19, 2024

openshift-merge-bot bot merged commit f7a8032 into openshift:main Dec 20, 2024
33 of 44 checks passed

openshift-cherrypick-robot mentioned this pull request Dec 20, 2024

[release-4.18] OCPBUGS-47502: aws: fix perm requirement for edge nodes #9334

Merged

OCPBUGS-45218: aws: fix perm requirement for edge nodes #9256

OCPBUGS-45218: aws: fix perm requirement for edge nodes #9256

Conversation

r4f4 commented Dec 2, 2024

openshift-ci-robot commented Dec 2, 2024

r4f4 commented Dec 2, 2024

r4f4 commented Dec 2, 2024

openshift-ci-robot commented Dec 2, 2024

patrickdillon commented Dec 5, 2024

openshift-ci bot commented Dec 5, 2024

patrickdillon commented Dec 5, 2024

r4f4 commented Dec 5, 2024

r4f4 commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbacbd left a comment

Choose a reason for hiding this comment

r4f4 commented Dec 10, 2024

openshift-ci-robot commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtulio Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtulio Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

r4f4 commented Dec 16, 2024

r4f4 commented Dec 17, 2024

r4f4 commented Dec 19, 2024

r4f4 commented Dec 19, 2024

r4f4 commented Dec 19, 2024

openshift-ci-robot commented Dec 19, 2024

openshift-ci-robot commented Dec 19, 2024

r4f4 commented Dec 19, 2024

openshift-ci bot commented Dec 19, 2024

openshift-ci-robot commented Dec 20, 2024

r4f4 commented Dec 20, 2024

openshift-ci bot commented Dec 20, 2024 • edited Loading

r4f4 commented Dec 20, 2024

openshift-ci bot commented Dec 20, 2024

openshift-ci-robot commented Dec 20, 2024

openshift-bot commented Dec 20, 2024

openshift-bot commented Dec 20, 2024

openshift-bot commented Dec 20, 2024

r4f4 commented Dec 20, 2024

openshift-cherrypick-robot commented Dec 20, 2024

openshift-bot commented Dec 20, 2024

mtulio Dec 10, 2024 •

edited

Loading

mtulio Dec 18, 2024 •

edited

Loading

openshift-ci bot commented Dec 20, 2024 •

edited

Loading