SPLAT-2685: Add permission check to destroy cluster command by vr4manta · Pull Request #10404 · openshift/installer

vr4manta · 2026-03-18T15:47:56Z

Changes

Added logic to check permissions before attempting destroy cluster
Enhanced AWS DHA permission checking for releasing DHA

Notes

Currently it seems destroy cluster does not check AWS account permissions. During the review of the #10079 it was requested to make sure we check for permissions for destroy. For DHA, there is no installation configuration (install-config) that defines the use of DHA. You can specify BYO DH and for that we can check that we have permission to describe the host to make sure its in the correct zone that the install-config is targeting. For destroy cluster, BYO only removes tags and that is a general permission that we check during install and that logic is provided by #10079 .

openshift-ci-robot · 2026-03-18T15:48:02Z

@vr4manta: This pull request references SPLAT-2685 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set.

Details

In response to this:

SPLAT-2685

Changes

Added logic to check permissions before attempting destroy cluster

Enhanced AWS DHA permission checking for releasing DHA

Notes

Currently it seems destroy cluster does not check AWS account permissions. During the review of the #10079 it was requested to make sure we check for permissions for destroy. For DHA, there is no installation configuration (install-config) that defines the use of DHA. You can specify BYO DH and for that we can check that we have permission to describe the host to make sure its in the correct zone that the install-config is targeting. For destroy cluster, BYO only removes tags and that is a general permission that we check during install and that logic is provided by #10079 .

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-ci · 2026-03-18T15:48:05Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

coderabbitai · 2026-03-18T15:48:06Z

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)

do-not-merge/work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 256c31c8-1e38-4709-9c9a-10b1b133cb60

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

📝 Coding Plan

Generate coding plan for human review comments

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci-robot · 2026-03-18T15:48:08Z

@vr4manta: This pull request references SPLAT-2685 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set.

Details

In response to this:

SPLAT-2685

Changes

Added logic to check permissions before attempting destroy cluster

Enhanced AWS DHA permission checking for releasing DHA

Notes

Currently it seems destroy cluster does not check AWS account permissions. During the review of the #10079 it was requested to make sure we check for permissions for destroy. For DHA, there is no installation configuration (install-config) that defines the use of DHA. You can specify BYO DH and for that we can check that we have permission to describe the host to make sure its in the correct zone that the install-config is targeting. For destroy cluster, BYO only removes tags and that is a general permission that we check during install and that logic is provided by #10079 .

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-ci · 2026-03-18T15:48:27Z

[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 assign tthvo for approval. For more information see the 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

mtulio

That's a good improvement in the destroy flow for DH, I have a few questions related to when to call it, as well verbosity of this new flow.

mtulio · 2026-03-18T18:09:14Z

pkg/destroy/aws/permissions.go

+	// Dedicated host permissions - always include DescribeHosts since we need to check
+	permissionGroups = append(permissionGroups, awssession.PermissionDedicatedHosts)


ec2:DescribeHosts will always be required on destroy flow?

mtulio · 2026-03-18T18:11:19Z

pkg/destroy/aws/permissions.go

+
+	// Build IAM endpoint URL
+	iamEndpoint := ""
+	for _, endpoint := range endpoints {


What about to just call it as endpoints is used only here?

Suggested change

for _, endpoint := range endpoints {

for _, endpoint := range metadata.AWS.ServiceEndpoints {

mtulio · 2026-03-18T18:16:57Z

pkg/destroy/aws/machines.go

+	kubeconfigPath := filepath.Join(rootDir, "auth", "kubeconfig")
+
+	// Check if kubeconfig exists
+	if _, err := os.Stat(kubeconfigPath); os.IsNotExist(err) {
+		logger.Debugf("Kubeconfig not found at %s, skipping dynamic dedicated host check", kubeconfigPath)
+		return false, nil
+	}
+


Do you think we can also evaluate the KUBECONFIG environment variable?
Is it an anti-pattern for installer? cc/wdyt @patrickdillon @tthvo

mtulio · 2026-03-18T19:13:13Z

pkg/destroy/destroyer.go

+		// Check if cluster has dynamic dedicated hosts
+		hasDynamicDHs, err := aws.HasDynamicDedicatedHosts(ctx, rootDir, logger)
+		if err != nil {
+			logger.WithError(err).Debug("Failed to check for dynamic dedicated hosts, continuing without dynamic DH permissions")
+			hasDynamicDHs = false
+		}
+		if hasDynamicDHs {
+			logger.Info("Dynamic dedicated hosts detected in cluster")
+		}
+


Considering this verification can grow, what do you think moving those validations to dedicated functions based on ValidateDestroyPermissions()?

mtulio · 2026-03-18T19:13:40Z

pkg/destroy/destroyer.go

+		// Validate AWS permissions for destroy operation
+		// This checks all required delete permissions plus dynamic DH permissions if needed
+		if err := aws.ValidateDestroyPermissions(ctx, clusterMetadata, hasDynamicDHs, logger); err != nil {
+			return nil, errors.Wrap(err, "AWS credential validation failed")


How acculturated is this message?

mtulio · 2026-03-18T19:17:07Z

pkg/destroy/aws/permissions.go

+// ValidateDestroyPermissions validates that AWS credentials have all necessary permissions
+// to destroy the cluster. This provides comprehensive upfront validation similar to install-time checks.
+func ValidateDestroyPermissions(ctx context.Context, metadata *types.ClusterMetadata, hasDynamicDedicatedHosts bool, logger logrus.FieldLogger) error {
+	logger.Info("Validating AWS permissions for cluster destroy")


Maybe using Debug level here?

mtulio · 2026-03-18T19:24:49Z

pkg/destroy/destroyer.go

 		return nil, errors.New("no platform configured in metadata")
 	}

+	// For AWS platforms, perform comprehensive permission validation upfront


I think we should move those functions to the "AWS run destroyer" context, something like RunWithContext, WDYT?

vr4manta added 2 commits March 18, 2026 11:41

Added destroy permission check

760d3e3

Added destroy permission check

7cdbd85

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

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 Mar 18, 2026

mtulio reviewed Mar 18, 2026

View reviewed changes

		// Dedicated host permissions - always include DescribeHosts since we need to check
		permissionGroups = append(permissionGroups, awssession.PermissionDedicatedHosts)

	for _, endpoint := range endpoints {
	for _, endpoint := range metadata.AWS.ServiceEndpoints {

Conversation

vr4manta commented Mar 18, 2026 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Notes

Uh oh!

openshift-ci-robot commented Mar 18, 2026 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Notes

Uh oh!

openshift-ci bot commented Mar 18, 2026

Uh oh!

coderabbitai bot commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

openshift-ci-robot commented Mar 18, 2026 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Notes

Uh oh!

openshift-ci bot commented Mar 18, 2026

Uh oh!

mtulio left a comment

Choose a reason for hiding this comment

Uh oh!

mtulio Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

mtulio Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

mtulio Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

mtulio Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

mtulio Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

mtulio Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

mtulio Mar 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vr4manta commented Mar 18, 2026 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 18, 2026 •

edited by openshift-ci bot

Loading

coderabbitai bot commented Mar 18, 2026 •

edited

Loading

openshift-ci-robot commented Mar 18, 2026 •

edited by openshift-ci bot

Loading