From 8185bee00be0e2ef4ea3ea0088fca496fd5b32d2 Mon Sep 17 00:00:00 2001 From: soypete Date: Mon, 13 Jun 2022 16:54:16 -0600 Subject: [PATCH] cmd/tailscale/cli: remove apply implicit prefs and maintain feature pairity; fixed failing tests Signed-off-by: soypete Signed-off-by: soypete --- cmd/tailscale/cli/cli_test.go | 144 +++++++--------------------------- cmd/tailscale/cli/up.go | 19 ++--- 2 files changed, 33 insertions(+), 130 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 1ada094fc..30302eadf 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -494,7 +494,7 @@ func TestCheckForAccidentalSettingReverts(t *testing.T) { if err != nil { t.Fatal(err) } - applyImplicitPrefs(newPrefs, tt.curPrefs, tt.curUser) + // applyImplicitPrefs(newPrefs, tt.curPrefs, tt.curUser) var got string if err := checkForAccidentalSettingReverts(newPrefs, tt.curPrefs, upCheckEnv{ goos: goos, @@ -779,6 +779,10 @@ func TestFlagAppliesToOS(t *testing.T) { } func TestUpdatePrefs(t *testing.T) { + os.Setenv("USER", "test_user") + t.Cleanup(func() { + os.Unsetenv("USER") + }) tests := []struct { name string flags []string // argv to be parsed into env.flagSet and env.upArgs @@ -871,7 +875,10 @@ func TestUpdatePrefs(t *testing.T) { CorpDNS: true, NetfilterMode: preftype.NetfilterOn, }, - env: upCheckEnv{backendState: "Running"}, + env: upCheckEnv{ + backendState: "Running", + user: os.Getenv("USER"), + }, wantSimpleUp: true, wantJustEditMP: &ipn.MaskedPrefs{WantRunningSet: true}, wantErrSubtr: "can't change --login-server without --force-reauth", @@ -886,34 +893,29 @@ func TestUpdatePrefs(t *testing.T) { CorpDNS: true, NetfilterMode: preftype.NetfilterOn, }, - env: upCheckEnv{backendState: "Running"}, + env: upCheckEnv{ + backendState: "Running", + user: os.Getenv("USER"), + }, }, { name: "operator_user_force_blank", flags: []string{"--operator="}, curPrefs: &ipn.Prefs{ - ControlURL: ipn.DefaultControlURL, - Persist: &persist.Persist{LoginName: "crawshaw.github"}, - OperatorUser: os.Getenv("USER"), + ControlURL: ipn.DefaultControlURL, + Persist: &persist.Persist{LoginName: "crawshaw.github"}, + OperatorUser: os.Getenv("USER"), + AllowSingleHosts: true, + CorpDNS: true, + NetfilterMode: preftype.NetfilterOn, + }, + env: upCheckEnv{ + backendState: "Running", + user: os.Getenv("USER"), }, - env: upCheckEnv{backendState: "Running"}, wantJustEditMP: &ipn.MaskedPrefs{ - AdvertiseRoutesSet: true, - AdvertiseTagsSet: true, - AllowSingleHostsSet: true, - ControlURLSet: true, - CorpDNSSet: true, - ExitNodeAllowLANAccessSet: true, - ExitNodeIDSet: true, - ExitNodeIPSet: true, - HostnameSet: true, - NetfilterModeSet: true, - NoSNATSet: true, - OperatorUserSet: true, - RouteAllSet: true, - RunSSHSet: true, - ShieldsUpSet: true, - WantRunningSet: true, + WantRunningSet: true, + OperatorUserSet: true, }, }, } @@ -943,113 +945,21 @@ func TestUpdatePrefs(t *testing.T) { if simpleUp != tt.wantSimpleUp { t.Fatalf("simpleUp=%v, want %v", simpleUp, tt.wantSimpleUp) } - var oldEditPrefs ipn.Prefs if justEditMP != nil { - oldEditPrefs = justEditMP.Prefs if tt.wantOperatorChange { if tt.curPrefs.OperatorUser == justEditMP.Prefs.OperatorUser { - t.Logf("current Operator User: %s, changes operator user: %s", tt.curPrefs.OperatorUser, justEditMP.OperatorUser) + t.Fatalf("current Operator User: %s, changes operator user: %s\n", tt.curPrefs.OperatorUser, justEditMP.OperatorUser) } } justEditMP.Prefs = ipn.Prefs{} // uninteresting } if !reflect.DeepEqual(justEditMP, tt.wantJustEditMP) { - t.Logf("justEditMP != wantJustEditMP; following diff omits the Prefs field, which was %+v", oldEditPrefs) - t.Fatalf("justEditMP: %v\n\n: ", cmp.Diff(justEditMP, tt.wantJustEditMP, cmpIP)) + t.Fatalf("justEditMP != wantJustEditMP; following diff omits the Prefs field, which was %+v\n", justEditMP) } }) } } -// func TestOperatorEnv (t *testing.T){ -// tests := []struct { -// name string -// flags []string // argv to be parsed into env.flagSet and env.upArgs -// curPrefs *ipn.Prefs -// env upCheckEnv // empty goos means "linux" - -// wantJustEditMP *ipn.MaskedPrefs -// wantErrSubtr string -// }{ -// { -// name: "operator_user_ank", -// flags: []string{"--operator="}, -// curPrefs: &ipn.Prefs{ -// ControlURL: ipn.DefaultControlURL, -// Persist: &persist.Persist{LoginName: "crawshaw.github"}, -// }, -// }, -// { -// name: "operator_user_force_blank", -// flags: []string{"--operator="}, -// curPrefs: &ipn.Prefs{ -// ControlURL: ipn.DefaultControlURL, -// Persist: &persist.Persist{LoginName: "crawshaw.github"}, -// }, -// }, -// { -// name: "operator_user_reset", -// flags: []string{"--reset"}, -// curPrefs: &ipn.Prefs{ -// ControlURL: ipn.DefaultControlURL, -// Persist: &persist.Persist{LoginName: "crawshaw.github"}, -// }, -// wantJustEditMP: &ipn.MaskedPrefs{ -// AdvertiseRoutesSet: true, -// AdvertiseTagsSet: true, -// AllowSingleHostsSet: true, -// ControlURLSet: true, -// CorpDNSSet: true, -// ExitNodeAllowLANAccessSet: true, -// ExitNodeIDSet: true, -// ExitNodeIPSet: true, -// HostnameSet: true, -// NetfilterModeSet: true, -// NoSNATSet: true, -// OperatorUserSet: true, -// RouteAllSet: true, -// RunSSHSet: true, -// ShieldsUpSet: true, -// WantRunningSet: true, -// }, -// }, -// } -// for _, tt := range tests { -// t.Run(tt.name, func(t *testing.T) { -// if tt.env.goos == "" { -// tt.env.goos = "linux" -// } -// tt.env.flagSet = newUpFlagSet(tt.env.goos, &tt.env.upArgs) -// flags := CleanUpArgs(tt.flags) -// tt.env.flagSet.Parse(flags) - -// newPrefs, err := prefsFromUpArgs(tt.env.upArgs, t.Logf, new(ipnstate.Status), tt.env.goos) -// if err != nil { -// t.Fatal(err) -// } -// _, justEditMP, err := updatePrefs(newPrefs, tt.curPrefs, tt.env) -// if err != nil { -// if tt.wantErrSubtr != "" { -// if !strings.Contains(err.Error(), tt.wantErrSubtr) { -// t.Fatalf("want error %q, got: %v", tt.wantErrSubtr, err) -// } -// return -// } -// t.Fatal(err) -// } -// var oldEditPrefs ipn.Prefs -// if justEditMP != nil { -// oldEditPrefs = justEditMP.Prefs -// justEditMP.Prefs = ipn.Prefs{} // uninteresting -// } -// if !reflect.DeepEqual(justEditMP, tt.wantJustEditMP) { -// t.Logf("justEditMP != wantJustEditMP; following diff omits the Prefs field, which was %+v", oldEditPrefs) -// t.Fatalf("justEditMP: %v\n\n: ", cmp.Diff(justEditMP, tt.wantJustEditMP, cmpIP)) -// } -// }) -// } -// } - var cmpIP = cmp.Comparer(func(a, b netaddr.IP) bool { return a == b }) diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 6955fdb42..df2f573b3 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -362,8 +362,6 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo // without changing any settings. func updatePrefs(prefs, curPrefs *ipn.Prefs, env upCheckEnv) (simpleUp bool, justEditMP *ipn.MaskedPrefs, err error) { if !env.upArgs.reset { - // applyImplicitPrefs(prefs, curPrefs, env.user) - if err := checkForAccidentalSettingReverts(prefs, curPrefs, env); err != nil { return false, nil, err } @@ -818,8 +816,14 @@ func checkForAccidentalSettingReverts(newPrefs, curPrefs *ipn.Prefs, env upCheck // Issue 3176. Old prefs had 'RouteAll: true' on disk, so ignore that. continue } + if flagName == "operator" { + // Issue 3808 -- this is os.Getenv by default so always explicitly set. + continue + } missing = append(missing, fmtFlagValueArg(flagName, valCur)) } + + // this is the last non-error return if len(missing) == 0 { return nil } @@ -853,17 +857,6 @@ type isBool interface { return errors.New(sb.String()) } -// applyImplicitPrefs mutates prefs to add implicit preferences. Currently -// this is just the operator user, which only needs to be set if it doesn't -// match the current user. -// -// curUser is os.getenv("user"). It's pulled out for testability. -func applyImplicitPrefs(prefs, oldPrefs *ipn.Prefs, curUser string) { - if prefs.OperatorUser == "" && oldPrefs.OperatorUser == curUser { - prefs.OperatorUser = oldPrefs.OperatorUser - } -} - func flagAppliesToOS(flag, goos string) bool { switch flag { case "netfilter-mode", "snat-subnet-routes":