Skip to content

[v13] integrations/operator: re-use the teleport client instead of creating a new one#34431

Merged
hugoShaka merged 2 commits intobranch/v13from
hugo/backport-34050-branch/v13
Nov 13, 2023
Merged

[v13] integrations/operator: re-use the teleport client instead of creating a new one#34431
hugoShaka merged 2 commits intobranch/v13from
hugo/backport-34050-branch/v13

Conversation

@hugoShaka
Copy link
Copy Markdown
Contributor

@hugoShaka hugoShaka commented Nov 9, 2023

Manual backport of #34050 to branch/v13

… a new one (#34050)

* integrations/operator: re-use the teleport client instead of creating a new one

* fix race condition

* address feedback + add godocs
Comment on lines +128 to +131
teleportClient, release, err := r.TeleportClientAccessor(ctx)
if err == nil {
defer release()
}
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.

Why are we not returning an error when err != nil ?

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.

Because we are processing the error and reporting it in the status, so we don't want to exit yet. A few lines below the error is aggregated with the status update error. err handling continues below.

We have 4 cases:

  • err != nil, updaterErr != nil
  • err != nil, updaterErr == nil
  • err == nil, updateErr != nil
  • err == nil, updaterErr != nil

We must release the teleportClient only on the last two cases, we can do two release invocation (one in the if err != nil || updateErr != nil { block, and one after) or do a single one before, then continue to process errors.

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 was scrolling and the pattern was different for the Role
But it seems it was already the case anyway
👍

Comment on lines +128 to +131
teleportClient, release, err := r.TeleportClientAccessor(ctx)
if err == nil {
defer release()
}
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 was scrolling and the pattern was different for the Role
But it seems it was already the case anyway
👍

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from strideynet November 10, 2023 15:10
@hugoShaka hugoShaka changed the title [v13] integrations/operator: re-use the teleport client instead of creating… [v13] integrations/operator: re-use the teleport client instead of creating a new one Nov 10, 2023
@hugoShaka hugoShaka added this pull request to the merge queue Nov 13, 2023
Merged via the queue into branch/v13 with commit 1a26a83 Nov 13, 2023
@hugoShaka hugoShaka deleted the hugo/backport-34050-branch/v13 branch November 13, 2023 16:50
This was referenced Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants