Added a new 'disableTLS' flag and changed 'disableMTLS' to only disab… (

googleforgames#1777) * Added a new 'disableTLS' flag and changed 'disableMTLS' to only disable client auth. * Conditionally disabled cert dir watching/reading on disableMTLS. * Added some keepalive parameters for health checks. * Changed logic to only have mTLS in the case of TLS. * Added a comment for why the keepalive options are necessary. Co-authored-by: Nikhil Athreya <nathreya@google.com>
ilkercelikyilmaz · Oct 23, 2020 · bbaff00 · bbaff00
1 parent 9dd8e43
commit bbaff00
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 41 deletions.
diff --git a/cmd/allocator/main.go b/cmd/allocator/main.go
@@ -43,6 +43,7 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials"
+	"google.golang.org/grpc/keepalive"
 	"google.golang.org/grpc/status"
 	"gopkg.in/fsnotify.v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -88,24 +89,14 @@ func main() {
 		return err
 	})
 
-	h := newServiceHandler(kubeClient, agonesClient, health, conf.MTLSDisabled)
+	h := newServiceHandler(kubeClient, agonesClient, health, conf.MTLSDisabled, conf.TLSDisabled)
 
 	listener, err := net.Listen("tcp", fmt.Sprintf(":%s", sslPort))
 	if err != nil {
 		logger.WithError(err).Fatalf("failed to listen on TCP port %s", sslPort)
 	}
 
-	if !h.mTLSDisabled {
-		// creates a new file watcher for client certificate folder
-		watcher, err := fsnotify.NewWatcher()
-		if err != nil {
-			logger.WithError(err).Fatal("could not create watcher for client certs")
-		}
-		defer watcher.Close() // nolint: errcheck
-		if err := watcher.Add(certDir); err != nil {
-			logger.WithError(err).Fatalf("cannot watch folder %s for secret changes", certDir)
-		}
-
+	if !h.tlsDisabled {
 		watcherTLS, err := fsnotify.NewWatcher()
 		if err != nil {
 			logger.WithError(err).Fatal("could not create watcher for tls certs")
@@ -130,23 +121,47 @@ func main() {
 						h.tlsMutex.Unlock()
 					}
 					logger.Infof("Tls directory change event %v", event)
-				case event := <-watcher.Events:
-					h.certMutex.Lock()
-					caCertPool, err := getCACertPool(certDir)
-					if err != nil {
-						logger.WithError(err).Error("could not load CA certs; keeping old ones")
-					} else {
-						h.caCertPool = caCertPool
-					}
-					logger.Infof("Certificate directory change event %v", event)
-					h.certMutex.Unlock()
 
-					// watch for errors
-				case err := <-watcher.Errors:
-					logger.WithError(err).Error("error watching for certificate directory")
+				// watch for errors
+				case err := <-watcherTLS.Errors:
+					logger.WithError(err).Error("error watching for TLS directory")
 				}
 			}
 		}()
+
+		if !h.mTLSDisabled {
+			// creates a new file watcher for client certificate folder
+			watcher, err := fsnotify.NewWatcher()
+			if err != nil {
+				logger.WithError(err).Fatal("could not create watcher for client certs")
+			}
+			defer watcher.Close() // nolint: errcheck
+			if err := watcher.Add(certDir); err != nil {
+				logger.WithError(err).Fatalf("cannot watch folder %s for secret changes", certDir)
+			}
+
+			go func() {
+				for {
+					select {
+					// watch for events
+					case event := <-watcher.Events:
+						h.certMutex.Lock()
+						caCertPool, err := getCACertPool(certDir)
+						if err != nil {
+							logger.WithError(err).Error("could not load CA certs; keeping old ones")
+						} else {
+							h.caCertPool = caCertPool
+						}
+						logger.Infof("Certificate directory change event %v", event)
+						h.certMutex.Unlock()
+
+					// watch for errors
+					case err := <-watcher.Errors:
+						logger.WithError(err).Error("error watching for certificate directory")
+					}
+				}
+			}()
+		}
 	}
 
 	opts := h.getServerOptions()
@@ -167,7 +182,7 @@ func main() {
 	logger.WithError(err).Fatal("allocation service crashed")
 }
 
-func newServiceHandler(kubeClient kubernetes.Interface, agonesClient versioned.Interface, health healthcheck.Handler, mTLSDisabled bool) *serviceHandler {
+func newServiceHandler(kubeClient kubernetes.Interface, agonesClient versioned.Interface, health healthcheck.Handler, mTLSDisabled bool, tlsDisabled bool) *serviceHandler {
 	defaultResync := 30 * time.Second
 	agonesInformerFactory := externalversions.NewSharedInformerFactory(agonesClient, defaultResync)
 	kubeInformerFactory := informers.NewSharedInformerFactory(kubeClient, defaultResync)
@@ -185,6 +200,7 @@ func newServiceHandler(kubeClient kubernetes.Interface, agonesClient versioned.I
 			return allocator.Allocate(gsa, stop)
 		},
 		mTLSDisabled: mTLSDisabled,
+		tlsDisabled:  tlsDisabled,
 	}
 
 	kubeInformerFactory.Start(stop)
@@ -193,22 +209,24 @@ func newServiceHandler(kubeClient kubernetes.Interface, agonesClient versioned.I
 		logger.WithError(err).Fatal("starting allocator failed.")
 	}
 
-	if !h.mTLSDisabled {
-		caCertPool, err := getCACertPool(certDir)
-		if err != nil {
-			logger.WithError(err).Fatal("could not load CA certs.")
-		}
-		h.certMutex.Lock()
-		h.caCertPool = caCertPool
-		h.certMutex.Unlock()
-
+	if !h.tlsDisabled {
 		tlsCert, err := readTLSCert()
 		if err != nil {
 			logger.WithError(err).Fatal("could not load TLS certs.")
 		}
 		h.tlsMutex.Lock()
 		h.tlsCert = tlsCert
 		h.tlsMutex.Unlock()
+
+		if !h.mTLSDisabled {
+			caCertPool, err := getCACertPool(certDir)
+			if err != nil {
+				logger.WithError(err).Fatal("could not load CA certs.")
+			}
+			h.certMutex.Lock()
+			h.caCertPool = caCertPool
+			h.certMutex.Unlock()
+		}
 	}
 
 	return &h
@@ -225,17 +243,34 @@ func readTLSCert() (*tls.Certificate, error) {
 // getServerOptions returns a list of GRPC server options.
 // Current options are TLS certs and opencensus stats handler.
 func (h *serviceHandler) getServerOptions() []grpc.ServerOption {
-	if h.mTLSDisabled {
+	if h.tlsDisabled {
 		return []grpc.ServerOption{grpc.StatsHandler(&ocgrpc.ServerHandler{})}
 	}
 
 	cfg := &tls.Config{
-		GetCertificate:        h.getTLSCert,
-		ClientAuth:            tls.RequireAnyClientCert,
-		VerifyPeerCertificate: h.verifyClientCertificate,
+		GetCertificate: h.getTLSCert,
+	}
+
+	if !h.mTLSDisabled {
+		cfg.ClientAuth = tls.RequireAnyClientCert
+		cfg.VerifyPeerCertificate = h.verifyClientCertificate
+	}
+
+	// Add options for creds and  OpenCensus stats handler to enable stats and tracing.
+	// The keepalive options are useful for efficiency purposes (keeping a single connection alive
+	// instead of constantly recreating connections), when placing the Agones allocator behind load balancers.
+	return []grpc.ServerOption{
+		grpc.Creds(credentials.NewTLS(cfg)),
+		grpc.StatsHandler(&ocgrpc.ServerHandler{}),
+		grpc.KeepaliveEnforcementPolicy(keepalive.EnforcementPolicy{
+			MinTime:             1 * time.Minute,
+			PermitWithoutStream: true,
+		}),
+		grpc.KeepaliveParams(keepalive.ServerParameters{
+			MaxConnectionIdle: 5 * time.Minute,
+			Timeout:           10 * time.Minute,
+		}),
 	}
-	// Add options for creds and OpenCensus stats handler to enable stats and tracing.
-	return []grpc.ServerOption{grpc.Creds(credentials.NewTLS(cfg)), grpc.StatsHandler(&ocgrpc.ServerHandler{})}
 }
 
 func (h *serviceHandler) getTLSCert(ch *tls.ClientHelloInfo) (*tls.Certificate, error) {
@@ -333,6 +368,7 @@ type serviceHandler struct {
 	tlsCert  *tls.Certificate
 
 	mTLSDisabled bool
+	tlsDisabled  bool
 }
 
 // Allocate implements the Allocate gRPC method definition

diff --git a/cmd/allocator/metrics.go b/cmd/allocator/metrics.go
@@ -33,13 +33,15 @@ const (
 	projectIDFlag                = "gcp-project-id"
 	stackdriverLabels            = "stackdriver-labels"
 	mTLSDisabledFlag             = "disable-mtls"
+	tlsDisabledFlag              = "disable-tls"
 )
 
 func init() {
 	registerMetricViews()
 }
 
 type config struct {
+	TLSDisabled       bool
 	MTLSDisabled      bool
 	PrometheusMetrics bool
 	Stackdriver       bool
@@ -54,12 +56,14 @@ func parseEnvFlags() config {
 	viper.SetDefault(projectIDFlag, "")
 	viper.SetDefault(stackdriverLabels, "")
 	viper.SetDefault(mTLSDisabledFlag, false)
+	viper.SetDefault(tlsDisabledFlag, false)
 
 	pflag.Bool(enablePrometheusMetricsFlag, viper.GetBool(enablePrometheusMetricsFlag), "Flag to activate metrics of Agones. Can also use PROMETHEUS_EXPORTER env variable.")
 	pflag.Bool(enableStackdriverMetricsFlag, viper.GetBool(enableStackdriverMetricsFlag), "Flag to activate stackdriver monitoring metrics for Agones. Can also use STACKDRIVER_EXPORTER env variable.")
 	pflag.String(projectIDFlag, viper.GetString(projectIDFlag), "GCP ProjectID used for Stackdriver, if not specified ProjectID from Application Default Credentials would be used. Can also use GCP_PROJECT_ID env variable.")
 	pflag.String(stackdriverLabels, viper.GetString(stackdriverLabels), "A set of default labels to add to all stackdriver metrics generated. By default metadata are automatically added using Kubernetes API and GCP metadata enpoint.")
 	pflag.Bool(mTLSDisabledFlag, viper.GetBool(mTLSDisabledFlag), "Flag to enable/disable mTLS in the allocator.")
+	pflag.Bool(tlsDisabledFlag, viper.GetBool(tlsDisabledFlag), "Flag to enable/disable TLS in the allocator.")
 	runtime.FeaturesBindFlags()
 	pflag.Parse()
 
@@ -69,6 +73,7 @@ func parseEnvFlags() config {
 	runtime.Must(viper.BindEnv(projectIDFlag))
 	runtime.Must(viper.BindEnv(stackdriverLabels))
 	runtime.Must(viper.BindEnv(mTLSDisabledFlag))
+	runtime.Must(viper.BindEnv(tlsDisabledFlag))
 	runtime.Must(viper.BindPFlags(pflag.CommandLine))
 	runtime.Must(runtime.FeaturesBindEnv())
 
@@ -80,6 +85,7 @@ func parseEnvFlags() config {
 		GCPProjectID:      viper.GetString(projectIDFlag),
 		StackdriverLabels: viper.GetString(stackdriverLabels),
 		MTLSDisabled:      viper.GetBool(mTLSDisabledFlag),
+		TLSDisabled:       viper.GetBool(tlsDisabledFlag),
 	}
 }
 

diff --git a/install/helm/agones/templates/service/allocation.yaml b/install/helm/agones/templates/service/allocation.yaml
@@ -124,6 +124,8 @@ spec:
           value: {{ .Values.agones.metrics.stackdriverLabels | quote }}
         - name: DISABLE_MTLS
           value: {{ .Values.agones.allocator.disableMTLS | quote }}
+        - name: DISABLE_TLS
+          value: {{ .Values.agones.allocator.disableTLS | quote }}
         - name: POD_NAME
           valueFrom:
             fieldRef:

diff --git a/install/helm/agones/values.yaml b/install/helm/agones/values.yaml
@@ -129,6 +129,7 @@ agones:
     generateTLS: true
     generateClientTLS: true
     disableMTLS: false
+    disableTLS: false
   image:
     registry: gcr.io/agones-images
     tag: 1.9.0-dev

diff --git a/install/yaml/install.yaml b/install/yaml/install.yaml
@@ -1576,6 +1576,8 @@ spec:
           value: ""
         - name: DISABLE_MTLS
           value: "false"
+        - name: DISABLE_TLS
+          value: "false"
         - name: POD_NAME
           valueFrom:
             fieldRef: