cmd/contour: Replace check function by log.Fatal() (#2827)

There are quite a few places in Contour that call a local check helper
that prints to standard error and exits. This approach loses error context
and makes it difficult for aggregated log collectors to recognize that
this is a fatal error message.

Fixes #2811, #2827.

Signed-off-by: Shailesh Suryawanshi <ss.shailesh28@gmail.com>

cmd/contour/certgen.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/projectcontour/contour/internal/certgen"
 	"github.com/projectcontour/contour/internal/k8s"
+	"github.com/sirupsen/logrus"
 	kingpin "gopkg.in/alecthomas/kingpin.v2"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/kubernetes"
@@ -123,7 +124,7 @@ func GenerateCerts(certConfig *certgenConfig) (map[string][]byte, error) {
 }
 
 // OutputCerts outputs the certs in certs as directed by config.
-func OutputCerts(config *certgenConfig, kubeclient *kubernetes.Clientset, certs map[string][]byte) {
+func OutputCerts(config *certgenConfig, kubeclient *kubernetes.Clientset, certs map[string][]byte) error {
 	secrets := []*corev1.Secret{}
 	force := certgen.NoOverwrite
 	if config.Overwrite {
@@ -137,34 +138,46 @@ func OutputCerts(config *certgenConfig, kubeclient *kubernetes.Clientset, certs
 		case "compact":
 			secrets = certgen.AsSecrets(config.Namespace, certs)
 		default:
-			check(fmt.Errorf("unsupported Secrets format %q", config.Format))
+			return fmt.Errorf("unsupported Secrets format %q", config.Format)
 		}
 	}
 
 	if config.OutputPEM {
 		fmt.Printf("Writing certificates to PEM files in %s/\n", config.OutputDir)
-		check(certgen.WriteCertsPEM(config.OutputDir, certs, force))
+		if err := certgen.WriteCertsPEM(config.OutputDir, certs, force); err != nil {
+			return fmt.Errorf("failed to write certificates to %q: %w", config.OutputDir, err)
+		}
 	}
 
 	if config.OutputYAML {
 		fmt.Printf("Writing %q format Secrets to YAML files in %s/\n", config.Format, config.OutputDir)
-		check(certgen.WriteSecretsYAML(config.OutputDir, secrets, force))
+		if err := certgen.WriteSecretsYAML(config.OutputDir, secrets, force); err != nil {
+			return fmt.Errorf("failed to write Secrets to %q: %w", config.OutputDir, err)
+		}
 	}
 
 	if config.OutputKube {
 		fmt.Printf("Writing %q format Secrets to namespace %q\n", config.Format, config.Namespace)
-		check(certgen.WriteSecretsKube(kubeclient, secrets, force))
+		if err := certgen.WriteSecretsKube(kubeclient, secrets, force); err != nil {
+			return fmt.Errorf("failed to write certificates to %q: %w", config.Namespace, err)
+		}
 	}
+	return nil
 }
 
-func doCertgen(config *certgenConfig) {
+func doCertgen(config *certgenConfig, log logrus.FieldLogger) {
 	generatedCerts, err := GenerateCerts(config)
-	check(err)
+	if err != nil {
+		log.WithError(err).Fatal("failed to generate certificates")
+	}
 
 	clients, err := k8s.NewClients(config.KubeConfig, config.InCluster)
 	if err != nil {
-		check(fmt.Errorf("failed to create Kubernetes client: %w", err))
+		log.WithError(err).Fatalf("failed to create Kubernetes client")
+	}
+
+	if oerr := OutputCerts(config, clients.ClientSet(), generatedCerts); oerr != nil {
+		log.WithError(oerr).Fatalf("failed output certificates")
 	}
 
-	OutputCerts(config, clients.ClientSet(), generatedCerts)
 }

cmd/contour/contour.go

@@ -14,7 +14,6 @@
 package main
 
 import (
-	"fmt"
 	"os"
 
 	resource "github.com/envoyproxy/go-control-plane/pkg/resource/v2"
@@ -70,9 +69,11 @@ func main() {
 	case sdmShutdown.FullCommand():
 		sdmShutdownCtx.shutdownHandler()
 	case bootstrap.FullCommand():
-		check(envoy.WriteBootstrap(bootstrapCtx))
+		if err := envoy.WriteBootstrap(bootstrapCtx); err != nil {
+			log.WithError(err).Fatal("failed to write bootstrap configuration")
+		}
 	case certgenApp.FullCommand():
-		doCertgen(certgenConfig)
+		doCertgen(certgenConfig, log)
 	case cds.FullCommand():
 		stream := client.ClusterStream()
 		watchstream(stream, resource.ClusterType, resources)
@@ -89,10 +90,9 @@ func main() {
 		stream := client.RouteStream()
 		watchstream(stream, resource.SecretType, resources)
 	case serve.FullCommand():
-		// parse args a second time so cli flags are applied
+		// Parse args a second time so cli flags are applied
 		// on top of any values sourced from -c's config file.
-		_, err := app.Parse(args)
-		check(err)
+		kingpin.MustParse(app.Parse(args))
 
 		// Reinitialize with the target debug level.
 		k8s.InitLogging(
@@ -105,7 +105,9 @@ func main() {
 		}
 
 		log.Infof("args: %v", args)
-		check(doServe(log, serveCtx))
+		if err := doServe(log, serveCtx); err != nil {
+			log.WithError(err).Fatal("Contour server failed")
+		}
 	case version.FullCommand():
 		println(build.PrintBuildInfo())
 	default:
@@ -114,10 +116,3 @@ func main() {
 	}
 
 }
-
-func check(err error) {
-	if err != nil {
-		fmt.Fprintln(os.Stderr, err)
-		os.Exit(1)
-	}
-}

cmd/contour/leadership.go

@@ -77,14 +77,14 @@ func setupLeadershipElection(g *workgroup.Group, log logrus.FieldLogger, ctx *se
 // newLeaderElector creates a new leaderelection.LeaderElector and associated
 // channels by which to observe elections and depositions.
 func newLeaderElector(log logrus.FieldLogger, ctx *serveContext, clients *k8s.Clients) (*leaderelection.LeaderElector, chan struct{}, chan struct{}) {
-
+	log = log.WithField("context", "leaderelection")
 	// leaderOK will block gRPC startup until it's closed.
 	leaderOK := make(chan struct{})
 	// deposed is closed by the leader election callback when
 	// we are deposed as leader so that we can clean up.
 	deposed := make(chan struct{})
 
-	rl := newResourceLock(ctx, clients)
+	rl := newResourceLock(ctx, clients, log)
 
 	// Make the leader elector, ready to be used in the Workgroup.
 	le, err := leaderelection.NewLeaderElector(leaderelection.LeaderElectionConfig{
@@ -107,13 +107,15 @@ func newLeaderElector(log logrus.FieldLogger, ctx *serveContext, clients *k8s.Cl
 			},
 		},
 	})
-	check(err)
+	if err != nil {
+		log.WithError(err).Fatal("failed to create leader elector")
+	}
 	return le, leaderOK, deposed
 }
 
 // newResourceLock creates a new resourcelock.Interface based on the Pod's name,
 // or a uuid if the name cannot be determined.
-func newResourceLock(ctx *serveContext, clients *k8s.Clients) resourcelock.Interface {
+func newResourceLock(ctx *serveContext, clients *k8s.Clients, log logrus.FieldLogger) resourcelock.Interface {
 	resourceLockID, found := os.LookupEnv("POD_NAME")
 	if !found {
 		resourceLockID = uuid.New().String()
@@ -133,6 +135,12 @@ func newResourceLock(ctx *serveContext, clients *k8s.Clients) resourcelock.Inter
 			Identity: resourceLockID,
 		},
 	)
-	check(err)
+	if err != nil {
+		log.WithError(err).
+			WithField("name", ctx.LeaderElectionConfig.Name).
+			WithField("namespace", ctx.LeaderElectionConfig.Namespace).
+			WithField("identity", resourceLockID).
+			Fatal("failed to create new resource lock")
+	}
 	return rl
 }

cmd/contour/serve.go

@@ -442,12 +442,12 @@ func doServe(log logrus.FieldLogger, ctx *serveContext) error {
 			grpcServer = xds.RegisterServer(
 				xds.NewContourServer(log, contour.ResourcesOf(resources)...),
 				registry,
-				ctx.grpcOptions()...)
+				ctx.grpcOptions(log)...)
 		case "envoy":
 			grpcServer = xds.RegisterServer(
 				server.NewServer(context.Background(), snapshotCache, nil),
 				registry,
-				ctx.grpcOptions()...)
+				ctx.grpcOptions(log)...)
 		default:
 			log.Fatalf("invalid xdsServerType %q configured", ctx.XDSServerType)
 		}

cmd/contour/servecontext.go

@@ -27,6 +27,7 @@ import (
 
 	"github.com/projectcontour/contour/internal/contour"
 	"github.com/projectcontour/contour/internal/envoy"
+	"github.com/sirupsen/logrus"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
 	"google.golang.org/grpc/keepalive"
@@ -294,7 +295,7 @@ type TimeoutConfig struct {
 // grpcOptions returns a slice of grpc.ServerOptions.
 // if ctx.PermitInsecureGRPC is false, the option set will
 // include TLS configuration.
-func (ctx *serveContext) grpcOptions() []grpc.ServerOption {
+func (ctx *serveContext) grpcOptions(log logrus.FieldLogger) []grpc.ServerOption {
 	opts := []grpc.ServerOption{
 		// By default the Go grpc library defaults to a value of ~100 streams per
 		// connection. This number is likely derived from the HTTP/2 spec:
@@ -316,7 +317,7 @@ func (ctx *serveContext) grpcOptions() []grpc.ServerOption {
 		}),
 	}
 	if !ctx.PermitInsecureGRPC {
-		tlsconfig := ctx.tlsconfig()
+		tlsconfig := ctx.tlsconfig(log)
 		creds := credentials.NewTLS(tlsconfig)
 		opts = append(opts, grpc.Creds(creds))
 	}
@@ -325,9 +326,11 @@ func (ctx *serveContext) grpcOptions() []grpc.ServerOption {
 
 // tlsconfig returns a new *tls.Config. If the context is not properly configured
 // for tls communication, tlsconfig returns nil.
-func (ctx *serveContext) tlsconfig() *tls.Config {
+func (ctx *serveContext) tlsconfig(log logrus.FieldLogger) *tls.Config {
 	err := ctx.verifyTLSFlags()
-	check(err)
+	if err != nil {
+		log.WithError(err).Fatal("failed to verify TLS flags")
+	}
 
 	// Define a closure that lazily loads certificates and key at TLS handshake
 	// to ensure that latest certificates are used in case they have been rotated.
@@ -356,16 +359,15 @@ func (ctx *serveContext) tlsconfig() *tls.Config {
 	}
 
 	// Attempt to load certificates and key to catch configuration errors early.
-	_, err = loadConfig()
-	check(err)
+	if _, lerr := loadConfig(); lerr != nil {
+		log.WithError(err).Fatal("failed to load certificate and key")
+	}
 
 	return &tls.Config{
 		ClientAuth: tls.RequireAndVerifyClientCert,
 		Rand:       rand.Reader,
 		GetConfigForClient: func(*tls.ClientHelloInfo) (*tls.Config, error) {
-			config, err := loadConfig()
-			check(err)
-			return config, err
+			return loadConfig()
 		},
 	}
 }

cmd/contour/servecontext_test.go

@@ -28,6 +28,7 @@ import (
 	"time"
 
 	"github.com/projectcontour/contour/internal/envoy"
+	"github.com/projectcontour/contour/internal/fixture"
 	"k8s.io/apimachinery/pkg/types"
 
 	"github.com/google/go-cmp/cmp"
@@ -338,7 +339,8 @@ func TestServeContextCertificateHandling(t *testing.T) {
 	checkFatalErr(t, err)
 
 	// Start a dummy server.
-	opts := ctx.grpcOptions()
+	log := fixture.NewTestLogger(t)
+	opts := ctx.grpcOptions(log)
 	g := grpc.NewServer(opts...)
 	if g == nil {
 		t.Error("failed to create server")