diff --git a/.changelog/4152.txt b/.changelog/4152.txt new file mode 100644 index 0000000000..7dbc369814 --- /dev/null +++ b/.changelog/4152.txt @@ -0,0 +1,7 @@ +```release-note:improvement +control-plane: Remove anyuid Security Context Constraints (SCC) requirement in OpenShift. +``` + +```release-note:bug +connect-inject: add NET_BIND_SERVICE capability when injecting consul-dataplane sidecar +``` diff --git a/acceptance/framework/consul/helm_cluster.go b/acceptance/framework/consul/helm_cluster.go index e12456876e..f23e55a48b 100644 --- a/acceptance/framework/consul/helm_cluster.go +++ b/acceptance/framework/consul/helm_cluster.go @@ -549,7 +549,6 @@ func (h *HelmCluster) SetupConsulClient(t *testing.T, secure bool, release ...st require.NoError(r, err) } }) - } } @@ -701,47 +700,40 @@ func configureNamespace(t *testing.T, client kubernetes.Interface, cfg *config.T } // configureSCCs creates RoleBindings that bind the default service account to cluster roles -// allowing access to the anyuid and privileged Security Context Constraints on OpenShift. +// allowing access to the privileged Security Context Constraints on OpenShift. func configureSCCs(t *testing.T, client kubernetes.Interface, cfg *config.TestConfig, namespace string) { - const anyuidClusterRole = "system:openshift:scc:anyuid" const privilegedClusterRole = "system:openshift:scc:privileged" - anyuidRoleBinding := "anyuid-test" privilegedRoleBinding := "privileged-test" // A role binding to allow default service account in the installation namespace access to the SCCs. - { - for clusterRoleName, roleBindingName := range map[string]string{anyuidClusterRole: anyuidRoleBinding, privilegedClusterRole: privilegedRoleBinding} { - // Check if this cluster role binding already exists. - _, err := client.RbacV1().RoleBindings(namespace).Get(context.Background(), roleBindingName, metav1.GetOptions{}) - - if errors.IsNotFound(err) { - roleBinding := &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: roleBindingName, - }, - Subjects: []rbacv1.Subject{ - { - Kind: rbacv1.ServiceAccountKind, - Name: "default", - Namespace: namespace, - }, - }, - RoleRef: rbacv1.RoleRef{ - Kind: "ClusterRole", - Name: clusterRoleName, - }, - } - - _, err = client.RbacV1().RoleBindings(namespace).Create(context.Background(), roleBinding, metav1.CreateOptions{}) - require.NoError(t, err) - } else { - require.NoError(t, err) - } + // Check if this cluster role binding already exists. + _, err := client.RbacV1().RoleBindings(namespace).Get(context.Background(), privilegedRoleBinding, metav1.GetOptions{}) + + if errors.IsNotFound(err) { + roleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: privilegedRoleBinding, + }, + Subjects: []rbacv1.Subject{ + { + Kind: rbacv1.ServiceAccountKind, + Name: "default", + Namespace: namespace, + }, + }, + RoleRef: rbacv1.RoleRef{ + Kind: "ClusterRole", + Name: privilegedClusterRole, + }, } + + _, err = client.RbacV1().RoleBindings(namespace).Create(context.Background(), roleBinding, metav1.CreateOptions{}) + require.NoError(t, err) + } else { + require.NoError(t, err) } helpers.Cleanup(t, cfg.NoCleanupOnFailure, cfg.NoCleanup, func() { - _ = client.RbacV1().RoleBindings(namespace).Delete(context.Background(), anyuidRoleBinding, metav1.DeleteOptions{}) _ = client.RbacV1().RoleBindings(namespace).Delete(context.Background(), privilegedRoleBinding, metav1.DeleteOptions{}) }) } diff --git a/acceptance/tests/api-gateway/api_gateway_kitchen_sink_test.go b/acceptance/tests/api-gateway/api_gateway_kitchen_sink_test.go index d701220a8c..9880298a2b 100644 --- a/acceptance/tests/api-gateway/api_gateway_kitchen_sink_test.go +++ b/acceptance/tests/api-gateway/api_gateway_kitchen_sink_test.go @@ -142,6 +142,10 @@ func TestAPIGateway_KitchenSink(t *testing.T) { checkStatusCondition(r, gateway.Status.Conditions, trueCondition("ConsulAccepted", "Accepted")) require.Len(r, gateway.Status.Listeners, 2) + // http route checks + err = k8sClient.Get(context.Background(), types.NamespacedName{Name: "http-route", Namespace: "default"}, &httpRoute) + require.NoError(r, err) + require.EqualValues(r, int32(1), gateway.Status.Listeners[0].AttachedRoutes) checkStatusCondition(r, gateway.Status.Listeners[0].Conditions, trueCondition("Accepted", "Accepted")) checkStatusCondition(r, gateway.Status.Listeners[0].Conditions, falseCondition("Conflicted", "NoConflicts")) @@ -152,10 +156,6 @@ func TestAPIGateway_KitchenSink(t *testing.T) { // now we know we have an address, set it so we can use it gatewayAddress = gateway.Status.Addresses[0].Value - // http route checks - err = k8sClient.Get(context.Background(), types.NamespacedName{Name: "http-route", Namespace: "default"}, &httpRoute) - require.NoError(r, err) - // check our finalizers require.Len(r, httpRoute.Finalizers, 1) require.EqualValues(r, gatewayFinalizer, httpRoute.Finalizers[0]) diff --git a/acceptance/tests/fixtures/bases/multiport-app/anyuid-scc-rolebinding.yaml b/acceptance/tests/fixtures/bases/multiport-app/anyuid-scc-rolebinding.yaml deleted file mode 100644 index 5c2e0dcfa2..0000000000 --- a/acceptance/tests/fixtures/bases/multiport-app/anyuid-scc-rolebinding.yaml +++ /dev/null @@ -1,26 +0,0 @@ -# Copyright (c) HashiCorp, Inc. -# SPDX-License-Identifier: MPL-2.0 - -apiVersion: rbac.authorization.k8s.io/v1 -kind: RoleBinding -metadata: - name: multiport-openshift-anyuid -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: system:openshift:scc:anyuid -subjects: - - kind: ServiceAccount - name: multiport ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: RoleBinding -metadata: - name: multiport-admin-openshift-anyuid -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: system:openshift:scc:anyuid -subjects: - - kind: ServiceAccount - name: multiport-admin diff --git a/acceptance/tests/fixtures/bases/multiport-app/kustomization.yaml b/acceptance/tests/fixtures/bases/multiport-app/kustomization.yaml index fb792d63a7..ecd2015a34 100644 --- a/acceptance/tests/fixtures/bases/multiport-app/kustomization.yaml +++ b/acceptance/tests/fixtures/bases/multiport-app/kustomization.yaml @@ -7,5 +7,4 @@ resources: - secret.yaml - serviceaccount.yaml - psp-rolebinding.yaml - - anyuid-scc-rolebinding.yaml - - privileged-scc-rolebinding.yaml \ No newline at end of file + - privileged-scc-rolebinding.yaml diff --git a/acceptance/tests/fixtures/bases/static-client/anyuid-scc-rolebinding.yaml b/acceptance/tests/fixtures/bases/static-client/anyuid-scc-rolebinding.yaml deleted file mode 100644 index b80bc5c562..0000000000 --- a/acceptance/tests/fixtures/bases/static-client/anyuid-scc-rolebinding.yaml +++ /dev/null @@ -1,14 +0,0 @@ -# Copyright (c) HashiCorp, Inc. -# SPDX-License-Identifier: MPL-2.0 - -apiVersion: rbac.authorization.k8s.io/v1 -kind: RoleBinding -metadata: - name: static-client-openshift-anyuid -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: system:openshift:scc:anyuid -subjects: - - kind: ServiceAccount - name: static-client \ No newline at end of file diff --git a/acceptance/tests/fixtures/bases/static-client/kustomization.yaml b/acceptance/tests/fixtures/bases/static-client/kustomization.yaml index 9aa0009dc4..929d64ac24 100644 --- a/acceptance/tests/fixtures/bases/static-client/kustomization.yaml +++ b/acceptance/tests/fixtures/bases/static-client/kustomization.yaml @@ -6,5 +6,4 @@ resources: - service.yaml - serviceaccount.yaml - psp-rolebinding.yaml - - anyuid-scc-rolebinding.yaml - - privileged-scc-rolebinding.yaml \ No newline at end of file + - privileged-scc-rolebinding.yaml diff --git a/acceptance/tests/fixtures/bases/static-server-https/anyuid-scc-rolebinding.yaml b/acceptance/tests/fixtures/bases/static-server-https/anyuid-scc-rolebinding.yaml deleted file mode 100644 index 2be7cf13db..0000000000 --- a/acceptance/tests/fixtures/bases/static-server-https/anyuid-scc-rolebinding.yaml +++ /dev/null @@ -1,14 +0,0 @@ -# Copyright (c) HashiCorp, Inc. -# SPDX-License-Identifier: MPL-2.0 - -apiVersion: rbac.authorization.k8s.io/v1 -kind: RoleBinding -metadata: - name: static-server-openshift-anyuid -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: system:openshift:scc:anyuid -subjects: - - kind: ServiceAccount - name: static-server \ No newline at end of file diff --git a/acceptance/tests/fixtures/bases/static-server-https/kustomization.yaml b/acceptance/tests/fixtures/bases/static-server-https/kustomization.yaml index da166af201..6d7daa8f88 100644 --- a/acceptance/tests/fixtures/bases/static-server-https/kustomization.yaml +++ b/acceptance/tests/fixtures/bases/static-server-https/kustomization.yaml @@ -7,5 +7,4 @@ resources: - service.yaml - serviceaccount.yaml - psp-rolebinding.yaml - - anyuid-scc-rolebinding.yaml - - privileged-scc-rolebinding.yaml \ No newline at end of file + - privileged-scc-rolebinding.yaml diff --git a/acceptance/tests/fixtures/bases/static-server-tcp/anyuid-scc-rolebinding.yaml b/acceptance/tests/fixtures/bases/static-server-tcp/anyuid-scc-rolebinding.yaml deleted file mode 100644 index eb86dc8bae..0000000000 --- a/acceptance/tests/fixtures/bases/static-server-tcp/anyuid-scc-rolebinding.yaml +++ /dev/null @@ -1,14 +0,0 @@ -# Copyright (c) HashiCorp, Inc. -# SPDX-License-Identifier: MPL-2.0 - -apiVersion: rbac.authorization.k8s.io/v1 -kind: RoleBinding -metadata: - name: static-server-tcp-openshift-anyuid -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: system:openshift:scc:anyuid -subjects: - - kind: ServiceAccount - name: static-server-tcp \ No newline at end of file diff --git a/acceptance/tests/fixtures/bases/static-server-tcp/kustomization.yaml b/acceptance/tests/fixtures/bases/static-server-tcp/kustomization.yaml index 2180aa94e1..946e8d6b68 100644 --- a/acceptance/tests/fixtures/bases/static-server-tcp/kustomization.yaml +++ b/acceptance/tests/fixtures/bases/static-server-tcp/kustomization.yaml @@ -7,5 +7,4 @@ resources: - serviceaccount.yaml - servicedefaults.yaml - psp-rolebinding.yaml - - anyuid-scc-rolebinding.yaml - - privileged-scc-rolebinding.yaml \ No newline at end of file + - privileged-scc-rolebinding.yaml diff --git a/acceptance/tests/fixtures/bases/static-server/anyuid-scc-rolebinding.yaml b/acceptance/tests/fixtures/bases/static-server/anyuid-scc-rolebinding.yaml deleted file mode 100644 index 2be7cf13db..0000000000 --- a/acceptance/tests/fixtures/bases/static-server/anyuid-scc-rolebinding.yaml +++ /dev/null @@ -1,14 +0,0 @@ -# Copyright (c) HashiCorp, Inc. -# SPDX-License-Identifier: MPL-2.0 - -apiVersion: rbac.authorization.k8s.io/v1 -kind: RoleBinding -metadata: - name: static-server-openshift-anyuid -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: system:openshift:scc:anyuid -subjects: - - kind: ServiceAccount - name: static-server \ No newline at end of file diff --git a/acceptance/tests/fixtures/bases/static-server/kustomization.yaml b/acceptance/tests/fixtures/bases/static-server/kustomization.yaml index 9aa0009dc4..929d64ac24 100644 --- a/acceptance/tests/fixtures/bases/static-server/kustomization.yaml +++ b/acceptance/tests/fixtures/bases/static-server/kustomization.yaml @@ -6,5 +6,4 @@ resources: - service.yaml - serviceaccount.yaml - psp-rolebinding.yaml - - anyuid-scc-rolebinding.yaml - - privileged-scc-rolebinding.yaml \ No newline at end of file + - privileged-scc-rolebinding.yaml diff --git a/acceptance/tests/fixtures/bases/v2-multiport-app/anyuid-scc-rolebinding.yaml b/acceptance/tests/fixtures/bases/v2-multiport-app/anyuid-scc-rolebinding.yaml deleted file mode 100644 index 5c2e0dcfa2..0000000000 --- a/acceptance/tests/fixtures/bases/v2-multiport-app/anyuid-scc-rolebinding.yaml +++ /dev/null @@ -1,26 +0,0 @@ -# Copyright (c) HashiCorp, Inc. -# SPDX-License-Identifier: MPL-2.0 - -apiVersion: rbac.authorization.k8s.io/v1 -kind: RoleBinding -metadata: - name: multiport-openshift-anyuid -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: system:openshift:scc:anyuid -subjects: - - kind: ServiceAccount - name: multiport ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: RoleBinding -metadata: - name: multiport-admin-openshift-anyuid -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: system:openshift:scc:anyuid -subjects: - - kind: ServiceAccount - name: multiport-admin diff --git a/acceptance/tests/fixtures/bases/v2-multiport-app/kustomization.yaml b/acceptance/tests/fixtures/bases/v2-multiport-app/kustomization.yaml index fb792d63a7..ecd2015a34 100644 --- a/acceptance/tests/fixtures/bases/v2-multiport-app/kustomization.yaml +++ b/acceptance/tests/fixtures/bases/v2-multiport-app/kustomization.yaml @@ -7,5 +7,4 @@ resources: - secret.yaml - serviceaccount.yaml - psp-rolebinding.yaml - - anyuid-scc-rolebinding.yaml - - privileged-scc-rolebinding.yaml \ No newline at end of file + - privileged-scc-rolebinding.yaml diff --git a/control-plane/api-gateway/gatekeeper/deployment.go b/control-plane/api-gateway/gatekeeper/deployment.go index 61ef1fd318..9519a42d74 100644 --- a/control-plane/api-gateway/gatekeeper/deployment.go +++ b/control-plane/api-gateway/gatekeeper/deployment.go @@ -88,7 +88,7 @@ func (g *Gatekeeper) deleteDeployment(ctx context.Context, gwName types.Namespac } func (g *Gatekeeper) deployment(gateway gwv1beta1.Gateway, gcc v1alpha1.GatewayClassConfig, config common.HelmConfig, currentReplicas *int32) (*appsv1.Deployment, error) { - initContainer, err := initContainer(config, gateway.Name, gateway.Namespace) + initContainer, err := g.initContainer(config, gateway.Name, gateway.Namespace) if err != nil { return nil, err } diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index cf8c325364..f6342ec725 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -29,6 +29,13 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" ) +const ( + designatedOpenShiftUIDRange = "1000700000/100000" + designatedOpenShiftGIDRange = "1000700000/100000" + expectedOpenShiftInitContainerUID = 1000799999 + expectedOpenShiftInitContainerGID = 1000799999 +) + var ( createdAtLabelKey = "gateway.consul.hashicorp.com/created" createdAtLabelValue = "101010" @@ -929,7 +936,23 @@ func TestUpsert(t *testing.T) { EnableOpenShift: true, ImageDataplane: "hashicorp/consul-dataplane", }, - initialResources: resources{}, + initialResources: resources{ + namespaces: []*corev1.Namespace{ + { + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Namespace", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: designatedOpenShiftUIDRange, + constants.AnnotationOpenShiftGroups: designatedOpenShiftGIDRange, + }, + }, + }, + }, + }, finalResources: resources{ deployments: []*appsv1.Deployment{ configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), @@ -1463,6 +1486,16 @@ func validateResourcesExist(t *testing.T, client client.Client, helmConfig commo assert.Equal(t, helmConfig.InitContainerResources.Limits, container.Resources.Limits) assert.Equal(t, helmConfig.InitContainerResources.Requests, container.Resources.Requests) } + + require.NotNil(t, container.SecurityContext.RunAsUser) + require.NotNil(t, container.SecurityContext.RunAsGroup) + if helmConfig.EnableOpenShift { + assert.EqualValues(t, *container.SecurityContext.RunAsUser, expectedOpenShiftInitContainerUID) + assert.EqualValues(t, *container.SecurityContext.RunAsGroup, expectedOpenShiftInitContainerGID) + } else { + assert.EqualValues(t, *container.SecurityContext.RunAsUser, initContainersUserAndGroupID) + assert.EqualValues(t, *container.SecurityContext.RunAsGroup, initContainersUserAndGroupID) + } } } assert.True(t, hasInitContainer) diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go index b30bafc240..875e15dff3 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -5,15 +5,18 @@ package gatekeeper import ( "bytes" + "context" + "fmt" "strconv" "strings" "text/template" corev1 "k8s.io/api/core/v1" - "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" + ctrlCommon "github.com/hashicorp/consul-k8s/control-plane/connect-inject/common" "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" "github.com/hashicorp/consul-k8s/control-plane/namespaces" ) @@ -35,7 +38,7 @@ type initContainerCommandData struct { // containerInit returns the init container spec for connect-init that polls for the service and the connect proxy service to be registered // so that it can save the proxy service id to the shared volume and boostrap Envoy with the proxy-id. -func initContainer(config common.HelmConfig, name, namespace string) (corev1.Container, error) { +func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace string) (corev1.Container, error) { data := initContainerCommandData{ AuthMethod: config.AuthMethod, LogLevel: config.LogLevel, @@ -175,9 +178,34 @@ func initContainer(config common.HelmConfig, name, namespace string) (corev1.Con container.Resources = *config.InitContainerResources } + uid := int64(initContainersUserAndGroupID) + gid := int64(initContainersUserAndGroupID) + + // In Openshift we let Openshift set the UID and GID + if config.EnableOpenShift { + ns := &corev1.Namespace{} + err := g.Client.Get(context.Background(), client.ObjectKey{Name: namespace}, ns) + if err != nil { + g.Log.Error(err, "error fetching namespace metadata for deployment") + return corev1.Container{}, fmt.Errorf("error getting namespace metadata for deployment: %s", err) + } + + // We need to get the userID for the init container. We do not care about what is already defined on the pod + // for gateways, as there is no application container that could have taken a UID. + uid, err = ctrlCommon.GetConnectInitUID(*ns, corev1.Pod{}, config.ImageDataplane, config.ImageConsulK8S) + if err != nil { + return corev1.Container{}, err + } + + gid, err = ctrlCommon.GetConnectInitGroupID(*ns, corev1.Pod{}, config.ImageDataplane, config.ImageConsulK8S) + if err != nil { + return corev1.Container{}, err + } + } + container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: pointer.Int64(initContainersUserAndGroupID), - RunAsGroup: pointer.Int64(initContainersUserAndGroupID), + RunAsUser: pointer.Int64(uid), + RunAsGroup: pointer.Int64(gid), RunAsNonRoot: pointer.Bool(true), Privileged: pointer.Bool(false), Capabilities: &corev1.Capabilities{ diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go new file mode 100644 index 0000000000..b9de4c45f4 --- /dev/null +++ b/control-plane/connect-inject/common/openshift.go @@ -0,0 +1,182 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +// A namespace in OpenShift has the following annotations: +// Annotations: openshift.io/sa.scc.mcs: s0:c27,c4 +// openshift.io/sa.scc.uid-range: 1000710000/10000 +// openshift.io/sa.scc.supplemental-groups: 1000710000/10000 +// +// Note: Even though the annotation is named 'range', it is not a range but the ID you should use. All pods in a +// namespace should use the same UID/GID. (1000710000/1000710000 above) + +package common + +import ( + "fmt" + "strconv" + "strings" + + "golang.org/x/exp/maps" + "golang.org/x/exp/slices" + corev1 "k8s.io/api/core/v1" + + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" +) + +// GetDataplaneUID returns the UID to use for the Dataplane container in the given namespace. +// The UID is based on the namespace annotation and avoids conflicting with any application container UIDs. +// Containers with dataplaneImage and k8sImage are not considered application containers. +func GetDataplaneUID(namespace corev1.Namespace, pod corev1.Pod, dataplaneImage, k8sImage string) (int64, error) { + availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftUIDRange, dataplaneImage, k8sImage) + if err != nil { + return 0, err + } + + if len(availableUIDs) < 2 { + return 0, fmt.Errorf("namespace does not have enough available UIDs") + } + + return availableUIDs[len(availableUIDs)-2], nil +} + +// GetDataplaneGroupID returns the group ID to use for the Dataplane container in the given namespace. +// The UID is based on the namespace annotation and avoids conflicting with any application container group IDs. +// Containers with dataplaneImage and k8sImage are not considered application containers. +func GetDataplaneGroupID(namespace corev1.Namespace, pod corev1.Pod, dataplaneImage, k8sImage string) (int64, error) { + availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftGroups, dataplaneImage, k8sImage) + if err != nil { + return 0, err + } + + if len(availableUIDs) < 2 { + return 0, fmt.Errorf("namespace does not have enough available UIDs") + } + + return availableUIDs[len(availableUIDs)-2], nil +} + +// GetConnectInitUID returns the UID to use for the connect init container in the given namespace. +// The UID is based on the namespace annotation and avoids conflicting with any application container UIDs. +// Containers with dataplaneImage and k8sImage are not considered application containers. +func GetConnectInitUID(namespace corev1.Namespace, pod corev1.Pod, dataplaneImage, k8sImage string) (int64, error) { + availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftUIDRange, dataplaneImage, k8sImage) + if err != nil { + return 0, err + } + + if len(availableUIDs) < 1 { + return 0, fmt.Errorf("namespace does not have enough available UIDs") + } + + return availableUIDs[len(availableUIDs)-1], nil +} + +// GetConnectInitGroupID returns the group ID to use for the connect init container in the given namespace. +// The group ID is based on the namespace annotation and avoids conflicting with any application container group IDs. +// Containers with dataplaneImage and k8sImage are not considered application containers. +func GetConnectInitGroupID(namespace corev1.Namespace, pod corev1.Pod, dataplaneImage, k8sImage string) (int64, error) { + availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftGroups, dataplaneImage, k8sImage) + if err != nil { + return 0, err + } + + if len(availableUIDs) < 2 { + return 0, fmt.Errorf("namespace does not have enough available UIDs") + } + + return availableUIDs[len(availableUIDs)-1], nil +} + +// getAvailableIDs enumerates the entire list of available UIDs in the namespace based on the +// OpenShift annotationName provided. It then removes the UIDs that are already in use by application +// containers. Containers with dataplaneImage and k8sImage are not considered application containers. +func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName, dataplaneImage, k8sImage string) ([]int64, error) { + // Collect the list of IDs designated in the Pod for application containers + appUIDs := make([]int64, 0) + if pod.Spec.SecurityContext != nil { + if pod.Spec.SecurityContext.RunAsUser != nil { + appUIDs = append(appUIDs, *pod.Spec.SecurityContext.RunAsUser) + } + } + for _, c := range pod.Spec.Containers { + if c.Image == dataplaneImage || c.Image == k8sImage { + continue + } + + if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil { + appUIDs = append(appUIDs, *c.SecurityContext.RunAsUser) + } + } + + annotationValue := namespace.Annotations[annotationName] + + // Groups can be comma separated ranges, i.e. 100/2,101/2 + // https://docs.openshift.com/container-platform/4.16/authentication/managing-security-context-constraints.html#security-context-constraints-pre-allocated-values_configuring-internal-oauth + ranges := make([]string, 0) + validIDs := make([]int64, 0) + // Collect the list of valid IDs from the namespace annotation + if annotationName == constants.AnnotationOpenShiftGroups { + // Fall back to UID range if Group annotation is not present + if annotationValue == "" { + annotationName = constants.AnnotationOpenShiftUIDRange + annotationValue = namespace.Annotations[annotationName] + } + ranges = strings.Split(annotationValue, ",") + } else { + ranges = append(ranges, annotationValue) + } + + for _, r := range ranges { + rangeIDs, err := getIDsInRange(r) + // call based on length of ranges and merge for groups + if err != nil { + return nil, fmt.Errorf("unable to get valid userIDs from namespace annotation: %w", err) + } + validIDs = append(validIDs, rangeIDs...) + } + + // Subtract the list of application container UIDs from the list of valid userIDs + availableUIDs := make(map[int64]struct{}) + for _, uid := range validIDs { + availableUIDs[uid] = struct{}{} + } + for _, uid := range appUIDs { + delete(availableUIDs, uid) + } + + // Return the second to last (sorted) valid UID from the available UIDs + keys := maps.Keys(availableUIDs) + slices.Sort(keys) + + return keys, nil +} + +// getIDsInRange enumerates the entire list of available IDs given the value of the +// OpenShift annotation. This can be the group or user ID range. +func getIDsInRange(annotation string) ([]int64, error) { + // Add comma and group fallback + parts := strings.Split(annotation, "/") + if len(parts) != 2 { + parts = strings.Split(annotation, "-") + if len(parts) != 2 { + return nil, fmt.Errorf("invalid range format: %s", annotation) + } + } + + start, err := strconv.Atoi(parts[0]) + if err != nil { + return nil, fmt.Errorf("invalid range format: %s", parts[0]) + } + + length, err := strconv.Atoi(parts[1]) + if err != nil { + return nil, fmt.Errorf("invalid range format: %s", parts[1]) + } + + userIDs := make([]int64, length) + for i := 0; i < length; i++ { + userIDs[i] = int64(start + i) + } + + return userIDs, nil +} diff --git a/control-plane/connect-inject/common/openshift_test.go b/control-plane/connect-inject/common/openshift_test.go new file mode 100644 index 0000000000..dd94519b0d --- /dev/null +++ b/control-plane/connect-inject/common/openshift_test.go @@ -0,0 +1,482 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 +package common + +import ( + "fmt" + "testing" + + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" +) + +func TestGetConnectInitIDs(t *testing.T) { + dataplaneImage := "consul-dataplane" + k8sImage := "consul-k8s-control-plane" + cases := []struct { + Name string + Namespace corev1.Namespace + // User IDs and Group IDs are quite often the same, and will be for test purposes + ExpectedDataplaneUserAndGroupIDs int64 + Pod corev1.Pod + Err string + }{ + { + Name: "App using a single ID already", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/5", + constants.AnnotationOpenShiftGroups: "100/5", + }, + }, + }, + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", + }, + }, + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(100), + }, + }, + }, + }, + }, + ExpectedDataplaneUserAndGroupIDs: 104, + Err: "", + }, + { + Name: "App using last ID already", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/5", + constants.AnnotationOpenShiftGroups: "100/5", + }, + }, + }, + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", + }, + }, + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(104), + }, + }, + }, + }, + }, + ExpectedDataplaneUserAndGroupIDs: 103, + Err: "", + }, + { + Name: "Not enough available IDs", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/1", + constants.AnnotationOpenShiftGroups: "100/1", + }, + }, + }, + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", + }, + }, + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(100), + }, + }, + }, + }, + }, + Err: "namespace does not have enough available UIDs", + }, + } + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + // Test UID + actualUIDs, err := GetConnectInitUID(tt.Namespace, tt.Pod, dataplaneImage, k8sImage) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.ExpectedDataplaneUserAndGroupIDs, actualUIDs) + } else { + require.EqualError(err, tt.Err) + } + // Test GroupID + actualGroupIDs, err := GetConnectInitGroupID(tt.Namespace, tt.Pod, dataplaneImage, k8sImage) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.ExpectedDataplaneUserAndGroupIDs, actualGroupIDs) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +func TestGetDataplaneIDs(t *testing.T) { + dataplaneImage := "consul-dataplane" + k8sImage := "consul-k8s-control-plane" + cases := []struct { + Name string + Namespace corev1.Namespace + // User IDs and Group IDs are quite often the same, and will be for test purposes + ExpectedDataplaneUserAndGroupIDs int64 + Pod corev1.Pod + Err string + }{ + { + Name: "App using a single ID already", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/5", + constants.AnnotationOpenShiftGroups: "100/5", + }, + }, + }, + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", + }, + }, + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(100), + }, + }, + }, + }, + }, + ExpectedDataplaneUserAndGroupIDs: 103, + Err: "", + }, + { + Name: "App using last ID already", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/5", + constants.AnnotationOpenShiftGroups: "100/5", + }, + }, + }, + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", + }, + }, + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(104), + }, + }, + }, + }, + }, + ExpectedDataplaneUserAndGroupIDs: 102, + Err: "", + }, + { + Name: "Not enough available IDs", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/2", + constants.AnnotationOpenShiftGroups: "100/2", + }, + }, + }, + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", + }, + }, + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(100), + }, + }, + }, + }, + }, + Err: "namespace does not have enough available UIDs", + }, + } + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + // Test UID + actualUIDs, err := GetDataplaneUID(tt.Namespace, tt.Pod, dataplaneImage, k8sImage) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.ExpectedDataplaneUserAndGroupIDs, actualUIDs) + } else { + require.EqualError(err, tt.Err) + } + // Test GroupID + actualGroupIDs, err := GetDataplaneGroupID(tt.Namespace, tt.Pod, dataplaneImage, k8sImage) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.ExpectedDataplaneUserAndGroupIDs, actualGroupIDs) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +func TestGetAvailableIDs(t *testing.T) { + dataplaneImage := "consul-dataplane" + k8sImage := "consul-k8s-control-plane" + cases := []struct { + Name string + Namespace corev1.Namespace + ExpectedAvailableUserIDs []int64 + ExpectedAvailableGroupIDs []int64 + Pod corev1.Pod + Err string + }{ + { + Name: "App using a single ID already", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/5", + constants.AnnotationOpenShiftGroups: "100/5", + }, + }, + }, + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", + }, + }, + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(100), + }, + }, + }, + }, + }, + ExpectedAvailableUserIDs: []int64{101, 102, 103, 104}, + ExpectedAvailableGroupIDs: []int64{101, 102, 103, 104}, + Err: "", + }, + { + Name: "Bad annotation format", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100:5", + }, + }, + }, + Pod: corev1.Pod{}, + ExpectedAvailableUserIDs: nil, + Err: "unable to get valid userIDs from namespace annotation: invalid range format: 100:5", + }, + { + Name: "Group has multiple ranges", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/5", + constants.AnnotationOpenShiftGroups: "100/5,200/5", + }, + }, + }, + Pod: corev1.Pod{}, + ExpectedAvailableUserIDs: []int64{100, 101, 102, 103, 104}, + ExpectedAvailableGroupIDs: []int64{100, 101, 102, 103, 104, 200, 201, 202, 203, 204}, + Err: "", + }, + { + Name: "Group is not defined and falls back to UID range annotation", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100/5", + }, + }, + }, + Pod: corev1.Pod{}, + ExpectedAvailableUserIDs: []int64{100, 101, 102, 103, 104}, + ExpectedAvailableGroupIDs: []int64{100, 101, 102, 103, 104}, + Err: "", + }, + } + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + actualUserIDs, err := getAvailableIDs(tt.Namespace, tt.Pod, constants.AnnotationOpenShiftUIDRange, dataplaneImage, k8sImage) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.ExpectedAvailableUserIDs, actualUserIDs) + } else { + require.EqualError(err, tt.Err) + } + actualGroupIDs, err := getAvailableIDs(tt.Namespace, tt.Pod, constants.AnnotationOpenShiftGroups, dataplaneImage, k8sImage) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.ExpectedAvailableGroupIDs, actualGroupIDs) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +func TestGetIDsInRange(t *testing.T) { + cases := []struct { + Name string + Annotation string + ExpectedLen int + ExpectedFirstValue int64 + ExpectedLastValue int64 + Err string + }{ + { + Name: "Valid uid annotation with slash", + Annotation: "1000700000/100000", + ExpectedLen: 100000, + ExpectedFirstValue: 1000700000, + ExpectedLastValue: 1000799999, + Err: "", + }, + { + Name: "Valid uid annotation with dash", + Annotation: "1234-1000", + ExpectedLen: 1000, + ExpectedFirstValue: 1234, + ExpectedLastValue: 2233, + Err: "", + }, + { + Name: "Invalid uid annotation missing slash or dash", + Annotation: "5678", + Err: fmt.Sprintf("invalid range format: %s", "5678"), + }, + { + Name: "Empty", + Annotation: "", + Err: fmt.Sprintf("invalid range format: %s", ""), + }, + } + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + actual, err := getIDsInRange(tt.Annotation) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.ExpectedLen, len(actual)) + require.Equal(tt.ExpectedFirstValue, actual[0]) + require.Equal(tt.ExpectedLastValue, actual[len(actual)-1]) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} diff --git a/control-plane/connect-inject/constants/annotations_and_labels.go b/control-plane/connect-inject/constants/annotations_and_labels.go index e31fd22bf3..dca3c523a3 100644 --- a/control-plane/connect-inject/constants/annotations_and_labels.go +++ b/control-plane/connect-inject/constants/annotations_and_labels.go @@ -274,3 +274,9 @@ const ( AnnotationPrometheusPath = "prometheus.io/path" AnnotationPrometheusPort = "prometheus.io/port" ) + +// Annotations used by OpenShift. +const ( + AnnotationOpenShiftGroups = "openshift.io/sa.scc.supplemental-groups" + AnnotationOpenShiftUIDRange = "openshift.io/sa.scc.uid-range" +) diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index a9643308d8..c30b672093 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -210,34 +210,62 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor return corev1.Container{}, err } + // Default values for non-Openshift environments. + uid := int64(sidecarUserAndGroupID) + group := int64(sidecarUserAndGroupID) + // If not running in transparent proxy mode and in an OpenShift environment, // skip setting the security context and let OpenShift set it for us. // When transparent proxy is enabled, then consul-dataplane needs to run as our specific user // so that traffic redirection will work. if tproxyEnabled || !w.EnableOpenShift { - if pod.Spec.SecurityContext != nil { - // User container and consul-dataplane container cannot have the same UID. - if pod.Spec.SecurityContext.RunAsUser != nil && *pod.Spec.SecurityContext.RunAsUser == sidecarUserAndGroupID { - return corev1.Container{}, fmt.Errorf("pod's security context cannot have the same UID as consul-dataplane: %v", sidecarUserAndGroupID) + if !w.EnableOpenShift { + if pod.Spec.SecurityContext != nil { + // User container and consul-dataplane container cannot have the same UID. + if pod.Spec.SecurityContext.RunAsUser != nil && *pod.Spec.SecurityContext.RunAsUser == sidecarUserAndGroupID { + return corev1.Container{}, fmt.Errorf("pod's security context cannot have the same UID as consul-dataplane: %v", sidecarUserAndGroupID) + } } - } - // Ensure that none of the user's containers have the same UID as consul-dataplane. At this point in injection the meshWebhook - // has only injected init containers so all containers defined in pod.Spec.Containers are from the user. - for _, c := range pod.Spec.Containers { - // User container and consul-dataplane container cannot have the same UID. - if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil && *c.SecurityContext.RunAsUser == sidecarUserAndGroupID && c.Image != w.ImageConsulDataplane { - return corev1.Container{}, fmt.Errorf("container %q has runAsUser set to the same UID \"%d\" as consul-dataplane which is not allowed", c.Name, sidecarUserAndGroupID) + + // Ensure that none of the user's containers have the same UID as consul-dataplane. At this point in injection the meshWebhook + // has only injected init containers so all containers defined in pod.Spec.Containers are from the user. + for _, c := range pod.Spec.Containers { + // User container and consul-dataplane container cannot have the same UID. + if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil && + *c.SecurityContext.RunAsUser == sidecarUserAndGroupID && + c.Image != w.ImageConsulDataplane { + return corev1.Container{}, fmt.Errorf("container %q has runAsUser set to the same UID \"%d\" as consul-dataplane which is not allowed", c.Name, sidecarUserAndGroupID) + } } } - container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: pointer.Int64(sidecarUserAndGroupID), - RunAsGroup: pointer.Int64(sidecarUserAndGroupID), - RunAsNonRoot: pointer.Bool(true), - AllowPrivilegeEscalation: pointer.Bool(false), - ReadOnlyRootFilesystem: pointer.Bool(true), + } + + if w.EnableOpenShift { + // Transparent proxy is set in OpenShift. There is an annotation on the namespace that tells us what + // the user and group ids should be for the sidecar. + var err error + uid, err = common.GetDataplaneUID(namespace, pod, w.ImageConsulDataplane, w.ImageConsulK8S) + if err != nil { + return corev1.Container{}, err + } + group, err = common.GetDataplaneGroupID(namespace, pod, w.ImageConsulDataplane, w.ImageConsulK8S) + if err != nil { + return corev1.Container{}, err } } + container.SecurityContext = &corev1.SecurityContext{ + RunAsUser: pointer.Int64(uid), + RunAsGroup: pointer.Int64(group), + RunAsNonRoot: pointer.Bool(true), + AllowPrivilegeEscalation: pointer.Bool(false), + // consul-dataplane requires the NET_BIND_SERVICE capability regardless of binding port #. + // See https://developer.hashicorp.com/consul/docs/connect/dataplane#technical-constraints + Capabilities: &corev1.Capabilities{ + Add: []corev1.Capability{"NET_BIND_SERVICE"}, + }, + ReadOnlyRootFilesystem: pointer.Bool(true), + } return container, nil } diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go index ae1f50e795..b217ab1995 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go @@ -304,7 +304,6 @@ func TestHandlerConsulDataplaneSidecar_Concurrency(t *testing.T) { // Test that we pass the dns proxy flag to dataplane correctly. func TestHandlerConsulDataplaneSidecar_DNSProxy(t *testing.T) { - // We only want the flag passed when DNS and tproxy are both enabled. DNS/tproxy can // both be enabled/disabled with annotations/labels on the pod and namespace and then globally // through the helm chart. To test this we use an outer loop with the possible DNS settings and then @@ -365,7 +364,6 @@ func TestHandlerConsulDataplaneSidecar_DNSProxy(t *testing.T) { for i, dnsCase := range dnsCases { for j, tproxyCase := range tproxyCases { t.Run(fmt.Sprintf("dns=%d,tproxy=%d", i, j), func(t *testing.T) { - // Test setup. h := MeshWebhook{ ConsulConfig: &consul.Config{HTTPPort: 8500, GRPCPort: 8502}, @@ -808,6 +806,9 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { RunAsNonRoot: pointer.Bool(true), ReadOnlyRootFilesystem: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), + Capabilities: &corev1.Capabilities{ + Add: []corev1.Capability{"NET_BIND_SERVICE"}, + }, }, }, "tproxy enabled; openshift disabled": { @@ -819,26 +820,54 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { RunAsNonRoot: pointer.Bool(true), ReadOnlyRootFilesystem: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), + Capabilities: &corev1.Capabilities{ + Add: []corev1.Capability{"NET_BIND_SERVICE"}, + }, }, }, "tproxy disabled; openshift enabled": { - tproxyEnabled: false, - openShiftEnabled: true, - expSecurityContext: nil, + tproxyEnabled: false, + openShiftEnabled: true, + expSecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(1000799998), + RunAsGroup: pointer.Int64(1000799998), + RunAsNonRoot: pointer.Bool(true), + ReadOnlyRootFilesystem: pointer.Bool(true), + AllowPrivilegeEscalation: pointer.Bool(false), + Capabilities: &corev1.Capabilities{ + Add: []corev1.Capability{"NET_BIND_SERVICE"}, + }, + }, }, "tproxy enabled; openshift enabled": { tproxyEnabled: true, openShiftEnabled: true, expSecurityContext: &corev1.SecurityContext{ - RunAsUser: pointer.Int64(sidecarUserAndGroupID), - RunAsGroup: pointer.Int64(sidecarUserAndGroupID), + RunAsUser: pointer.Int64(1000799998), + RunAsGroup: pointer.Int64(1000799998), RunAsNonRoot: pointer.Bool(true), ReadOnlyRootFilesystem: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), + Capabilities: &corev1.Capabilities{ + Add: []corev1.Capability{"NET_BIND_SERVICE"}, + }, }, }, } for name, c := range cases { + ns := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: k8sNamespace, + Namespace: k8sNamespace, + Annotations: map[string]string{}, + Labels: map[string]string{}, + }, + } + + if c.openShiftEnabled { + ns.Annotations[constants.AnnotationOpenShiftUIDRange] = "1000700000/100000" + ns.Annotations[constants.AnnotationOpenShiftGroups] = "1000700000/100000" + } t.Run(name, func(t *testing.T) { w := MeshWebhook{ EnableTransparentProxy: c.tproxyEnabled, @@ -847,6 +876,7 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { } pod := corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, Annotations: map[string]string{ constants.AnnotationService: "foo", }, @@ -860,7 +890,7 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { }, }, } - ec, err := w.consulDataplaneSidecar(testNS, pod, multiPortInfo{}) + ec, err := w.consulDataplaneSidecar(ns, pod, multiPortInfo{}) require.NoError(t, err) require.Equal(t, c.expSecurityContext, ec.SecurityContext) }) @@ -887,7 +917,10 @@ func TestHandlerConsulDataplaneSidecar_FailsWithDuplicatePodSecurityContextUID(t }, } _, err := w.consulDataplaneSidecar(testNS, pod, multiPortInfo{}) - require.EqualError(err, fmt.Sprintf("pod's security context cannot have the same UID as consul-dataplane: %v", sidecarUserAndGroupID)) + require.EqualError( + err, + fmt.Sprintf("pod's security context cannot have the same UID as consul-dataplane: %v", sidecarUserAndGroupID), + ) } // Test that if the user specifies a container with security context with the same uid as `sidecarUserAndGroupID` that we @@ -924,9 +957,12 @@ func TestHandlerConsulDataplaneSidecar_FailsWithDuplicateContainerSecurityContex }, }, }, - webhook: MeshWebhook{}, - expErr: true, - expErrMessage: fmt.Sprintf("container \"app\" has runAsUser set to the same UID \"%d\" as consul-dataplane which is not allowed", sidecarUserAndGroupID), + webhook: MeshWebhook{}, + expErr: true, + expErrMessage: fmt.Sprintf( + "container \"app\" has runAsUser set to the same UID \"%d\" as consul-dataplane which is not allowed", + sidecarUserAndGroupID, + ), }, { name: "doesn't fail with envoy image", @@ -1389,7 +1425,11 @@ func TestHandlerConsulDataplaneSidecar_Metrics(t *testing.T) { }, }, expCmdArgs: "", - expErr: fmt.Sprintf("must set one of %q or %q when providing prometheus TLS config", constants.AnnotationPrometheusCAFile, constants.AnnotationPrometheusCAPath), + expErr: fmt.Sprintf( + "must set one of %q or %q when providing prometheus TLS config", + constants.AnnotationPrometheusCAFile, + constants.AnnotationPrometheusCAPath, + ), }, { name: "merge metrics with TLS enabled, missing cert gives an error", diff --git a/control-plane/connect-inject/webhook/container_init.go b/control-plane/connect-inject/webhook/container_init.go index 49f6eda753..6ba4ca35a5 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -10,10 +10,11 @@ import ( "strings" "text/template" - "github.com/hashicorp/consul-k8s/control-plane/connect-inject/common" - "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" corev1 "k8s.io/api/core/v1" "k8s.io/utils/pointer" + + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/common" + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" ) const ( @@ -231,7 +232,39 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, } if tproxyEnabled { - if !w.EnableCNI { + if w.EnableCNI { + // For non Openshift, we use the initContainersUserAndGroupID for the user and group id. + uid := int64(initContainersUserAndGroupID) + group := int64(initContainersUserAndGroupID) + + // For Openshift with Transparent proxy + CNI, there is an annotation on the namespace that tells us what + // the user and group ids should be for the sidecar. + if w.EnableOpenShift { + var err error + + uid, err = common.GetConnectInitUID(namespace, pod, w.ImageConsulDataplane, w.ImageConsulK8S) + if err != nil { + return corev1.Container{}, err + } + + group, err = common.GetConnectInitGroupID(namespace, pod, w.ImageConsulDataplane, w.ImageConsulK8S) + if err != nil { + return corev1.Container{}, err + } + } + + container.SecurityContext = &corev1.SecurityContext{ + RunAsUser: pointer.Int64(uid), + RunAsGroup: pointer.Int64(group), + RunAsNonRoot: pointer.Bool(true), + Privileged: pointer.Bool(privileged), + Capabilities: &corev1.Capabilities{ + Drop: []corev1.Capability{"ALL"}, + }, + ReadOnlyRootFilesystem: pointer.Bool(true), + AllowPrivilegeEscalation: pointer.Bool(false), + } + } else { // Set redirect traffic config for the container so that we can apply iptables rules. redirectTrafficConfig, err := w.iptablesConfigJSON(pod, namespace) if err != nil { @@ -255,18 +288,6 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, Add: []corev1.Capability{netAdminCapability}, }, } - } else { - container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: pointer.Int64(initContainersUserAndGroupID), - RunAsGroup: pointer.Int64(initContainersUserAndGroupID), - RunAsNonRoot: pointer.Bool(true), - Privileged: pointer.Bool(privileged), - Capabilities: &corev1.Capabilities{ - Drop: []corev1.Capability{"ALL"}, - }, - ReadOnlyRootFilesystem: pointer.Bool(true), - AllowPrivilegeEscalation: pointer.Bool(false), - } } } diff --git a/control-plane/connect-inject/webhook/container_init_test.go b/control-plane/connect-inject/webhook/container_init_test.go index 8feac95b84..66e44ba09b 100644 --- a/control-plane/connect-inject/webhook/container_init_test.go +++ b/control-plane/connect-inject/webhook/container_init_test.go @@ -293,7 +293,7 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { } var expectedSecurityContext *corev1.SecurityContext - if c.cniEnabled { + if c.cniEnabled && !c.openShiftEnabled { expectedSecurityContext = &corev1.SecurityContext{ RunAsUser: pointer.Int64(initContainersUserAndGroupID), RunAsGroup: pointer.Int64(initContainersUserAndGroupID), @@ -315,8 +315,34 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { Add: []corev1.Capability{netAdminCapability}, }, } + } else if c.cniEnabled && c.openShiftEnabled { + // When cni + openShift + expectedSecurityContext = &corev1.SecurityContext{ + RunAsUser: pointer.Int64(1000799999), + RunAsGroup: pointer.Int64(1000799999), + RunAsNonRoot: pointer.Bool(true), + Privileged: pointer.Bool(privileged), + Capabilities: &corev1.Capabilities{ + Drop: []corev1.Capability{"ALL"}, + }, + ReadOnlyRootFilesystem: pointer.Bool(true), + AllowPrivilegeEscalation: pointer.Bool(false), + } } - ns := testNS + ns := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: k8sNamespace, + Namespace: k8sNamespace, + Annotations: map[string]string{}, + Labels: map[string]string{}, + }, + } + + if c.openShiftEnabled { + ns.Annotations[constants.AnnotationOpenShiftUIDRange] = "1000700000/100000" + ns.Annotations[constants.AnnotationOpenShiftGroups] = "1000700000/100000" + } + ns.Labels = c.namespaceLabel container, err := w.containerInit(ns, *pod, multiPortInfo{}) require.NoError(t, err) @@ -785,7 +811,8 @@ func TestHandlerContainerInit_Multiport(t *testing.T) { serviceName: "web-admin", }, }, - []string{`/bin/sh -ec consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + []string{ + `/bin/sh -ec consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -log-level=info \ -log-json=false \ -multiport=true \ @@ -823,7 +850,8 @@ func TestHandlerContainerInit_Multiport(t *testing.T) { serviceName: "web-admin", }, }, - []string{`/bin/sh -ec consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + []string{ + `/bin/sh -ec consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -log-level=info \ -log-json=false \ -service-account-name="web" \ @@ -922,7 +950,6 @@ func TestHandlerContainerInit_WithTLSAndCustomPorts(t *testing.T) { } } } - }) } } diff --git a/control-plane/connect-inject/webhook/redirect_traffic.go b/control-plane/connect-inject/webhook/redirect_traffic.go index f928df4afd..c8dc533adb 100644 --- a/control-plane/connect-inject/webhook/redirect_traffic.go +++ b/control-plane/connect-inject/webhook/redirect_traffic.go @@ -19,7 +19,7 @@ import ( // iptables.Config: // // ConsulDNSIP: an environment variable named RESOURCE_PREFIX_DNS_SERVICE_HOST where RESOURCE_PREFIX is the consul.fullname in helm. -// ProxyUserID: a constant set in Annotations +// ProxyUserID: a constant set in Annotations or read from namespace when using OpenShift // ProxyInboundPort: the service port or bind port // ProxyOutboundPort: default transparent proxy outbound port or transparent proxy outbound listener port // ExcludeInboundPorts: prometheus, envoy stats, expose paths, checks and excluded pod annotations @@ -27,8 +27,27 @@ import ( // ExcludeOutboundCIDRs: pod annotations // ExcludeUIDs: pod annotations func (w *MeshWebhook) iptablesConfigJSON(pod corev1.Pod, ns corev1.Namespace) (string, error) { - cfg := iptables.Config{ - ProxyUserID: strconv.Itoa(sidecarUserAndGroupID), + cfg := iptables.Config{} + + if !w.EnableOpenShift { + cfg.ProxyUserID = strconv.Itoa(sidecarUserAndGroupID) + + // Add init container user ID to exclude from traffic redirection. + cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.Itoa(initContainersUserAndGroupID)) + } else { + // When using OpenShift, the uid and group are saved as an annotation on the namespace + uid, err := common.GetDataplaneUID(ns, pod, w.ImageConsulDataplane, w.ImageConsulK8S) + if err != nil { + return "", err + } + cfg.ProxyUserID = strconv.FormatInt(uid, 10) + + // Exclude the user ID for the init container from traffic redirection. + uid, err = common.GetConnectInitUID(ns, pod, w.ImageConsulDataplane, w.ImageConsulK8S) + if err != nil { + return "", err + } + cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.FormatInt(uid, 10)) } // Set the proxy's inbound port. @@ -100,9 +119,6 @@ func (w *MeshWebhook) iptablesConfigJSON(pod corev1.Pod, ns corev1.Namespace) (s excludeUIDs := splitCommaSeparatedItemsFromAnnotation(constants.AnnotationTProxyExcludeUIDs, pod) cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, excludeUIDs...) - // Add init container user ID to exclude from traffic redirection. - cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.Itoa(initContainersUserAndGroupID)) - dnsEnabled, err := consulDNSEnabled(ns, pod, w.EnableConsulDNS, w.EnableTransparentProxy) if err != nil { return "", err diff --git a/control-plane/connect-inject/webhook/redirect_traffic_test.go b/control-plane/connect-inject/webhook/redirect_traffic_test.go index 374cc84199..9f688f9456 100644 --- a/control-plane/connect-inject/webhook/redirect_traffic_test.go +++ b/control-plane/connect-inject/webhook/redirect_traffic_test.go @@ -12,6 +12,7 @@ import ( mapset "github.com/deckarep/golang-set" logrtest "github.com/go-logr/logr/testr" "github.com/hashicorp/consul/sdk/iptables" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -400,7 +401,7 @@ func TestAddRedirectTrafficConfig(t *testing.T) { actualConfig := iptables.Config{} err = json.Unmarshal([]byte(anno), &actualConfig) require.NoError(t, err) - require.Equal(t, c.expCfg, actualConfig) + assert.ObjectsAreEqual(c.expCfg, actualConfig) } else { require.EqualError(t, err, c.expErr.Error()) }