From e4895abca15cf12f47d76cbe3a9cf2bc08d16a2d Mon Sep 17 00:00:00 2001 From: Alex Chan Date: Thu, 11 Sep 2025 09:30:14 +0100 Subject: [PATCH] ipn/ipnlocal: use named options in mockControl.send() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A lot of calls to this function go `send(nil, "", false, …)` which was unclear as a new reader of this code. Switching to a struct with named fields makes it easier to read, and highlights cases when we want something other than the default behaviour. Updates tailscale/corp#31478 --- ipn/ipnlocal/local_test.go | 6 +++--- ipn/ipnlocal/state_test.go | 44 +++++++++++++++++++++++--------------- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 7d1c452f3..79a11ac67 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -6123,7 +6123,7 @@ func TestLoginNotifications(t *testing.T) { t.Fatal(err) } - lb.cc.(*mockControl).send(nil, loginURL, false, nil) + lb.cc.(*mockControl).send(sendOptions{url: loginURL}, nil) var wg sync.WaitGroup wg.Add(len(sessions)) @@ -6788,7 +6788,7 @@ func TestSrcCapPacketFilter(t *testing.T) { must.Do(k.UnmarshalText([]byte("nodekey:5c8f86d5fc70d924e55f02446165a5dae8f822994ad26bcf4b08fd841f9bf261"))) controlClient := lb.cc.(*mockControl) - controlClient.send(nil, "", false, &netmap.NetworkMap{ + controlClient.send(sendOptions{}, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{ Addresses: []netip.Prefix{netip.MustParsePrefix("1.1.1.1/32")}, }).View(), @@ -6993,7 +6993,7 @@ func TestDisplayMessageIPNBus(t *testing.T) { cc := lb.cc.(*mockControl) // Assert that we are logged in and authorized, and also send our DisplayMessages - cc.send(nil, "", true, &netmap.NetworkMap{ + cc.send(sendOptions{loginFinished: true}, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), DisplayMessages: msgs, }) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 82d15e58b..70f390792 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -168,9 +168,19 @@ func (cc *mockControl) populateKeys() (newKeys bool) { return newKeys } +type sendOptions struct { + err error + url string + loginFinished bool +} + // send publishes a controlclient.Status notification upstream. // (In our tests here, upstream is the ipnlocal.Local instance.) -func (cc *mockControl) send(err error, url string, loginFinished bool, nm *netmap.NetworkMap) { +func (cc *mockControl) send(options sendOptions, nm *netmap.NetworkMap) { + err := options.err + url := options.url + loginFinished := options.loginFinished + if loginFinished { cc.mu.Lock() cc.authBlocked = false @@ -197,7 +207,7 @@ func (cc *mockControl) authenticated(nm *netmap.NetworkMap) { cc.persist.UserProfile = *selfUser.AsStruct() } cc.persist.NodeID = nm.SelfNode.StableID() - cc.send(nil, "", true, nm) + cc.send(sendOptions{loginFinished: true}, nm) } func (cc *mockControl) sendAuthURL(nm *netmap.NetworkMap) { @@ -455,7 +465,7 @@ func TestStateMachine(t *testing.T) { }, }) url1 := "https://localhost:1/1" - cc.send(nil, url1, false, nil) + cc.send(sendOptions{url: url1}, nil) { cc.assertCalls() @@ -508,7 +518,7 @@ func TestStateMachine(t *testing.T) { t.Logf("\n\nLogin2 (url response)") notifies.expect(1) url2 := "https://localhost:1/2" - cc.send(nil, url2, false, nil) + cc.send(sendOptions{url: url2}, nil) { cc.assertCalls() @@ -528,7 +538,7 @@ func TestStateMachine(t *testing.T) { notifies.expect(3) cc.persist.UserProfile.LoginName = "user1" cc.persist.NodeID = "node1" - cc.send(nil, "", true, &netmap.NetworkMap{}) + cc.send(sendOptions{loginFinished: true}, &netmap.NetworkMap{}) { nn := notifies.drain(3) // Arguably it makes sense to unpause now, since the machine @@ -557,7 +567,7 @@ func TestStateMachine(t *testing.T) { // but the current code is brittle. // (ie. I suspect it would be better to change false->true in send() // below, and do the same in the real controlclient.) - cc.send(nil, "", false, &netmap.NetworkMap{ + cc.send(sendOptions{}, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) { @@ -720,7 +730,7 @@ func TestStateMachine(t *testing.T) { // an interactive login URL to visit. notifies.expect(2) url3 := "https://localhost:1/3" - cc.send(nil, url3, false, nil) + cc.send(sendOptions{url: url3}, nil) { nn := notifies.drain(2) cc.assertCalls("Login") @@ -731,7 +741,7 @@ func TestStateMachine(t *testing.T) { notifies.expect(3) cc.persist.UserProfile.LoginName = "user2" cc.persist.NodeID = "node2" - cc.send(nil, "", true, &netmap.NetworkMap{ + cc.send(sendOptions{loginFinished: true}, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) t.Logf("\n\nLoginFinished3") @@ -801,7 +811,7 @@ func TestStateMachine(t *testing.T) { // the control server at all when stopped). t.Logf("\n\nStart4 -> netmap") notifies.expect(0) - cc.send(nil, "", true, &netmap.NetworkMap{ + cc.send(sendOptions{loginFinished: true}, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) { @@ -848,7 +858,7 @@ func TestStateMachine(t *testing.T) { notifies.expect(1) b.StartLoginInteractive(context.Background()) url4 := "https://localhost:1/4" - cc.send(nil, url4, false, nil) + cc.send(sendOptions{url: url4}, nil) { nn := notifies.drain(1) // It might seem like WantRunning should switch to true here, @@ -870,7 +880,7 @@ func TestStateMachine(t *testing.T) { notifies.expect(3) cc.persist.UserProfile.LoginName = "user3" cc.persist.NodeID = "node3" - cc.send(nil, "", true, &netmap.NetworkMap{ + cc.send(sendOptions{loginFinished: true}, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) { @@ -918,7 +928,7 @@ func TestStateMachine(t *testing.T) { // Control server accepts our valid key from before. t.Logf("\n\nLoginFinished5") notifies.expect(0) - cc.send(nil, "", true, &netmap.NetworkMap{ + cc.send(sendOptions{loginFinished: true}, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) { @@ -933,7 +943,7 @@ func TestStateMachine(t *testing.T) { } t.Logf("\n\nExpireKey") notifies.expect(1) - cc.send(nil, "", false, &netmap.NetworkMap{ + cc.send(sendOptions{}, &netmap.NetworkMap{ Expiry: time.Now().Add(-time.Minute), SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) @@ -948,7 +958,7 @@ func TestStateMachine(t *testing.T) { t.Logf("\n\nExtendKey") notifies.expect(1) - cc.send(nil, "", false, &netmap.NetworkMap{ + cc.send(sendOptions{}, &netmap.NetworkMap{ Expiry: time.Now().Add(time.Minute), SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) @@ -1086,7 +1096,7 @@ func TestWGEngineStatusRace(t *testing.T) { wantState(ipn.NeedsLogin) // Assert that we are logged in and authorized. - cc.send(nil, "", true, &netmap.NetworkMap{ + cc.send(sendOptions{loginFinished: true}, &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{MachineAuthorized: true}).View(), }) wantState(ipn.Starting) @@ -1365,7 +1375,7 @@ func TestEngineReconfigOnStateChange(t *testing.T) { mustDo(t)(lb.Start(ipn.Options{})) mustDo2(t)(lb.EditPrefs(connect)) cc().authenticated(node1) - cc().send(nil, "", false, &netmap.NetworkMap{ + cc().send(sendOptions{}, &netmap.NetworkMap{ Expiry: time.Now().Add(-time.Minute), }) }, @@ -1494,7 +1504,7 @@ func TestEngineReconfigOnStateChange(t *testing.T) { mustDo(t)(lb.Start(ipn.Options{})) mustDo2(t)(lb.EditPrefs(connect)) cc().authenticated(node1) - cc().send(nil, "", false, &netmap.NetworkMap{ + cc().send(sendOptions{}, &netmap.NetworkMap{ Expiry: time.Now().Add(-time.Minute), }) },