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.

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

Small code enhancements #3712

Merged
merged 3 commits into from
Aug 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 30 additions & 0 deletions .golangci.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
[linters-settings]

[linters-settings.govet]
check-shadowing = true

[linters-settings.golint]
min-confidence = 0.0

[linters-settings.gocyclo]
min-complexity = 22.0

[linters-settings.maligned]
suggest-new = true

[linters-settings.goconst]
min-len = 2.0
min-occurrences = 2.0

[linters-settings.misspell]
locale = "US"

[linters]
enable-all = true
disable = [
"maligned",
"lll",
"gas",
"dupl",
"prealloc"
]
2 changes: 1 addition & 1 deletion cluster/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ func (s *datastoreTransaction) Commit(object Object) error {
}
err = s.kv.StoreConfig(s.Datastore.meta)
if err != nil {
return fmt.Errorf("StoreConfig error: %s", err)
return fmt.Errorf("storeConfig error: %s", err)
}

err = s.remoteLock.Unlock()
Expand Down
2 changes: 1 addition & 1 deletion cmd/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"syscall"
)

// ContextWithSignal create a context cancelled when SIGINT or SIGTERM are notified
Copy link
Contributor

@dduportal dduportal Aug 2, 2018

Choose a reason for hiding this comment

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

So we target 🇺🇸English instead of 🇬🇧 English right? (I see the same for behavior also)

Not a blocking thing nor to be changed. Just wanted to check :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes exactly

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// ContextWithSignal creates a context canceled when SIGINT or SIGTERM are notified
func ContextWithSignal(ctx context.Context) context.Context {
newCtx, cancel := context.WithCancel(ctx)
signals := make(chan os.Signal)
Expand Down
8 changes: 5 additions & 3 deletions cmd/traefik/traefik.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,9 @@ func runCmd(globalConfiguration *configuration.GlobalConfiguration, configFile s

http.DefaultTransport.(*http.Transport).Proxy = http.ProxyFromEnvironment

roundrobin.SetDefaultWeight(0)
if err := roundrobin.SetDefaultWeight(0); err != nil {
log.Error(err)
}

globalConfiguration.SetEffectiveConfiguration(configFile)
globalConfiguration.ValidateConfiguration()
Expand All @@ -182,8 +184,8 @@ func runCmd(globalConfiguration *configuration.GlobalConfiguration, configFile s

acmeprovider := globalConfiguration.InitACMEProvider()
if acmeprovider != nil {
err := providerAggregator.AddProvider(acmeprovider)
if err != nil {

if err := providerAggregator.AddProvider(acmeprovider); err != nil {
log.Errorf("Error initializing provider ACME: %v", err)
acmeprovider = nil
}
Expand Down
4 changes: 3 additions & 1 deletion configuration/router/internal_router.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ func (wm *WithMiddleware) AddRoutes(systemRouter *mux.Router) {
wm.router.AddRoutes(realRouter)

if len(wm.routerMiddlewares) > 0 {
realRouter.Walk(wrapRoute(wm.routerMiddlewares))
if err := realRouter.Walk(wrapRoute(wm.routerMiddlewares)); err != nil {
log.Error(err)
}
}
}

Expand Down
26 changes: 21 additions & 5 deletions configuration/router/internal_router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/containous/traefik/acme"
"github.com/containous/traefik/api"
"github.com/containous/traefik/configuration"
"github.com/containous/traefik/log"
"github.com/containous/traefik/ping"
acmeprovider "github.com/containous/traefik/provider/acme"
"github.com/containous/traefik/safe"
Expand Down Expand Up @@ -104,20 +105,35 @@ func TestWithMiddleware(t *testing.T) {
router := WithMiddleware{
router: MockInternalRouterFunc(func(systemRouter *mux.Router) {
systemRouter.Handle("/test", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte("router"))

if _, err := w.Write([]byte("router")); err != nil {
log.Error(err)
}
}))
}),
routerMiddlewares: []negroni.Handler{
negroni.HandlerFunc(func(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
rw.Write([]byte("before middleware1|"))
if _, err := rw.Write([]byte("before middleware1|")); err != nil {
log.Error(err)
}

next.ServeHTTP(rw, r)
rw.Write([]byte("|after middleware1"))

if _, err := rw.Write([]byte("|after middleware1")); err != nil {
log.Error(err)
}

}),
negroni.HandlerFunc(func(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) {
rw.Write([]byte("before middleware2|"))
if _, err := rw.Write([]byte("before middleware2|")); err != nil {
log.Error(err)
}

next.ServeHTTP(rw, r)
rw.Write([]byte("|after middleware2"))

if _, err := rw.Write([]byte("|after middleware2")); err != nil {
log.Error(err)
}
}),
},
}
Expand Down
2 changes: 1 addition & 1 deletion docs/configuration/acme.md
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ It is not possible to request a double wildcard certificate for a domain (for ex
Due to ACME limitation it is not possible to define wildcards in SANs (alternative domains). Thus, the wildcard domain has to be defined as a main domain.
Most likely the root domain should receive a certificate too, so it needs to be specified as SAN and 2 `DNS-01` challenges are executed.
In this case the generated DNS TXT record for both domains is the same.
Eventhough this behaviour is [DNS RFC](https://community.letsencrypt.org/t/wildcard-issuance-two-txt-records-for-the-same-name/54528/2) compliant, it can lead to problems as all DNS providers keep DNS records cached for a certain time (TTL) and this TTL can be superior to the challenge timeout making the `DNS-01` challenge fail.
Eventhough this behavior is [DNS RFC](https://community.letsencrypt.org/t/wildcard-issuance-two-txt-records-for-the-same-name/54528/2) compliant, it can lead to problems as all DNS providers keep DNS records cached for a certain time (TTL) and this TTL can be superior to the challenge timeout making the `DNS-01` challenge fail.
The Træfik ACME client library [LEGO](https://github.com/xenolf/lego) supports some but not all DNS providers to work around this issue.
The [`provider` table](/configuration/acme/#provider) indicates if they allow generating certificates for a wildcard domain and its root domain.

Expand Down
4 changes: 2 additions & 2 deletions docs/configuration/backends/ecs.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,9 @@ Træfik needs the following policy to read ECS information:
}
```

## Labels: overriding default behaviour
## Labels: overriding default behavior

Labels can be used on task containers to override default behaviour:
Labels can be used on task containers to override default behavior:

| Label | Description |
|------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
Expand Down
30 changes: 17 additions & 13 deletions healthcheck/healthcheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,30 +130,34 @@ func (hc *HealthCheck) execute(ctx context.Context, backend *BackendConfig) {
func (hc *HealthCheck) checkBackend(backend *BackendConfig) {
enabledURLs := backend.LB.Servers()
var newDisabledURLs []*url.URL
for _, url := range backend.disabledURLs {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason to make the variable name a single letter instead of a noun?

Copy link
Member Author

Choose a reason for hiding this comment

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

because url is also a package name

Copy link
Contributor

Choose a reason for hiding this comment

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

Is s/u/aDisabledUrl/ a possibility (or something meaningful)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Has been changed into disableURL

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

for _, disableURL := range backend.disabledURLs {
serverUpMetricValue := float64(0)
if err := checkHealth(url, backend); err == nil {
log.Warnf("Health check up: Returning to server list. Backend: %q URL: %q", backend.name, url.String())
backend.LB.UpsertServer(url, roundrobin.Weight(1))
if err := checkHealth(disableURL, backend); err == nil {
log.Warnf("Health check up: Returning to server list. Backend: %q URL: %q", backend.name, disableURL.String())
if err := backend.LB.UpsertServer(disableURL, roundrobin.Weight(1)); err != nil {
log.Error(err)
}
serverUpMetricValue = 1
} else {
log.Warnf("Health check still failing. Backend: %q URL: %q Reason: %s", backend.name, url.String(), err)
newDisabledURLs = append(newDisabledURLs, url)
log.Warnf("Health check still failing. Backend: %q URL: %q Reason: %s", backend.name, disableURL.String(), err)
newDisabledURLs = append(newDisabledURLs, disableURL)
}
labelValues := []string{"backend", backend.name, "url", url.String()}
labelValues := []string{"backend", backend.name, "url", disableURL.String()}
hc.metrics.BackendServerUpGauge().With(labelValues...).Set(serverUpMetricValue)
}
backend.disabledURLs = newDisabledURLs

for _, url := range enabledURLs {
for _, enableURL := range enabledURLs {
serverUpMetricValue := float64(1)
if err := checkHealth(url, backend); err != nil {
log.Warnf("Health check failed: Remove from server list. Backend: %q URL: %q Reason: %s", backend.name, url.String(), err)
backend.LB.RemoveServer(url)
backend.disabledURLs = append(backend.disabledURLs, url)
if err := checkHealth(enableURL, backend); err != nil {
log.Warnf("Health check failed: Remove from server list. Backend: %q URL: %q Reason: %s", backend.name, enableURL.String(), err)
if err := backend.LB.RemoveServer(enableURL); err != nil {
log.Error(err)
}
backend.disabledURLs = append(backend.disabledURLs, enableURL)
serverUpMetricValue = 0
}
labelValues := []string{"backend", backend.name, "url", url.String()}
labelValues := []string{"backend", backend.name, "url", enableURL.String()}
hc.metrics.BackendServerUpGauge().With(labelValues...).Set(serverUpMetricValue)
}
}
Expand Down
2 changes: 1 addition & 1 deletion healthcheck/healthcheck_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func TestSetBackendsConfiguration(t *testing.T) {
t.Run(test.desc, func(t *testing.T) {
t.Parallel()

// The context is passed to the health check and canonically cancelled by
// The context is passed to the health check and canonically canceled by
// the test server once all expected requests have been received.
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
Expand Down
4 changes: 3 additions & 1 deletion hostresolver/hostresolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ func (hr *Resolver) CNAMEFlatten(host string) (string, string) {
request = resolv.Record
}

hr.cache.Add(host, strings.Join(result, ","), cacheDuration)
if err := hr.cache.Add(host, strings.Join(result, ","), cacheDuration); err != nil {
log.Error(err)
}
}

return result[0], result[len(result)-1]
Expand Down
9 changes: 7 additions & 2 deletions integration/access_log_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"time"

"github.com/containous/traefik/integration/try"
"github.com/containous/traefik/log"
"github.com/containous/traefik/middlewares/accesslog"
"github.com/go-check/check"
checker "github.com/vdemeester/shakers"
Expand Down Expand Up @@ -324,13 +325,17 @@ func digestParts(resp *http.Response) map[string]string {

func getMD5(data string) string {
digest := md5.New()
digest.Write([]byte(data))
if _, err := digest.Write([]byte(data)); err != nil {
log.Error(err)
}
return fmt.Sprintf("%x", digest.Sum(nil))
}

func getCnonce() string {
b := make([]byte, 8)
io.ReadFull(rand.Reader, b)
if _, err := io.ReadFull(rand.Reader, b); err != nil {
log.Error(err)
}
return fmt.Sprintf("%x", b)[:16]
}

Expand Down
6 changes: 4 additions & 2 deletions integration/consul_catalog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,8 @@ func (s *ConsulCatalogSuite) TestRetryWithConsulServer(c *check.C) {

// Scale consul to 1
s.composeProject.Scale(c, "consul", 1)
s.waitToElectConsulLeader()
err = s.waitToElectConsulLeader()
c.Assert(err, checker.IsNil)

whoami := s.composeProject.Container(c, "whoami1")
// Register service
Expand Down Expand Up @@ -576,7 +577,8 @@ func (s *ConsulCatalogSuite) TestServiceWithMultipleHealthCheck(c *check.C) {

// Scale consul to 1
s.composeProject.Scale(c, "consul", 1)
s.waitToElectConsulLeader()
err = s.waitToElectConsulLeader()
c.Assert(err, checker.IsNil)

whoami := s.composeProject.Container(c, "whoami1")
// Register service
Expand Down
3 changes: 2 additions & 1 deletion integration/etcd3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,8 @@ func (s *Etcd3Suite) TestCommandStoreConfig(c *check.C) {
c.Assert(err, checker.IsNil)

// wait for traefik finish without error
cmd.Wait()
err = cmd.Wait()
c.Assert(err, checker.IsNil)

// CHECK
checkmap := map[string]string{
Expand Down
5 changes: 3 additions & 2 deletions integration/etcd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -411,8 +411,9 @@ func (s *EtcdSuite) TestCommandStoreConfig(c *check.C) {
err := cmd.Start()
c.Assert(err, checker.IsNil)

// wait for Træfik finish without error
cmd.Wait()
// wait for traefik finish without error
err = cmd.Wait()
c.Assert(err, checker.IsNil)

// CHECK
checkmap := map[string]string{
Expand Down
4 changes: 3 additions & 1 deletion integration/fake_dns_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@ func (s *handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) {
auth.Minttl = 1
m.Ns = append(m.Ns, auth)

w.WriteMsg(m)
if err := w.WriteMsg(m); err != nil {
log.Fatalf("Failed to write message %v", err)
}
}

func startFakeDNSServer() *dns.Server {
Expand Down
7 changes: 6 additions & 1 deletion integration/grpc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

"github.com/containous/traefik/integration/helloworld"
"github.com/containous/traefik/integration/try"
"github.com/containous/traefik/log"
"github.com/go-check/check"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials"
Expand Down Expand Up @@ -47,7 +48,11 @@ func (s *myserver) StreamExample(in *helloworld.StreamExampleRequest, server hel
for i := range data {
data[i] = randCharset[rand.Intn(len(randCharset))]
}
server.Send(&helloworld.StreamExampleReply{Data: string(data)})

if err := server.Send(&helloworld.StreamExampleReply{Data: string(data)}); err != nil {
log.Error(err)
}

<-s.stopStreamExample
return nil
}
Expand Down
5 changes: 4 additions & 1 deletion integration/https_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,10 @@ func modifyCertificateConfFileContent(c *check.C, certFileName, confFileName, en
defer func() {
f.Close()
}()
f.Truncate(0)

err = f.Truncate(0)
c.Assert(err, checker.IsNil)

// If certificate file is not provided, just truncate the configuration file
if len(certFileName) > 0 {
tlsConf := types.Configuration{
Expand Down
4 changes: 2 additions & 2 deletions middlewares/accesslog/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ const (
DataTableKey key = "LogDataTable"

// CommonFormat is the common logging format (CLF)
CommonFormat = "common"
CommonFormat string = "common"

// JSONFormat is the JSON logging format
JSONFormat = "json"
JSONFormat string = "json"
)

type logHandlerParams struct {
Expand Down
6 changes: 5 additions & 1 deletion middlewares/accesslog/logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"time"

"github.com/containous/flaeg/parse"
"github.com/containous/traefik/log"
"github.com/containous/traefik/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -626,7 +627,10 @@ func doLogging(t *testing.T, config *types.AccessLog) {
}

func logWriterTestHandlerFunc(rw http.ResponseWriter, r *http.Request) {
rw.Write([]byte(testContent))
if _, err := rw.Write([]byte(testContent)); err != nil {
log.Error(err)
}

rw.WriteHeader(testStatus)

logDataTable := GetLogDataTable(r)
Expand Down
5 changes: 4 additions & 1 deletion middlewares/auth/forward.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ func Forward(config *types.Forward, w http.ResponseWriter, r *http.Request, next

tracing.LogResponseCode(tracing.GetSpan(r), forwardResponse.StatusCode)
w.WriteHeader(forwardResponse.StatusCode)
w.Write(body)

if _, err = w.Write(body); err != nil {
log.Error(err)
}
return
}

Expand Down
6 changes: 5 additions & 1 deletion middlewares/cbreaker.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package middlewares
import (
"net/http"

"github.com/containous/traefik/log"
"github.com/containous/traefik/middlewares/tracing"
"github.com/vulcand/oxy/cbreaker"
)
Expand All @@ -27,7 +28,10 @@ func NewCircuitBreakerOptions(expression string) cbreaker.CircuitBreakerOption {
tracing.LogEventf(r, "blocked by circuit-breaker (%q)", expression)

w.WriteHeader(http.StatusServiceUnavailable)
w.Write([]byte(http.StatusText(http.StatusServiceUnavailable)))

if _, err := w.Write([]byte(http.StatusText(http.StatusServiceUnavailable))); err != nil {
log.Error(err)
}
}))
}

Expand Down