Skip to content

Commit

Permalink
ci: add more lints (tailscale#7909)
Browse files Browse the repository at this point in the history 
This is a follow-up to tailscale#7905 that adds two more linters and fixes the corresponding findings. As per the previous PR, this only flags things that are "obviously" wrong, and fixes the issues found.

Signed-off-by: Andrew Dunham <[email protected]>
Change-Id: I8739bdb7bc4f75666a7385a7a26d56ec13741b7c
  • Loading branch information
@andrew-d
andrew-d authored Apr 20, 2023
1 parent 5acc7c4 commit f85dc6f
Show file tree
Hide file tree
Showing 9 changed files with 63 additions and 40 deletions.
28 changes: 28 additions & 0 deletions .golangci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ linters:
# enable in the list below.
disable-all: true
enable:
- bidichk
- gofmt
- goimports
- misspell
- revive

# Configuration for how we run golangci-lint
run:
Expand All @@ -23,6 +25,9 @@ issues:

# Per-linter settings are contained in this top-level key
linters-settings:
# Enable all rules by default; we don't use invisible unicode runes.
bidichk:

gofmt:
rewrite-rules:
- pattern: 'interface{}'
Expand All @@ -31,3 +36,26 @@ linters-settings:
goimports:

misspell:

revive:
enable-all-rules: false
ignore-generated-header: true
rules:
- name: atomic
- name: context-keys-type
- name: defer
arguments: [[
# Calling 'recover' at the time a defer is registered (i.e. "defer recover()") has no effect.
"immediate-recover",
# Calling 'recover' outside of a deferred function has no effect
"recover",
# Returning values from a deferred function has no effect
"return",
]]
- name: duplicated-imports
- name: errorf
- name: string-of-int
- name: time-equal
- name: unconditional-recursion
- name: useless-break
- name: waitgroup-by-value
5 changes: 2 additions & 3 deletions cmd/k8s-operator/operator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
"go.uber.org/zap"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -670,11 +669,11 @@ func expectedSTS(stsName, secretName, hostname string) *appsv1.StatefulSet {
},
},
},
Containers: []v1.Container{
Containers: []corev1.Container{
{
Name: "tailscale",
Image: "tailscale/tailscale",
Env: []v1.EnvVar{
Env: []corev1.EnvVar{
{Name: "TS_USERSPACE", Value: "false"},
{Name: "TS_AUTH_ONCE", Value: "true"},
{Name: "TS_DEST_IP", Value: "10.20.30.40"},
Expand Down
1 change: 0 additions & 1 deletion cmd/tailscale/cli/netcheck.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ func printReport(dm *tailcfg.DERPMap, report *netcheck.Report) error {
var err error
switch netcheckArgs.format {
case "":
break
case "json":
j, err = json.MarshalIndent(report, "", "\t")
case "json-line":
Expand Down
2 changes: 1 addition & 1 deletion hostinfo/hostinfo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ func DisabledEtcAptSource() bool {
return false
}
mod := fi.ModTime()
if c, ok := etcAptSrcCache.Load().(etcAptSrcResult); ok && c.mod == mod {
if c, ok := etcAptSrcCache.Load().(etcAptSrcResult); ok && c.mod.Equal(mod) {
return c.disabled
}
f, err := os.Open(path)
Expand Down
4 changes: 2 additions & 2 deletions ipn/ipnlocal/expiry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ func TestNextPeerExpiry(t *testing.T) {
em := newExpiryManager(t.Logf)
em.timeNow = func() time.Time { return now }
got := em.nextPeerExpiry(tt.netmap, now)
if got != tt.want {
if !got.Equal(tt.want) {
t.Errorf("got %q, want %q", got.Format(time.RFC3339), tt.want.Format(time.RFC3339))
} else if !got.IsZero() && got.Before(now) {
t.Errorf("unexpectedly got expiry %q before now %q", got.Format(time.RFC3339), now.Format(time.RFC3339))
Expand All @@ -269,7 +269,7 @@ func TestNextPeerExpiry(t *testing.T) {
}
got := em.nextPeerExpiry(nm, now)
want := now.Add(30 * time.Second)
if got != want {
if !got.Equal(want) {
t.Errorf("got %q, want %q", got.Format(time.RFC3339), want.Format(time.RFC3339))
}
})
Expand Down
2 changes: 1 addition & 1 deletion ipn/ipnlocal/local.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ func (b *LocalBackend) SetComponentDebugLogging(component string, until time.Tim
// unchanged when the timer actually fires.
b.mu.Lock()
defer b.mu.Unlock()
if ls := b.componentLogUntil[component]; ls.until == until {
if ls := b.componentLogUntil[component]; ls.until.Equal(until) {
setEnabled(false)
b.logf("debugging logging for component %q disabled (by timer)", component)
}
Expand Down
13 changes: 6 additions & 7 deletions ipn/ipnlocal/serve.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"net/url"
"os"
"path"
pathpkg "path"
"strconv"
"strings"
"sync"
Expand Down Expand Up @@ -420,19 +419,19 @@ func (b *LocalBackend) getServeHandler(r *http.Request) (_ ipn.HTTPHandlerView,
if h, ok := wsc.Handlers().GetOk(r.URL.Path); ok {
return h, r.URL.Path, true
}
path := path.Clean(r.URL.Path)
pth := path.Clean(r.URL.Path)
for {
withSlash := path + "/"
withSlash := pth + "/"
if h, ok := wsc.Handlers().GetOk(withSlash); ok {
return h, withSlash, true
}
if h, ok := wsc.Handlers().GetOk(path); ok {
return h, path, true
if h, ok := wsc.Handlers().GetOk(pth); ok {
return h, pth, true
}
if path == "/" {
if pth == "/" {
return z, "", false
}
path = pathpkg.Dir(path)
pth = path.Dir(pth)
}
}

Expand Down
47 changes: 23 additions & 24 deletions net/dns/resolver/tsdns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"time"

miekdns "github.com/miekg/dns"
"golang.org/x/net/dns/dnsmessage"
dns "golang.org/x/net/dns/dnsmessage"
"tailscale.com/net/netaddr"
"tailscale.com/net/tsdial"
Expand Down Expand Up @@ -1121,15 +1120,15 @@ func TestHandleExitNodeDNSQueryWithNetPkg(t *testing.T) {

t.Run("no_such_host", func(t *testing.T) {
res, err := handleExitNodeDNSQueryWithNetPkg(context.Background(), t.Logf, backResolver, &response{
Header: dnsmessage.Header{
Header: dns.Header{
ID: 123,
Response: true,
OpCode: 0, // query
},
Question: dnsmessage.Question{
Name: dnsmessage.MustNewName("nx-domain.test."),
Type: dnsmessage.TypeA,
Class: dnsmessage.ClassINET,
Question: dns.Question{
Name: dns.MustNewName("nx-domain.test."),
Type: dns.TypeA,
Class: dns.ClassINET,
},
})
if err != nil {
Expand All @@ -1156,74 +1155,74 @@ func TestHandleExitNodeDNSQueryWithNetPkg(t *testing.T) {
}

tests := []struct {
Type dnsmessage.Type
Type dns.Type
Name string
Check func(t testing.TB, got []byte)
}{
{
Type: dnsmessage.TypeA,
Type: dns.TypeA,
Name: "one-a.test.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x01\x00\x00\x00\x00\x05one-a\x04test\x00\x00\x01\x00\x01\x05one-a\x04test\x00\x00\x01\x00\x01\x00\x00\x02X\x00\x04\x01\x02\x03\x04"),
},
{
Type: dnsmessage.TypeA,
Type: dns.TypeA,
Name: "two-a.test.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x02\x00\x00\x00\x00\x05two-a\x04test\x00\x00\x01\x00\x01\xc0\f\x00\x01\x00\x01\x00\x00\x02X\x00\x04\x01\x02\x03\x04\xc0\f\x00\x01\x00\x01\x00\x00\x02X\x00\x04\x05\x06\a\b"),
},
{
Type: dnsmessage.TypeAAAA,
Type: dns.TypeAAAA,
Name: "one-aaaa.test.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x01\x00\x00\x00\x00\bone-aaaa\x04test\x00\x00\x1c\x00\x01\bone-aaaa\x04test\x00\x00\x1c\x00\x01\x00\x00\x02X\x00\x10\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02"),
},
{
Type: dnsmessage.TypeAAAA,
Type: dns.TypeAAAA,
Name: "two-aaaa.test.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x02\x00\x00\x00\x00\btwo-aaaa\x04test\x00\x00\x1c\x00\x01\xc0\f\x00\x1c\x00\x01\x00\x00\x02X\x00\x10\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\xc0\f\x00\x1c\x00\x01\x00\x00\x02X\x00\x10\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x04"),
},
{
Type: dnsmessage.TypePTR,
Type: dns.TypePTR,
Name: "4.3.2.1.in-addr.arpa.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x01\x00\x00\x00\x00\x014\x013\x012\x011\ain-addr\x04arpa\x00\x00\f\x00\x01\x014\x013\x012\x011\ain-addr\x04arpa\x00\x00\f\x00\x01\x00\x00\x02X\x00\t\x03foo\x03com\x00"),
},
{
Type: dnsmessage.TypeCNAME,
Type: dns.TypeCNAME,
Name: "cname.test.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x01\x00\x00\x00\x00\x05cname\x04test\x00\x00\x05\x00\x01\x05cname\x04test\x00\x00\x05\x00\x01\x00\x00\x02X\x00\x10\nthe-target\x03foo\x00"),
},

// No records of various types
{
Type: dnsmessage.TypeA,
Type: dns.TypeA,
Name: "no-records.test.",
Check: matchPacked("\x00{\x84\x03\x00\x01\x00\x00\x00\x00\x00\x00\nno-records\x04test\x00\x00\x01\x00\x01"),
},
{
Type: dnsmessage.TypeAAAA,
Type: dns.TypeAAAA,
Name: "no-records.test.",
Check: matchPacked("\x00{\x84\x03\x00\x01\x00\x00\x00\x00\x00\x00\nno-records\x04test\x00\x00\x1c\x00\x01"),
},
{
Type: dnsmessage.TypeCNAME,
Type: dns.TypeCNAME,
Name: "no-records.test.",
Check: matchPacked("\x00{\x84\x03\x00\x01\x00\x00\x00\x00\x00\x00\nno-records\x04test\x00\x00\x05\x00\x01"),
},
{
Type: dnsmessage.TypeSRV,
Type: dns.TypeSRV,
Name: "no-records.test.",
Check: matchPacked("\x00{\x84\x03\x00\x01\x00\x00\x00\x00\x00\x00\nno-records\x04test\x00\x00!\x00\x01"),
},
{
Type: dnsmessage.TypeTXT,
Type: dns.TypeTXT,
Name: "txt.test.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x03\x00\x00\x00\x00\x03txt\x04test\x00\x00\x10\x00\x01\x03txt\x04test\x00\x00\x10\x00\x01\x00\x00\x02X\x00\t\btxt1=one\x03txt\x04test\x00\x00\x10\x00\x01\x00\x00\x02X\x00\t\btxt2=two\x03txt\x04test\x00\x00\x10\x00\x01\x00\x00\x02X\x00\v\ntxt3=three"),
},
{
Type: dnsmessage.TypeSRV,
Type: dns.TypeSRV,
Name: "srv.test.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x02\x00\x00\x00\x00\x03srv\x04test\x00\x00!\x00\x01\x03srv\x04test\x00\x00!\x00\x01\x00\x00\x02X\x00\x0f\x00\x01\x00\x02\x00\x03\x03foo\x03com\x00\x03srv\x04test\x00\x00!\x00\x01\x00\x00\x02X\x00\x0f\x00\x04\x00\x05\x00\x06\x03bar\x03com\x00"),
},
{
Type: dnsmessage.TypeNS,
Type: dns.TypeNS,
Name: "ns.test.",
Check: matchPacked("\x00{\x84\x00\x00\x01\x00\x02\x00\x00\x00\x00\x02ns\x04test\x00\x00\x02\x00\x01\x02ns\x04test\x00\x00\x02\x00\x01\x00\x00\x02X\x00\t\x03ns1\x03foo\x00\x02ns\x04test\x00\x00\x02\x00\x01\x00\x00\x02X\x00\t\x03ns2\x03bar\x00"),
},
Expand All @@ -1232,15 +1231,15 @@ func TestHandleExitNodeDNSQueryWithNetPkg(t *testing.T) {
for _, tt := range tests {
t.Run(fmt.Sprintf("%v_%v", tt.Type, strings.Trim(tt.Name, ".")), func(t *testing.T) {
got, err := handleExitNodeDNSQueryWithNetPkg(context.Background(), t.Logf, backResolver, &response{
Header: dnsmessage.Header{
Header: dns.Header{
ID: 123,
Response: true,
OpCode: 0, // query
},
Question: dnsmessage.Question{
Name: dnsmessage.MustNewName(tt.Name),
Question: dns.Question{
Name: dns.MustNewName(tt.Name),
Type: tt.Type,
Class: dnsmessage.ClassINET,
Class: dns.ClassINET,
},
})
if err != nil {
Expand Down
1 change: 0 additions & 1 deletion wgengine/magicsock/magicsock.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3160,7 +3160,6 @@ func (c *Conn) goroutinesRunningLocked() bool {
if c.activeDerp != nil {
select {
case <-c.derpStarted:
break
default:
return true
}
Expand Down

0 comments on commit f85dc6f

Please sign in to comment.