feat(AWS OIDC): deleting integration removes associated resources by alexhemard · Pull Request #59607 · gravitational/teleport

alexhemard · 2025-09-25T18:54:34Z

Description

This PR will remove associated resources when removing an AWS OIDC Integration.

DiscoveryConfig with an AWS Matcher in the spec
App Servers associated with the Integration (AWS Console)
Include a reminder to clean up AWS Resources used by the integration w/ a link to the AWS Resource Explorer

Screenshots

Delete AWS OIDC Integration Dialog

changelog: Deleting an AWS OIDC integration will remove associated Teleport Discovery Configs and App servers that reference the integration

r0mant · 2025-09-29T16:49:27Z

@marcoandredinis @kimlisa Can you folks review this?

marcoandredinis · 2025-09-30T15:37:37Z

+func (s *Service) deleteAWSOIDCAssociatedResources(ctx context.Context, authCtx *authz.Context, ig types.Integration) error {
+	s.logger.DebugContext(ctx, "Deleting DiscoveryConfig associated with integration", "integration", ig.GetName())
+
+	if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig, types.VerbDelete, types.VerbList); err != nil {


This raises a couple of questions:
A) let's say a user only has integration.create and integration.delete access (no discovery_config.delete/list/update).
They can create an integration but cannot delete it.
It will be weird to see access denied to list discovery_config when trying to delete an integration
I'm not sure what we could do here.
I think we have a couple of options:

do not remove the resources which you don't have access to (this is, hopefully, an edge case)

require discovery_config.* permissions when users try to create an integration

return an error but return a better error message explaining that they need the discovery_config.delete/list/update permissions in order to delete the integration.

B) it's missing the discovery_config.update verb, given that we might end up updating the resource (in case it has other matchers)
Can we add that verb as well?

C) We are also listing and, possibly, deleting an AppServer.
I think we should also add that as a pre-req.

do not remove the resources which you don't have access to (this is, hopefully, an edge case)

seems 'not great' to silently skip this given there's a parameter to delete associated resources needed

require discovery_config.* permissions when users try to create an integration

seems like this could be removed after the fact

return an error but return a better error message explaining that they need the discovery_config.delete/list/update permissions in order to delete the integration.

👍 I think a better error message is appropriate here

B) it's missing the discovery_config.update verb

I'll add the missing verbs, app server check

r0mant

Looks reasonable with a few comments/suggestions.

r0mant · 2025-09-30T20:20:28Z

+	if err := authCtx.CheckAccessToKind(types.KindDiscoveryConfig,
+		types.VerbList, types.VerbUpdate, types.VerbDelete); err != nil {
+		return trace.AccessDenied(
+			`deleting AWS OIDC integration requires 'list', 'update', and 'delete' ` +
+				`permissions for 'discovery_config' kind to remove associated resources`)
+	}
+
+	if err := authCtx.CheckAccessToKind(types.KindAppServer,
+		types.VerbList, types.VerbDelete); err != nil {
+		return trace.AccessDenied(
+			`deleting AWS OIDC integration requires 'list' and 'delete' ` +
+				`permissions for 'app_server' kind to remove associated resources`)
+	}


All the other methods here check access to KindIntegration which I think we should follow here as well. It would be weird if creating the integration requires you to have access to one resource and deleting it to another. Since this only removes resources specific to this integration, it should be ok to check against integration kind.

This is meant to be called from DeleteIntegration following the pattern from deleteGitHubAssociatedResources https://github.com/gravitational/teleport/blob/master/lib/auth/integration/integrationv1/service.go#L505-L527

The integration delete check occurs here https://github.com/gravitational/teleport/blob/master/lib/auth/integration/integrationv1/service.go#L384-L386

Would it suffice to add a comment that an access check to KindIntegration is expected ?

Hmm I see. In that case it is probably fine, and yeah, a comment would be helpful. Do we require permissions to create/update KindDiscoveryConfig and KindAppServer when creating the integration? @marcoandredinis

We don't require those permissions when creating the integration.
They are only checked when using the flows:

discovery_config.<verb> when setting up auto discover

app_server.<verb> when enabling AWS App Access

r0mant · 2025-09-30T23:46:00Z

+		// Update discovery_config if any AWSMatchers were removed
+		if len(filteredAWSMatchers) != len(config.Spec.AWS) {
+			config.Spec.AWS = filteredAWSMatchers
+			if _, err := s.backend.UpdateDiscoveryConfig(ctx, config); err != nil {
+				return trace.Wrap(err)
+			}
+		}


TBH I feel like updating discovery config to remove integration-related rules is a bit wonky.

Per my understanding this can only happen if someone explicitly creates a discovery config that has rules that reference an integration and other rules that don't. Our guided flows don't do that so you can only get a discovery config that has integration-specific matchers.

Given this is created by users, I don't think that silently removing the rules from it is good UX, even though I don't imagine this is a common use-case. I would recommend that we return an error to users in this case saying "this integration X is referenced by discovery config Y, please use the command Z to see the config and update it before removing this integration".

In general, we should not be removing or modifying stuff that users created. @marcoandredinis do we have any indicator that a discovery config was created by the guided flow? We should only be deleting the resources created by the integration and leaving others in place (and potentially blocking integration deletion with a message like I mentioned above).

do we have any indicator that a discovery config was created by the guided flow?

We don't, but I agree we should.
We could add a label which indicates a direct relation with the integration:

teleport.dev/integration: <integration-name> teleport.dev/remove-on-integration-deletion: true

We could start implementing this (or a variation of it).
Then, it would be obvious to the user that this discovery config will be deleted if the integration is deleted.

For existing discovery configs, we still need to be smart about whether to delete them or not:

delete them if it only has integration-based matchers

return an error if there's at least one discovery config which uses another integration, or no integration at all

r0mant · 2025-10-01T23:57:33Z

+		_, err := uuid.Parse(config.GetName())
+		if err != nil {
+			continue
+		}


More of a nit, but I would move this down under if config.IsMatchersEmpty() condition so we don't do parsing work unless we know this is a config we want to remove. And with a comment explaining that if this is not a valid UUID then it wasn't created by the guided flow so we shouldn't touch it.

r0mant · 2025-10-02T00:10:54Z

+	// Delete discovery configs with
+	// 1. valid UUID name
+	// 2. only contains an AWSMatcher associated with integration
+	for config, err := range clientutils.Resources(ctx, s.backend.ListDiscoveryConfigs) {


s.backend.ListDiscoveryConfigs

We should be using cache, not backend directly for reads.

I also looked at how AWS OIDC Service is being initialized and I don't think it's initialized correctly:

teleport/lib/auth/grpcserver.go

Line 5788 in cd6f55c

Cache: cfg.AuthServer,

It gets passed Auth directly, not Auth's caching client like for example here:

teleport/lib/auth/grpcserver.go

Line 5774 in cd6f55c

Cache: cfg.AuthServer.Cache,

cc @marcoandredinis Any idea why?

You are right 👍
Just raised a PR to start using the cache in AWS OIDC Service: #59855

r0mant · 2025-10-02T00:11:54Z

+	}
+
+	// Delete AWS access app_server associated with this integration
+	appServers, err := s.backend.GetApplicationServers(ctx, defaults.Namespace)


This also should be using cache, not backend. A cluster can have tens (or even hundreds) of thousands of application servers and this decimate backend.

alexhemard · 2025-10-02T22:57:21Z

changes:

Use cache for listing discovery_configs and app_servers
Return an error if DiscoveryConfigs matching the integration were found without a valid UUID name (or has additional matchers)
Removed checks for discovery config / app server verbs

r0mant

lgtm with a couple remaining comments

marcoandredinis

I'll defer to @kimlisa the frontend changes, but backend looks good, thank you.

marcoandredinis · 2025-10-06T08:06:44Z

+	// TODO(alexhemard): follow up work needed to add explicit labels for
+	// resources created by integration rather than rely on implicit rules


Can we create an issue to track this instead?

Opened #60072

kimlisa

with some comments

kimlisa · 2025-10-06T21:33:52Z

+        <Alert
+          kind="warning"
+          primaryAction={{
+            content: 'AWS Resource Explorer',
+            href: awsResourceExplorerUrl,
+          }}
+        >
+          There may be AWS resources created by this integration that require
+          manual clean up. Visit the AWS Resource Explorer to see resources with
+          tags matching this integration.
+        </Alert>


is it implied that users will be on the correct AWS account when clicking on the link? i just happened to have integrated two different aws accounts, and it could look like an empty list, if the integration we're trying to delete is not the one we are signed into?

maybe we can emphasize the aws account ID somewhere like in the title perhaps

kimlisa · 2025-10-06T21:34:14Z

+          Teleport resources used for auto-discovery that reference this
+          integration will also be removed.


in case user doesn't read to the end 😆 (like me)

Suggested change

Teleport resources used for auto-discovery that reference this

integration will also be removed.

Teleport will also remove resources used for auto-discovery that reference this

integration.

kimlisa · 2025-10-06T21:34:32Z

+        <Alert kind="info">
+          Teleport resources used for auto-discovery that reference this
+          integration will also be removed.


not a blocker, but i'd consult with UX folks and get consistent design. for git server this similar message "Teleport will delete" is inside a warning banner instead and is at the bottom of the dialogue.

backport-bot-workflows · 2025-10-07T16:02:01Z

@alexhemard See the table below for backport results.

Branch	Result
branch/v18	Create PR

…9607)

github-actions bot added size/md ui labels Sep 25, 2025

github-actions bot requested review from flyinghermit and kimlisa September 25, 2025 18:55

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch from 28fa1c3 to fe5ad9e Compare September 25, 2025 18:56

alexhemard commented Sep 25, 2025

View reviewed changes

Comment thread lib/auth/integration/integrationv1/awsoidc.go Outdated

r0mant requested review from marcoandredinis and removed request for flyinghermit September 29, 2025 16:49

marcoandredinis reviewed Sep 29, 2025

View reviewed changes

Comment thread lib/auth/integration/integrationv1/awsoidc.go Outdated

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch 2 times, most recently from 3e2fd5f to c36ae03 Compare September 30, 2025 14:50

alexhemard requested a review from marcoandredinis September 30, 2025 15:26

marcoandredinis reviewed Sep 30, 2025

View reviewed changes

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch 3 times, most recently from 341a2ab to 998e4fa Compare September 30, 2025 18:22

alexhemard requested a review from marcoandredinis September 30, 2025 18:28

r0mant reviewed Sep 30, 2025

View reviewed changes

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch from 998e4fa to 01178e2 Compare September 30, 2025 23:26

r0mant reviewed Sep 30, 2025

View reviewed changes

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch 2 times, most recently from 5ea58a4 to cd6f55c Compare October 1, 2025 19:03

r0mant requested changes Oct 2, 2025

View reviewed changes

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch 4 times, most recently from bb1d993 to 855b380 Compare October 2, 2025 22:14

alexhemard requested a review from r0mant October 2, 2025 22:57

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch from 855b380 to 5a8ebf1 Compare October 3, 2025 13:35

r0mant approved these changes Oct 4, 2025

View reviewed changes

Comment thread lib/auth/integration/integrationv1/awsoidc.go Outdated

Comment thread lib/auth/integration/integrationv1/awsoidc.go Outdated

Comment thread lib/auth/integration/integrationv1/awsoidc.go

Comment thread lib/auth/integration/integrationv1/awsoidc.go

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch from 5a8ebf1 to a94fec2 Compare October 4, 2025 15:26

marcoandredinis approved these changes Oct 6, 2025

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from kimlisa October 6, 2025 08:30

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch from a94fec2 to 7f7d2c4 Compare October 6, 2025 17:26

r0mant requested a review from kimlisa October 6, 2025 20:24

kimlisa approved these changes Oct 6, 2025

View reviewed changes

feat(AWS OIDC): deleting integration removes associated resources

b2a8b05

alexhemard force-pushed the alexhemard/aws-oidc-cleanup branch from 7f7d2c4 to b2a8b05 Compare October 7, 2025 14:40

alexhemard added the backport/branch/v18 label Oct 7, 2025

alexhemard enabled auto-merge October 7, 2025 15:39

alexhemard added this pull request to the merge queue Oct 7, 2025

Merged via the queue into master with commit 9f682da Oct 7, 2025
46 checks passed

alexhemard deleted the alexhemard/aws-oidc-cleanup branch October 7, 2025 16:00

This was referenced Oct 7, 2025

[v18] feat(AWS OIDC): deleting integration removes associated resources #60018

Merged

feat(AWS OIDC): add labels to resources created by integration #60072

Open

rhammonds-teleport pushed a commit that referenced this pull request Nov 6, 2025

feat(AWS OIDC): deleting integration removes associated resources (#5…

4c542b3

…9607)

		// TODO(alexhemard): follow up work needed to add explicit labels for
		// resources created by integration rather than rely on implicit rules

		Teleport resources used for auto-discovery that reference this
		integration will also be removed.

Conversation

alexhemard commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots

Uh oh!

Uh oh!

r0mant commented Sep 29, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r0mant left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

alexhemard commented Oct 2, 2025

Uh oh!

r0mant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcoandredinis left a comment

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!

Uh oh!

kimlisa left a comment

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!

Uh oh!

Uh oh!

backport-bot-workflows bot commented Oct 7, 2025

Uh oh!

Reviewers

alexhemard commented Sep 25, 2025 •

edited

Loading