Skip to content

check integration reference in the current state of the plugin resource#56138

Merged
flyinghermit merged 3 commits intomasterfrom
sshah/fix-integration-reference
Jun 27, 2025
Merged

check integration reference in the current state of the plugin resource#56138
flyinghermit merged 3 commits intomasterfrom
sshah/fix-integration-reference

Conversation

@flyinghermit
Copy link
Copy Markdown
Contributor

In the DeleteIntegration we need to ensure that the integration which is being deleted is not referenced by the AWS identity center plugin. If it is not referenced, a backend condition was added to ensure that the revision of the plugin is not changed between the reference check and the final backend delete op. This condition incorrectly referenced "integration name" instead of "plugin name" in the plugin backend key. This led the revision value never match during the delete operation, preventing deletion of integration.

There is another issue with the current conditional deletion approach based on revision value. Given the plugin spec holds plugin status field, which is expected to be updated frequently in tandem to the service runtime status, users might still occasionally hit revision mismatch error when trying to delete the integration.

Since the integration field can only be manually configured when creating/editing the plugin and the system checks for existence of integration resource before creating the plugin, imho the ux issue created with the revision mismatch outweighs the prevention ensured by the conditional deletion. As such, the integrationReferencedByAWSICPlugin is now updated to only check for integration reference in the identity center plugin in the current state of the plugin resource in the backend.

changelog: Fixed an issue that prevented deletion of an integration resource if AWS Identity Center plugin was installed in the Teleport cluster.

@flyinghermit
Copy link
Copy Markdown
Contributor Author

Note to reviewer: I am open to keep the conditional deletion and only fix the core issue to provide correct backend key name. if you think its worth keeping it.

@flyinghermit flyinghermit changed the title remove delete integration conditional revision check for aws identity center plugin reference check integration reference in the current state of the plugin resource Jun 26, 2025
@flyinghermit flyinghermit marked this pull request as ready for review June 26, 2025 17:45
@github-actions github-actions Bot requested review from camscale and ravicious June 26, 2025 17:46
Copy link
Copy Markdown
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

The check was already racey with regards to new plugins added during the check so I imagine it was never a particularly hard requirement, but what will happen if we end up with a plugin that references an integration that no longer exists?

Comment thread lib/services/local/integrations.go Outdated
switch pluginV1.GetType() {
case types.PluginType(types.PluginTypeAWSIdentityCenter):
if awsIC := pluginV1.Spec.GetAwsIc(); awsIC != nil {
if awsIC.IntegrationName == name {
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.

// IntegrationName is the Teleport OIDC integration used to gain access to the
// AWS account. May be empty if [CredentialsSource] is SYSTEM.
// DEPRECATED: Use [Credentials] instead. DELETE in Teleport 19+
string integration_name = 1 [deprecated = true];

I fee like we should switch to https://github.com/gravitational/teleport/blob/master/api/proto/teleport/legacy/types/types.proto#L7369

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. The plugin create plugin handle still references integration name field https://github.com/gravitational/teleport.e/blob/master/lib/web/plugindescriptor_aws_ic.go#L85C5-L87C59. I think it would be better to update all the references at once.

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from tigrato June 27, 2025 08:24
@flyinghermit
Copy link
Copy Markdown
Contributor Author

but what will happen if we end up with a plugin that references an integration that no longer exists?

Existing access to AWS will not be affected but the identity center service will not be able to provision new account assignment and de-provisioning existing assignments.

@flyinghermit flyinghermit enabled auto-merge June 27, 2025 14:04
@flyinghermit flyinghermit added this pull request to the merge queue Jun 27, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 27, 2025
@flyinghermit flyinghermit added this pull request to the merge queue Jun 27, 2025
Merged via the queue into master with commit 607f847 Jun 27, 2025
40 checks passed
@flyinghermit flyinghermit deleted the sshah/fix-integration-reference branch June 27, 2025 14:46
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@flyinghermit See the table below for backport results.

Branch Result
branch/v17 Failed
branch/v18 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants