Change to camel-case for insecureAcceptExternalUnencryptedHttpRequests

- Use camel-case in the static configmap
- Parse the value into a boolean in the go struct instead of a string
- Add test for when unsupported value is used in the configmap
- Run the config_test.go tests in parallel
- Update some paragraphs in configure-supervisor.md for clarity

deploy/supervisor/helpers.lib.yaml

@@ -52,7 +52,7 @@ _: #@ template.replace(data.values.custom_labels)
 #@       "defaultTLSCertificateSecret": defaultResourceNameWithSuffix("default-tls-certificate"),
 #@     },
 #@     "labels": labels(),
-#@     "insecure_accept_external_unencrypted_http_requests": data.values.deprecated_insecure_accept_external_unencrypted_http_requests
+#@     "insecureAcceptExternalUnencryptedHttpRequests": data.values.deprecated_insecure_accept_external_unencrypted_http_requests
 #@   }
 #@   if data.values.log_level:
 #@     config["logLevel"] = getAndValidateLogLevel()

deploy/supervisor/values.yaml

@@ -122,7 +122,7 @@ endpoints:
 #! interface, including interfaces that are listening for traffic from outside the pod. This value is being introduced
 #! to ease the transition to the new loopback interface validation for the HTTP port for any users who need more time
 #! to change their ingress strategy to avoid using plain HTTP into the Supervisor pods.
-#! This value is being introduced and immediately deprecated. It will be removed in some future release, at which time
+#! This value is immediately deprecated upon its introduction. It will be removed in some future release, at which time
 #! traffic from outside the pod will need to be sent to the HTTPS listener instead, with no simple workaround available.
 #! Allowed values are true (boolean), "true" (string), false (boolean), and "false" (string). The default is false.
 #! Optional.

internal/config/supervisor/config.go

@@ -131,9 +131,9 @@ func validateEndpoint(endpoint Endpoint) error {
 	}
 }
 
-func validateAdditionalHTTPEndpointRequirements(endpoint Endpoint, allowExternalHTTP string) error {
+func validateAdditionalHTTPEndpointRequirements(endpoint Endpoint, allowExternalHTTP stringOrBoolAsBool) error {
 	if endpoint.Network == NetworkTCP && !addrIsOnlyOnLoopback(endpoint.Address) {
-		if allowExternalHTTP == "true" {
+		if allowExternalHTTP {
 			// Log that the validation should have been triggered.
 			plog.Warning("Listening on non-loopback interfaces for the HTTP port is deprecated and will be removed " +
 				"in a future release. Your current configuration would not be allowed in that future release. " +

internal/config/supervisor/config_test.go

@@ -39,7 +39,7 @@ func TestFromPath(t *testing.T) {
 				  http:
 				    network: tcp
 					address: 127.0.0.1:1234
-				insecure_accept_external_unencrypted_http_requests: false
+				insecureAcceptExternalUnencryptedHttpRequests: false
 			`),
 			wantConfig: &Config{
 				APIGroupSuffix: pointer.StringPtr("some.suffix.com"),
@@ -60,7 +60,7 @@ func TestFromPath(t *testing.T) {
 						Address: "127.0.0.1:1234",
 					},
 				},
-				AllowExternalHTTP: "false",
+				AllowExternalHTTP: false,
 			},
 		},
 		{
@@ -85,7 +85,7 @@ func TestFromPath(t *testing.T) {
 						Network: "disabled",
 					},
 				},
-				AllowExternalHTTP: "",
+				AllowExternalHTTP: false,
 			},
 		},
 		{
@@ -131,7 +131,7 @@ func TestFromPath(t *testing.T) {
 			wantError: `validate http endpoint: unknown network "bar"`,
 		},
 		{
-			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecure_accept_external_unencrypted_http_requests missing",
+			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecureAcceptExternalUnencryptedHttpRequests missing",
 			yaml: here.Doc(`
 				---
 				names:
@@ -146,7 +146,7 @@ func TestFromPath(t *testing.T) {
 			wantError: `validate http endpoint: http listener address ":8080" for "tcp" network may only bind to loopback interfaces`,
 		},
 		{
-			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecure_accept_external_unencrypted_http_requests set to boolean false",
+			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecureAcceptExternalUnencryptedHttpRequests set to boolean false",
 			yaml: here.Doc(`
 				---
 				names:
@@ -157,12 +157,22 @@ func TestFromPath(t *testing.T) {
 				  http:
 				    network: tcp
 					address: :8080
-				insecure_accept_external_unencrypted_http_requests: false
+				insecureAcceptExternalUnencryptedHttpRequests: false
 			`),
 			wantError: `validate http endpoint: http listener address ":8080" for "tcp" network may only bind to loopback interfaces`,
 		},
 		{
-			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecure_accept_external_unencrypted_http_requests set to string false",
+			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecureAcceptExternalUnencryptedHttpRequests set to unsupported value",
+			yaml: here.Doc(`
+				---
+				names:
+				  defaultTLSCertificateSecret: my-secret-name
+				insecureAcceptExternalUnencryptedHttpRequests: "garbage" # this will be treated as the default, which is false
+			`),
+			wantError: `decode yaml: error unmarshaling JSON: while decoding JSON: invalid value for boolean`,
+		},
+		{
+			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecureAcceptExternalUnencryptedHttpRequests set to string false",
 			yaml: here.Doc(`
 				---
 				names:
@@ -173,12 +183,12 @@ func TestFromPath(t *testing.T) {
 				  http:
 				    network: tcp
 					address: :8080
-				insecure_accept_external_unencrypted_http_requests: "false"
+				insecureAcceptExternalUnencryptedHttpRequests: "false"
 			`),
 			wantError: `validate http endpoint: http listener address ":8080" for "tcp" network may only bind to loopback interfaces`,
 		},
 		{
-			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecure_accept_external_unencrypted_http_requests set to boolean true",
+			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecureAcceptExternalUnencryptedHttpRequests set to boolean true",
 			yaml: here.Doc(`
 				---
 				names:
@@ -187,7 +197,7 @@ func TestFromPath(t *testing.T) {
 				  http:
 				    network: tcp
 					address: :1234
-				insecure_accept_external_unencrypted_http_requests: true
+				insecureAcceptExternalUnencryptedHttpRequests: true
 			`),
 			wantConfig: &Config{
 				APIGroupSuffix: pointer.StringPtr("pinniped.dev"),
@@ -205,11 +215,11 @@ func TestFromPath(t *testing.T) {
 						Address: ":1234",
 					},
 				},
-				AllowExternalHTTP: "true",
+				AllowExternalHTTP: true,
 			},
 		},
 		{
-			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecure_accept_external_unencrypted_http_requests set to string true",
+			name: "http endpoint uses tcp but binds to more than only loopback interfaces with insecureAcceptExternalUnencryptedHttpRequests set to string true",
 			yaml: here.Doc(`
 				---
 				names:
@@ -218,7 +228,7 @@ func TestFromPath(t *testing.T) {
 				  http:
 				    network: tcp
 					address: :1234
-				insecure_accept_external_unencrypted_http_requests: "true"
+				insecureAcceptExternalUnencryptedHttpRequests: "true"
 			`),
 			wantConfig: &Config{
 				APIGroupSuffix: pointer.StringPtr("pinniped.dev"),
@@ -236,7 +246,7 @@ func TestFromPath(t *testing.T) {
 						Address: ":1234",
 					},
 				},
-				AllowExternalHTTP: "true",
+				AllowExternalHTTP: true,
 			},
 		},
 		{
@@ -297,6 +307,8 @@ func TestFromPath(t *testing.T) {
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
 			// Write yaml to temp file
 			f, err := ioutil.TempFile("", "pinniped-test-config-yaml-*")
 			require.NoError(t, err)
@@ -388,6 +400,8 @@ func TestAddrIsOnlyOnLoopback(t *testing.T) {
 	for _, test := range tests {
 		tt := test
 		t.Run(fmt.Sprintf("address %s should be %t", tt.addr, tt.want), func(t *testing.T) {
+			t.Parallel()
+
 			require.Equal(t, tt.want, addrIsOnlyOnLoopback(tt.addr))
 		})
 	}

internal/config/supervisor/types.go

@@ -3,16 +3,20 @@
 
 package supervisor
 
-import "go.pinniped.dev/internal/plog"
+import (
+	"errors"
+
+	"go.pinniped.dev/internal/plog"
+)
 
 // Config contains knobs to setup an instance of the Pinniped Supervisor.
 type Config struct {
-	APIGroupSuffix    *string           `json:"apiGroupSuffix,omitempty"`
+	APIGroupSuffix    *string            `json:"apiGroupSuffix,omitempty"`
-	Labels            map[string]string `json:"labels"`
+	Labels            map[string]string  `json:"labels"`
-	NamesConfig       NamesConfigSpec   `json:"names"`
+	NamesConfig       NamesConfigSpec    `json:"names"`
-	LogLevel          plog.LogLevel     `json:"logLevel"`
+	LogLevel          plog.LogLevel      `json:"logLevel"`
-	Endpoints         *Endpoints        `json:"endpoints"`
+	Endpoints         *Endpoints         `json:"endpoints"`
-	AllowExternalHTTP string            `json:"insecure_accept_external_unencrypted_http_requests"`
+	AllowExternalHTTP stringOrBoolAsBool `json:"insecureAcceptExternalUnencryptedHttpRequests"`
 }
 
 // NamesConfigSpec configures the names of some Kubernetes resources for the Supervisor.
@@ -29,3 +33,17 @@ type Endpoint struct {
 	Network string `json:"network"`
 	Address string `json:"address"`
 }
+
+type stringOrBoolAsBool bool
+
+func (sb *stringOrBoolAsBool) UnmarshalJSON(b []byte) error {
+	switch string(b) {
+	case "true", `"true"`:
+		*sb = true
+	case "false", `"false"`:
+		*sb = false
+	default:
+		return errors.New("invalid value for boolean")
+	}
+	return nil
+}

site/content/docs/howto/configure-supervisor.md

@@ -29,13 +29,17 @@ It is recommended that the traffic to these endpoints should be encrypted via TL
 Supervisor pods, even when crossing boundaries that are entirely inside the Kubernetes cluster.
 The credentials and tokens that are handled by these endpoints are too sensitive to transmit without encryption.
 
-In all versions of the Supervisor app so far, there are both HTTP and HTTPS ports available for use by default.
+In previous versions of the Supervisor app, there were both HTTP and HTTPS ports available for use by default.
 These ports each host all the Supervisor's endpoints. Unfortunately, this has caused some confusion in the community
 and some blog posts have been written which demonstrate using the HTTP port in such a way that a portion of the traffic's
-path is unencrypted. **Anything which exposes the non-TLS HTTP port outside the Pod should be considered deprecated**.
+path is unencrypted. Newer versions of the Supervisor disable the HTTP port by default to make it more clear that
-A future version of the Supervisor app may include a breaking change to adjust the default behavior of the HTTP port
+the Supervisor app is not intended to receive non-TLS HTTP traffic from outside the Pod. Furthermore, in these newer versions,
-to only listen on 127.0.0.1 (or perhaps even to be disabled) to make it more clear that the Supervisor app is not intended
+when the HTTP listener is configured to be enabled it may only listen on loopback interfaces for traffic from within its own pod.
-to receive non-TLS HTTP traffic from outside the Pod.
+To aid in transition for impacted users, the old behavior of allowing the HTTP listener to receive traffic from
+outside the pod may be re-enabled using the
+`deprecated_insecure_accept_external_unencrypted_http_requests` value in
+[values.yaml](https://github.com/vmware-tanzu/pinniped/blob/main/deploy/supervisor/values.yaml),
+until that setting is removed in a future release.
 
 Because there are many ways to expose TLS services from a Kubernetes cluster, the Supervisor app leaves this up to the user.
 The most common ways are:
@@ -54,12 +58,9 @@ The most common ways are:
    traffic using TLS on the backend (upstream) into the Supervisor's Pods. It would not be secure for the OIDC protocol
    to use HTTP, because the user's secret OIDC tokens would be transmitted across the network without encryption.
    If your Ingress controller does not support this feature, then consider using one of the other configurations
-   described here instead of using an Ingress. The backend of the Ingress would typically point to a NodePort or
+   described here instead of using an Ingress. (Please refer to the paragraph above regarding the deprecation of the HTTP listener for more
-   LoadBalancer Service which exposes the HTTPS port 8443 of the Supervisor pods. (Using plain HTTP from the Ingress
+   information.) The backend of the Ingress would typically point to a NodePort or LoadBalancer Service which exposes
-   to the Supervisor pods is deprecated and will be removed in a future release. To aid in transition, it may be enabled
+   the HTTPS port 8443 of the Supervisor pods.
-   using the `insecure_accept_external_unencrypted_http_requests` value in
-   [values.yaml](https://github.com/vmware-tanzu/pinniped/blob/main/deploy/supervisor/values.yaml),
-   until that setting is removed in a future release.)
 
    The required configuration of the Ingress is specific to your cluster's Ingress Controller, so please refer to the
    documentation from your Kubernetes provider. If you are using a cluster from a cloud provider, then you'll probably
@@ -81,24 +82,25 @@ The most common ways are:
 
    If your service mesh is capable of transparently encrypting traffic all the way into the
    Supervisor Pods, then you should use that capability. In this case, it may make sense to configure the Supervisor's
-   HTTP port to only listen on a Unix domain socket, such as when the service mesh injects a sidecar container that can
+   HTTP port to listen on a Unix domain socket, such as when the service mesh injects a sidecar container that can
-   securely access the socket from within the same Pod.
+   securely access the socket from within the same Pod. Alternatively, the HTTP port can be configured as a TCP listener
+   on loopback interfaces to receive traffic from sidecar containers.
    See the `endpoints` option in [deploy/supervisor/values.yml](https://github.com/vmware-tanzu/pinniped/blob/main/deploy/supervisor/values.yaml)
    for more information.
-   This would prevent any unencrypted traffic from accidentally being transmitted from outside the Pod into the
+   Using either a Unix domain socket or a loopback interface listener would prevent any unencrypted traffic from
-   Supervisor app's HTTP port.
+   accidentally being transmitted from outside the Pod into the Supervisor app's HTTP port.
 
    For example, the following high level steps cover configuring Istio for use with the Supervisor:
 
-   - Update the http listener to use a Unix domain socket
+   - Update the HTTP listener to use a Unix domain socket
      i.e. `--data-value-yaml 'endpoints={"http":{"network":"unix","address":"/pinniped_socket/socketfile.sock"}}'`
    - Arrange for the Istio sidecar to be injected into the Supervisor app with an appropriate `IstioIngressListener`
      i.e `defaultEndpoint: unix:///pinniped_socket/socketfile.sock`
    - Mount the socket volume into the Istio sidecar container by including the appropriate annotation on the Supervisor pods
      i.e. `sidecar.istio.io/userVolumeMount: '{"socket":{"mountPath":"/pinniped_socket"}}'`
-   - Disable the https listener and update the deployment health checks as desired
+   - Disable the HTTPS listener and update the deployment health checks as desired
 
-   For service meshes that do not support Unix domain sockets, the http listener should be configured to listen on 127.0.0.1.
+   For service meshes that do not support Unix domain sockets, the HTTP listener should be configured as a TCP listener on a loopback interface.
 
 ## Creating a Service to expose the Supervisor app's endpoints within the cluster