-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Use new autoupdate APIs in discovery service #51758
Conversation
d1cda36
to
f88f765
Compare
// Name implements Getter | ||
func (g ProxyVersionGetter) Name() string { | ||
return g.name | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Narrator voice: Name was not, in fact, part of the version.Getter interface.
309d3c5
to
eb4b739
Compare
eb4b739
to
8f5db63
Compare
Co-authored-by: Tiago Silva <[email protected]>
lib/web/integrations_awsoidc.go
Outdated
if err != nil { | ||
return "", trace.Wrap(err) | ||
h.logger.ErrorContext(r.Context(), "Cannot read autoupdate target version", "error", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we were terminating the flow on error. Should we do the same? Or should we default to current's teleport.Version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good question.
With the new autoupdate system I think that we are adding more moving parts and increasing the probability of errors when getting the autoupdate version. I think we should always fallback to the local proxy version in case of error because:
- it's a better UX to get the install script working instead of a random error "token might be expired", you have a working node/integration
- by definition we still enable autoupdates, so potential version drifts will get corrected over time, the core feature of updating the nodes/agents is not affected
Based on this discussion I changed the join scripts to also fallback to their own version instead of failing.
This PR replaces every direct usage of the proxy's autoupdate channels from RFD 109 by
agentAutoUpdateVersion()
which will look at the RFD-184 autoupdate_agent_rollout resource first, then fallback to RFD-109 proxy channels.Fixes an issue found during the autoupdate test plan: discovery service and discover UI don't use the version from
autoupdate_agent_rollout
, this causes version inconsistencies.There are two gotchas in the PR:
version.Getter.GetVersion()
returned versions with the leading "v" whileagentAutoUpdateVersion()
doesn't have the leading v.Changelog: Scripts installing nodes no longer fail if they cannot discover the autoupdate version, they send a warning and use the version from their own Teleport binary.
Part of: RFD-184
Goal (internal): https://github.com/gravitational/cloud/issues/10289