Skip to content

Commit

Permalink
Bring up cmd/flags test coverage to 100% (#3691)
Browse files Browse the repository at this point in the history
  • Loading branch information
yurishkuro authored May 20, 2022
1 parent 24d2ccf commit 07af654
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 138 deletions.
4 changes: 3 additions & 1 deletion cmd/flags/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ func (s *AdminServer) initFromViper(v *viper.Viper, logger *zap.Logger) error {

s.adminHostPort = v.GetString(adminHTTPHostPort)
var tlsAdminHTTP tlscfg.Options
s.tlsCertWatcherCloser = &tlsAdminHTTP
tlsAdminHTTP, err := tlsAdminHTTPFlagsConfig.InitFromViper(v)
if err != nil {
return fmt.Errorf("failed to parse admin server TLS options: %w", err)
Expand All @@ -97,6 +96,9 @@ func (s *AdminServer) initFromViper(v *viper.Viper, logger *zap.Logger) error {
return err
}
s.tlsCfg = tlsCfg
s.tlsCertWatcherCloser = &tlsAdminHTTP
} else {
s.tlsCertWatcherCloser = io.NopCloser(nil)
}
return nil
}
Expand Down
179 changes: 44 additions & 135 deletions cmd/flags/admin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (

"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
"github.com/jaegertracing/jaeger/pkg/healthcheck"
"github.com/jaegertracing/jaeger/ports"
)

Expand Down Expand Up @@ -58,7 +59,42 @@ func TestAdminServerHandlesPortZero(t *testing.T) {
assert.Greater(t, port, 0)
}

func TestCollectorAdminWithFailedFlags(t *testing.T) {
func TestAdminHealthCheck(t *testing.T) {
adminServer := NewAdminServer(":0")
status := adminServer.HC().Get()
assert.Equal(t, healthcheck.Unavailable, status)
}

func TestAdminFailToServe(t *testing.T) {
l, err := net.Listen("tcp", ":0")
require.NoError(t, err)
l.Close() // forcing Serve on a closed connection

adminServer := NewAdminServer(":0")
v, command := config.Viperize(adminServer.AddFlags)
command.ParseFlags([]string{})
zapCore, logs := observer.New(zap.InfoLevel)
logger := zap.New(zapCore)

require.NoError(t, adminServer.initFromViper(v, logger))

adminServer.serveWithListener(l)
defer adminServer.Close()

waitForEqual(t, healthcheck.Broken, func() interface{} { return adminServer.HC().Get() })

logEntries := logs.TakeAll()
var matchedEntry string
for _, log := range logEntries {
if strings.Contains(log.Message, "failed to serve") {
matchedEntry = log.Message
break
}
}
assert.Contains(t, matchedEntry, "failed to serve")
}

func TestAdminWithFailedFlags(t *testing.T) {
adminServer := NewAdminServer(fmt.Sprintf(":%d", ports.CollectorAdminHTTP))
zapCore, _ := observer.New(zap.InfoLevel)
logger := zap.New(zapCore)
Expand All @@ -75,44 +111,10 @@ func TestCollectorAdminWithFailedFlags(t *testing.T) {

func TestAdminServerTLS(t *testing.T) {
testCases := []struct {
name string
serverTLSFlags []string
clientTLS tlscfg.Options
expectTLSClientErr bool
expectAdminClientErr bool
expectServerFail bool
name string
serverTLSFlags []string
clientTLS tlscfg.Options
}{
{
name: "should fail with TLS client to untrusted TLS server",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
ServerName: "example.com",
},
expectTLSClientErr: true,
expectAdminClientErr: true,
expectServerFail: false,
},
{
name: "should fail with TLS client to trusted TLS server with incorrect hostname",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "nonEmpty",
},
expectTLSClientErr: true,
expectAdminClientErr: true,
expectServerFail: false,
},
{
name: "should pass with TLS client to trusted TLS server with correct hostname",
serverTLSFlags: []string{
Expand All @@ -125,84 +127,6 @@ func TestAdminServerTLS(t *testing.T) {
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
},
expectTLSClientErr: false,
expectAdminClientErr: false,
expectServerFail: false,
},
{
name: "should fail with TLS client without cert to trusted TLS server requiring cert",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
"--admin.http.tls.client-ca=" + testCertKeyLocation + "/example-CA-cert.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
},
expectTLSClientErr: false,
expectServerFail: false,
expectAdminClientErr: true,
},
{
name: "should pass with TLS client with cert to trusted TLS server requiring cert",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
"--admin.http.tls.client-ca=" + testCertKeyLocation + "/example-CA-cert.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
CertPath: testCertKeyLocation + "/example-client-cert.pem",
KeyPath: testCertKeyLocation + "/example-client-key.pem",
},
expectTLSClientErr: false,
expectServerFail: false,
expectAdminClientErr: false,
},
{
name: "should fail with TLS client without cert to trusted TLS server requiring cert from a different CA",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
"--admin.http.tls.client-ca=" + testCertKeyLocation + "/wrong-CA-cert.pem", // NB: wrong CA
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
CertPath: testCertKeyLocation + "/example-client-cert.pem",
KeyPath: testCertKeyLocation + "/example-client-key.pem",
},
expectTLSClientErr: false,
expectServerFail: false,
expectAdminClientErr: true,
},
{
name: "should fail with TLS client with cert to trusted TLS server with incorrect TLS min",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
"--admin.http.tls.client-ca=" + testCertKeyLocation + "/example-CA-cert.pem",
"--admin.http.tls.min-version=1.5",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
CertPath: testCertKeyLocation + "/example-client-cert.pem",
KeyPath: testCertKeyLocation + "/example-client-key.pem",
},
expectTLSClientErr: true,
expectServerFail: true,
expectAdminClientErr: false,
},
}

Expand All @@ -217,11 +141,6 @@ func TestAdminServerTLS(t *testing.T) {
logger := zap.New(zapCore)

err = adminServer.initFromViper(v, logger)

if test.expectServerFail {
require.Error(t, err)
return
}
require.NoError(t, err)

adminServer.Serve()
Expand All @@ -231,13 +150,8 @@ func TestAdminServerTLS(t *testing.T) {
require.NoError(t, err0)
dialer := &net.Dialer{Timeout: 2 * time.Second}
conn, clientError := tls.DialWithDialer(dialer, "tcp", fmt.Sprintf("localhost:%d", ports.CollectorAdminHTTP), clientTLSCfg)

if test.expectTLSClientErr {
require.Error(t, clientError)
} else {
require.NoError(t, clientError)
require.Nil(t, conn.Close())
}
require.NoError(t, clientError)
require.Nil(t, conn.Close())

client := &http.Client{
Transport: &http.Transport{
Expand All @@ -246,13 +160,8 @@ func TestAdminServerTLS(t *testing.T) {
}

response, requestError := client.Get(fmt.Sprintf("https://localhost:%d", ports.CollectorAdminHTTP))

if test.expectAdminClientErr {
require.Error(t, requestError)
} else {
require.NoError(t, requestError)
require.NotNil(t, response)
}
require.NoError(t, requestError)
require.NotNil(t, response)
})
}
}
1 change: 1 addition & 0 deletions cmd/flags/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
)

func TestParseJaegerTags(t *testing.T) {
assert.Nil(t, ParseJaegerTags(""))

jaegerTags := fmt.Sprintf("%s,%s,%s,%s,%s,%s",
"key=value",
Expand Down
4 changes: 2 additions & 2 deletions cmd/flags/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type Service struct {
// AdminPort is the HTTP port number for admin server.
AdminPort int

// NoStorage indicates that storage type CLI flag is not applicable
// NoStorage indicates that storage-type CLI flag is not applicable
NoStorage bool

// Admin is the admin server that hosts the health check and metrics endpoints.
Expand Down Expand Up @@ -112,7 +112,7 @@ func (s *Service) Start(v *viper.Viper) error {
s.MetricsFactory = metricsFactory

if err = s.Admin.initFromViper(v, s.Logger); err != nil {
s.Logger.Fatal("Failed to initialize admin server", zap.Error(err))
return fmt.Errorf("cannot initialize admin server: %w", err)
}
if h := metricsBuilder.Handler(); h != nil {
route := metricsBuilder.HTTPRoute
Expand Down
114 changes: 114 additions & 0 deletions cmd/flags/service_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
// Copyright (c) 2022 The Jaeger Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package flags

import (
"flag"
"os"
"reflect"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/atomic"

"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/healthcheck"
)

func TestAddFlags(t *testing.T) {
s := NewService(0)
s.AddFlags(new(flag.FlagSet))

s.NoStorage = true
s.AddFlags(new(flag.FlagSet))
}

func TestStartErrors(t *testing.T) {
scenarios := []struct {
name string
flags []string
expErr string
}{
{
name: "bad config",
flags: []string{"--config-file=invalid-file-name"},
expErr: "cannot load config file",
},
{
name: "bad log level",
flags: []string{"--log-level=invalid-log-level"},
expErr: "cannot create logger",
},
{
name: "bad metrics backend",
flags: []string{"--metrics-backend=invalid-metrics-backend"},
expErr: "cannot create metrics factory",
},
{
name: "bad admin TLS",
flags: []string{"--admin.http.tls.enabled=true", "--admin.http.tls.cert=invalid-cert"},
expErr: "cannot initialize admin server",
},
{
name: "bad host:port",
flags: []string{"--admin.http.host-port=invalid"},
expErr: "cannot start the admin server",
},
{
name: "clean start",
flags: []string{},
},
}
for _, test := range scenarios {
t.Run(test.name, func(t *testing.T) {
s := NewService( /*default port=*/ 0)
v, cmd := config.Viperize(s.AddFlags)
err := cmd.ParseFlags(test.flags)
assert.NoError(t, err)
err = s.Start(v)
if test.expErr != "" {
require.Error(t, err)
assert.Contains(t, err.Error(), test.expErr)
return
}
assert.NoError(t, err)

stopped := atomic.NewBool(false)
shutdown := func() {
stopped.Store(true)
}
go s.RunAndThen(shutdown)

waitForEqual(t, healthcheck.Ready, func() interface{} { return s.HC().Get() })
s.SetHealthCheckStatus(healthcheck.Unavailable)
waitForEqual(t, healthcheck.Unavailable, func() interface{} { return s.HC().Get() })

s.signalsChannel <- os.Interrupt
waitForEqual(t, true, func() interface{} { return stopped.Load() })
})
}
}

func waitForEqual(t *testing.T, expected interface{}, getter func() interface{}) {
for i := 0; i < 1000; i++ {
value := getter()
if reflect.DeepEqual(value, expected) {
return
}
time.Sleep(10 * time.Millisecond)
}
assert.Equal(t, expected, getter())
}

0 comments on commit 07af654

Please sign in to comment.