[teleport-update] Support for Enterprise/FIPS migration #49451
Conversation
sclevine
added
the
no-changelog
lib/autoupdate/agent/config.go
Outdated
| parts := strings.Split(dir, "_") | ||
| var out Revision | ||
| if len(parts) == 0 { | ||
| return out, fmt.Errorf("dir name empty") |
There was a problem hiding this comment.
Use errors.New or trace.BadParameter here?
There was a problem hiding this comment.
Ah, I meant to use trace.Errorf. I know now that trace.Errorf breaks stack traces when used for wrapping. Are there any issues with using it outside of wrapping?
| return "" | ||
| case FlagEnterprise: | ||
| return "ent" | ||
| case FlagFIPS: |
There was a problem hiding this comment.
Are these bit flags? Any risk of us converting to "unknown" if both FlagEnterprise and FlagFIPS is set? Maybe this helps?
| case FlagFIPS: | |
| case FlagFIPS, FlagEnterprise|FlagFIPS: |
There was a problem hiding this comment.
These are bit flags. This method is just intended for single-bit contexts, similar to String(). The calling code ensures that FlagFIPS implies FlagEnterprise (and ensures that the order is always consistent).
lib/autoupdate/agent/config.go
Outdated
| return trace.Wrap(err) | ||
| } | ||
| if i == nil { | ||
| return trace.Errorf("nil install flags while parsing YAML") |
There was a problem hiding this comment.
| return trace.Errorf("nil install flags while parsing YAML") | |
| return trace.BadParameter("nil install flags while parsing YAML") |
There was a problem hiding this comment.
What's the motivation for using a typed error in these contexts? The YAML parser (or calling code) doesn't look for BadParameter.
There was a problem hiding this comment.
Convention? We have an error type that is used heavily throughout the code base whose semantics match the use case here.
I'd be asking the inverse - why wouldn't we want to use it?
There was a problem hiding this comment.
Happy to follow convention here -- just trying to understand this better.
why wouldn't we want to use it?
Using the same set of generic typed errors without expected matchers throughout the codebase seems like it would make it easy to introduce bugs in error handling. For example, someone may be looking for a specific BadParameter error further up the stack, but match this one from a different call stack instead. Or someone may replace this BadParameter with something else (because most BadParameter calls aren't actually matched) and break calling code by accident.
This is why standard library error types that are broadly used also are very specific (e.g., file does not exist).
public-teleport-github-review-bot
bot
removed request for
bl-nero and
hugoShaka
cleanup formatting revert updater rename cleanup Update lib/autoupdate/agent/config.go Co-authored-by: Zac Bergquist <[email protected]> feedback
sclevine
force-pushed
the
sclevine/teleport-update-suffix
branch
from
df1b765 to
04e3f9a
Compare
This PR adds support for migrating agents to/from Enterprise or Enterprise FIPS editions of Teleport
This is accomplished by storing install flags in update.yaml, and appending
_entor_ent_fipsto version directories.The
teleport-updatebinary will be used to enable, disable, and trigger automatic Teleport agent updates. The new auto-updates system manages a local installation of the cluster-specified version of Teleport stored in/var/lib/teleport/versions.RFD: #47126
Goal (internal): https://github.com/gravitational/cloud/issues/10289