Updates based on code review

Signed-off-by: Margo Crawford <margaretc@vmware.com>

internal/config/supervisor/config.go

@@ -24,6 +24,12 @@ const (
 	NetworkDisabled = "disabled"
 	NetworkUnix     = "unix"
 	NetworkTCP      = "tcp"
+
+	// Use 10250 because it happens to be the same port on which the Kubelet listens, so some cluster types
+	// are more permissive with servers that run on this port. For example, GKE private clusters do not
+	// allow traffic from the control plane to most ports, but do allow traffic to port 10250. This allows
+	// the Concierge to work without additional configuration on these types of clusters.
+	aggregatedAPIServerPortDefault = 10250
 )
 
 // FromPath loads an Config from a provided local file path, inserts any
@@ -50,6 +56,12 @@ func FromPath(ctx context.Context, path string) (*Config, error) {
 		return nil, fmt.Errorf("validate apiGroupSuffix: %w", err)
 	}
 
+	maybeSetAggregatedAPIServerPortDefaults(&config.AggregatedAPIServerPort)
+
+	if err := validateServerPort(config.AggregatedAPIServerPort); err != nil {
+		return nil, fmt.Errorf("validate aggregatedAPIServerPort: %w", err)
+	}
+
 	if err := validateNames(&config.NamesConfig); err != nil {
 		return nil, fmt.Errorf("validate names: %w", err)
 	}
@@ -105,6 +117,12 @@ func validateAPIGroupSuffix(apiGroupSuffix string) error {
 	return groupsuffix.Validate(apiGroupSuffix)
 }
 
+func maybeSetAggregatedAPIServerPortDefaults(port **int64) {
+	if *port == nil {
+		*port = pointer.Int64Ptr(aggregatedAPIServerPortDefault)
+	}
+}
+
 func validateNames(names *NamesConfigSpec) error {
 	missingNames := []string{}
 	if names.DefaultTLSCertificateSecret == "" {
@@ -193,3 +211,11 @@ func addrIsOnlyOnLoopback(addr string) bool {
 	}
 	return ip.IsLoopback()
 }
+
+func validateServerPort(port *int64) error {
+	// It cannot be below 1024 because the container is not running as root.
+	if *port < 1024 || *port > 65535 {
+		return constable.Error("must be within range 1024 to 65535")
+	}
+	return nil
+}

internal/config/supervisor/config_test.go

@@ -43,6 +43,7 @@ func TestFromPath(t *testing.T) {
 					address: 127.0.0.1:1234
 				insecureAcceptExternalUnencryptedHttpRequests: false
 				logLevel: trace
+				aggregatedAPIServerPort: 12345
 			`),
 			wantConfig: &Config{
 				APIGroupSuffix: pointer.StringPtr("some.suffix.com"),
@@ -68,6 +69,7 @@ func TestFromPath(t *testing.T) {
 				Log: plog.LogSpec{
 					Level: plog.LevelTrace,
 				},
+				AggregatedAPIServerPort: pointer.Int64Ptr(12345),
 			},
 		},
 		{
@@ -91,6 +93,7 @@ func TestFromPath(t *testing.T) {
 				log:
 				  level: info
 				  format: text
+				aggregatedAPIServerPort: 12345
 			`),
 			wantConfig: &Config{
 				APIGroupSuffix: pointer.StringPtr("some.suffix.com"),
@@ -116,6 +119,7 @@ func TestFromPath(t *testing.T) {
 					Level:  plog.LevelInfo,
 					Format: plog.FormatText,
 				},
+				AggregatedAPIServerPort: pointer.Int64Ptr(12345),
 			},
 		},
 		{
@@ -166,6 +170,7 @@ func TestFromPath(t *testing.T) {
 					Level:  plog.LevelTrace,
 					Format: plog.FormatText,
 				},
+				AggregatedAPIServerPort: pointer.Int64Ptr(10250),
 			},
 		},
 		{
@@ -202,7 +207,8 @@ func TestFromPath(t *testing.T) {
 						Network: "disabled",
 					},
 				},
-				AllowExternalHTTP: false,
+				AllowExternalHTTP:       false,
+				AggregatedAPIServerPort: pointer.Int64Ptr(10250),
 			},
 		},
 		{
@@ -332,7 +338,8 @@ func TestFromPath(t *testing.T) {
 						Address: ":1234",
 					},
 				},
-				AllowExternalHTTP: true,
+				AllowExternalHTTP:       true,
+				AggregatedAPIServerPort: pointer.Int64Ptr(10250),
 			},
 		},
 		{
@@ -363,7 +370,8 @@ func TestFromPath(t *testing.T) {
 						Address: ":1234",
 					},
 				},
-				AllowExternalHTTP: true,
+				AllowExternalHTTP:       true,
+				AggregatedAPIServerPort: pointer.Int64Ptr(10250),
 			},
 		},
 		{
@@ -420,6 +428,22 @@ func TestFromPath(t *testing.T) {
 			`),
 			wantError: "validate apiGroupSuffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')",
 		},
+		{
+			name: "AggregatedAPIServerPortDefault too small",
+			yaml: here.Doc(`
+				---
+				aggregatedAPIServerPort: 1023
+			`),
+			wantError: "validate aggregatedAPIServerPort: must be within range 1024 to 65535",
+		},
+		{
+			name: "AggregatedAPIServerPortDefault too large",
+			yaml: here.Doc(`
+				---
+				aggregatedAPIServerPort: 65536
+			`),
+			wantError: "validate aggregatedAPIServerPort: must be within range 1024 to 65535",
+		},
 	}
 	for _, test := range tests {
 		test := test

internal/config/supervisor/types.go

@@ -15,10 +15,11 @@ type Config struct {
 	Labels         map[string]string `json:"labels"`
 	NamesConfig    NamesConfigSpec   `json:"names"`
 	// Deprecated: use log.level instead
-	LogLevel          *plog.LogLevel     `json:"logLevel"`
-	Log               plog.LogSpec       `json:"log"`
-	Endpoints         *Endpoints         `json:"endpoints"`
-	AllowExternalHTTP stringOrBoolAsBool `json:"insecureAcceptExternalUnencryptedHttpRequests"`
+	LogLevel                *plog.LogLevel     `json:"logLevel"`
+	Log                     plog.LogSpec       `json:"log"`
+	Endpoints               *Endpoints         `json:"endpoints"`
+	AllowExternalHTTP       stringOrBoolAsBool `json:"insecureAcceptExternalUnencryptedHttpRequests"`
+	AggregatedAPIServerPort *int64             `json:"aggregatedAPIServerPort"`
 }
 
 // NamesConfigSpec configures the names of some Kubernetes resources for the Supervisor.