2021-04-10 21:56:18 -07:00
// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
package cli
import (
2021-04-17 20:35:20 -07:00
"bytes"
"encoding/json"
2021-04-10 21:56:18 -07:00
"flag"
2021-04-17 20:35:20 -07:00
"fmt"
"strings"
2021-04-10 21:56:18 -07:00
"testing"
2021-04-17 19:14:59 -07:00
"inet.af/netaddr"
2021-04-10 21:56:18 -07:00
"tailscale.com/ipn"
2021-04-17 20:35:20 -07:00
"tailscale.com/ipn/ipnstate"
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
"tailscale.com/types/logger"
2021-04-17 20:35:20 -07:00
"tailscale.com/types/preftype"
2021-04-10 21:56:18 -07:00
)
// Test that checkForAccidentalSettingReverts's updateMaskedPrefsFromUpFlag can handle
// all flags. This will panic if a new flag creeps in that's unhandled.
func TestUpdateMaskedPrefsFromUpFlag ( t * testing . T ) {
mp := new ( ipn . MaskedPrefs )
upFlagSet . VisitAll ( func ( f * flag . Flag ) {
updateMaskedPrefsFromUpFlag ( mp , f . Name )
} )
}
func TestCheckForAccidentalSettingReverts ( t * testing . T ) {
f := func ( flags ... string ) map [ string ] bool {
m := make ( map [ string ] bool )
for _ , f := range flags {
m [ f ] = true
}
return m
}
tests := [ ] struct {
name string
flagSet map [ string ] bool
curPrefs * ipn . Prefs
2021-04-21 15:47:59 -07:00
curUser string // os.Getenv("USER") on the client side
2021-04-10 21:56:18 -07:00
mp * ipn . MaskedPrefs
want string
} {
{
name : "bare_up_means_up" ,
flagSet : f ( ) ,
curPrefs : & ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-10 21:56:18 -07:00
WantRunning : false ,
Hostname : "foo" ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
WantRunning : true ,
} ,
WantRunningSet : true ,
} ,
want : "" ,
} ,
{
name : "losing_hostname" ,
flagSet : f ( "accept-dns" ) ,
curPrefs : & ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-10 21:56:18 -07:00
WantRunning : false ,
Hostname : "foo" ,
CorpDNS : true ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-10 21:56:18 -07:00
WantRunning : true ,
CorpDNS : true ,
} ,
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURLSet : true ,
2021-04-10 21:56:18 -07:00
WantRunningSet : true ,
CorpDNSSet : true ,
} ,
want : ` 'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --hostname is not specified but its default value of "" differs from current value "foo" ` ,
} ,
{
name : "hostname_changing_explicitly" ,
flagSet : f ( "hostname" ) ,
curPrefs : & ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-10 21:56:18 -07:00
WantRunning : false ,
Hostname : "foo" ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-10 21:56:18 -07:00
WantRunning : true ,
Hostname : "bar" ,
} ,
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURLSet : true ,
2021-04-10 21:56:18 -07:00
WantRunningSet : true ,
HostnameSet : true ,
} ,
want : "" ,
} ,
{
name : "hostname_changing_empty_explicitly" ,
flagSet : f ( "hostname" ) ,
curPrefs : & ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-10 21:56:18 -07:00
WantRunning : false ,
Hostname : "foo" ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-10 21:56:18 -07:00
WantRunning : true ,
Hostname : "" ,
} ,
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURLSet : true ,
2021-04-10 21:56:18 -07:00
WantRunningSet : true ,
HostnameSet : true ,
} ,
want : "" ,
} ,
2021-04-17 19:14:59 -07:00
{
name : "empty_slice_equals_nil_slice" ,
flagSet : f ( "hostname" ) ,
curPrefs : & ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-17 19:14:59 -07:00
AdvertiseRoutes : [ ] netaddr . IPPrefix { } ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURL : ipn . DefaultControlURL ,
2021-04-17 19:14:59 -07:00
AdvertiseRoutes : nil ,
} ,
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
ControlURLSet : true ,
} ,
want : "" ,
} ,
{
// Issue 1725: "tailscale up --authkey=..." (or other non-empty flags) works from
// a fresh server's initial prefs.
name : "up_with_default_prefs" ,
flagSet : f ( "authkey" ) ,
curPrefs : ipn . NewPrefs ( ) ,
mp : & ipn . MaskedPrefs {
Prefs : * defaultPrefsFromUpArgs ( t ) ,
WantRunningSet : true ,
2021-04-17 19:14:59 -07:00
} ,
want : "" ,
} ,
2021-04-21 15:47:59 -07:00
{
name : "implicit_operator_change" ,
flagSet : f ( "hostname" ) ,
curPrefs : & ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
OperatorUser : "alice" ,
} ,
curUser : "eve" ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
} ,
ControlURLSet : true ,
} ,
want : ` 'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --operator is not specified but its default value of "" differs from current value "alice" ` ,
} ,
{
name : "implicit_operator_matches_shell_user" ,
flagSet : f ( "hostname" ) ,
curPrefs : & ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
OperatorUser : "alice" ,
} ,
curUser : "alice" ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
} ,
ControlURLSet : true ,
} ,
want : "" ,
} ,
2021-04-21 21:41:13 -07:00
{
name : "error_advertised_routes_exit_node_removed" ,
flagSet : f ( "advertise-routes" ) ,
curPrefs : & ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
AdvertiseRoutes : [ ] netaddr . IPPrefix {
netaddr . MustParseIPPrefix ( "10.0.42.0/24" ) ,
netaddr . MustParseIPPrefix ( "0.0.0.0/0" ) ,
netaddr . MustParseIPPrefix ( "::/0" ) ,
} ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
AdvertiseRoutes : [ ] netaddr . IPPrefix {
netaddr . MustParseIPPrefix ( "10.0.42.0/24" ) ,
} ,
} ,
AdvertiseRoutesSet : true ,
} ,
want : "'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --advertise-exit-node flag not mentioned but currently advertised routes are an exit node" ,
} ,
{
name : "advertised_routes_exit_node_removed" ,
flagSet : f ( "advertise-routes" , "advertise-exit-node" ) ,
curPrefs : & ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
AdvertiseRoutes : [ ] netaddr . IPPrefix {
netaddr . MustParseIPPrefix ( "10.0.42.0/24" ) ,
netaddr . MustParseIPPrefix ( "0.0.0.0/0" ) ,
netaddr . MustParseIPPrefix ( "::/0" ) ,
} ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
AdvertiseRoutes : [ ] netaddr . IPPrefix {
netaddr . MustParseIPPrefix ( "10.0.42.0/24" ) ,
} ,
} ,
AdvertiseRoutesSet : true ,
} ,
want : "" ,
} ,
{
name : "advertised_routes_includes_the_0_routes" , // but no --advertise-exit-node
flagSet : f ( "advertise-routes" ) ,
curPrefs : & ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
AdvertiseRoutes : [ ] netaddr . IPPrefix {
netaddr . MustParseIPPrefix ( "10.0.42.0/24" ) ,
netaddr . MustParseIPPrefix ( "0.0.0.0/0" ) ,
netaddr . MustParseIPPrefix ( "::/0" ) ,
} ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
AdvertiseRoutes : [ ] netaddr . IPPrefix {
netaddr . MustParseIPPrefix ( "11.1.43.0/24" ) ,
netaddr . MustParseIPPrefix ( "0.0.0.0/0" ) ,
netaddr . MustParseIPPrefix ( "::/0" ) ,
} ,
} ,
AdvertiseRoutesSet : true ,
} ,
want : "" ,
} ,
{
name : "advertised_routes_includes_only_one_0_route" , // and no --advertise-exit-node
flagSet : f ( "advertise-routes" ) ,
curPrefs : & ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
AdvertiseRoutes : [ ] netaddr . IPPrefix {
netaddr . MustParseIPPrefix ( "10.0.42.0/24" ) ,
netaddr . MustParseIPPrefix ( "0.0.0.0/0" ) ,
netaddr . MustParseIPPrefix ( "::/0" ) ,
} ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
AdvertiseRoutes : [ ] netaddr . IPPrefix {
netaddr . MustParseIPPrefix ( "11.1.43.0/24" ) ,
netaddr . MustParseIPPrefix ( "0.0.0.0/0" ) ,
} ,
} ,
AdvertiseRoutesSet : true ,
} ,
want : "'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --advertise-exit-node flag not mentioned but currently advertised routes are an exit node" ,
} ,
2021-04-23 10:56:08 -07:00
{
name : "exit_node_clearing" , // Issue 1777
flagSet : f ( "exit-node" ) ,
curPrefs : & ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
ExitNodeID : "fooID" ,
} ,
mp : & ipn . MaskedPrefs {
Prefs : ipn . Prefs {
ControlURL : ipn . DefaultControlURL ,
ExitNodeIP : netaddr . IP { } ,
} ,
ExitNodeIPSet : true ,
} ,
want : "" ,
} ,
2021-04-10 21:56:18 -07:00
}
for _ , tt := range tests {
t . Run ( tt . name , func ( t * testing . T ) {
var got string
2021-04-21 15:47:59 -07:00
if err := checkForAccidentalSettingReverts ( tt . flagSet , tt . curPrefs , tt . mp , tt . curUser ) ; err != nil {
2021-04-10 21:56:18 -07:00
got = err . Error ( )
}
if got != tt . want {
t . Errorf ( "unexpected result\n got: %s\nwant: %s\n" , got , tt . want )
}
} )
}
}
2021-04-17 20:35:20 -07:00
ipn{,/ipnlocal}, cmd/tailscale/cli: don't check pref reverts on initial up
The ipn.NewPrefs func returns a populated ipn.Prefs for historical
reasons. It's not used or as important as it once was, but it hasn't
yet been removed. Meanwhile, it contains some default values that are
used on some platforms. Notably, for this bug (#1725), Windows/Mac use
its Prefs.RouteAll true value (to accept subnets), but Linux users
have always gotten a "false" value for that, because that's what
cmd/tailscale's CLI default flag is _for all operating systems_. That
meant that "tailscale up" was rightfully reporting that the user was
changing an implicit setting: RouteAll was changing from true with
false with the user explicitly saying so.
An obvious fix might be to change ipn.NewPrefs to return
Prefs.RouteAll == false on some platforms, but the logic is
complicated by darwin: we want RouteAll true on windows, android, ios,
and the GUI mac app, but not the CLI tailscaled-on-macOS mode. But
even if we used build tags (e.g. the "redo" build tag) to determine
what the default is, that then means we have duplicated and differing
"defaults" between both the CLI up flags and ipn.NewPrefs. Furthering
that complication didn't seem like a good idea.
So, changing the NewPrefs defaults is too invasive at this stage of
the release, as is removing the NewPrefs func entirely.
Instead, tweak slightly the semantics of the ipn.Prefs.ControlURL
field. This now defines that a ControlURL of the empty string means
both "we're uninitialized" and also "just use the default".
Then, once we have the "empty-string-means-unintialized" semantics,
use that to suppress "tailscale up"'s recent implicit-setting-revert
checking safety net, if we've never initialized Tailscale yet.
And update/add tests.
Fixes #1725
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2021-04-17 22:50:58 -07:00
func defaultPrefsFromUpArgs ( t testing . TB ) * ipn . Prefs {
upFlagSet . Parse ( nil ) // populates upArgs
if upFlagSet . Lookup ( "netfilter-mode" ) == nil && upArgs . netfilterMode == "" {
// This flag is not compiled on on-Linux platforms,
// but prefsFromUpArgs requires it be populated.
upArgs . netfilterMode = defaultNetfilterMode ( )
}
prefs , err := prefsFromUpArgs ( upArgs , logger . Discard , new ( ipnstate . Status ) , "linux" )
if err != nil {
t . Fatalf ( "defaultPrefsFromUpArgs: %v" , err )
}
prefs . WantRunning = true
return prefs
}
2021-04-17 20:35:20 -07:00
func TestPrefsFromUpArgs ( t * testing . T ) {
tests := [ ] struct {
name string
args upArgsT
goos string // runtime.GOOS; empty means linux
st * ipnstate . Status // or nil
want * ipn . Prefs
wantErr string
wantWarn string
} {
{
name : "zero" ,
goos : "windows" ,
args : upArgsT { } ,
want : & ipn . Prefs {
WantRunning : true ,
NoSNAT : true ,
NetfilterMode : preftype . NetfilterOn , // silly, but default from ipn.NewPref currently
} ,
} ,
{
name : "error_advertise_route_invalid_ip" ,
args : upArgsT {
advertiseRoutes : "foo" ,
} ,
wantErr : ` "foo" is not a valid IP address or CIDR prefix ` ,
} ,
{
name : "error_advertise_route_unmasked_bits" ,
args : upArgsT {
advertiseRoutes : "1.2.3.4/16" ,
} ,
wantErr : ` 1.2.3.4/16 has non-address bits set; expected 1.2.0.0/16 ` ,
} ,
{
name : "error_exit_node_bad_ip" ,
args : upArgsT {
exitNodeIP : "foo" ,
} ,
wantErr : ` invalid IP address "foo" for --exit-node: unable to parse IP ` ,
} ,
{
name : "error_exit_node_allow_lan_without_exit_node" ,
args : upArgsT {
exitNodeAllowLANAccess : true ,
} ,
wantErr : ` --exit-node-allow-lan-access can only be used with --exit-node ` ,
} ,
{
name : "error_tag_prefix" ,
args : upArgsT {
advertiseTags : "foo" ,
} ,
wantErr : ` tag: "foo": tags must start with 'tag:' ` ,
} ,
{
name : "error_long_hostname" ,
args : upArgsT {
hostname : strings . Repeat ( "a" , 300 ) ,
} ,
wantErr : ` hostname too long: 300 bytes (max 256) ` ,
} ,
{
name : "error_linux_netfilter_empty" ,
args : upArgsT {
netfilterMode : "" ,
} ,
wantErr : ` invalid value --netfilter-mode="" ` ,
} ,
{
name : "error_linux_netfilter_bogus" ,
args : upArgsT {
netfilterMode : "bogus" ,
} ,
wantErr : ` invalid value --netfilter-mode="bogus" ` ,
} ,
{
name : "error_exit_node_ip_is_self_ip" ,
args : upArgsT {
exitNodeIP : "100.105.106.107" ,
} ,
st : & ipnstate . Status {
TailscaleIPs : [ ] netaddr . IP { netaddr . MustParseIP ( "100.105.106.107" ) } ,
} ,
wantErr : ` cannot use 100.105.106.107 as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node? ` ,
} ,
{
name : "warn_linux_netfilter_nodivert" ,
goos : "linux" ,
args : upArgsT {
netfilterMode : "nodivert" ,
} ,
wantWarn : "netfilter=nodivert; add iptables calls to ts-* chains manually." ,
want : & ipn . Prefs {
WantRunning : true ,
NetfilterMode : preftype . NetfilterNoDivert ,
NoSNAT : true ,
} ,
} ,
{
name : "warn_linux_netfilter_off" ,
goos : "linux" ,
args : upArgsT {
netfilterMode : "off" ,
} ,
wantWarn : "netfilter=off; configure iptables yourself." ,
want : & ipn . Prefs {
WantRunning : true ,
NetfilterMode : preftype . NetfilterOff ,
NoSNAT : true ,
} ,
} ,
}
for _ , tt := range tests {
t . Run ( tt . name , func ( t * testing . T ) {
var warnBuf bytes . Buffer
warnf := func ( format string , a ... interface { } ) {
fmt . Fprintf ( & warnBuf , format , a ... )
}
goos := tt . goos
if goos == "" {
goos = "linux"
}
st := tt . st
if st == nil {
st = new ( ipnstate . Status )
}
got , err := prefsFromUpArgs ( tt . args , warnf , st , goos )
gotErr := fmt . Sprint ( err )
if tt . wantErr != "" {
if tt . wantErr != gotErr {
t . Errorf ( "wrong error.\n got error: %v\nwant error: %v\n" , gotErr , tt . wantErr )
}
return
}
if err != nil {
t . Fatal ( err )
}
if tt . want == nil {
t . Fatal ( "tt.want is nil" )
}
if ! got . Equals ( tt . want ) {
jgot , _ := json . MarshalIndent ( got , "" , "\t" )
jwant , _ := json . MarshalIndent ( tt . want , "" , "\t" )
if bytes . Equal ( jgot , jwant ) {
t . Logf ( "prefs differ only in non-JSON-visible ways (nil/non-nil zero-length arrays)" )
}
t . Errorf ( "wrong prefs\n got: %s\nwant: %s\n\ngot: %s\nwant: %s\n" ,
got . Pretty ( ) , tt . want . Pretty ( ) ,
jgot , jwant ,
)
}
} )
}
}