use os.Exit rather than panic, read env var closer to where we use it.

mlunadia · Jul 14, 2021 · 355d824 · 355d824
1 parent 124fd0d
commit 355d824
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 27 deletions.
diff --git a/cmd/controller/run.go b/cmd/controller/run.go
@@ -8,6 +8,7 @@ import (
 	"net/http"         // Pprof related
 	_ "net/http/pprof" // Pprof related
 	"os"
+	"strconv"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -34,7 +35,8 @@ import (
 )
 
 const (
-	PprofListenAddr = "0.0.0.0:6060"
+	PprofListenAddr       = "0.0.0.0:6060"
+	kappctrlAPIPORTEnvKey = "KAPPCTRL_API_PORT"
 )
 
 type Options struct {
@@ -90,13 +92,22 @@ func Run(opts Options, runLog logr.Logger) {
 		os.Exit(1)
 	}
 
-	appFactory := AppFactory{
-		coreClient: coreClient,
-		kcConfig:   kcConfig,
-		appClient:  kcClient,
+	// assign bindPort to env var KAPPCTRL_API_PORT if available
+	var bindPort int
+	if apiPort, ok := os.LookupEnv(kappctrlAPIPORTEnvKey); ok {
+		var err error
+		if bindPort, err = strconv.Atoi(apiPort); err != nil {
+			runLog.Error(fmt.Errorf("%s environment variable must be an integer", kappctrlAPIPORTEnvKey), "reading server port")
+			os.Exit(1)
+		} else {
+			runLog.Error(err, "reading server port")
+			os.Exit(1)
+		}
+	} else {
+		runLog.Error(fmt.Errorf("os call failed to read env var %s", kappctrlAPIPORTEnvKey), "reading server port")
+		os.Exit(1)
 	}
-
-	server, err := apiserver.NewAPIServer(restConfig, coreClient, kcClient, opts.PackagingGloablNS)
+	server, err := apiserver.NewAPIServer(restConfig, coreClient, kcClient, opts.PackagingGloablNS, bindPort)
 	if err != nil {
 		runLog.Error(err, "creating server")
 		os.Exit(1)
@@ -111,6 +122,12 @@ func Run(opts Options, runLog logr.Logger) {
 	refTracker := reftracker.NewAppRefTracker()
 	updateStatusTracker := reftracker.NewAppUpdateStatus()
 
+	appFactory := AppFactory{
+		coreClient: coreClient,
+		kcConfig:   kcConfig,
+		appClient:  kcClient,
+	}
+
 	{ // add controller for apps
 		schApp := handlers.NewSecretHandler(runLog, refTracker, updateStatusTracker)
 		cfgmhApp := handlers.NewConfigMapHandler(runLog, refTracker, updateStatusTracker)

diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go
@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"net"
 	"os"
-	"strconv"
 	"time"
 
 	"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/openapi"
@@ -43,15 +42,13 @@ const (
 
 	TokenPath = "/token-dir"
 
-	kappctrlNSEnvKey      = "KAPPCTRL_SYSTEM_NAMESPACE"
-	kappctrlAPIPORTEnvKey = "KAPPCTRL_API_PORT"
-	apiServiceName        = "v1alpha1.data.packaging.carvel.dev"
+	kappctrlNSEnvKey = "KAPPCTRL_SYSTEM_NAMESPACE"
+	apiServiceName   = "v1alpha1.data.packaging.carvel.dev"
 )
 
 var (
-	Scheme   = runtime.NewScheme()
-	Codecs   = serializer.NewCodecFactory(Scheme)
-	bindPort int
+	Scheme = runtime.NewScheme()
+	Codecs = serializer.NewCodecFactory(Scheme)
 )
 
 func init() {
@@ -67,16 +64,6 @@ func init() {
 		&metav1.APIGroup{},
 		&metav1.APIResourceList{},
 	)
-
-	// assign bindPort to env var KAPPCTRL_API_PORT if available
-	if apiPort, ok := os.LookupEnv(kappctrlAPIPORTEnvKey); ok {
-		var err error
-		if bindPort, err = strconv.Atoi(apiPort); err != nil {
-			panic(fmt.Sprintf("%s environment variable must be an integer", kappctrlAPIPORTEnvKey))
-		}
-	} else {
-		panic(fmt.Sprintf("%s environment variable must be provided", kappctrlAPIPORTEnvKey))
-	}
 }
 
 type APIServer struct {
@@ -85,13 +72,13 @@ type APIServer struct {
 	aggClient aggregatorclient.Interface
 }
 
-func NewAPIServer(clientConfig *rest.Config, coreClient kubernetes.Interface, kcClient kcclient.Interface, globalNamespace string) (*APIServer, error) {
+func NewAPIServer(clientConfig *rest.Config, coreClient kubernetes.Interface, kcClient kcclient.Interface, globalNamespace string, bindPort int) (*APIServer, error) {
 	aggClient, err := aggregatorclient.NewForConfig(clientConfig)
 	if err != nil {
 		return nil, fmt.Errorf("building aggregation client: %v", err)
 	}
 
-	config, err := newServerConfig(aggClient)
+	config, err := newServerConfig(aggClient, bindPort)
 	if err != nil {
 		return nil, err
 	}
@@ -158,13 +145,18 @@ func (as *APIServer) isReady() (bool, error) {
 	return false, nil
 }
 
-func newServerConfig(aggClient aggregatorclient.Interface) (*genericapiserver.RecommendedConfig, error) {
+func newServerConfig(aggClient aggregatorclient.Interface, bindPort int) (*genericapiserver.RecommendedConfig, error) {
 	recommendedOptions := genericoptions.NewRecommendedOptions("", Codecs.LegacyCodec(v1alpha1.SchemeGroupVersion))
 	recommendedOptions.Etcd = nil
 
 	// Set the PairName and CertDirectory to generate the certificate files.
 	recommendedOptions.SecureServing.ServerCert.CertDirectory = selfSignedCertDir
 	recommendedOptions.SecureServing.ServerCert.PairName = "kapp-controller"
+
+	// ports below 1024 are probably the wrong port, see https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers#Well-known_ports
+	if bindPort < 1024 {
+		return nil, fmt.Errorf("error initializing API Port to %v - try passing a port above 1023", bindPort)
+	}
 	recommendedOptions.SecureServing.BindPort = bindPort
 
 	if err := recommendedOptions.SecureServing.MaybeDefaultWithSelfSignedCerts("kapp-controller", []string{apiServiceEndoint()}, []net.IP{net.ParseIP("127.0.0.1")}); err != nil {