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.

Sign up for GitHub

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

Jump to bottom

tstest/integration: still flaky from bad "up" behavior #12028

Closed
bradfitz opened this issue May 6, 2024 · 0 comments · Fixed by #12033
Closed

tstest/integration: still flaky from bad "up" behavior #12028

bradfitz opened this issue May 6, 2024 · 0 comments · Fixed by #12033
Assignees
@bradfitz
Labels
testing

Comments

@bradfitz
Copy link
Member

bradfitz commented May 6, 2024

CI failed post-submit on f3d2fd2 (#12026)

=== RUN   TestAddPingRequest
=== PAUSE TestAddPingRequest
=== CONT  TestAddPingRequest
    integration_test.go:1037: DERP httpsrv listener: 127.0.0.1:45981
    integration_test.go:552: Running /tmp/2357602576/tailscale --socket=/tmp/TestAddPingRequest1835765856/001/tailscale.sock up --login-server=http://127.0.0.1:34687/ --reset ...
    integration_test.go:1118: panic: [unexpected] controlclient: TryLogin called on https://controlplane.tailscale.com/
    integration_test.go:1118: 
    integration_test.go:1118: goroutine 107 [running]:
    integration_test.go:1118: tailscale.com/control/controlclient.(*Direct).TryLogin(0xc00026ec00, {0x1ad8548, 0xc000040870}, 0x0, 0x2)
    integration_test.go:1118: 	tailscale.com/control/controlclient/direct.go:402 +0x294
    integration_test.go:1118: tailscale.com/control/controlclient.(*Auto).authRoutine(0xc00027e500)
    integration_test.go:1118: 	tailscale.com/control/controlclient/auto.go:341 +0x7f3
    integration_test.go:1118: created by tailscale.com/control/controlclient.(*Auto).Start in goroutine 51
    integration_test.go:1118: 	tailscale.com/control/controlclient/auto.go:228 +0x8e
    integration_test.go:552: up: unexpected EOF
        , exit status 1
    stuntest.go:63: STUN server shutdown
--- FAIL: TestAddPingRequest (5.32s)

and @maisem got:

=== RUN   TestTwoNodes
=== PAUSE TestTwoNodes
=== CONT  TestTwoNodes
    integration_test.go:1154: DERP httpsrv listener: 127.0.0.1:40985
    integration_test.go:390: node1 SOCKS5 addr: 127.0.0.1:43593
    integration_test.go:391: node2 SOCKS5 addr: 127.0.0.1:44891
    integration_test.go:394: n1 is listening
    integration_test.go:396: n2 is listening
    integration_test.go:397: Running /tmp/1519636403/tailscale --socket=/tmp/TestTwoNodes1375173484/001/tailscale.sock up --login-server=http://127.0.0.1:33717/ --reset ...
    integration_test.go:1235: panic: [unexpected] controlclient: TryLogin called on https://controlplane.tailscale.com/
    integration_test.go:1235: 
    integration_test.go:1235: goroutine 116 [running]:
    integration_test.go:1235: tailscale.com/control/controlclient.(*Direct).TryLogin(0xc00026f200, {0x1ada828, 0xc0000404b0}, 0x0, 0x2)
    integration_test.go:1235: 	tailscale.com/control/controlclient/direct.go:402 +0x294
    integration_test.go:1235: tailscale.com/control/controlclient.(*Auto).authRoutine(0xc000338c80)
    integration_test.go:1235: 	tailscale.com/control/controlclient/auto.go:341 +0x7f3
    integration_test.go:1235: created by tailscale.com/control/controlclient.(*Auto).Start in goroutine 8
    integration_test.go:1235: 	tailscale.com/control/controlclient/auto.go:228 +0x8e
    integration_test.go:397: up: unexpected EOF
        , exit status 1
    integration_test.go:383: writing tailscaled logs to n1.log and n2.log
    stuntest.go:63: STUN server shutdown
--- FAIL: TestTwoNodes (4.19s)

and

=== RUN   TestNATPing
=== PAUSE TestNATPing
=== CONT  TestNATPing
    integration_test.go:1154: DERP httpsrv listener: 127.0.0.1:36615
    integration_test.go:853: Running /tmp/4183590076/tailscale --socket=/tmp/TestNATPing1448219320/001/tailscale.sock up --login-server=http://127.0.0.1:46491/ --reset ...
    integration_test.go:853: Running /tmp/4183590076/tailscale --socket=/tmp/TestNATPing1448219320/006/tailscale.sock up --login-server=http://127.0.0.1:46491/ --reset ...
=== RUN   TestNATPing/v6=false/no_nat
--- PASS: TestNATPing/v6=false/no_nat (0.05s)
=== RUN   TestNATPing/v6=false/n1_has_external_ip
--- PASS: TestNATPing/v6=false/n1_has_external_ip (0.04s)
=== RUN   TestNATPing/v6=false/n2_has_external_ip
--- PASS: TestNATPing/v6=false/n2_has_external_ip (0.06s)
=== RUN   TestNATPing/v6=false/both_have_external_ips
--- PASS: TestNATPing/v6=false/both_have_external_ips (0.15s)
    integration_test.go:1154: DERP httpsrv listener: 127.0.0.1:41557
    integration_test.go:853: Running /tmp/4183590076/tailscale --socket=/tmp/TestNATPing1448219320/037/tailscale.sock up --login-server=http://127.0.0.1:32833/ --reset ...
    integration_test.go:1235: panic: [unexpected] controlclient: TryLogin called on https://controlplane.tailscale.com/
    integration_test.go:1235: 
    integration_test.go:1235: goroutine 26 [running]:
    integration_test.go:1235: tailscale.com/control/controlclient.(*Direct).TryLogin(0xc0000cd200, {0x12eb0d0, 0xc00003e730}, 0x0, 0x2)
    integration_test.go:1235: 	tailscale.com/control/controlclient/direct.go:402 +0x1b4
    integration_test.go:1235: tailscale.com/control/controlclient.(*Auto).authRoutine(0xc000306a00)
    integration_test.go:1235: 	tailscale.com/control/controlclient/auto.go:341 +0x4a5
    integration_test.go:1235: created by tailscale.com/control/controlclient.(*Auto).Start in goroutine 21
    integration_test.go:1235: 	tailscale.com/control/controlclient/auto.go:228 +0x56
    integration_test.go:853: up: unexpected EOF
        , exit status 1
    stuntest.go:63: STUN server shutdown
    stuntest.go:63: STUN server shutdown
--- FAIL: TestNATPing (4.66s)

The fight continues.
@bradfitz bradfitz self-assigned this May 6, 2024
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: attempt to fix 'up', deflake te…
217efa7 
…sts more

Updates #12028

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@bradfitz bradfitz mentioned this issue May 7, 2024
Merged
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: attempt to fix 'up', deflake te…
d4bc38d 
…sts more

Updates #12028

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
control/controlclient: early return and outdent some code
af00467 
I found this too hard to read before.

This is pulled out of #12033 as it's unrelated cleanup in retrospect.

Updates #12028

Change-Id: I727c47e573217e3d1973c5b66a76748139cf79ee
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@bradfitz bradfitz mentioned this issue May 7, 2024
Merged
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
control/controlclient: early return and outdent some code
80df8ff 
I found this too hard to read before.

This is pulled out of #12033 as it's unrelated cleanup in retrospect.

Updates #12028

Change-Id: I727c47e573217e3d1973c5b66a76748139cf79ee
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
88752df 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
c2738f1 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
52c2028 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
5aa1960 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
645751b 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
36e4e95 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
fef2474 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
5e86b8f 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
a6cfc3c 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
15316fc 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
191d303 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 7, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
70e97b2 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
ipn/ipnserver: close a small race in ipnserver, ~simplify code
8edd4b1 
There was a small window in ipnserver after we assigned a LocalBackend
to the ipnserver's atomic but before we Start'ed it where our
initalization Start could conflict with API calls from the LocalAPI.

Simplify that a bit and lay out the rules in the docs.

Updates #12028

Change-Id: Ic5f5e4861e26340599184e20e308e709edec68b1
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@bradfitz bradfitz mentioned this issue May 8, 2024
Merged
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
ipn/ipnserver: close a small race in ipnserver, ~simplify code
bac14bf 
There was a small window in ipnserver after we assigned a LocalBackend
to the ipnserver's atomic but before we Start'ed it where our
initalization Start could conflict with API calls from the LocalAPI.

Simplify that a bit and lay out the rules in the docs.

Updates #12028

Change-Id: Ic5f5e4861e26340599184e20e308e709edec68b1
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
ipn/ipnserver: close a small race in ipnserver, ~simplify code
364fad3 
There was a small window in ipnserver after we assigned a LocalBackend
to the ipnserver's atomic but before we Start'ed it where our
initalization Start could conflict with API calls from the LocalAPI.

Simplify that a bit and lay out the rules in the docs.

Updates #12028

Change-Id: Ic5f5e4861e26340599184e20e308e709edec68b1
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
d0e2eb7 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
ipn/ipnserver: close a small race in ipnserver, ~simplify code
727c0d6 
There was a small window in ipnserver after we assigned a LocalBackend
to the ipnserver's atomic but before we Start'ed it where our
initalization Start could conflict with API calls from the LocalAPI.

Simplify that a bit and lay out the rules in the docs.

Updates #12028

Change-Id: Ic5f5e4861e26340599184e20e308e709edec68b1
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
19e3450 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
9fa8502 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
93bc240 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 8, 2024
@bradfitz
cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more
e968b0e 
The CLI's "up" is kinda chaotic and LocalBackend.Start is kinda
chaotic and they both need to be redone/deleted (respectively), but
this fixes some buggy behavior meanwhile. We were previously calling
StartLoginInteractive (to start the controlclient's RegisterRequest)
redundantly in some cases, causing test flakes depending on timing and
up's weird state machine.

We only need to call StartLoginInteractive in the client if Start itself
doesn't. But Start doesn't tell us that. So cheat a bit and a put the
information about whether there's a current NodeKey in the ipn.Status.
It used to be accessible over LocalAPI via GetPrefs as a private key but
we removed that for security. But a bool is fine.

So then only call StartLoginInteractive if that bool is false and don't
do it in the WatchIPNBus loop.

Fixes #12028
Updates #12042

Change-Id: I0923c3f704a9d6afd825a858eb9a63ca7c1df294
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 10, 2024
@bradfitz
ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field
d4e53d7 
The previous LocalBackend & CLI 'up' changes improved some stuff, but
might've been too aggressive in some edge cases.

This simplifies the authURL vs authURLSticky distinction and removes
the interact field, which seemed to just just be about duplicate URL
suppression in IPN bus, back from when the IPN bus was a single client
at a time. This moves that suppression to a different spot.

Updates #12028
Updates #12042

Change-Id: I1f8800b1e82ccc1c8a0d7abba559e7404ddf41e4
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@bradfitz bradfitz mentioned this issue May 10, 2024
Merged
maisem pushed a commit that referenced this issue May 13, 2024
@bradfitz @maisem
ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field
50e3cb2 
The previous LocalBackend & CLI 'up' changes improved some stuff, but
might've been too aggressive in some edge cases.

This simplifies the authURL vs authURLSticky distinction and removes
the interact field, which seemed to just just be about duplicate URL
suppression in IPN bus, back from when the IPN bus was a single client
at a time. This moves that suppression to a different spot.

Updates #12028
Updates #12042

Change-Id: I1f8800b1e82ccc1c8a0d7abba559e7404ddf41e4
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 13, 2024
@bradfitz
ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field
fa67a5b 
The previous LocalBackend & CLI 'up' changes improved some stuff, but
might've been too aggressive in some edge cases.

This simplifies the authURL vs authURLSticky distinction and removes
the interact field, which seemed to just just be about duplicate URL
suppression in IPN bus, back from when the IPN bus was a single client
at a time. This moves that suppression to a different spot.

Updates #12028
Updates #12042

Change-Id: I1f8800b1e82ccc1c8a0d7abba559e7404ddf41e4
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 13, 2024
@bradfitz
ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field
fa0c2ea 
The previous LocalBackend & CLI 'up' changes improved some stuff, but
might've been too aggressive in some edge cases.

This simplifies the authURL vs authURLSticky distinction and removes
the interact field, which seemed to just just be about duplicate URL
suppression in IPN bus, back from when the IPN bus was a single client
at a time. This moves that suppression to a different spot.

Fixes #12119
Updates #12028
Updates #12042

Change-Id: I1f8800b1e82ccc1c8a0d7abba559e7404ddf41e4
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 14, 2024
@bradfitz
ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field
8aa5c35 
The previous LocalBackend & CLI 'up' changes improved some stuff, but
might've been too aggressive in some edge cases.

This simplifies the authURL vs authURLSticky distinction and removes
the interact field, which seemed to just just be about duplicate URL
suppression in IPN bus, back from when the IPN bus was a single client
at a time. This moves that suppression to a different spot.

Fixes #12119
Updates #12028
Updates #12042

Change-Id: I1f8800b1e82ccc1c8a0d7abba559e7404ddf41e4
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this issue May 14, 2024
@bradfitz
ipn/ipnlocal: simplify authURL vs authURLSticky, remove interact field
32cb8a3 
The previous LocalBackend & CLI 'up' changes improved some stuff, but
might've been too aggressive in some edge cases.

This simplifies the authURL vs authURLSticky distinction and removes
the interact field, which seemed to just just be about duplicate URL
suppression in IPN bus, back from when the IPN bus was a single client
at a time. This moves that suppression to a different spot.

Fixes #12119
Updates #12028
Updates #12042

Change-Id: I1f8800b1e82ccc1c8a0d7abba559e7404ddf41e4
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
(cherry picked from commit 8aa5c35)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bradfitz bradfitz

Labels
testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cmd/tailscale,controlclient,ipnlocal: fix 'up', deflake tests more tailscale/tailscale
1 participant
@bradfitz