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

Fix staticcheck failures for pkg/proxy/... #81886

Merged
merged 1 commit into from Dec 24, 2019
Merged

Fix staticcheck failures for pkg/proxy/... #81886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 0 additions & 5 deletions hack/.staticcheck_failures
View file
Open in desktop
Expand Up @@ -17,11 +17,6 @@ pkg/kubelet/pluginmanager/operationexecutor
pkg/kubelet/pluginmanager/pluginwatcher
pkg/kubelet/remote
pkg/probe/http
pkg/proxy/healthcheck
pkg/proxy/iptables
pkg/proxy/userspace
pkg/proxy/winkernel
pkg/proxy/winuserspace
pkg/registry/autoscaling/horizontalpodautoscaler/storage
pkg/registry/core/namespace/storage
pkg/registry/core/persistentvolumeclaim/storage
Expand Down
3 changes: 1 addition & 2 deletions pkg/proxy/healthcheck/proxier_health.go
View file
Open in desktop
Expand Up @@ -52,7 +52,6 @@ type ProxierHealthServer struct {
clock clock.Clock

addr string
port int32
healthTimeout time.Duration
recorder record.EventRecorder
nodeRef *v1.ObjectReference
Expand Down Expand Up @@ -159,5 +158,5 @@ func (h healthzHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
lastUpdated = currentTime

}
fmt.Fprintf(resp, fmt.Sprintf(`{"lastUpdated": %q,"currentTime": %q}`, lastUpdated, currentTime))
fmt.Fprintf(resp, `{"lastUpdated": %q,"currentTime": %q}`, lastUpdated, currentTime)
}
2 changes: 1 addition & 1 deletion pkg/proxy/healthcheck/service_health.go
View file
Open in desktop
Expand Up @@ -163,7 +163,7 @@ func (h hcHandler) ServeHTTP(resp http.ResponseWriter, req *http.Request) {
} else {
resp.WriteHeader(http.StatusOK)
}
fmt.Fprintf(resp, strings.Trim(dedent.Dedent(fmt.Sprintf(`
fmt.Fprint(resp, strings.Trim(dedent.Dedent(fmt.Sprintf(`
{
"service": {
"namespace": %q,
Expand Down
15 changes: 6 additions & 9 deletions pkg/proxy/iptables/proxier.go
View file
Open in desktop
Expand Up @@ -727,14 +727,14 @@ func (proxier *Proxier) deleteEndpointConnections(connectionMap []proxy.ServiceE
const endpointChainsNumberThreshold = 1000

// Assumes proxier.mu is held.
func (proxier *Proxier) appendServiceCommentLocked(args []string, svcName string) {
func (proxier *Proxier) appendServiceCommentLocked(args []string, svcName string) []string {
// Not printing these comments, can reduce size of iptables (in case of large
// number of endpoints) even by 40%+. So if total number of endpoint chains
// is large enough, we simply drop those comments.
if proxier.endpointChainsNumber > endpointChainsNumberThreshold {
return
return args
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above suggests that not returning the args was an explicit decision. I'm not sure that re-adding them is the right choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need return args here, otherwise on L1187 in this file will replace args by nil - see #81886 (comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed in #81563

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we wait for that PR to merge?

}
args = append(args, "-m", "comment", "--comment", svcName)
return append(args, "-m", "comment", "--comment", svcName)
}

// This is where all of the iptables-save/restore calls happen.
Expand Down Expand Up @@ -1266,7 +1266,7 @@ func (proxier *Proxier) syncProxyRules() {
args = append(args[:0],
"-A", string(svcChain),
)
proxier.appendServiceCommentLocked(args, svcNameString)
args = proxier.appendServiceCommentLocked(args, svcNameString)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means that the return value of appendServiceCommentLocked was previously not used, so comments were never added. PR #81563 was also opened to address this, but because this was also caught by staticcheck I have also fixed it in this PR.

args = append(args,
"-m", "recent", "--name", string(endpointChain),
"--rcheck", "--seconds", strconv.Itoa(svcInfo.StickyMaxAgeSeconds()), "--reap",
Expand All @@ -1278,13 +1278,10 @@ func (proxier *Proxier) syncProxyRules() {

// Now write loadbalancing & DNAT rules.
n := len(endpointChains)
localEndpoints := make([]*endpointsInfo, 0)
localEndpointChains := make([]utiliptables.Chain, 0)
for i, endpointChain := range endpointChains {
// Write ingress loadbalancing & DNAT rules only for services that request OnlyLocal traffic.
if svcInfo.OnlyNodeLocalEndpoints() && endpoints[i].IsLocal {
// These slices parallel each other; must be kept in sync
localEndpoints = append(localEndpoints, endpoints[i])
localEndpointChains = append(localEndpointChains, endpointChains[i])
}

Expand All @@ -1296,7 +1293,7 @@ func (proxier *Proxier) syncProxyRules() {

// Balancing rules in the per-service chain.
args = append(args[:0], "-A", string(svcChain))
proxier.appendServiceCommentLocked(args, svcNameString)
args = proxier.appendServiceCommentLocked(args, svcNameString)
if i < (n - 1) {
// Each rule is a probabilistic match.
args = append(args,
Expand All @@ -1310,7 +1307,7 @@ func (proxier *Proxier) syncProxyRules() {

// Rules in the per-endpoint chain.
args = append(args[:0], "-A", string(endpointChain))
proxier.appendServiceCommentLocked(args, svcNameString)
args = proxier.appendServiceCommentLocked(args, svcNameString)
// Handle traffic that loops back to the originator with SNAT.
writeLine(proxier.natRules, append(args,
"-s", utilproxy.ToCIDR(net.ParseIP(epIP)),
Expand Down
18 changes: 9 additions & 9 deletions pkg/proxy/iptables/proxier_test.go
View file
Open in desktop
Expand Up @@ -2358,15 +2358,15 @@ COMMIT
-A KUBE-MARK-MASQ -j MARK --set-xmark
-A KUBE-SERVICES -m comment --comment "ns1/svc1: cluster IP" -m tcp -p tcp -d 172.20.1.1/32 --dport 0 ! -s 10.0.0.0/24 -j KUBE-MARK-MASQ
-A KUBE-SERVICES -m comment --comment "ns1/svc1: cluster IP" -m tcp -p tcp -d 172.20.1.1/32 --dport 0 -j KUBE-SVC-AHZNAGK3SCETOS2T
-A KUBE-SVC-AHZNAGK3SCETOS2T -m statistic --mode random --probability 0.3333333333 -j KUBE-SEP-PXD6POUVGD2I37UY
-A KUBE-SEP-PXD6POUVGD2I37UY -s 10.0.1.1/32 -j KUBE-MARK-MASQ
-A KUBE-SEP-PXD6POUVGD2I37UY -m tcp -p tcp -j DNAT --to-destination 10.0.1.1:80
-A KUBE-SVC-AHZNAGK3SCETOS2T -m statistic --mode random --probability 0.5000000000 -j KUBE-SEP-SOKZUIT7SCEVIP33
-A KUBE-SEP-SOKZUIT7SCEVIP33 -s 10.0.1.2/32 -j KUBE-MARK-MASQ
-A KUBE-SEP-SOKZUIT7SCEVIP33 -m tcp -p tcp -j DNAT --to-destination 10.0.1.2:80
-A KUBE-SVC-AHZNAGK3SCETOS2T -j KUBE-SEP-WVE3FAB34S7NZGDJ
-A KUBE-SEP-WVE3FAB34S7NZGDJ -s 10.0.1.3/32 -j KUBE-MARK-MASQ
-A KUBE-SEP-WVE3FAB34S7NZGDJ -m tcp -p tcp -j DNAT --to-destination 10.0.1.3:80
-A KUBE-SVC-AHZNAGK3SCETOS2T -m comment --comment ns1/svc1: -m statistic --mode random --probability 0.3333333333 -j KUBE-SEP-PXD6POUVGD2I37UY
-A KUBE-SEP-PXD6POUVGD2I37UY -m comment --comment ns1/svc1: -s 10.0.1.1/32 -j KUBE-MARK-MASQ
-A KUBE-SEP-PXD6POUVGD2I37UY -m comment --comment ns1/svc1: -m tcp -p tcp -j DNAT --to-destination 10.0.1.1:80
-A KUBE-SVC-AHZNAGK3SCETOS2T -m comment --comment ns1/svc1: -m statistic --mode random --probability 0.5000000000 -j KUBE-SEP-SOKZUIT7SCEVIP33
-A KUBE-SEP-SOKZUIT7SCEVIP33 -m comment --comment ns1/svc1: -s 10.0.1.2/32 -j KUBE-MARK-MASQ
-A KUBE-SEP-SOKZUIT7SCEVIP33 -m comment --comment ns1/svc1: -m tcp -p tcp -j DNAT --to-destination 10.0.1.2:80
-A KUBE-SVC-AHZNAGK3SCETOS2T -m comment --comment ns1/svc1: -j KUBE-SEP-WVE3FAB34S7NZGDJ
-A KUBE-SEP-WVE3FAB34S7NZGDJ -m comment --comment ns1/svc1: -s 10.0.1.3/32 -j KUBE-MARK-MASQ
-A KUBE-SEP-WVE3FAB34S7NZGDJ -m comment --comment ns1/svc1: -m tcp -p tcp -j DNAT --to-destination 10.0.1.3:80
-A KUBE-SERVICES -m comment --comment "kubernetes service nodeports; NOTE: this must be the last rule in this chain" -m addrtype --dst-type LOCAL -j KUBE-NODEPORTS
COMMIT
`
Expand Down
2 changes: 1 addition & 1 deletion pkg/proxy/userspace/proxysocket.go
View file
Open in desktop
Expand Up @@ -290,7 +290,7 @@ func (udp *udpProxySocket) proxyClient(cliAddr net.Addr, svrConn net.Conn, activ
klog.Errorf("SetDeadline failed: %v", err)
break
}
n, err = udp.WriteTo(buffer[0:n], cliAddr)
_, err = udp.WriteTo(buffer[0:n], cliAddr)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we ignoring this output? Is the linter complaining that it is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, from the commit message:

pkg/proxy/userspace/proxysocket.go:293:3: this value of n is never used (SA4006)

if err != nil {
if !logTimeout(err) {
klog.Errorf("WriteTo failed: %v", err)
Expand Down
1 change: 1 addition & 0 deletions pkg/proxy/winkernel/BUILD
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ go_library(
"//pkg/proxy/apis/config:go_default_library",
"//pkg/proxy/config:go_default_library",
"//pkg/proxy/healthcheck:go_default_library",
"//pkg/proxy/metrics:go_default_library",
"//pkg/util/async:go_default_library",
"//staging/src/k8s.io/api/core/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
Expand Down
11 changes: 0 additions & 11 deletions pkg/proxy/winkernel/metrics.go
View file
Open in desktop
Expand Up @@ -18,7 +18,6 @@ package winkernel

import (
"sync"
"time"

"k8s.io/component-base/metrics"
"k8s.io/component-base/metrics/legacyregistry"
Expand Down Expand Up @@ -69,13 +68,3 @@ func RegisterMetrics() {
legacyregistry.MustRegister(SyncProxyRulesLastTimestamp)
})
}

// Gets the time since the specified start in microseconds.
func sinceInMicroseconds(start time.Time) float64 {
return float64(time.Since(start).Nanoseconds() / time.Microsecond.Nanoseconds())
}

// Gets the time since the specified start in seconds.
func sinceInSeconds(start time.Time) float64 {
return time.Since(start).Seconds()
}
5 changes: 3 additions & 2 deletions pkg/proxy/winkernel/proxier.go
View file
Open in desktop
Expand Up @@ -47,6 +47,7 @@ import (
"k8s.io/kubernetes/pkg/proxy/apis/config"
proxyconfig "k8s.io/kubernetes/pkg/proxy/config"
"k8s.io/kubernetes/pkg/proxy/healthcheck"
"k8s.io/kubernetes/pkg/proxy/metrics"
"k8s.io/kubernetes/pkg/util/async"
)

Expand Down Expand Up @@ -1000,8 +1001,8 @@ func (proxier *Proxier) syncProxyRules() {

start := time.Now()
defer func() {
SyncProxyRulesLatency.Observe(sinceInSeconds(start))
DeprecatedSyncProxyRulesLatency.Observe(sinceInMicroseconds(start))
SyncProxyRulesLatency.Observe(metrics.SinceInSeconds(start))
DeprecatedSyncProxyRulesLatency.Observe(metrics.SinceInMicroseconds(start))
klog.V(4).Infof("syncProxyRules took %v", time.Since(start))
}()
// don't sync rules till we've received services and endpoints
Expand Down
23 changes: 0 additions & 23 deletions pkg/proxy/winuserspace/proxier.go
View file
Open in desktop
Expand Up @@ -91,8 +91,6 @@ type Proxier struct {
serviceMap map[ServicePortPortalName]*serviceInfo
syncPeriod time.Duration
udpIdleTimeout time.Duration
portMapMutex sync.Mutex
portMap map[portMapKey]*portMapValue
numProxyLoops int32 // use atomic ops to access this; mostly for testing
netsh netsh.Interface
hostIP net.IP
Expand All @@ -101,26 +99,6 @@ type Proxier struct {
// assert Proxier is a proxy.Provider
var _ proxy.Provider = &Proxier{}

// A key for the portMap. The ip has to be a string because slices can't be map
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these pieces removed because they are not used?

Copy link
Contributor Author

@praseodym praseodym Sep 25, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, from the commit message:

pkg/proxy/winuserspace/proxier.go:88:2: field portMapMutex is unused (U1000)
pkg/proxy/winuserspace/proxier.go:112:2: field owner is unused (U1000)
pkg/proxy/winuserspace/proxier.go:113:2: field socket is unused (U1000)

(exact line numbers are different due to rebase / other changes)

// keys.
type portMapKey struct {
ip string
port int
protocol v1.Protocol
}

func (k *portMapKey) String() string {
return fmt.Sprintf("%s/%s", net.JoinHostPort(k.ip, strconv.Itoa(k.port)), k.protocol)
}

// A value for the portMap
type portMapValue struct {
owner ServicePortPortalName
socket interface {
Close() error
}
}

var (
// ErrProxyOnLocalhost is returned by NewProxier if the user requests a proxier on
// the loopback address. May be checked for by callers of NewProxier to know whether
Expand Down Expand Up @@ -154,7 +132,6 @@ func createProxier(loadBalancer LoadBalancer, listenIP net.IP, netsh netsh.Inter
return &Proxier{
loadBalancer: loadBalancer,
serviceMap: make(map[ServicePortPortalName]*serviceInfo),
portMap: make(map[portMapKey]*portMapValue),
syncPeriod: syncPeriod,
udpIdleTimeout: udpIdleTimeout,
netsh: netsh,
Expand Down
2 changes: 1 addition & 1 deletion pkg/proxy/winuserspace/proxysocket.go
View file
Open in desktop
Expand Up @@ -617,7 +617,7 @@ func (udp *udpProxySocket) proxyClient(cliAddr net.Addr, svrConn net.Conn, activ
klog.Errorf("SetDeadline failed: %v", err)
break
}
n, err = udp.WriteTo(buffer[0:n], cliAddr)
_, err = udp.WriteTo(buffer[0:n], cliAddr)
if err != nil {
if !logTimeout(err) {
klog.Errorf("WriteTo failed: %v", err)
Expand Down