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

maintenance window agent export logic #23062

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

fspmarshall
Copy link
Contributor

@fspmarshall fspmarshall commented Mar 14, 2023

Implementation of the agent-side logic corresponding to #22850

This PR introduces maintenance window export logic for the kube controller and systemd unit based external upgraders. When enabled, the teleport agent periodically loads maintenance window info from auth and exports it to an appropriate location for the upgrader to consume it.

The behavior is toggled by setting an environment variable (TELEPORT_EXT_UPGRADER=kube|unit). This flag is not intended to be set by users. Upgrader packages will set the appropriate value automatically.

Note: this PR is currently pointing toward fspmarshall/maintenance-window-auth-v2 and will be rebased to master once the changes it depends on get merged.

@fspmarshall fspmarshall force-pushed the fspmarshall/maintenance-window-auth-v2 branch 3 times, most recently from d9a79d4 to bb98e4a Compare March 21, 2023 17:51
@fspmarshall fspmarshall force-pushed the fspmarshall/maintenance-window-agent branch from 9b29ab6 to c9704de Compare March 28, 2023 05:59
@fspmarshall fspmarshall marked this pull request as ready for review March 28, 2023 06:09
@github-actions github-actions bot requested review from mdwn and ryanclark March 28, 2023 06:10
Copy link
Contributor

@mdwn mdwn left a comment

Choose a reason for hiding this comment

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

These are largely nits. Feel free to push back on whatever!

lib/versioncontrol/maintenancewindow/maintenancewindow.go Outdated Show resolved Hide resolved
lib/versioncontrol/maintenancewindow/maintenancewindow.go Outdated Show resolved Hide resolved
// Exporter is a helper used to
type Exporter[C contextLike] struct {
cfg ExporterConfig[C]
closeContext context.Context
Copy link
Contributor

Choose a reason for hiding this comment

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

I was pinged on a review recently and advised not to store context in structs. Seems like a reasonable idea. It'd be easy enough to adjust the call to Run in service.go to just say something like:

		process.RegisterCriticalFunc("maintenancewindow.export", func() error {
                    return exporter.Run(process.ExitContext())
                )

I don't know that Close would be necessary if you did this, as the context would be canceled as part of process shutdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to push back on this one... storing a request scoped (or potentially request-scoped) context in a struct is bad, because it makes it too easy to accidentally introduce weird memory leaks. This is because request-scoped context frequently hold onto references to lots of request-scoped date (e.g. certs).

Using context.Background() to create a cancellation helper for a component that does cancellable I/O is fine IMO. Rather than bake our own alternative closure mechanism, if I were to deviate from this pattern I'd probably deviate by creating a custom CloseContext helper type that implements context.Context and never contains any context values (i.e. a context type that statically promises that it does not hold references to request-scoped values).

We could use process.ExitContext(), but IMO that's not a great pattern. I've found myself leaning toward the opinion that persistent components should be explicitly rather than implicitly closed where possible. Mostly because this allows control of the order of closure. Context cancellation cancels all layers simultaneously, which has caused confusing errors for us in the past. Mostly a moot point here, since no other components rely on exporter being healthy, but it's a pattern I'm trying to force myself to stick to.

return nil
}

func (e *Exporter[C]) event(event testEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this a lot!

@mdwn mdwn self-requested a review March 31, 2023 14:49
@fspmarshall fspmarshall force-pushed the fspmarshall/maintenance-window-auth-v2 branch from 2279020 to 8a328e1 Compare April 4, 2023 16:41
@public-teleport-github-review-bot

@fspmarshall - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

@fspmarshall fspmarshall force-pushed the fspmarshall/maintenance-window-agent branch from 5cd6d5f to d31ba43 Compare April 4, 2023 17:06
@github-advanced-security
Copy link

You have successfully added a new Trivy configuration .github/workflows/trivy.yaml:trivy. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@fspmarshall fspmarshall force-pushed the fspmarshall/maintenance-window-auth-v2 branch 10 times, most recently from 717f489 to 6be6771 Compare April 10, 2023 23:26
Base automatically changed from fspmarshall/maintenance-window-auth-v2 to master April 11, 2023 00:38
@fspmarshall fspmarshall force-pushed the fspmarshall/maintenance-window-agent branch from d31ba43 to bd290e2 Compare April 11, 2023 02:39
adds client-side logic for exporting maintenance windows
for external updaters. export behavior is enabled via
env var (`TELEPORT_EXT_UPGRADER=kube|unit`).
@fspmarshall fspmarshall force-pushed the fspmarshall/maintenance-window-agent branch 2 times, most recently from 1534588 to 827af87 Compare April 11, 2023 04:50
@fspmarshall fspmarshall added this pull request to the merge queue Apr 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2023
@fspmarshall fspmarshall added this pull request to the merge queue Apr 11, 2023
Merged via the queue into master with commit b2a3023 Apr 11, 2023
23 checks passed
@fspmarshall fspmarshall deleted the fspmarshall/maintenance-window-agent branch April 11, 2023 06:44
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.

None yet

3 participants