cli: put a lock around login #362

mxyng · 2021-09-29T17:59:00Z

prevent too many prompts from showing up

Fixes #351

internal/cmd/cmd.go

jmorganca · 2021-09-29T20:33:01Z

internal/cmd/cmd.go

+	}
+
+	loginPid := filepath.Join(homeDir, ".infra", "login.pid")
+	if _, err := os.Stat(loginPid); err == nil {


Does this mean other kubectl commands will block, or will they error? If they error clients like Infra App this may cause a strange situation where the first command works but follow up ones just error

Subsequent commands should error. I don't have Infra App installed so don't know exactly how it would interact. I feel like erroring out would be the best case scenario. Blocking kubectl might have additional side effects as well.

$ k get deploy Error: login already in progress Unable to connect to the server: getting credentials: exec plugin is configured to use API version client.authentication.k8s.io/v1beta1, plugin returned version client.authentication.k8s.io/__internal

To add: not sure how easy it would be for all these invocations of login() to wait on each other (especially since they may be separate processes) – but there may be a useful file lock library we can leverage for this (saw a few like https://github.com/zbiljic/go-filelock - but they seem very old)

I found a more recent library that implements file locking using sync.RWMutex. It provides a nice interface to build the behaviour we want.

Now, infra login will create a lock such that other infra login attempts will block until the lock is freed or it expires (5m). Once it's unlocked, it will not attempt a login. Instead it will continue assuming the other attempt successfully logged in.

internal/cmd/cmd.go

mxyng · 2021-09-29T23:12:19Z

internal/cmd/cmd.go

+		ctx, cancel := context.WithTimeout(context.Background(), time.Minute*5)
+		defer cancel()
+
+		_, err = lock.TryLockContext(ctx, time.Second*1)


1 second interval between retries

internal/cmd/cmd.go

jmorganca · 2021-09-30T00:22:56Z

Very elegant! Nice work!!

jmorganca · 2021-09-30T03:01:27Z

Just tested this out a bunch with a super short token duration and it feels very good for both kubectl and other tools such as Infra App! No more lost data / errors or multiple tabs opening up. Just a super smooth re-login flow.

One important challenge to think about here: would there ever be a case (e.g. apps like Infra App or a long running scripts using kubectl) where a browser may suddenly pop open unexpectedly as tokens expire? If so this could really surprise users since an Okta login will randomly pop up – and they may not even know why or what it's for. The user may be on an important screen sharing zoom call – this would definitely alarm them

Do we have thoughts on how to handle this? Maybe via a refresh flow that uses their existing token vs requiring a full auth (this could be something we allow for later)? Or maybe we can have a more subtle step before the window when it's time to re-auth (perhaps a prompt on the CLI if interactive? or something else if non interactive? –– maybe a notification even?)

BruceMacD · 2021-09-30T13:11:54Z

Really good points around the pop-up. I hate that too personally, it's not good to expect users to enter their credentials into windows that randomly pop up. Gives you a bad reflex to enter your credentials on demand without questioning it.

I think adding a refresh flow (in a separate issue) is probably the correct solution. I like the more contextual identity provider re-authentication, but I don't know if it would be possible with every identity provider.

jmorganca · 2021-09-30T15:13:16Z

Another issue to capture in this area: there may also be the case where a computer is left idle and every 5 minutes a popup will come up since the lock will release. Another item to think about around the re-authentication experience since in say after 8 hours of inactivity that may still be 96 popups (if I understand correctly! just a hunch based on how this is implemented)

ssoroka · 2021-09-30T15:18:02Z

I could potentially see problems here where one login screen opens and the user doesn't notice or ignores it, then subsequent logins don't work because we're locked on the one login window. Seems like we may have caused the problem by having infra creds try to automatically login

ssoroka · 2021-09-30T16:43:47Z

infra creds and infra login should probably use github.com/mattn/go-isatty to determine if there's a user at the end of the request, or just a piece of software, and customize accordingly.

mxyng · 2021-09-30T19:31:57Z

term.IsTerminal seems to be doing what I expected. Still need to test with Infra App. Would appreciate some help in that area.

$ infra creds docker-desktop </dev/null
Error: 403 Forbidden
$ infra creds docker-desktop           
✓ Logging in with Okta...

It also looks to block login prompt in desktop apps like Lens.

- prevent too many prompts from showing up

- block login while another instance has the lock - assume the other instance is successful and exit with success if instance is able to get the lock and proceed without showing login prompt

BruceMacD · 2021-10-01T16:01:02Z

internal/cmd/cmd.go

@@ -50,7 +51,7 @@ type Config struct {
 type ErrUnauthenticated struct{}

 func (e *ErrUnauthenticated) Error() string {


Not related to this PR where this is an existing error, but maybe we should move the errors package to the root level and put this there

BruceMacD · 2021-10-01T16:01:41Z

Tested again locally by switching on infra.app and this was handled gracefully

No login prompt

mxyng force-pushed the lock-login branch from 0648c1f to f7c2f41 Compare September 29, 2021 19:30

mxyng requested a review from BruceMacD September 29, 2021 19:50

mxyng changed the title ~~put a lock around login~~ cli: put a lock around login Sep 29, 2021

jmorganca reviewed Sep 29, 2021

View reviewed changes

internal/cmd/cmd.go Outdated Show resolved Hide resolved

jmorganca reviewed Sep 29, 2021

View reviewed changes

mxyng commented Sep 29, 2021

View reviewed changes

internal/cmd/cmd.go Show resolved Hide resolved

mxyng commented Sep 29, 2021

View reviewed changes

BruceMacD reviewed Sep 29, 2021

View reviewed changes

internal/cmd/cmd.go Outdated Show resolved Hide resolved

mxyng force-pushed the lock-login branch from d71ee26 to a5f9e62 Compare September 29, 2021 23:59

BruceMacD approved these changes Sep 30, 2021

View reviewed changes

mxyng mentioned this pull request Sep 30, 2021

Remove more unused local proxy code #368

Merged

mxyng force-pushed the lock-login branch 4 times, most recently from 2d55b14 to 8892bbe Compare September 30, 2021 22:47

mxyng added 8 commits September 30, 2021 16:29

put a lock around login

a87abea

- prevent too many prompts from showing up

use flock instead of implementing it myself

50c44a5

- block login while another instance has the lock - assume the other instance is successful and exit with success if instance is able to get the lock and proceed without showing login prompt

linting

3767b15

check terminal and abort if not interactive

2c114c4

use unauthenticated message

d389054

consistent usage of homeDir

9779055

lint

404b1e5

more lint

8895b11

mxyng force-pushed the lock-login branch from bee3582 to 8895b11 Compare September 30, 2021 23:31

mxyng requested a review from BruceMacD September 30, 2021 23:35

jmorganca force-pushed the main branch from 4fc3cc6 to b969106 Compare October 1, 2021 00:06

BruceMacD approved these changes Oct 1, 2021

View reviewed changes

mxyng merged commit d881286 into main Oct 1, 2021

mxyng deleted the lock-login branch October 1, 2021 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: put a lock around login #362

cli: put a lock around login #362

mxyng commented Sep 29, 2021

jmorganca Sep 29, 2021

mxyng Sep 29, 2021

mxyng Sep 29, 2021 •

edited

jmorganca Sep 29, 2021

mxyng Sep 29, 2021

mxyng Sep 29, 2021

jmorganca commented Sep 30, 2021

jmorganca commented Sep 30, 2021 •

edited

BruceMacD commented Sep 30, 2021

jmorganca commented Sep 30, 2021 •

edited

ssoroka commented Sep 30, 2021

ssoroka commented Sep 30, 2021

mxyng commented Sep 30, 2021 •

edited

BruceMacD Oct 1, 2021

BruceMacD commented Oct 1, 2021 •

edited

		@@ -50,7 +51,7 @@ type Config struct {
		type ErrUnauthenticated struct{}

		func (e *ErrUnauthenticated) Error() string {

cli: put a lock around login #362

cli: put a lock around login #362

Conversation

mxyng commented Sep 29, 2021

jmorganca Sep 29, 2021

Choose a reason for hiding this comment

mxyng Sep 29, 2021

Choose a reason for hiding this comment

mxyng Sep 29, 2021 • edited

Choose a reason for hiding this comment

jmorganca Sep 29, 2021

Choose a reason for hiding this comment

mxyng Sep 29, 2021

Choose a reason for hiding this comment

mxyng Sep 29, 2021

Choose a reason for hiding this comment

jmorganca commented Sep 30, 2021

jmorganca commented Sep 30, 2021 • edited

BruceMacD commented Sep 30, 2021

jmorganca commented Sep 30, 2021 • edited

ssoroka commented Sep 30, 2021

ssoroka commented Sep 30, 2021

mxyng commented Sep 30, 2021 • edited

BruceMacD Oct 1, 2021

Choose a reason for hiding this comment

BruceMacD commented Oct 1, 2021 • edited

mxyng Sep 29, 2021 •

edited

jmorganca commented Sep 30, 2021 •

edited

jmorganca commented Sep 30, 2021 •

edited

mxyng commented Sep 30, 2021 •

edited

BruceMacD commented Oct 1, 2021 •

edited