Skip to content

OCPBUGS-98384: fix bastion cleanup KeyPair leak by capturing infraID/region eagerly #8982

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 1 commit into from
Jul 20, 2026
Merged

OCPBUGS-98384: fix bastion cleanup KeyPair leak by capturing infraID/region eagerly #8982

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 deletions test/e2e/util/dump/journals.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"path/filepath"
"strings"
"testing"
"time"

hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
bastionaws "github.com/openshift/hypershift/cmd/bastion/aws"
Expand Down Expand Up @@ -161,13 +162,17 @@ func setupBastion(t *testing.T, ctx context.Context, hc *hyperv1.HostedCluster,
if err != nil {
return "", err
}
infraID := hc.Spec.InfraID
region := hc.Spec.Platform.AWS.Region
t.Cleanup(func() {
destroyCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)

@ironcladlou ironcladlou Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I need to look through the code again to see how context management has evolved, but is there no secondary context being managed that could be used here instead of background + local arbitrary timeout?

@bryan-cox bryan-cox Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, there's no secondary context managed in the e2e framework that's available here. The ctx parameter to setupBastion is the test context, which may already be canceled when t.Cleanup fires — specifically, the cleanup teardown path at hypershift_framework.go:334 passes context.Background() explicitly. The testContext (process-wide, only canceled on SIGINT/SIGTERM) would work in principle, but it's not accessible from setupBastion and threading it through the call chain would change the signatures of DumpJournals, newClusterDumper, and teardownHostedCluster.

context.Background() with a bounded timeout is the established pattern for cleanup operations in this codebase — the integration tests use the same approach (e.g. test/integration/framework/run.go:45,83), and test/e2e/util/aws.go:121,395 both use context.WithTimeout(context.Background(), 2*time.Minute) in cleanup paths. The 5-minute timeout here accounts for the bastion destroy needing to sequentially delete the EC2 instance, security group, and keypair.

AI-assisted response via Claude Code

@ironcladlou ironcladlou Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Prow will issue a SIGTERM which drives cancellation, and then a SIGKILL after the grace period, so I don't see an opportunity to drive a secondary cancellation within the cleanup... that said, it raises the question why use a timeout at all? Could let the cleanup block forever until it either finishes or Prow kills the whole process, no arbitrary timeout required?

https://docs.ci.openshift.org/architecture/timeouts/#handling-sigterm-in-a-test-process

@bryan-cox bryan-cox Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point — the testContext signal handler at e2e_test.go:220-224 does cancel on SIGTERM, so Prow's kill chain would propagate. But testContext isn't accessible from setupBastion without threading it through DumpJournals → newClusterDumper → teardownHostedCluster.

A bare context.Background() without timeout would work if this cleanup were the only thing running, but t.Cleanup functions fire LIFO — if the bastion AWS call hangs (network partition, throttled API), it blocks all subsequent cleanups from running until Prow SIGKILLs the process. The 5-minute bound ensures other cleanup functions (HC destroy, namespace deletion) still get a chance to execute within Prow's grace period.

That said, 5 minutes is generous — the destroy typically completes in under 30 seconds (delete instance, delete SG, delete keypair). Happy to adjust the timeout if you think a shorter bound makes more sense.

AI-assisted response via Claude Code

@ironcladlou ironcladlou Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How many functions are in the cleanup stack? What is a "fair" timeout to allocate to each? How would you compute that statically or at runtime? If this timeout is exceeded and it falls through to the next, you still leaked. Is it better to try each one until pass or overall timeout, or give arbitrary inconsistent timeouts to everything in the stack? etc. etc.

Not sure what is the right answer. If our best current approach is limited to arbitrary timeout assignments, I guess I would err on the side of shorter to improve the chance of other items in the stack executing and ensuring we're logging leaks. Maybe take a look at the current grace timeouts in the prow config to see what budget we're working with.

Post-hoc detection / backstopping is probably our best bet in any case as we'll never be able to account for the orchestrator SIGTERM'ing us in any case, so as long as the things we're creating are being appropriately tagged and/or isolated in accounts, risk of cleanup issues like this can be reduced

@bryan-cox bryan-cox Jul 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right that there's no principled way to statically budget timeouts across the cleanup stack — that's a real framework gap worth tracking separately.

For this PR though, the primary fix is the eager capture of infraID/region (lines 165-166), which directly prevents the KeyPair leak. The timeout is a secondary defensive measure, consistent with existing patterns in the codebase:

  • aws.go:121 — PutRolePolicy cleanup: context.WithTimeout(context.Background(), 2*time.Minute)
  • aws.go:395 — CreateCapacityReservation cleanup: context.WithTimeout(context.Background(), 2*time.Minute)

The main framework teardown (hypershift_framework.go:334) uses bare context.Background() with no timeout at all, so there's no existing budget system to integrate with.

Happy to shorten this to 2 minutes to match the existing cleanup timeout pattern and improve the odds for subsequent cleanup items, but I think the broader cleanup orchestration work (managed cleanup context with budget allocation) is a separate enhancement. The immediate priority is stopping the keypair leaks on every Karpenter test run.

Want me to drop to 2 minutes, or is 5 minutes acceptable as-is?

AI-assisted response via Claude Code

@ironcladlou ironcladlou Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Your call, this isn't blocking feedback

defer cancel()
destroyBastion := bastionaws.DestroyBastionOpts{
Namespace: hc.Namespace,
Name: hc.Name,
InfraID: infraID,
Region: region,
AWSCredentialsFile: awsCreds,
}
if err := destroyBastion.Run(ctx, zapr.NewLoggerWithOptions(destroyLogger)); err != nil {
if err := destroyBastion.Run(destroyCtx, zapr.NewLoggerWithOptions(destroyLogger)); err != nil {
t.Logf("error destroying bastion: %v", err)
}
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand Down