MWI: Add lib/boundkeypair for bound keypair backend implementation#54766
MWI: Add lib/boundkeypair for bound keypair backend implementation#54766timothyb89 merged 3 commits intomasterfrom
Conversation
This splits the backend implementation of lib/boundkeypair out from the main bound keypair joining PR in #54371. It makes no changes beyond those already reviewed in that PR.
timothyb89
added
machine-id
no-changelog
|
Reviewer note, this just exists to reduce the changed LoC count in #54371 and to appease the size check. This package is relatively isolated and could be split out of the larger PR without much effort. |
| ) | ||
|
|
||
| func newTestKeypair(t *testing.T) crypto.Signer { | ||
| key, err := cryptosuites.GenerateKey(context.Background(), func(ctx context.Context) (types.SignatureAlgorithmSuite, error) { |
There was a problem hiding this comment.
Many of these helper funcs could do with a t.Helper() call.
| key, err := cryptosuites.GenerateKey(context.Background(), func(ctx context.Context) (types.SignatureAlgorithmSuite, error) { | |
| t.Helper() | |
| key, err := cryptosuites.GenerateKey(context.Background(), func(ctx context.Context) (types.SignatureAlgorithmSuite, error) { |
There was a problem hiding this comment.
Ah, good call. Fixed now, thanks!
|
|
||
| var mu sync.Mutex | ||
|
|
||
| var experimentEnabled = os.Getenv("TELEPORT_BOUND_KEYPAIR_JOINING_EXPERIMENT") == "1" |
There was a problem hiding this comment.
Can we use strconv.ParseBool here just to be consistent with other similar env vars?
You might also consider a TELEPORT_UNSTABLE_ prefix for the var name to be consistent with similar approaches we've taken in the past. This communicates to the person who enables the feature that it's early and subject to change in the future.
There was a problem hiding this comment.
I've changed the impl to use strconv.ParseBool(). We've been copying and pasting this for our feature flags for a while and I see why we went with the simpler implementation for these hacky flags, handling the error case does make things a bit uglier.
There was a problem hiding this comment.
I typically don't handle the error case - if you get an error from ParseBool you are guaranteed to get false (not enabled).
There was a problem hiding this comment.
Ah, good call, that's much simpler. Thanks!
| } | ||
|
|
||
| // SetEnabled sets the experiment enabled flag. | ||
| func SetEnabled(enabled bool) { |
There was a problem hiding this comment.
Do we need to be able to change this at runtime? It might be simpler to check the env var on startup and require a process restart to pick up changes.
There was a problem hiding this comment.
Yes, it's used in tests, unfortunately. I'm okay with it being a bit ugly since it won't stick around for long.
| // You should have received a copy of the GNU Affero General Public License | ||
| // along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| package experiment |
There was a problem hiding this comment.
I don't love the name of the package. At the call site, it's going to look something like if experiment.Enabled() { ... and the first question the reader will have is "what experiment?"
There was a problem hiding this comment.
That's fair enough, I've renamed the package.
- Renames the experiment package - Fixes missing test helper flags - Renames the experiment env var - Uses strconv.ParseBool()
…54766) * MWI: Add lib/boundkeypair for bound keypair backend implementation This splits the backend implementation of lib/boundkeypair out from the main bound keypair joining PR in #54371. It makes no changes beyond those already reviewed in that PR. * Apply code review suggestions - Renames the experiment package - Fixes missing test helper flags - Renames the experiment env var - Uses strconv.ParseBool() * Simplify experiment flag
…ion (#54766) (#55079) * MWI: Add lib/boundkeypair for bound keypair backend implementation (#54766) * MWI: Add lib/boundkeypair for bound keypair backend implementation This splits the backend implementation of lib/boundkeypair out from the main bound keypair joining PR in #54371. It makes no changes beyond those already reviewed in that PR. * Apply code review suggestions - Renames the experiment package - Fixes missing test helper flags - Renames the experiment env var - Uses strconv.ParseBool() * Simplify experiment flag * Fix broken test
4 participants
This splits the backend implementation of lib/boundkeypair out from the main bound keypair joining PR in #54371. It makes no changes beyond those already reviewed in that PR.