Skip to content
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.

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

Support email plugin status sink #48044

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Conversation

bernardjkim
Copy link
Contributor

This PR enables the Email plugin to emit its plugin status.
This change is part of the work required to support Teleport-hosted Email plugins.

@bernardjkim bernardjkim added access-requests no-changelog Indicates that a PR does not require a changelog entry labels Oct 28, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48044.d3pp5qlev8mo18.amplifyapp.com

integrations/access/email/mailers.go Outdated Show resolved Hide resolved
integrations/access/email/mailers.go Outdated Show resolved Hide resolved
integrations/access/email/mailers.go Outdated Show resolved Hide resolved
integrations/access/email/mailers.go Show resolved Hide resolved
integrations/access/email/mailers.go Show resolved Hide resolved
integrations/access/email/mailers.go Outdated Show resolved Hide resolved
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from tigrato October 29, 2024 21:06
@bernardjkim bernardjkim added this pull request to the merge queue Oct 30, 2024
Merged via the queue into master with commit 2d5ec57 Oct 30, 2024
42 checks passed
@bernardjkim bernardjkim deleted the bernard/email-status-sink branch October 30, 2024 00:20
@public-teleport-github-review-bot

@bernardjkim See the table below for backport results.

Branch Result
branch/v15 Create PR
branch/v16 Create PR
branch/v17 Create PR

ctx, cancel := context.WithTimeout(req.Context(), statusEmitTimeout)
defer cancel()

status := common.StatusFromStatusCode(resp.StatusCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

@bernardjkim

sorry for the delay. A few months ago, we began adding dedicated status entries for plugins, where we include detailed error information like the error code, a human-readable message, and the original error itself. We’ve implemented this approach for Entra, GitLab, and others.

I suggest doing the same here. A simple "running/not running" status doesn’t provide meaningful insight, especially when running plugins in teleport cloud tenants where users don't have direct access to logs.

Examples of good information:

  • can't dial to mailgun
  • credentials invalid (expired)
  • message rejected...

check

https://github.com/gravitational/teleport/blob/64922dffc15846a395f765fc524988227e6a3bc2/api/types/types.pb.go#L17107-L17130

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, that is a good point. The generic "running / not running" status isn't all that helpful. I'll work on a follow up PR soon to improve the status entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking in #48604 so I don't forget

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