From 016d3434c03a2c39032927efe5e8fdde0fa4d8e4 Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Tue, 26 Mar 2024 13:34:13 -0400 Subject: [PATCH 01/38] Remove SCC requirement for anyuid for OpenShift --- acceptance/framework/consul/helm_cluster.go | 7 +- .../multiport-app/anyuid-scc-rolebinding.yaml | 26 --- .../bases/multiport-app/kustomization.yaml | 3 +- .../static-client/anyuid-scc-rolebinding.yaml | 14 -- .../bases/static-client/kustomization.yaml | 3 +- .../anyuid-scc-rolebinding.yaml | 14 -- .../static-server-https/kustomization.yaml | 3 +- .../anyuid-scc-rolebinding.yaml | 14 -- .../static-server-tcp/kustomization.yaml | 3 +- .../static-server/anyuid-scc-rolebinding.yaml | 14 -- .../bases/static-server/kustomization.yaml | 3 +- .../anyuid-scc-rolebinding.yaml | 26 --- .../bases/v2-multiport-app/kustomization.yaml | 3 +- .../connect-inject/common/openshift.go | 129 +++++++++++ .../connect-inject/common/openshift_test.go | 219 ++++++++++++++++++ .../constants/annotations_and_labels.go | 6 + .../webhook/consul_dataplane_sidecar.go | 134 ++++++++--- .../webhook/consul_dataplane_sidecar_test.go | 42 +++- .../connect-inject/webhook/container_init.go | 52 ++++- .../webhook/container_init_test.go | 37 ++- .../webhook/redirect_traffic.go | 16 +- control-plane/go.mod | 2 +- 22 files changed, 587 insertions(+), 183 deletions(-) delete mode 100644 acceptance/tests/fixtures/bases/multiport-app/anyuid-scc-rolebinding.yaml delete mode 100644 acceptance/tests/fixtures/bases/static-client/anyuid-scc-rolebinding.yaml delete mode 100644 acceptance/tests/fixtures/bases/static-server-https/anyuid-scc-rolebinding.yaml delete mode 100644 acceptance/tests/fixtures/bases/static-server-tcp/anyuid-scc-rolebinding.yaml delete mode 100644 acceptance/tests/fixtures/bases/static-server/anyuid-scc-rolebinding.yaml delete mode 100644 acceptance/tests/fixtures/bases/v2-multiport-app/anyuid-scc-rolebinding.yaml create mode 100644 control-plane/connect-inject/common/openshift.go create mode 100644 control-plane/connect-inject/common/openshift_test.go diff --git a/acceptance/framework/consul/helm_cluster.go b/acceptance/framework/consul/helm_cluster.go index 3471478bb5..48e67bb5bf 100644 --- a/acceptance/framework/consul/helm_cluster.go +++ b/acceptance/framework/consul/helm_cluster.go @@ -678,16 +678,14 @@ 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} { + for clusterRoleName, roleBindingName := range map[string]string{privilegedClusterRole: privilegedRoleBinding} { // Check if this cluster role binding already exists. _, err := client.RbacV1().RoleBindings(namespace).Get(context.Background(), roleBindingName, metav1.GetOptions{}) @@ -718,7 +716,6 @@ func configureSCCs(t *testing.T, client kubernetes.Interface, cfg *config.TestCo } 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/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/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go new file mode 100644 index 0000000000..1c36182303 --- /dev/null +++ b/control-plane/connect-inject/common/openshift.go @@ -0,0 +1,129 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +// Function copied from: +// https://github.com/openshift/apiserver-library-go/blob/release-4.17/pkg/securitycontextconstraints/sccmatching/matcher.go +// Apache 2.0 license: https://github.com/openshift/apiserver-library-go/blob/release-4.17/LICENSE + +// 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" + + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" + corev1 "k8s.io/api/core/v1" +) + +// Get the user id from the OpenShift annotation 'openshift.io/sa.scc.uid-range' +func GetOpenShiftUID(ns *corev1.Namespace) (int64, error) { + annotation, ok := ns.Annotations[constants.AnnotationOpenShiftUIDRange] + if !ok { + return 0, fmt.Errorf("unable to find annotation %s", constants.AnnotationOpenShiftUIDRange) + } + if len(annotation) == 0 { + return 0, fmt.Errorf("found annotation %s but it was empty", constants.AnnotationOpenShiftUIDRange) + } + + uid, err := parseOpenShiftUID(annotation) + if err != nil { + return 0, err + } + + return uid, nil +} + +// Parse the UID "range" from the annotation string. The annotation can either have a '/' or '-' as a separator. +// '-' is the old style of UID from when it used to be an actual range. +func parseOpenShiftUID(val string) (int64, error) { + var uid int64 + var err error + if strings.Contains(val, "/") { + str := strings.Split(val, "/") + uid, err = strconv.ParseInt(str[0], 10, 64) + if err != nil { + return 0, err + } + } + if strings.Contains(val, "-") { + str := strings.Split(val, "-") + uid, err = strconv.ParseInt(str[0], 10, 64) + if err != nil { + return 0, err + } + } + + if !strings.Contains(val, "/") && !strings.Contains(val, "-") { + return 0, fmt.Errorf( + "annotation %s contains an invalid format for value %s", + constants.AnnotationOpenShiftUIDRange, + val, + ) + } + + return uid, nil +} + +// Get the user id from the OpenShift annotation 'openshift.io/sa.scc.supplemental-groups' +// Fall back to the UID annotation if the group annotation does not exist. The values should +// be the same. +func GetOpenShiftGroup(ns *corev1.Namespace) (int64, error) { + annotation, ok := ns.Annotations[constants.AnnotationOpenShiftGroups] + if !ok { + // fall back to UID annotation + annotation, ok = ns.Annotations[constants.AnnotationOpenShiftUIDRange] + if !ok { + return 0, fmt.Errorf( + "unable to find annotation %s or %s", + constants.AnnotationOpenShiftGroups, + constants.AnnotationOpenShiftUIDRange, + ) + } + } + if len(annotation) == 0 { + return 0, fmt.Errorf("found annotation %s but it was empty", constants.AnnotationOpenShiftGroups) + } + + uid, err := parseOpenShiftGroup(annotation) + if err != nil { + return 0, err + } + + return uid, nil +} + +// Parse the group from the annotation string. The annotation can either have a '/' or ',' as a separator. +// ',' is the old style of UID from when it used to be an actual range. +func parseOpenShiftGroup(val string) (int64, error) { + var group int64 + var err error + if strings.Contains(val, "/") { + str := strings.Split(val, "/") + group, err = strconv.ParseInt(str[0], 10, 64) + if err != nil { + return 0, err + } + } + if strings.Contains(val, ",") { + str := strings.Split(val, ",") + group, err = strconv.ParseInt(str[0], 10, 64) + if err != nil { + return 0, err + } + } + + if !strings.Contains(val, "/") && !strings.Contains(val, ",") { + return 0, fmt.Errorf("annotation %s contains an invalid format for value %s", constants.AnnotationOpenShiftGroups, val) + } + + return group, 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..c31b26c9d1 --- /dev/null +++ b/control-plane/connect-inject/common/openshift_test.go @@ -0,0 +1,219 @@ +// 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" +) + +func TestOpenShiftUID(t *testing.T) { + cases := []struct { + Name string + Namespace func() *corev1.Namespace + Expected int64 + Err string + }{ + { + Name: "Valid uid annotation with slash", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "1000700000/100000", + }, + }, + } + return ns + }, + Expected: 1000700000, + Err: "", + }, + { + Name: "Valid uid annotation with dash", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "1234-1000", + }, + }, + } + return ns + }, + Expected: 1234, + Err: "", + }, + { + Name: "Invalid uid annotation missing slash or dash", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + // annotation should have a slash '/' or dash '-' + constants.AnnotationOpenShiftUIDRange: "5678", + }, + }, + } + return ns + }, + Expected: 0, + Err: fmt.Sprintf( + "annotation %s contains an invalid format for value %s", + constants.AnnotationOpenShiftUIDRange, + "5678", + ), + }, + { + Name: "Missing uid annotation", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + }, + } + return ns + }, + Expected: 0, + Err: fmt.Sprintf("unable to find annotation %s", constants.AnnotationOpenShiftUIDRange), + }, + } + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + actual, err := GetOpenShiftUID(tt.Namespace()) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.Expected, actual) + } else { + require.EqualError(err, tt.Err) + } + }) + } +} + +func TestOpenShiftGroup(t *testing.T) { + cases := []struct { + Name string + Namespace func() *corev1.Namespace + Expected int64 + Err string + }{ + { + Name: "Valid group annotation with slash", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftGroups: "123456789/1000", + }, + }, + } + return ns + }, + Expected: 123456789, + Err: "", + }, + { + Name: "Valid group annotation with comma", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftGroups: "1234,1000", + }, + }, + } + return ns + }, + Expected: 1234, + Err: "", + }, + { + Name: "Invalid group annotation missing slash or comma", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + // annotation should have a slash '/' or comma ',' + constants.AnnotationOpenShiftGroups: "5678", + }, + }, + } + return ns + }, + Expected: 0, + Err: fmt.Sprintf( + "annotation %s contains an invalid format for value %s", + constants.AnnotationOpenShiftGroups, + "5678", + ), + }, + { + Name: "Missing group annotation, fall back to UID annotation", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + // annotation should have a slash '/' or comma ',' + constants.AnnotationOpenShiftUIDRange: "9012/1000", + }, + }, + } + return ns + }, + Expected: 9012, + Err: "", + }, + { + Name: "Missing both group and fallback uid annotation", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + }, + } + return ns + }, + Expected: 0, + Err: fmt.Sprintf( + "unable to find annotation %s or %s", + constants.AnnotationOpenShiftGroups, + constants.AnnotationOpenShiftUIDRange, + ), + }, + } + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + require := require.New(t) + actual, err := GetOpenShiftGroup(tt.Namespace()) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.Expected, actual) + } 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 a069668d7d..f524a8ff19 100644 --- a/control-plane/connect-inject/constants/annotations_and_labels.go +++ b/control-plane/connect-inject/constants/annotations_and_labels.go @@ -234,3 +234,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..fa2654d021 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -24,7 +24,11 @@ const ( consulDataplaneDNSBindPort = 8600 ) -func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod corev1.Pod, mpi multiPortInfo) (corev1.Container, error) { +func (w *MeshWebhook) consulDataplaneSidecar( + namespace corev1.Namespace, + pod corev1.Pod, + mpi multiPortInfo, +) (corev1.Container, error) { resources, err := w.sidecarResources(pod) if err != nil { return corev1.Container{}, err @@ -215,33 +219,70 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor // 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) + // In non-OpenShift environments we set the User and group ID for the sidecare to our values. + 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), + } + } else { + // 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. + uid, err := common.GetOpenShiftUID(&namespace) + if err != nil { + return corev1.Container{}, err + } + group, err := common.GetOpenShiftGroup(&namespace) + 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), + ReadOnlyRootFilesystem: pointer.Bool(true), } - } - container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: pointer.Int64(sidecarUserAndGroupID), - RunAsGroup: pointer.Int64(sidecarUserAndGroupID), - RunAsNonRoot: pointer.Bool(true), - AllowPrivilegeEscalation: pointer.Bool(false), - ReadOnlyRootFilesystem: pointer.Bool(true), } } + // When running tproxy and OpenShift, + return container, nil } -func (w *MeshWebhook) getContainerSidecarArgs(namespace corev1.Namespace, mpi multiPortInfo, bearerTokenFile string, pod corev1.Pod) ([]string, error) { +func (w *MeshWebhook) getContainerSidecarArgs( + namespace corev1.Namespace, + mpi multiPortInfo, + bearerTokenFile string, + pod corev1.Pod, +) ([]string, error) { proxyIDFileName := "/consul/connect-inject/proxyid" if mpi.serviceName != "" { proxyIDFileName = fmt.Sprintf("/consul/connect-inject/proxyid-%s", mpi.serviceName) @@ -382,7 +423,14 @@ func (w *MeshWebhook) getContainerSidecarArgs(namespace corev1.Namespace, mpi mu } if serviceMetricsPath != "" && serviceMetricsPort != "" { - args = append(args, "-telemetry-prom-service-metrics-url="+fmt.Sprintf("http://127.0.0.1:%s%s", serviceMetricsPort, serviceMetricsPath)) + args = append( + args, + "-telemetry-prom-service-metrics-url="+fmt.Sprintf( + "http://127.0.0.1:%s%s", + serviceMetricsPort, + serviceMetricsPath, + ), + ) } // Pull the TLS config from the relevant annotations. @@ -409,13 +457,23 @@ func (w *MeshWebhook) getContainerSidecarArgs(namespace corev1.Namespace, mpi mu // Validate required Prometheus TLS config is present if set. if prometheusCAFile != "" || prometheusCAPath != "" || prometheusCertFile != "" || prometheusKeyFile != "" { if prometheusCAFile == "" && prometheusCAPath == "" { - return nil, fmt.Errorf("must set one of %q or %q when providing prometheus TLS config", constants.AnnotationPrometheusCAFile, constants.AnnotationPrometheusCAPath) + return nil, fmt.Errorf( + "must set one of %q or %q when providing prometheus TLS config", + constants.AnnotationPrometheusCAFile, + constants.AnnotationPrometheusCAPath, + ) } if prometheusCertFile == "" { - return nil, fmt.Errorf("must set %q when providing prometheus TLS config", constants.AnnotationPrometheusCertFile) + return nil, fmt.Errorf( + "must set %q when providing prometheus TLS config", + constants.AnnotationPrometheusCertFile, + ) } if prometheusKeyFile == "" { - return nil, fmt.Errorf("must set %q when providing prometheus TLS config", constants.AnnotationPrometheusKeyFile) + return nil, fmt.Errorf( + "must set %q when providing prometheus TLS config", + constants.AnnotationPrometheusKeyFile, + ) } // TLS config has been validated, add them to the consul-dataplane cmd args args = append(args, "-telemetry-prom-ca-certs-file="+prometheusCAFile, @@ -495,7 +553,12 @@ func (w *MeshWebhook) sidecarResources(pod corev1.Pod) (corev1.ResourceRequireme if anno, ok := pod.Annotations[constants.AnnotationSidecarProxyCPULimit]; ok { cpuLimit, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", constants.AnnotationSidecarProxyCPULimit, anno, err) + return corev1.ResourceRequirements{}, fmt.Errorf( + "parsing annotation %s:%q: %s", + constants.AnnotationSidecarProxyCPULimit, + anno, + err, + ) } resources.Limits[corev1.ResourceCPU] = cpuLimit } else if w.DefaultProxyCPULimit != zeroQuantity { @@ -506,7 +569,12 @@ func (w *MeshWebhook) sidecarResources(pod corev1.Pod) (corev1.ResourceRequireme if anno, ok := pod.Annotations[constants.AnnotationSidecarProxyCPURequest]; ok { cpuRequest, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", constants.AnnotationSidecarProxyCPURequest, anno, err) + return corev1.ResourceRequirements{}, fmt.Errorf( + "parsing annotation %s:%q: %s", + constants.AnnotationSidecarProxyCPURequest, + anno, + err, + ) } resources.Requests[corev1.ResourceCPU] = cpuRequest } else if w.DefaultProxyCPURequest != zeroQuantity { @@ -517,7 +585,12 @@ func (w *MeshWebhook) sidecarResources(pod corev1.Pod) (corev1.ResourceRequireme if anno, ok := pod.Annotations[constants.AnnotationSidecarProxyMemoryLimit]; ok { memoryLimit, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", constants.AnnotationSidecarProxyMemoryLimit, anno, err) + return corev1.ResourceRequirements{}, fmt.Errorf( + "parsing annotation %s:%q: %s", + constants.AnnotationSidecarProxyMemoryLimit, + anno, + err, + ) } resources.Limits[corev1.ResourceMemory] = memoryLimit } else if w.DefaultProxyMemoryLimit != zeroQuantity { @@ -528,7 +601,12 @@ func (w *MeshWebhook) sidecarResources(pod corev1.Pod) (corev1.ResourceRequireme if anno, ok := pod.Annotations[constants.AnnotationSidecarProxyMemoryRequest]; ok { memoryRequest, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", constants.AnnotationSidecarProxyMemoryRequest, anno, err) + return corev1.ResourceRequirements{}, fmt.Errorf( + "parsing annotation %s:%q: %s", + constants.AnnotationSidecarProxyMemoryRequest, + anno, + err, + ) } resources.Requests[corev1.ResourceMemory] = memoryRequest } else if w.DefaultProxyMemoryRequest != zeroQuantity { 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..4a8386c493 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}, @@ -830,8 +828,8 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { tproxyEnabled: true, openShiftEnabled: true, expSecurityContext: &corev1.SecurityContext{ - RunAsUser: pointer.Int64(sidecarUserAndGroupID), - RunAsGroup: pointer.Int64(sidecarUserAndGroupID), + RunAsUser: pointer.Int64(1000700000), + RunAsGroup: pointer.Int64(1000700000), RunAsNonRoot: pointer.Bool(true), ReadOnlyRootFilesystem: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), @@ -839,6 +837,19 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { }, } 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 +858,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 +872,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 +899,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 +939,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 +1407,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..7a4ec0e274 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -256,16 +256,41 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, }, } } 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), + if !w.EnableOpenShift { + 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), + } + } else { + // Transparent proxy + CNI 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. + uid, err := common.GetOpenShiftUID(&namespace) + if err != nil { + return corev1.Container{}, err + } + group, err := common.GetOpenShiftGroup(&namespace) + 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(false), + Capabilities: &corev1.Capabilities{ + Drop: []corev1.Capability{"ALL"}, + }, + ReadOnlyRootFilesystem: pointer.Bool(true), + AllowPrivilegeEscalation: pointer.Bool(false), + } + } } } @@ -276,7 +301,12 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, // consulDNSEnabled returns true if Consul DNS should be enabled for this pod. // It returns an error when the annotation value cannot be parsed by strconv.ParseBool or if we are unable // to read the pod's namespace label when it exists. -func consulDNSEnabled(namespace corev1.Namespace, pod corev1.Pod, globalDNSEnabled bool, globalTProxyEnabled bool) (bool, error) { +func consulDNSEnabled( + namespace corev1.Namespace, + pod corev1.Pod, + globalDNSEnabled bool, + globalTProxyEnabled bool, +) (bool, error) { // DNS is only possible when tproxy is also enabled because it relies // on traffic being redirected. tproxy, err := common.TransparentProxyEnabled(namespace, pod, globalTProxyEnabled) diff --git a/control-plane/connect-inject/webhook/container_init_test.go b/control-plane/connect-inject/webhook/container_init_test.go index 8feac95b84..5896c0c0eb 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(1000700000), + RunAsGroup: pointer.Int64(1000700000), + 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..e6de09f448 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,18 @@ 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) + } else { + // When using OpenShift, the uid and group are saved as an annotation on the namespace + uid, err := common.GetOpenShiftUID(&ns) + if err != nil { + return "", err + } + cfg.ProxyUserID = strconv.FormatInt(uid, 10) + } // Set the proxy's inbound port. diff --git a/control-plane/go.mod b/control-plane/go.mod index 33470a6cb5..36a4db2f70 100644 --- a/control-plane/go.mod +++ b/control-plane/go.mod @@ -164,4 +164,4 @@ require ( sigs.k8s.io/yaml v1.3.0 // indirect ) -go 1.20 +go 1.21 From 6fc6a2b31ee25fd038a9ddc5890ae2638e0d2694 Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Tue, 26 Mar 2024 13:39:11 -0400 Subject: [PATCH 02/38] add changelog entry --- .changelog/3813.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/3813.txt diff --git a/.changelog/3813.txt b/.changelog/3813.txt new file mode 100644 index 0000000000..59ef045467 --- /dev/null +++ b/.changelog/3813.txt @@ -0,0 +1,3 @@ +```release-note:improvement +control-plane: Remove anyuid Security Context Constraints (SCC) requirement in OpenShift. +``` From 22f02dabe8852cae8b068b0b3223f4aee43fc17d Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Tue, 26 Mar 2024 13:55:05 -0400 Subject: [PATCH 03/38] fix linter --- control-plane/connect-inject/common/openshift.go | 2 +- .../connect-inject/constants/annotations_and_labels.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index 1c36182303..a19b52a6e0 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -24,7 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" ) -// Get the user id from the OpenShift annotation 'openshift.io/sa.scc.uid-range' +// Get the user id from the OpenShift annotation 'openshift.io/sa.scc.uid-range'. func GetOpenShiftUID(ns *corev1.Namespace) (int64, error) { annotation, ok := ns.Annotations[constants.AnnotationOpenShiftUIDRange] if !ok { diff --git a/control-plane/connect-inject/constants/annotations_and_labels.go b/control-plane/connect-inject/constants/annotations_and_labels.go index f524a8ff19..a0a59ce91b 100644 --- a/control-plane/connect-inject/constants/annotations_and_labels.go +++ b/control-plane/connect-inject/constants/annotations_and_labels.go @@ -235,7 +235,7 @@ const ( AnnotationPrometheusPort = "prometheus.io/port" ) -// Annotations used by OpenShift +// Annotations used by OpenShift. const ( AnnotationOpenShiftGroups = "openshift.io/sa.scc.supplemental-groups" AnnotationOpenShiftUIDRange = "openshift.io/sa.scc.uid-range" From 6dacaee653d4e656527666f4b7c9db7e7b2926af Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Wed, 27 Mar 2024 10:27:42 -0400 Subject: [PATCH 04/38] Update with review comments --- acceptance/framework/consul/helm_cluster.go | 53 +++++++++---------- .../connect-inject/common/openshift.go | 13 ++--- .../connect-inject/common/openshift_test.go | 17 ++++++ .../webhook/consul_dataplane_sidecar.go | 5 +- 4 files changed, 49 insertions(+), 39 deletions(-) diff --git a/acceptance/framework/consul/helm_cluster.go b/acceptance/framework/consul/helm_cluster.go index 48e67bb5bf..54032b2978 100644 --- a/acceptance/framework/consul/helm_cluster.go +++ b/acceptance/framework/consul/helm_cluster.go @@ -526,7 +526,6 @@ func (h *HelmCluster) SetupConsulClient(t *testing.T, secure bool, release ...st require.NoError(r, err) } }) - } } @@ -684,35 +683,31 @@ func configureSCCs(t *testing.T, client kubernetes.Interface, cfg *config.TestCo 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{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() { diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index a19b52a6e0..e8e2f555e8 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -24,7 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" ) -// Get the user id from the OpenShift annotation 'openshift.io/sa.scc.uid-range'. +// GetOpenShiftUID gets the user id from the OpenShift annotation 'openshift.io/sa.scc.uid-range'. func GetOpenShiftUID(ns *corev1.Namespace) (int64, error) { annotation, ok := ns.Annotations[constants.AnnotationOpenShiftUIDRange] if !ok { @@ -42,8 +42,9 @@ func GetOpenShiftUID(ns *corev1.Namespace) (int64, error) { return uid, nil } -// Parse the UID "range" from the annotation string. The annotation can either have a '/' or '-' as a separator. -// '-' is the old style of UID from when it used to be an actual range. +// parseOpenShiftUID parses the UID "range" from the annotation string. The annotation can either have a '/' or '-' +// as a separator. '-' is the old style of UID from when it used to be an actual range. +// Example annotation value: "1000700000/100000". func parseOpenShiftUID(val string) (int64, error) { var uid int64 var err error @@ -73,7 +74,7 @@ func parseOpenShiftUID(val string) (int64, error) { return uid, nil } -// Get the user id from the OpenShift annotation 'openshift.io/sa.scc.supplemental-groups' +// GetOpenShiftGroup gets the group from OpenShift annotation 'openshift.io/sa.scc.supplemental-groups' // Fall back to the UID annotation if the group annotation does not exist. The values should // be the same. func GetOpenShiftGroup(ns *corev1.Namespace) (int64, error) { @@ -101,8 +102,8 @@ func GetOpenShiftGroup(ns *corev1.Namespace) (int64, error) { return uid, nil } -// Parse the group from the annotation string. The annotation can either have a '/' or ',' as a separator. -// ',' is the old style of UID from when it used to be an actual range. +// parseOpenShiftGroup parses the group from the annotation string. The annotation can either have a '/' or ',' +// as a separator. ',' is the old style of UID from when it used to be an actual range. func parseOpenShiftGroup(val string) (int64, error) { var group int64 var err error diff --git a/control-plane/connect-inject/common/openshift_test.go b/control-plane/connect-inject/common/openshift_test.go index c31b26c9d1..e4a5178c7a 100644 --- a/control-plane/connect-inject/common/openshift_test.go +++ b/control-plane/connect-inject/common/openshift_test.go @@ -89,6 +89,23 @@ func TestOpenShiftUID(t *testing.T) { Expected: 0, Err: fmt.Sprintf("unable to find annotation %s", constants.AnnotationOpenShiftUIDRange), }, + { + Name: "Empty", + Namespace: func() *corev1.Namespace { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "", + }, + }, + } + return ns + }, + Expected: 0, + Err: "found annotation openshift.io/sa.scc.uid-range but it was empty", + }, } for _, tt := range cases { t.Run(tt.Name, func(t *testing.T) { diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index fa2654d021..f9c5c60727 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -219,7 +219,7 @@ func (w *MeshWebhook) consulDataplaneSidecar( // 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 { - // In non-OpenShift environments we set the User and group ID for the sidecare to our values. + // In non-OpenShift environments we set the User and group ID for the sidecar to our values. if !w.EnableOpenShift { if pod.Spec.SecurityContext != nil { // User container and consul-dataplane container cannot have the same UID. @@ -271,9 +271,6 @@ func (w *MeshWebhook) consulDataplaneSidecar( } } } - - // When running tproxy and OpenShift, - return container, nil } From b2c3b505b14b976c667b9ade8c8b970a42020267 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 27 Mar 2024 13:42:00 -0400 Subject: [PATCH 05/38] add some print statements --- .../tests/api-gateway/api_gateway_kitchen_sink_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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..14b8fa9644 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,12 @@ 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) + t.Log("Melisa httpRoute -----------------------", httpRoute.Status) + require.NoError(r, err) + + t.Log("Melisa -----------------------", gateway.Status) 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 +158,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]) From da0ebc16c7d0ebcaca18aa15c3a73a24bffbf92d Mon Sep 17 00:00:00 2001 From: Curt Bushko Date: Thu, 28 Mar 2024 11:39:21 -0400 Subject: [PATCH 06/38] remove debug output from api gateway kitchen sink --- acceptance/tests/api-gateway/api_gateway_kitchen_sink_test.go | 2 -- 1 file changed, 2 deletions(-) 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 14b8fa9644..9880298a2b 100644 --- a/acceptance/tests/api-gateway/api_gateway_kitchen_sink_test.go +++ b/acceptance/tests/api-gateway/api_gateway_kitchen_sink_test.go @@ -144,10 +144,8 @@ func TestAPIGateway_KitchenSink(t *testing.T) { // http route checks err = k8sClient.Get(context.Background(), types.NamespacedName{Name: "http-route", Namespace: "default"}, &httpRoute) - t.Log("Melisa httpRoute -----------------------", httpRoute.Status) require.NoError(r, err) - t.Log("Melisa -----------------------", gateway.Status) 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")) From 7f57d814b0867752b16c129a09d8cdbe1fd7dd37 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Tue, 25 Jun 2024 10:01:42 -0400 Subject: [PATCH 07/38] Reorganize if statement that is hard to understand --- .../connect-inject/webhook/container_init.go | 77 ++++++++++--------- 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/control-plane/connect-inject/webhook/container_init.go b/control-plane/connect-inject/webhook/container_init.go index 7a4ec0e274..15d0fcb8c5 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -231,47 +231,13 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, } if tproxyEnabled { - if !w.EnableCNI { - // Set redirect traffic config for the container so that we can apply iptables rules. - redirectTrafficConfig, err := w.iptablesConfigJSON(pod, namespace) - if err != nil { - return corev1.Container{}, err - } - container.Env = append(container.Env, - corev1.EnvVar{ - Name: "CONSUL_REDIRECT_TRAFFIC_CONFIG", - Value: redirectTrafficConfig, - }) - - // Running consul connect redirect-traffic with iptables - // requires both being a root user and having NET_ADMIN capability. - container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: pointer.Int64(rootUserAndGroupID), - RunAsGroup: pointer.Int64(rootUserAndGroupID), - // RunAsNonRoot overrides any setting in the Pod so that we can still run as root here as required. - RunAsNonRoot: pointer.Bool(false), - Privileged: pointer.Bool(privileged), - Capabilities: &corev1.Capabilities{ - Add: []corev1.Capability{netAdminCapability}, - }, - } - } else { - if !w.EnableOpenShift { - 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), - } - } else { + if w.EnableCNI { + if w.EnableOpenShift { // Transparent proxy + CNI 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. uid, err := common.GetOpenShiftUID(&namespace) + // TODO: Melisa remove below logging statement + w.Log.Info("Melisa ---------------------------------------OpenShift UID", "uid", uid) if err != nil { return corev1.Container{}, err } @@ -290,7 +256,42 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, ReadOnlyRootFilesystem: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), } + } 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), + } + } + } else { + // Set redirect traffic config for the container so that we can apply iptables rules. + redirectTrafficConfig, err := w.iptablesConfigJSON(pod, namespace) + if err != nil { + return corev1.Container{}, err + } + container.Env = append(container.Env, + corev1.EnvVar{ + Name: "CONSUL_REDIRECT_TRAFFIC_CONFIG", + Value: redirectTrafficConfig, + }) + // Running consul connect redirect-traffic with iptables + // requires both being a root user and having NET_ADMIN capability. + container.SecurityContext = &corev1.SecurityContext{ + RunAsUser: pointer.Int64(rootUserAndGroupID), + RunAsGroup: pointer.Int64(rootUserAndGroupID), + // RunAsNonRoot overrides any setting in the Pod so that we can still run as root here as required. + RunAsNonRoot: pointer.Bool(false), + Privileged: pointer.Bool(privileged), + Capabilities: &corev1.Capabilities{ + Add: []corev1.Capability{netAdminCapability}, + }, } } } From 0707f39b89df6738efdc85889312686429594dc9 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 26 Jun 2024 10:16:35 -0400 Subject: [PATCH 08/38] Updates Api Gateway to use the UID and GID given to it by Openshift --- .../api-gateway/gatekeeper/deployment.go | 2 +- control-plane/api-gateway/gatekeeper/init.go | 28 ++++++++++++++++--- 2 files changed, 25 insertions(+), 5 deletions(-) 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/init.go b/control-plane/api-gateway/gatekeeper/init.go index b30bafc240..d53db718c6 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -10,7 +10,6 @@ import ( "text/template" corev1 "k8s.io/api/core/v1" - "k8s.io/utils/pointer" "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" @@ -35,7 +34,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 +174,30 @@ func initContainer(config common.HelmConfig, name, namespace string) (corev1.Con container.Resources = *config.InitContainerResources } + // TODO: Melisa we will need this if we match the style of what the webhook does + //ns := &corev1.Namespace{} + // // TODO: contexts + //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 nil, fmt.Errorf("error getting namespace metadata for deployment: %s", err) + //} + + uid := pointer.Int64(initContainersUserAndGroupID) + groupID := pointer.Int64(initContainersUserAndGroupID) + + // In Openshift we let Openshift set the UID and GID + // TODO: Melisa will probably clean this up to match what webhook does + if config.EnableOpenShift { + uid = nil + groupID = nil + } + container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: pointer.Int64(initContainersUserAndGroupID), - RunAsGroup: pointer.Int64(initContainersUserAndGroupID), + RunAsUser: uid, + RunAsGroup: groupID, RunAsNonRoot: pointer.Bool(true), Privileged: pointer.Bool(false), Capabilities: &corev1.Capabilities{ From c07c0eb4ff89795cc7bd1a09544774b82ef8c4f0 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 26 Jun 2024 10:48:07 -0400 Subject: [PATCH 09/38] Reorg creation of security context for the webhook container init, now we can use one security context with just a switch for privileged. --- .../connect-inject/webhook/container_init.go | 52 ++++++++----------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/control-plane/connect-inject/webhook/container_init.go b/control-plane/connect-inject/webhook/container_init.go index 15d0fcb8c5..eba9fe86e9 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -232,42 +232,36 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, if tproxyEnabled { if w.EnableCNI { + // For non Openshift, we use the initContainersUserAndGroupID for the user and group id. + uid := int64(initContainersUserAndGroupID) + group := int64(initContainersUserAndGroupID) + + // For Transparent proxy + CNI set in OpenShift. There is an annotation on the namespace that tells us what + // the user and group ids should be for the sidecar. if w.EnableOpenShift { - // Transparent proxy + CNI 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. - uid, err := common.GetOpenShiftUID(&namespace) - // TODO: Melisa remove below logging statement - w.Log.Info("Melisa ---------------------------------------OpenShift UID", "uid", uid) + var err error + + uid, err = common.GetOpenShiftUID(&namespace) + if err != nil { return corev1.Container{}, err } - group, err := common.GetOpenShiftGroup(&namespace) + group, err = common.GetOpenShiftGroup(&namespace) 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(false), - Capabilities: &corev1.Capabilities{ - Drop: []corev1.Capability{"ALL"}, - }, - ReadOnlyRootFilesystem: pointer.Bool(true), - AllowPrivilegeEscalation: pointer.Bool(false), - } - } 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), - } + } + + 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. From cb7f6713cbbe5490533b00a6816e5cb97efadb23 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 26 Jun 2024 14:29:40 -0400 Subject: [PATCH 10/38] Adds net_bind_service capability from https://github.com/hashicorp/consul-k8s/pull/4066 --- .../webhook/consul_dataplane_sidecar.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index f9c5c60727..effec7bc62 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -219,8 +219,8 @@ func (w *MeshWebhook) consulDataplaneSidecar( // 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 { - // In non-OpenShift environments we set the User and group ID for the sidecar to our values. if !w.EnableOpenShift { + // In non-OpenShift environments we set the User and group ID for the sidecar to our values. 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 { @@ -249,7 +249,12 @@ func (w *MeshWebhook) consulDataplaneSidecar( RunAsGroup: pointer.Int64(sidecarUserAndGroupID), RunAsNonRoot: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), - ReadOnlyRootFilesystem: pointer.Bool(true), + // 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), } } else { // Transparent proxy is set in OpenShift. There is an annotation on the namespace that tells us what @@ -267,7 +272,12 @@ func (w *MeshWebhook) consulDataplaneSidecar( RunAsGroup: pointer.Int64(group), RunAsNonRoot: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), - ReadOnlyRootFilesystem: pointer.Bool(true), + // 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), } } } From 0ae97a05a782621ca74b501281f76d8e18198b58 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 26 Jun 2024 16:03:21 -0400 Subject: [PATCH 11/38] Reorganizes dataplane sidecar logic in webhook to be a bit more understandable --- .../webhook/consul_dataplane_sidecar.go | 53 +++++++++---------- .../connect-inject/webhook/container_init.go | 2 +- 2 files changed, 26 insertions(+), 29 deletions(-) diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index effec7bc62..02f3df0cc9 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -219,8 +219,12 @@ func (w *MeshWebhook) consulDataplaneSidecar( // 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 { + + // Default values for non-Openshift environments. + uid := int64(sidecarUserAndGroupID) + group := int64(sidecarUserAndGroupID) + if !w.EnableOpenShift { - // In non-OpenShift environments we set the User and group ID for the sidecar to our values. 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 { @@ -230,6 +234,7 @@ func (w *MeshWebhook) consulDataplaneSidecar( ) } } + // TODO: Melisa why are we concerned about this // 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 { @@ -244,41 +249,33 @@ func (w *MeshWebhook) consulDataplaneSidecar( ) } } - container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: pointer.Int64(sidecarUserAndGroupID), - RunAsGroup: pointer.Int64(sidecarUserAndGroupID), - 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), - } - } else { + } + + 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. - uid, err := common.GetOpenShiftUID(&namespace) + var err error + uid, err = common.GetOpenShiftUID(&namespace) if err != nil { return corev1.Container{}, err } - group, err := common.GetOpenShiftGroup(&namespace) + group, err = common.GetOpenShiftGroup(&namespace) 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), - } + } + + 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/container_init.go b/control-plane/connect-inject/webhook/container_init.go index eba9fe86e9..3d9f9b1ace 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -236,7 +236,7 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, uid := int64(initContainersUserAndGroupID) group := int64(initContainersUserAndGroupID) - // For Transparent proxy + CNI set in OpenShift. There is an annotation on the namespace that tells us what + // 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 From cd4a6872d8c5f138d22ddf43c2791d59c07d1ca3 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 26 Jun 2024 16:36:36 -0400 Subject: [PATCH 12/38] Adds tests for NET_BIND_SERVICE from here: https://github.com/hashicorp/consul-k8s/pull/4066 --- .../webhook/consul_dataplane_sidecar_test.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) 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 4a8386c493..2b7cdf7bf0 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go @@ -806,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": { @@ -817,12 +820,19 @@ 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{ + Capabilities: &corev1.Capabilities{ + Add: []corev1.Capability{"NET_BIND_SERVICE"}, + }, + }, }, "tproxy enabled; openshift enabled": { tproxyEnabled: true, @@ -833,6 +843,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"}, + }, }, }, } From e20ff5ba7614743abcd4dda37724b96899ee7f0b Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 26 Jun 2024 16:39:02 -0400 Subject: [PATCH 13/38] Adds changelog --- .changelog/{3813.txt => 4152.txt} | 4 ++++ 1 file changed, 4 insertions(+) rename .changelog/{3813.txt => 4152.txt} (51%) diff --git a/.changelog/3813.txt b/.changelog/4152.txt similarity index 51% rename from .changelog/3813.txt rename to .changelog/4152.txt index 59ef045467..7dbc369814 100644 --- a/.changelog/3813.txt +++ b/.changelog/4152.txt @@ -1,3 +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 +``` From bfe7be895efc6bbebb3ba384f51ecb5f8d8530a2 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Wed, 26 Jun 2024 16:53:50 -0400 Subject: [PATCH 14/38] Didn't reorg the dataplane sidecar in the webhook correctly --- .../webhook/consul_dataplane_sidecar.go | 57 +++++++++---------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index 02f3df0cc9..e09d45dc21 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -214,16 +214,15 @@ func (w *MeshWebhook) consulDataplaneSidecar( 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 { - - // Default values for non-Openshift environments. - uid := int64(sidecarUserAndGroupID) - group := int64(sidecarUserAndGroupID) - if !w.EnableOpenShift { if pod.Spec.SecurityContext != nil { // User container and consul-dataplane container cannot have the same UID. @@ -250,34 +249,34 @@ func (w *MeshWebhook) consulDataplaneSidecar( } } } + } - 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.GetOpenShiftUID(&namespace) - if err != nil { - return corev1.Container{}, err - } - group, err = common.GetOpenShiftGroup(&namespace) - if err != nil { - return corev1.Container{}, err - } + 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.GetOpenShiftUID(&namespace) + 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), + group, err = common.GetOpenShiftGroup(&namespace) + 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 } From 1b74985fbb528fb51c26498375e22a12efd9e5e4 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Thu, 27 Jun 2024 12:45:28 -0400 Subject: [PATCH 15/38] Fix dataplane sidecar test for webhook --- .../connect-inject/webhook/consul_dataplane_sidecar_test.go | 5 +++++ 1 file changed, 5 insertions(+) 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 2b7cdf7bf0..4bbf24bc45 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go @@ -829,6 +829,11 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { tproxyEnabled: false, openShiftEnabled: true, expSecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(1000700000), + RunAsGroup: pointer.Int64(1000700000), + RunAsNonRoot: pointer.Bool(true), + ReadOnlyRootFilesystem: pointer.Bool(true), + AllowPrivilegeEscalation: pointer.Bool(false), Capabilities: &corev1.Capabilities{ Add: []corev1.Capability{"NET_BIND_SERVICE"}, }, From 38387fed74fe41c36721400ab5001a4508510395 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Thu, 27 Jun 2024 15:35:10 -0400 Subject: [PATCH 16/38] Remove some formatting changes, making PR larger than it needs to be. --- .../webhook/consul_dataplane_sidecar.go | 77 +++---------------- .../connect-inject/webhook/container_init.go | 7 +- 2 files changed, 13 insertions(+), 71 deletions(-) diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index e09d45dc21..44c275e4a0 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -24,11 +24,7 @@ const ( consulDataplaneDNSBindPort = 8600 ) -func (w *MeshWebhook) consulDataplaneSidecar( - namespace corev1.Namespace, - pod corev1.Pod, - mpi multiPortInfo, -) (corev1.Container, error) { +func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod corev1.Pod, mpi multiPortInfo) (corev1.Container, error) { resources, err := w.sidecarResources(pod) if err != nil { return corev1.Container{}, err @@ -227,10 +223,7 @@ func (w *MeshWebhook) consulDataplaneSidecar( 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, - ) + return corev1.Container{}, fmt.Errorf("pod's security context cannot have the same UID as consul-dataplane: %v", sidecarUserAndGroupID) } } // TODO: Melisa why are we concerned about this @@ -241,11 +234,7 @@ func (w *MeshWebhook) consulDataplaneSidecar( 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, - ) + 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) } } } @@ -280,12 +269,7 @@ func (w *MeshWebhook) consulDataplaneSidecar( return container, nil } -func (w *MeshWebhook) getContainerSidecarArgs( - namespace corev1.Namespace, - mpi multiPortInfo, - bearerTokenFile string, - pod corev1.Pod, -) ([]string, error) { +func (w *MeshWebhook) getContainerSidecarArgs(namespace corev1.Namespace, mpi multiPortInfo, bearerTokenFile string, pod corev1.Pod) ([]string, error) { proxyIDFileName := "/consul/connect-inject/proxyid" if mpi.serviceName != "" { proxyIDFileName = fmt.Sprintf("/consul/connect-inject/proxyid-%s", mpi.serviceName) @@ -426,14 +410,7 @@ func (w *MeshWebhook) getContainerSidecarArgs( } if serviceMetricsPath != "" && serviceMetricsPort != "" { - args = append( - args, - "-telemetry-prom-service-metrics-url="+fmt.Sprintf( - "http://127.0.0.1:%s%s", - serviceMetricsPort, - serviceMetricsPath, - ), - ) + args = append(args, "-telemetry-prom-service-metrics-url="+fmt.Sprintf("http://127.0.0.1:%s%s", serviceMetricsPort, serviceMetricsPath)) } // Pull the TLS config from the relevant annotations. @@ -460,23 +437,13 @@ func (w *MeshWebhook) getContainerSidecarArgs( // Validate required Prometheus TLS config is present if set. if prometheusCAFile != "" || prometheusCAPath != "" || prometheusCertFile != "" || prometheusKeyFile != "" { if prometheusCAFile == "" && prometheusCAPath == "" { - return nil, fmt.Errorf( - "must set one of %q or %q when providing prometheus TLS config", - constants.AnnotationPrometheusCAFile, - constants.AnnotationPrometheusCAPath, - ) + return nil, fmt.Errorf("must set one of %q or %q when providing prometheus TLS config", constants.AnnotationPrometheusCAFile, constants.AnnotationPrometheusCAPath) } if prometheusCertFile == "" { - return nil, fmt.Errorf( - "must set %q when providing prometheus TLS config", - constants.AnnotationPrometheusCertFile, - ) + return nil, fmt.Errorf("must set %q when providing prometheus TLS config", constants.AnnotationPrometheusCertFile) } if prometheusKeyFile == "" { - return nil, fmt.Errorf( - "must set %q when providing prometheus TLS config", - constants.AnnotationPrometheusKeyFile, - ) + return nil, fmt.Errorf("must set %q when providing prometheus TLS config", constants.AnnotationPrometheusKeyFile) } // TLS config has been validated, add them to the consul-dataplane cmd args args = append(args, "-telemetry-prom-ca-certs-file="+prometheusCAFile, @@ -556,12 +523,7 @@ func (w *MeshWebhook) sidecarResources(pod corev1.Pod) (corev1.ResourceRequireme if anno, ok := pod.Annotations[constants.AnnotationSidecarProxyCPULimit]; ok { cpuLimit, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf( - "parsing annotation %s:%q: %s", - constants.AnnotationSidecarProxyCPULimit, - anno, - err, - ) + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", constants.AnnotationSidecarProxyCPULimit, anno, err) } resources.Limits[corev1.ResourceCPU] = cpuLimit } else if w.DefaultProxyCPULimit != zeroQuantity { @@ -572,12 +534,7 @@ func (w *MeshWebhook) sidecarResources(pod corev1.Pod) (corev1.ResourceRequireme if anno, ok := pod.Annotations[constants.AnnotationSidecarProxyCPURequest]; ok { cpuRequest, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf( - "parsing annotation %s:%q: %s", - constants.AnnotationSidecarProxyCPURequest, - anno, - err, - ) + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", constants.AnnotationSidecarProxyCPURequest, anno, err) } resources.Requests[corev1.ResourceCPU] = cpuRequest } else if w.DefaultProxyCPURequest != zeroQuantity { @@ -588,12 +545,7 @@ func (w *MeshWebhook) sidecarResources(pod corev1.Pod) (corev1.ResourceRequireme if anno, ok := pod.Annotations[constants.AnnotationSidecarProxyMemoryLimit]; ok { memoryLimit, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf( - "parsing annotation %s:%q: %s", - constants.AnnotationSidecarProxyMemoryLimit, - anno, - err, - ) + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", constants.AnnotationSidecarProxyMemoryLimit, anno, err) } resources.Limits[corev1.ResourceMemory] = memoryLimit } else if w.DefaultProxyMemoryLimit != zeroQuantity { @@ -604,12 +556,7 @@ func (w *MeshWebhook) sidecarResources(pod corev1.Pod) (corev1.ResourceRequireme if anno, ok := pod.Annotations[constants.AnnotationSidecarProxyMemoryRequest]; ok { memoryRequest, err := resource.ParseQuantity(anno) if err != nil { - return corev1.ResourceRequirements{}, fmt.Errorf( - "parsing annotation %s:%q: %s", - constants.AnnotationSidecarProxyMemoryRequest, - anno, - err, - ) + return corev1.ResourceRequirements{}, fmt.Errorf("parsing annotation %s:%q: %s", constants.AnnotationSidecarProxyMemoryRequest, anno, err) } resources.Requests[corev1.ResourceMemory] = memoryRequest } else if w.DefaultProxyMemoryRequest != zeroQuantity { diff --git a/control-plane/connect-inject/webhook/container_init.go b/control-plane/connect-inject/webhook/container_init.go index 3d9f9b1ace..a3ccddb82b 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -296,12 +296,7 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, // consulDNSEnabled returns true if Consul DNS should be enabled for this pod. // It returns an error when the annotation value cannot be parsed by strconv.ParseBool or if we are unable // to read the pod's namespace label when it exists. -func consulDNSEnabled( - namespace corev1.Namespace, - pod corev1.Pod, - globalDNSEnabled bool, - globalTProxyEnabled bool, -) (bool, error) { +func consulDNSEnabled(namespace corev1.Namespace, pod corev1.Pod, globalDNSEnabled bool, globalTProxyEnabled bool) (bool, error) { // DNS is only possible when tproxy is also enabled because it relies // on traffic being redirected. tproxy, err := common.TransparentProxyEnabled(namespace, pod, globalTProxyEnabled) From 16bb1debb663a3fab36985fd8a46bae00a7ce031 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 28 Jun 2024 12:22:26 -0400 Subject: [PATCH 17/38] Uses the Openshift IDs for gateway init container --- control-plane/api-gateway/gatekeeper/init.go | 44 ++++++++++++------- .../webhook/consul_dataplane_sidecar.go | 2 +- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go index d53db718c6..ffc57237cb 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -5,14 +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" ) @@ -174,30 +178,36 @@ func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace stri container.Resources = *config.InitContainerResources } - // TODO: Melisa we will need this if we match the style of what the webhook does - //ns := &corev1.Namespace{} - // // TODO: contexts - //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 nil, fmt.Errorf("error getting namespace metadata for deployment: %s", err) - //} + 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) + } - uid := pointer.Int64(initContainersUserAndGroupID) - groupID := pointer.Int64(initContainersUserAndGroupID) + uid := int64(initContainersUserAndGroupID) + group := int64(initContainersUserAndGroupID) // In Openshift we let Openshift set the UID and GID - // TODO: Melisa will probably clean this up to match what webhook does if config.EnableOpenShift { - uid = nil - groupID = nil + var err error + + uid, err = ctrlCommon.GetOpenShiftUID(ns) + + if err != nil { + return corev1.Container{}, err + } + group, err = ctrlCommon.GetOpenShiftGroup(ns) + if err != nil { + return corev1.Container{}, err + } } container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: uid, - RunAsGroup: groupID, + RunAsUser: pointer.Int64(uid), + RunAsGroup: pointer.Int64(group), RunAsNonRoot: pointer.Bool(true), Privileged: pointer.Bool(false), Capabilities: &corev1.Capabilities{ diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index 44c275e4a0..1266322ede 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -226,7 +226,7 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor return corev1.Container{}, fmt.Errorf("pod's security context cannot have the same UID as consul-dataplane: %v", sidecarUserAndGroupID) } } - // TODO: Melisa why are we concerned about this + // 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 { From 79a4b45d46e19e16b80a7cc827d6046c6f6f55ba Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 28 Jun 2024 12:49:52 -0400 Subject: [PATCH 18/38] We only need to check the namespace annotations for Openshift --- control-plane/api-gateway/gatekeeper/init.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go index ffc57237cb..0c12771a30 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -178,21 +178,19 @@ func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace stri container.Resources = *config.InitContainerResources } - 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) - } - uid := int64(initContainersUserAndGroupID) group := int64(initContainersUserAndGroupID) // In Openshift we let Openshift set the UID and GID if config.EnableOpenShift { - var err error + 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) + } uid, err = ctrlCommon.GetOpenShiftUID(ns) From 22dad5e08c6e82863812af2f9b71955ff1bbe418 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 28 Jun 2024 12:53:20 -0400 Subject: [PATCH 19/38] Update test for Openshift to include namespace with annotations --- .../api-gateway/gatekeeper/gatekeeper_test.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index cf8c325364..003d653379 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -929,7 +929,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: "1000700000/100000", + constants.AnnotationOpenShiftGroups: "1000700000/100000", + }, + }, + }, + }, + }, finalResources: resources{ deployments: []*appsv1.Deployment{ configureDeployment(name, namespace, labels, 3, nil, nil, "", "1"), From 4ba20cd9d89eaed4f16af1b01e1e7b0f5c9afe48 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 28 Jun 2024 13:01:44 -0400 Subject: [PATCH 20/38] Update test to actually test for UID and Group --- control-plane/api-gateway/gatekeeper/gatekeeper_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index 003d653379..7bcee54d8a 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -20,6 +20,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" @@ -1479,6 +1480,11 @@ 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) } + + if helmConfig.EnableOpenShift { + assert.Equal(t, container.SecurityContext.RunAsUser, pointer.Int64(1000700000)) + assert.Equal(t, container.SecurityContext.RunAsGroup, pointer.Int64(1000700000)) + } } } assert.True(t, hasInitContainer) From c06d654f6596e8ffc29d10dd29e232ec86776090 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Wed, 3 Jul 2024 18:26:37 -0400 Subject: [PATCH 21/38] Use separate user/group IDs for app, init + dataplane containers --- control-plane/api-gateway/gatekeeper/init.go | 4 +- .../connect-inject/common/openshift.go | 78 ++++++++++++++----- .../connect-inject/common/openshift_test.go | 7 +- .../webhook/consul_dataplane_sidecar.go | 4 +- .../connect-inject/webhook/container_init.go | 11 +-- .../webhook/redirect_traffic.go | 14 +++- 6 files changed, 84 insertions(+), 34 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go index 0c12771a30..32141f0d1a 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -192,12 +192,12 @@ func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace stri return corev1.Container{}, fmt.Errorf("error getting namespace metadata for deployment: %s", err) } - uid, err = ctrlCommon.GetOpenShiftUID(ns) + uid, err = ctrlCommon.GetOpenShiftUID(ns, ctrlCommon.SelectFirstInRange) if err != nil { return corev1.Container{}, err } - group, err = ctrlCommon.GetOpenShiftGroup(ns) + group, err = ctrlCommon.GetOpenShiftGroup(ns, ctrlCommon.SelectFirstInRange) if err != nil { return corev1.Container{}, err } diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index e8e2f555e8..c978bdde2b 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -20,12 +20,14 @@ import ( "strconv" "strings" - "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" corev1 "k8s.io/api/core/v1" + + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" ) // GetOpenShiftUID gets the user id from the OpenShift annotation 'openshift.io/sa.scc.uid-range'. -func GetOpenShiftUID(ns *corev1.Namespace) (int64, error) { +// Select the last in the range so we don't conflict with any ID assigned to application containers. +func GetOpenShiftUID(ns *corev1.Namespace, selector idSelector) (int64, error) { annotation, ok := ns.Annotations[constants.AnnotationOpenShiftUIDRange] if !ok { return 0, fmt.Errorf("unable to find annotation %s", constants.AnnotationOpenShiftUIDRange) @@ -34,7 +36,7 @@ func GetOpenShiftUID(ns *corev1.Namespace) (int64, error) { return 0, fmt.Errorf("found annotation %s but it was empty", constants.AnnotationOpenShiftUIDRange) } - uid, err := parseOpenShiftUID(annotation) + uid, err := parseOpenShiftUID(annotation, selector) if err != nil { return 0, err } @@ -45,15 +47,11 @@ func GetOpenShiftUID(ns *corev1.Namespace) (int64, error) { // parseOpenShiftUID parses the UID "range" from the annotation string. The annotation can either have a '/' or '-' // as a separator. '-' is the old style of UID from when it used to be an actual range. // Example annotation value: "1000700000/100000". -func parseOpenShiftUID(val string) (int64, error) { +func parseOpenShiftUID(val string, selector idSelector) (int64, error) { var uid int64 var err error if strings.Contains(val, "/") { - str := strings.Split(val, "/") - uid, err = strconv.ParseInt(str[0], 10, 64) - if err != nil { - return 0, err - } + return selectIDInRange(val, selector) } if strings.Contains(val, "-") { str := strings.Split(val, "-") @@ -77,7 +75,8 @@ func parseOpenShiftUID(val string) (int64, error) { // GetOpenShiftGroup gets the group from OpenShift annotation 'openshift.io/sa.scc.supplemental-groups' // Fall back to the UID annotation if the group annotation does not exist. The values should // be the same. -func GetOpenShiftGroup(ns *corev1.Namespace) (int64, error) { +// Select the last in the range so we don't conflict with any ID assigned randomly to application containers. +func GetOpenShiftGroup(ns *corev1.Namespace, selector idSelector) (int64, error) { annotation, ok := ns.Annotations[constants.AnnotationOpenShiftGroups] if !ok { // fall back to UID annotation @@ -94,25 +93,21 @@ func GetOpenShiftGroup(ns *corev1.Namespace) (int64, error) { return 0, fmt.Errorf("found annotation %s but it was empty", constants.AnnotationOpenShiftGroups) } - uid, err := parseOpenShiftGroup(annotation) + gid, err := parseOpenShiftGroup(annotation, selector) if err != nil { return 0, err } - return uid, nil + return gid, nil } // parseOpenShiftGroup parses the group from the annotation string. The annotation can either have a '/' or ',' // as a separator. ',' is the old style of UID from when it used to be an actual range. -func parseOpenShiftGroup(val string) (int64, error) { +func parseOpenShiftGroup(val string, selector idSelector) (int64, error) { var group int64 var err error if strings.Contains(val, "/") { - str := strings.Split(val, "/") - group, err = strconv.ParseInt(str[0], 10, 64) - if err != nil { - return 0, err - } + return selectIDInRange(val, selector) } if strings.Contains(val, ",") { str := strings.Split(val, ",") @@ -128,3 +123,50 @@ func parseOpenShiftGroup(val string) (int64, error) { return group, nil } + +type idSelector func(values []int64) (int64, error) + +var SelectFirstInRange idSelector = func(values []int64) (int64, error) { + if len(values) < 1 { + return 0, fmt.Errorf("range must have at least 1 value") + } + return values[0], nil +} + +var SelectSidecarID idSelector = func(values []int64) (int64, error) { + if len(values) < 2 { + return 0, fmt.Errorf("range must have at least 2 values") + } + return values[len(values)-2], nil +} + +var SelectInitContainerID idSelector = func(values []int64) (int64, error) { + if len(values) < 1 { + return 0, fmt.Errorf("range must have at least 1 value") + } + return values[len(values)-1], nil +} + +func selectIDInRange(value string, selector idSelector) (int64, error) { + parts := strings.Split(value, "/") + if len(parts) != 2 { + return 0, fmt.Errorf("invalid range format: %s", value) + } + + start, err := strconv.Atoi(parts[0]) + if err != nil { + return 0, fmt.Errorf("invalid range format: %s", parts[0]) + } + + length, err := strconv.Atoi(parts[1]) + if err != nil { + return 0, fmt.Errorf("invalid range format: %s", parts[1]) + } + + values := make([]int64, length) + for i := 0; i < length; i++ { + values[i] = int64(start + i) + } + + return selector(values) +} diff --git a/control-plane/connect-inject/common/openshift_test.go b/control-plane/connect-inject/common/openshift_test.go index e4a5178c7a..2f25e3a6e8 100644 --- a/control-plane/connect-inject/common/openshift_test.go +++ b/control-plane/connect-inject/common/openshift_test.go @@ -6,10 +6,11 @@ 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" + + "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" ) func TestOpenShiftUID(t *testing.T) { @@ -110,7 +111,7 @@ func TestOpenShiftUID(t *testing.T) { for _, tt := range cases { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - actual, err := GetOpenShiftUID(tt.Namespace()) + actual, err := GetOpenShiftUID(tt.Namespace(), SelectFirstInRange) if tt.Err == "" { require.NoError(err) require.Equal(tt.Expected, actual) @@ -224,7 +225,7 @@ func TestOpenShiftGroup(t *testing.T) { for _, tt := range cases { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - actual, err := GetOpenShiftGroup(tt.Namespace()) + actual, err := GetOpenShiftGroup(tt.Namespace(), SelectFirstInRange) if tt.Err == "" { require.NoError(err) require.Equal(tt.Expected, actual) diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index 1266322ede..8b78c50a7e 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -244,11 +244,11 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor // 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.GetOpenShiftUID(&namespace) + uid, err = common.GetOpenShiftUID(&namespace, common.SelectSidecarID) if err != nil { return corev1.Container{}, err } - group, err = common.GetOpenShiftGroup(&namespace) + group, err = common.GetOpenShiftGroup(&namespace, common.SelectSidecarID) if err != nil { return corev1.Container{}, err } diff --git a/control-plane/connect-inject/webhook/container_init.go b/control-plane/connect-inject/webhook/container_init.go index a3ccddb82b..e6ee49c424 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 ( @@ -241,12 +242,12 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, if w.EnableOpenShift { var err error - uid, err = common.GetOpenShiftUID(&namespace) - + uid, err = common.GetOpenShiftUID(&namespace, common.SelectInitContainerID) if err != nil { return corev1.Container{}, err } - group, err = common.GetOpenShiftGroup(&namespace) + + group, err = common.GetOpenShiftGroup(&namespace, common.SelectInitContainerID) if err != nil { return corev1.Container{}, err } diff --git a/control-plane/connect-inject/webhook/redirect_traffic.go b/control-plane/connect-inject/webhook/redirect_traffic.go index e6de09f448..25e089514b 100644 --- a/control-plane/connect-inject/webhook/redirect_traffic.go +++ b/control-plane/connect-inject/webhook/redirect_traffic.go @@ -31,14 +31,23 @@ func (w *MeshWebhook) iptablesConfigJSON(pod corev1.Pod, ns corev1.Namespace) (s 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.GetOpenShiftUID(&ns) + uid, err := common.GetOpenShiftUID(&ns, common.SelectSidecarID) 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.GetOpenShiftGroup(&ns, common.SelectInitContainerID) + if err != nil { + return "", err + } + cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.FormatInt(uid, 10)) } // Set the proxy's inbound port. @@ -110,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 From 886baddee364525eb87edf6e2705a80d338549b6 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 5 Jul 2024 15:19:41 -0400 Subject: [PATCH 22/38] Will not work, worried power is going to go out. --- .../connect-inject/common/openshift.go | 152 ++++++++---------- .../webhook/consul_dataplane_sidecar.go | 3 +- 2 files changed, 73 insertions(+), 82 deletions(-) diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index c978bdde2b..c53054b424 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -20,108 +20,98 @@ import ( "strconv" "strings" - corev1 "k8s.io/api/core/v1" - "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" + "golang.org/x/exp/maps" + "golang.org/x/exp/slices" + corev1 "k8s.io/api/core/v1" ) -// GetOpenShiftUID gets the user id from the OpenShift annotation 'openshift.io/sa.scc.uid-range'. -// Select the last in the range so we don't conflict with any ID assigned to application containers. -func GetOpenShiftUID(ns *corev1.Namespace, selector idSelector) (int64, error) { - annotation, ok := ns.Annotations[constants.AnnotationOpenShiftUIDRange] - if !ok { - return 0, fmt.Errorf("unable to find annotation %s", constants.AnnotationOpenShiftUIDRange) - } - if len(annotation) == 0 { - return 0, fmt.Errorf("found annotation %s but it was empty", constants.AnnotationOpenShiftUIDRange) - } - - uid, err := parseOpenShiftUID(annotation, selector) +func GetSidecarUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { + availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftUIDRange]) if err != nil { return 0, err } - return uid, nil + if len(availableUIDs) < 2 { + return 0, fmt.Errorf("namespace does not have enough available UIDs") + } + + return availableUIDs[len(availableUIDs)-2], nil } -// parseOpenShiftUID parses the UID "range" from the annotation string. The annotation can either have a '/' or '-' -// as a separator. '-' is the old style of UID from when it used to be an actual range. -// Example annotation value: "1000700000/100000". -func parseOpenShiftUID(val string, selector idSelector) (int64, error) { - var uid int64 - var err error - if strings.Contains(val, "/") { - return selectIDInRange(val, selector) - } - if strings.Contains(val, "-") { - str := strings.Split(val, "-") - uid, err = strconv.ParseInt(str[0], 10, 64) - if err != nil { - return 0, err - } +func GetSidecarGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { + availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftGroups]) + if err != nil { + return 0, err } - if !strings.Contains(val, "/") && !strings.Contains(val, "-") { - return 0, fmt.Errorf( - "annotation %s contains an invalid format for value %s", - constants.AnnotationOpenShiftUIDRange, - val, - ) + if len(availableUIDs) < 2 { + return 0, fmt.Errorf("namespace does not have enough available UIDs") } - return uid, nil + return availableUIDs[len(availableUIDs)-2], nil } -// GetOpenShiftGroup gets the group from OpenShift annotation 'openshift.io/sa.scc.supplemental-groups' -// Fall back to the UID annotation if the group annotation does not exist. The values should -// be the same. -// Select the last in the range so we don't conflict with any ID assigned randomly to application containers. -func GetOpenShiftGroup(ns *corev1.Namespace, selector idSelector) (int64, error) { - annotation, ok := ns.Annotations[constants.AnnotationOpenShiftGroups] - if !ok { - // fall back to UID annotation - annotation, ok = ns.Annotations[constants.AnnotationOpenShiftUIDRange] - if !ok { - return 0, fmt.Errorf( - "unable to find annotation %s or %s", - constants.AnnotationOpenShiftGroups, - constants.AnnotationOpenShiftUIDRange, - ) - } +func GetConnectInitUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { + availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftUIDRange]) + if err != nil { + return 0, err } - if len(annotation) == 0 { - return 0, fmt.Errorf("found annotation %s but it was empty", constants.AnnotationOpenShiftGroups) + + if len(availableUIDs) < 1 { + return 0, fmt.Errorf("namespace does not have enough available UIDs") } - gid, err := parseOpenShiftGroup(annotation, selector) + return availableUIDs[len(availableUIDs)-1], nil +} + +func GetConnectInitGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { + availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftGroups]) if err != nil { return 0, err } - return gid, nil + if len(availableUIDs) < 2 { + return 0, fmt.Errorf("namespace does not have enough available UIDs") + } + + return availableUIDs[len(availableUIDs)-1], nil } -// parseOpenShiftGroup parses the group from the annotation string. The annotation can either have a '/' or ',' -// as a separator. ',' is the old style of UID from when it used to be an actual range. -func parseOpenShiftGroup(val string, selector idSelector) (int64, error) { - var group int64 - var err error - if strings.Contains(val, "/") { - return selectIDInRange(val, selector) - } - if strings.Contains(val, ",") { - str := strings.Split(val, ",") - group, err = strconv.ParseInt(str[0], 10, 64) - if err != nil { - return 0, err +func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName 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.SecurityContext != nil && c.SecurityContext.RunAsUser != nil { + appUIDs = append(appUIDs, *c.SecurityContext.RunAsUser) + } + } + + // Collect the list of valid UIDs from the namespace annotation + validUIDs, err := GetAllValidUserIDsFromNamespace(namespace.Annotations[annotationName]) + if err != nil { + return nil, fmt.Errorf("unable to get valid userIDs from namespace annotation: %w", err) + } - if !strings.Contains(val, "/") && !strings.Contains(val, ",") { - return 0, fmt.Errorf("annotation %s contains an invalid format for value %s", constants.AnnotationOpenShiftGroups, val) + // Subtract the list of application container UIDs from the list of valid userIDs + availableUIDs := make(map[int64]struct{}) + for _, uid := range validUIDs { + availableUIDs[uid] = struct{}{} + } + for _, uid := range appUIDs { + delete(availableUIDs, uid) } - return group, nil + // Return the second to last (sorted) valid UID from the available UIDs + keys := maps.Keys(availableUIDs) + slices.Sort(keys) + + return keys, nil } type idSelector func(values []int64) (int64, error) @@ -147,26 +137,26 @@ var SelectInitContainerID idSelector = func(values []int64) (int64, error) { return values[len(values)-1], nil } -func selectIDInRange(value string, selector idSelector) (int64, error) { - parts := strings.Split(value, "/") +func GetAllValidUserIDsFromNamespace(annotation string) ([]int64, error) { + parts := strings.Split(annotation, "/") if len(parts) != 2 { - return 0, fmt.Errorf("invalid range format: %s", value) + return nil, fmt.Errorf("invalid range format: %s", annotation) } start, err := strconv.Atoi(parts[0]) if err != nil { - return 0, fmt.Errorf("invalid range format: %s", parts[0]) + return nil, fmt.Errorf("invalid range format: %s", parts[0]) } length, err := strconv.Atoi(parts[1]) if err != nil { - return 0, fmt.Errorf("invalid range format: %s", parts[1]) + return nil, fmt.Errorf("invalid range format: %s", parts[1]) } - values := make([]int64, length) + userIDs := make([]int64, length) for i := 0; i < length; i++ { - values[i] = int64(start + i) + userIDs[i] = int64(start + i) } - return selector(values) + return userIDs, nil } diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index 8b78c50a7e..ac6202217a 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -244,7 +244,8 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor // 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.GetOpenShiftUID(&namespace, common.SelectSidecarID) + uid, err = common.GetSidecarUID(namespace, pod) + //uid, err = common.GetOpenShiftUID(&namespace, common.SelectSidecarID) if err != nil { return corev1.Container{}, err } From 8df66522a1aa11eb5a7ffbf3e68271756b767a0f Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 5 Jul 2024 15:28:14 -0400 Subject: [PATCH 23/38] Finishes updating Openshift get userIDs and groupIDs, still need to update tests --- control-plane/api-gateway/gatekeeper/init.go | 6 ++++-- control-plane/connect-inject/common/openshift.go | 4 ++-- control-plane/connect-inject/common/openshift_test.go | 3 +++ .../connect-inject/webhook/consul_dataplane_sidecar.go | 5 ++--- control-plane/connect-inject/webhook/container_init.go | 4 ++-- 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go index 32141f0d1a..0c96796d13 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -192,12 +192,14 @@ func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace stri return corev1.Container{}, fmt.Errorf("error getting namespace metadata for deployment: %s", err) } - uid, err = ctrlCommon.GetOpenShiftUID(ns, ctrlCommon.SelectFirstInRange) + // We need to get the userID for the dataplane, 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.GetDataplaneUID(*ns, corev1.Pod{}) if err != nil { return corev1.Container{}, err } - group, err = ctrlCommon.GetOpenShiftGroup(ns, ctrlCommon.SelectFirstInRange) + group, err = ctrlCommon.GetDataplaneGroupID(*ns, corev1.Pod{}) if err != nil { return corev1.Container{}, err } diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index c53054b424..16c29e0a52 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -26,7 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" ) -func GetSidecarUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { +func GetDataplaneUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftUIDRange]) if err != nil { return 0, err @@ -39,7 +39,7 @@ func GetSidecarUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { return availableUIDs[len(availableUIDs)-2], nil } -func GetSidecarGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { +func GetDataplaneGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftGroups]) if err != nil { return 0, err diff --git a/control-plane/connect-inject/common/openshift_test.go b/control-plane/connect-inject/common/openshift_test.go index 2f25e3a6e8..dbf9266028 100644 --- a/control-plane/connect-inject/common/openshift_test.go +++ b/control-plane/connect-inject/common/openshift_test.go @@ -13,6 +13,9 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" ) +// TODO: Melisa add a test for application taking last UID/GroupID in the range (also second to last) +// TODO: Melisa also add similar to gateway + func TestOpenShiftUID(t *testing.T) { cases := []struct { Name string diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index ac6202217a..3ceacb6025 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -244,12 +244,11 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor // 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.GetSidecarUID(namespace, pod) - //uid, err = common.GetOpenShiftUID(&namespace, common.SelectSidecarID) + uid, err = common.GetDataplaneUID(namespace, pod) if err != nil { return corev1.Container{}, err } - group, err = common.GetOpenShiftGroup(&namespace, common.SelectSidecarID) + group, err = common.GetDataplaneGroupID(namespace, pod) if err != nil { return corev1.Container{}, err } diff --git a/control-plane/connect-inject/webhook/container_init.go b/control-plane/connect-inject/webhook/container_init.go index e6ee49c424..c024f33cd1 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -242,12 +242,12 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, if w.EnableOpenShift { var err error - uid, err = common.GetOpenShiftUID(&namespace, common.SelectInitContainerID) + uid, err = common.GetConnectInitUID(namespace, pod) if err != nil { return corev1.Container{}, err } - group, err = common.GetOpenShiftGroup(&namespace, common.SelectInitContainerID) + group, err = common.GetConnectInitGroupID(namespace, pod) if err != nil { return corev1.Container{}, err } From 83abb12ebb58b9a55f450aa2b5971ab6e5fcf3a5 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 5 Jul 2024 15:36:14 -0400 Subject: [PATCH 24/38] Fix missing call updates for Openshift IDs --- control-plane/connect-inject/webhook/redirect_traffic.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/control-plane/connect-inject/webhook/redirect_traffic.go b/control-plane/connect-inject/webhook/redirect_traffic.go index 25e089514b..cffef909cb 100644 --- a/control-plane/connect-inject/webhook/redirect_traffic.go +++ b/control-plane/connect-inject/webhook/redirect_traffic.go @@ -36,18 +36,18 @@ func (w *MeshWebhook) iptablesConfigJSON(pod corev1.Pod, ns corev1.Namespace) (s 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.GetOpenShiftUID(&ns, common.SelectSidecarID) + uid, err := common.GetDataplaneUID(ns, pod) 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.GetOpenShiftGroup(&ns, common.SelectInitContainerID) + gid, err := common.GetDataplaneGroupID(ns, pod) if err != nil { return "", err } - cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.FormatInt(uid, 10)) + cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.FormatInt(gid, 10)) } // Set the proxy's inbound port. From 7c0aedcf7aa0da65c9ea8aa7d1d73e75e8ac3c31 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 5 Jul 2024 15:58:38 -0400 Subject: [PATCH 25/38] Rename function --- control-plane/connect-inject/common/openshift.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index 16c29e0a52..cb9f51f62b 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -92,8 +92,8 @@ func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName } } - // Collect the list of valid UIDs from the namespace annotation - validUIDs, err := GetAllValidUserIDsFromNamespace(namespace.Annotations[annotationName]) + // Collect the list of valid IDs from the namespace annotation + validUIDs, err := GetAllValidIDsFromNamespace(namespace.Annotations[annotationName]) if err != nil { return nil, fmt.Errorf("unable to get valid userIDs from namespace annotation: %w", err) } @@ -137,7 +137,7 @@ var SelectInitContainerID idSelector = func(values []int64) (int64, error) { return values[len(values)-1], nil } -func GetAllValidUserIDsFromNamespace(annotation string) ([]int64, error) { +func GetAllValidIDsFromNamespace(annotation string) ([]int64, error) { parts := strings.Split(annotation, "/") if len(parts) != 2 { return nil, fmt.Errorf("invalid range format: %s", annotation) From 189f093569a701c267e5800943895d7f037803f3 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Fri, 5 Jul 2024 16:00:18 -0400 Subject: [PATCH 26/38] Need name of annotation, not value --- control-plane/connect-inject/common/openshift.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index cb9f51f62b..ca74bbb1aa 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -27,7 +27,7 @@ import ( ) func GetDataplaneUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { - availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftUIDRange]) + availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftUIDRange) if err != nil { return 0, err } @@ -40,7 +40,7 @@ func GetDataplaneUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) } func GetDataplaneGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { - availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftGroups]) + availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftGroups) if err != nil { return 0, err } @@ -53,7 +53,7 @@ func GetDataplaneGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, err } func GetConnectInitUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { - availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftUIDRange]) + availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftUIDRange) if err != nil { return 0, err } @@ -66,7 +66,7 @@ func GetConnectInitUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error } func GetConnectInitGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { - availableUIDs, err := getAvailableIDs(namespace, pod, namespace.Annotations[constants.AnnotationOpenShiftGroups]) + availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftGroups) if err != nil { return 0, err } From ab16a35da6ea8e3f70176ad2ef75aa81a59691cb Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Fri, 5 Jul 2024 17:32:34 -0400 Subject: [PATCH 27/38] Fix bug in iptables config generation --- control-plane/connect-inject/webhook/redirect_traffic.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/control-plane/connect-inject/webhook/redirect_traffic.go b/control-plane/connect-inject/webhook/redirect_traffic.go index cffef909cb..b62c2b614c 100644 --- a/control-plane/connect-inject/webhook/redirect_traffic.go +++ b/control-plane/connect-inject/webhook/redirect_traffic.go @@ -43,11 +43,11 @@ func (w *MeshWebhook) iptablesConfigJSON(pod corev1.Pod, ns corev1.Namespace) (s cfg.ProxyUserID = strconv.FormatInt(uid, 10) // Exclude the user ID for the init container from traffic redirection. - gid, err := common.GetDataplaneGroupID(ns, pod) + uid, err = common.GetConnectInitUID(ns, pod) if err != nil { return "", err } - cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.FormatInt(gid, 10)) + cfg.ExcludeUIDs = append(cfg.ExcludeUIDs, strconv.FormatInt(uid, 10)) } // Set the proxy's inbound port. From 5d715de91f8754e24edd095dc6a25fa6bac2745c Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Fri, 5 Jul 2024 17:33:01 -0400 Subject: [PATCH 28/38] Skip dataplane + init containers when building list of application userIDs to skip over --- control-plane/connect-inject/common/openshift.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index ca74bbb1aa..4174a65b62 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -20,10 +20,11 @@ import ( "strconv" "strings" - "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" "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" ) func GetDataplaneUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { @@ -87,6 +88,14 @@ func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName } } for _, c := range pod.Spec.Containers { + if strings.HasPrefix(c.Name, "consul-dataplane") { + continue + } + + if strings.HasPrefix(c.Name, "consul-connect-inject-init") { + continue + } + if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil { appUIDs = append(appUIDs, *c.SecurityContext.RunAsUser) } From fa695f7267b097c18e5f2114c5a8709306a56ec1 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Fri, 5 Jul 2024 17:38:18 -0400 Subject: [PATCH 29/38] Remove unused code + unnecessary function export --- .../connect-inject/common/openshift.go | 31 ++----------------- 1 file changed, 2 insertions(+), 29 deletions(-) diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index 4174a65b62..406b8e4483 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -1,10 +1,6 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -// Function copied from: -// https://github.com/openshift/apiserver-library-go/blob/release-4.17/pkg/securitycontextconstraints/sccmatching/matcher.go -// Apache 2.0 license: https://github.com/openshift/apiserver-library-go/blob/release-4.17/LICENSE - // 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 @@ -102,7 +98,7 @@ func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName } // Collect the list of valid IDs from the namespace annotation - validUIDs, err := GetAllValidIDsFromNamespace(namespace.Annotations[annotationName]) + validUIDs, err := getIDsInRange(namespace.Annotations[annotationName]) if err != nil { return nil, fmt.Errorf("unable to get valid userIDs from namespace annotation: %w", err) } @@ -123,30 +119,7 @@ func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName return keys, nil } -type idSelector func(values []int64) (int64, error) - -var SelectFirstInRange idSelector = func(values []int64) (int64, error) { - if len(values) < 1 { - return 0, fmt.Errorf("range must have at least 1 value") - } - return values[0], nil -} - -var SelectSidecarID idSelector = func(values []int64) (int64, error) { - if len(values) < 2 { - return 0, fmt.Errorf("range must have at least 2 values") - } - return values[len(values)-2], nil -} - -var SelectInitContainerID idSelector = func(values []int64) (int64, error) { - if len(values) < 1 { - return 0, fmt.Errorf("range must have at least 1 value") - } - return values[len(values)-1], nil -} - -func GetAllValidIDsFromNamespace(annotation string) ([]int64, error) { +func getIDsInRange(annotation string) ([]int64, error) { parts := strings.Split(annotation, "/") if len(parts) != 2 { return nil, fmt.Errorf("invalid range format: %s", annotation) From 28a40fadc8aefe113e5625f1e1a8d75d9aa7b4f9 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Mon, 8 Jul 2024 11:19:51 -0400 Subject: [PATCH 30/38] Use correct function for API gateway initi container UID --- control-plane/api-gateway/gatekeeper/init.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go index 0c96796d13..cd0fb55042 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -179,27 +179,25 @@ func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace stri } uid := int64(initContainersUserAndGroupID) - group := 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) + 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 dataplane, we do not care about what is already defined on the pod + // 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.GetDataplaneUID(*ns, corev1.Pod{}) - + uid, err = ctrlCommon.GetConnectInitUID(*ns, corev1.Pod{}) if err != nil { return corev1.Container{}, err } - group, err = ctrlCommon.GetDataplaneGroupID(*ns, corev1.Pod{}) + + gid, err = ctrlCommon.GetDataplaneGroupID(*ns, corev1.Pod{}) if err != nil { return corev1.Container{}, err } @@ -207,7 +205,7 @@ func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace stri container.SecurityContext = &corev1.SecurityContext{ RunAsUser: pointer.Int64(uid), - RunAsGroup: pointer.Int64(group), + RunAsGroup: pointer.Int64(gid), RunAsNonRoot: pointer.Bool(true), Privileged: pointer.Bool(false), Capabilities: &corev1.Capabilities{ From dd166517d818bd050d45a0ac859b82501593902b Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Mon, 8 Jul 2024 11:42:42 -0400 Subject: [PATCH 31/38] Update gatekeeper tests for OpenShift --- .../api-gateway/gatekeeper/gatekeeper_test.go | 21 ++++++++++++++----- control-plane/api-gateway/gatekeeper/init.go | 2 +- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go index 7bcee54d8a..f6342ec725 100644 --- a/control-plane/api-gateway/gatekeeper/gatekeeper_test.go +++ b/control-plane/api-gateway/gatekeeper/gatekeeper_test.go @@ -20,7 +20,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" @@ -30,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" @@ -940,8 +946,8 @@ func TestUpsert(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "default", Annotations: map[string]string{ - constants.AnnotationOpenShiftUIDRange: "1000700000/100000", - constants.AnnotationOpenShiftGroups: "1000700000/100000", + constants.AnnotationOpenShiftUIDRange: designatedOpenShiftUIDRange, + constants.AnnotationOpenShiftGroups: designatedOpenShiftGIDRange, }, }, }, @@ -1481,9 +1487,14 @@ func validateResourcesExist(t *testing.T, client client.Client, helmConfig commo 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.Equal(t, container.SecurityContext.RunAsUser, pointer.Int64(1000700000)) - assert.Equal(t, container.SecurityContext.RunAsGroup, pointer.Int64(1000700000)) + 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) } } } diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go index cd0fb55042..4fec339126 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -197,7 +197,7 @@ func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace stri return corev1.Container{}, err } - gid, err = ctrlCommon.GetDataplaneGroupID(*ns, corev1.Pod{}) + gid, err = ctrlCommon.GetConnectInitGroupID(*ns, corev1.Pod{}) if err != nil { return corev1.Container{}, err } From f087c92077c254490f8e0d81a0558ee6ec0ee914 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Mon, 8 Jul 2024 12:06:54 -0400 Subject: [PATCH 32/38] Exclude init + dataplane containers based on image instead of name prefix --- control-plane/api-gateway/gatekeeper/init.go | 4 +- .../connect-inject/common/openshift.go | 41 ++++++++++++------- .../webhook/consul_dataplane_sidecar.go | 4 +- .../connect-inject/webhook/container_init.go | 4 +- .../webhook/redirect_traffic.go | 4 +- 5 files changed, 35 insertions(+), 22 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/init.go b/control-plane/api-gateway/gatekeeper/init.go index 4fec339126..875e15dff3 100644 --- a/control-plane/api-gateway/gatekeeper/init.go +++ b/control-plane/api-gateway/gatekeeper/init.go @@ -192,12 +192,12 @@ func (g Gatekeeper) initContainer(config common.HelmConfig, name, namespace stri // 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{}) + 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{}) + gid, err = ctrlCommon.GetConnectInitGroupID(*ns, corev1.Pod{}, config.ImageDataplane, config.ImageConsulK8S) if err != nil { return corev1.Container{}, err } diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index 406b8e4483..114e82893a 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -23,8 +23,11 @@ import ( "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" ) -func GetDataplaneUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { - availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftUIDRange) +// 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 } @@ -36,8 +39,11 @@ func GetDataplaneUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) return availableUIDs[len(availableUIDs)-2], nil } -func GetDataplaneGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { - availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftGroups) +// 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 } @@ -49,8 +55,11 @@ func GetDataplaneGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, err return availableUIDs[len(availableUIDs)-2], nil } -func GetConnectInitUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { - availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftUIDRange) +// 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 } @@ -62,8 +71,11 @@ func GetConnectInitUID(namespace corev1.Namespace, pod corev1.Pod) (int64, error return availableUIDs[len(availableUIDs)-1], nil } -func GetConnectInitGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, error) { - availableUIDs, err := getAvailableIDs(namespace, pod, constants.AnnotationOpenShiftGroups) +// 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 } @@ -75,7 +87,10 @@ func GetConnectInitGroupID(namespace corev1.Namespace, pod corev1.Pod) (int64, e return availableUIDs[len(availableUIDs)-1], nil } -func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName string) ([]int64, error) { +// 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 { @@ -84,11 +99,7 @@ func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName } } for _, c := range pod.Spec.Containers { - if strings.HasPrefix(c.Name, "consul-dataplane") { - continue - } - - if strings.HasPrefix(c.Name, "consul-connect-inject-init") { + if c.Image == dataplaneImage || c.Image == k8sImage { continue } @@ -119,6 +130,8 @@ func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName 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) { parts := strings.Split(annotation, "/") if len(parts) != 2 { diff --git a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go index 3ceacb6025..c30b672093 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar.go @@ -244,11 +244,11 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor // 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) + uid, err = common.GetDataplaneUID(namespace, pod, w.ImageConsulDataplane, w.ImageConsulK8S) if err != nil { return corev1.Container{}, err } - group, err = common.GetDataplaneGroupID(namespace, pod) + group, err = common.GetDataplaneGroupID(namespace, pod, w.ImageConsulDataplane, w.ImageConsulK8S) if err != nil { return corev1.Container{}, err } diff --git a/control-plane/connect-inject/webhook/container_init.go b/control-plane/connect-inject/webhook/container_init.go index c024f33cd1..6ba4ca35a5 100644 --- a/control-plane/connect-inject/webhook/container_init.go +++ b/control-plane/connect-inject/webhook/container_init.go @@ -242,12 +242,12 @@ func (w *MeshWebhook) containerInit(namespace corev1.Namespace, pod corev1.Pod, if w.EnableOpenShift { var err error - uid, err = common.GetConnectInitUID(namespace, pod) + uid, err = common.GetConnectInitUID(namespace, pod, w.ImageConsulDataplane, w.ImageConsulK8S) if err != nil { return corev1.Container{}, err } - group, err = common.GetConnectInitGroupID(namespace, pod) + group, err = common.GetConnectInitGroupID(namespace, pod, w.ImageConsulDataplane, w.ImageConsulK8S) if err != nil { return corev1.Container{}, err } diff --git a/control-plane/connect-inject/webhook/redirect_traffic.go b/control-plane/connect-inject/webhook/redirect_traffic.go index b62c2b614c..c8dc533adb 100644 --- a/control-plane/connect-inject/webhook/redirect_traffic.go +++ b/control-plane/connect-inject/webhook/redirect_traffic.go @@ -36,14 +36,14 @@ func (w *MeshWebhook) iptablesConfigJSON(pod corev1.Pod, ns corev1.Namespace) (s 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) + 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) + uid, err = common.GetConnectInitUID(ns, pod, w.ImageConsulDataplane, w.ImageConsulK8S) if err != nil { return "", err } From 92bcb82a4b0c0b9eb0982b361d42d7e77e1d8c0a Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Mon, 8 Jul 2024 12:44:07 -0400 Subject: [PATCH 33/38] Updates Openshift and tests to test for dashes in ranges, and comma separated list for groups --- .../connect-inject/common/openshift.go | 35 +- .../connect-inject/common/openshift_test.go | 489 ++++++++++++------ 2 files changed, 359 insertions(+), 165 deletions(-) diff --git a/control-plane/connect-inject/common/openshift.go b/control-plane/connect-inject/common/openshift.go index 114e82893a..b9de4c45f4 100644 --- a/control-plane/connect-inject/common/openshift.go +++ b/control-plane/connect-inject/common/openshift.go @@ -108,15 +108,36 @@ func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName, } } + 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 - validUIDs, err := getIDsInRange(namespace.Annotations[annotationName]) - if err != nil { - return nil, fmt.Errorf("unable to get valid userIDs from namespace annotation: %w", err) + 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 validUIDs { + for _, uid := range validIDs { availableUIDs[uid] = struct{}{} } for _, uid := range appUIDs { @@ -133,9 +154,13 @@ func getAvailableIDs(namespace corev1.Namespace, pod corev1.Pod, annotationName, // 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 { - return nil, fmt.Errorf("invalid range format: %s", annotation) + parts = strings.Split(annotation, "-") + if len(parts) != 2 { + return nil, fmt.Errorf("invalid range format: %s", annotation) + } } start, err := strconv.Atoi(parts[0]) diff --git a/control-plane/connect-inject/common/openshift_test.go b/control-plane/connect-inject/common/openshift_test.go index dbf9266028..a83f64fb6f 100644 --- a/control-plane/connect-inject/common/openshift_test.go +++ b/control-plane/connect-inject/common/openshift_test.go @@ -6,118 +6,231 @@ 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" - - "github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants" + "k8s.io/utils/pointer" ) // TODO: Melisa add a test for application taking last UID/GroupID in the range (also second to last) -// TODO: Melisa also add similar to gateway -func TestOpenShiftUID(t *testing.T) { +//func TestGetDatplaneUID(t *testing.T) { +// cases := []struct { +// Name string +// Namespace func() *corev1.Namespace +// Pod func() *corev1.Pod +// Expected int64 +// Err string +// }{ +// { +// Name: "Valid uid annotation with slash", +// Namespace: func() *corev1.Namespace { +// ns := &corev1.Namespace{ +// ObjectMeta: metav1.ObjectMeta{ +// Name: "default", +// Namespace: "default", +// Annotations: map[string]string{ +// constants.AnnotationOpenShiftUIDRange: "1000700000/100000", +// }, +// }, +// } +// pod := &corev1.Pod{ +// ObjectMeta: metav1.ObjectMeta{ +// Name: "pod", +// } +// } +// return ns +// }, +// Expected: 1000700000, +// Err: "", +// }, +// { +// Name: "Valid uid annotation with dash", +// Namespace: func() *corev1.Namespace { +// ns := &corev1.Namespace{ +// ObjectMeta: metav1.ObjectMeta{ +// Name: "default", +// Namespace: "default", +// Annotations: map[string]string{ +// constants.AnnotationOpenShiftUIDRange: "1234-1000", +// }, +// }, +// } +// return ns +// }, +// Expected: 1234, +// Err: "", +// }, +// { +// Name: "Invalid uid annotation missing slash or dash", +// Namespace: func() *corev1.Namespace { +// ns := &corev1.Namespace{ +// ObjectMeta: metav1.ObjectMeta{ +// Name: "default", +// Namespace: "default", +// Annotations: map[string]string{ +// // annotation should have a slash '/' or dash '-' +// constants.AnnotationOpenShiftUIDRange: "5678", +// }, +// }, +// } +// return ns +// }, +// Expected: 0, +// Err: fmt.Sprintf( +// "annotation %s contains an invalid format for value %s", +// constants.AnnotationOpenShiftUIDRange, +// "5678", +// ), +// }, +// { +// Name: "Missing uid annotation", +// Namespace: func() *corev1.Namespace { +// ns := &corev1.Namespace{ +// ObjectMeta: metav1.ObjectMeta{ +// Name: "default", +// Namespace: "default", +// }, +// } +// return ns +// }, +// Expected: 0, +// Err: fmt.Sprintf("unable to find annotation %s", constants.AnnotationOpenShiftUIDRange), +// }, +// { +// Name: "Empty", +// Namespace: func() *corev1.Namespace { +// ns := &corev1.Namespace{ +// ObjectMeta: metav1.ObjectMeta{ +// Name: "default", +// Namespace: "default", +// Annotations: map[string]string{ +// constants.AnnotationOpenShiftUIDRange: "", +// }, +// }, +// } +// return ns +// }, +// Expected: 0, +// Err: "found annotation openshift.io/sa.scc.uid-range but it was empty", +// }, +// } +// for _, tt := range cases { +// t.Run(tt.Name, func(t *testing.T) { +// require := require.New(t) +// actual, err := GetDataplaneUID(tt.Namespace(), tt.Pod) +// if tt.Err == "" { +// require.NoError(err) +// require.Equal(tt.Expected, actual) +// } else { +// require.EqualError(err, tt.Err) +// } +// }) +// } +//} + +func TestGetDataplaneIDs(t *testing.T) { cases := []struct { Name string - Namespace func() *corev1.Namespace - Expected int64 - Err 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: "Valid uid annotation with slash", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", - Annotations: map[string]string{ - constants.AnnotationOpenShiftUIDRange: "1000700000/100000", - }, + 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", }, - } - return ns + }, }, - Expected: 1000700000, - Err: "", - }, - { - Name: "Valid uid annotation with dash", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", - Annotations: map[string]string{ - constants.AnnotationOpenShiftUIDRange: "1234-1000", + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", }, }, - } - return ns - }, - Expected: 1234, - Err: "", - }, - { - Name: "Invalid uid annotation missing slash or dash", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", - Annotations: map[string]string{ - // annotation should have a slash '/' or dash '-' - constants.AnnotationOpenShiftUIDRange: "5678", + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(100), + }, }, }, - } - return ns + }, }, - Expected: 0, - Err: fmt.Sprintf( - "annotation %s contains an invalid format for value %s", - constants.AnnotationOpenShiftUIDRange, - "5678", - ), + ExpectedDataplaneUserAndGroupIDs: 103, + Err: "", }, { - Name: "Missing uid annotation", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", + 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", }, - } - return ns + }, }, - Expected: 0, - Err: fmt.Sprintf("unable to find annotation %s", constants.AnnotationOpenShiftUIDRange), - }, - { - Name: "Empty", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", - Annotations: map[string]string{ - constants.AnnotationOpenShiftUIDRange: "", + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", }, }, - } - return ns + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(100), + }, + }, + }, + }, }, - Expected: 0, - Err: "found annotation openshift.io/sa.scc.uid-range but it was empty", + Err: "namespace does not have enough available UIDs", }, } for _, tt := range cases { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) - actual, err := GetOpenShiftUID(tt.Namespace(), SelectFirstInRange) + // Test UID + actualUIDs, err := GetDataplaneUID(tt.Namespace, tt.Pod) if tt.Err == "" { require.NoError(err) - require.Equal(tt.Expected, actual) + require.Equal(tt.ExpectedDataplaneUserAndGroupIDs, actualUIDs) + } else { + require.EqualError(err, tt.Err) + } + // Test GroupID + actualGroupIDs, err := GetDataplaneGroupID(tt.Namespace, tt.Pod) + if tt.Err == "" { + require.NoError(err) + require.Equal(tt.ExpectedDataplaneUserAndGroupIDs, actualGroupIDs) } else { require.EqualError(err, tt.Err) } @@ -125,113 +238,169 @@ func TestOpenShiftUID(t *testing.T) { } } -func TestOpenShiftGroup(t *testing.T) { +func TestGetAvailableIDs(t *testing.T) { cases := []struct { - Name string - Namespace func() *corev1.Namespace - Expected int64 - Err string + Name string + Namespace corev1.Namespace + ExpectedAvailableUserIDs []int64 + ExpectedAvailableGroupIDs []int64 + Pod corev1.Pod + Err string }{ { - Name: "Valid group annotation with slash", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", - Annotations: map[string]string{ - constants.AnnotationOpenShiftGroups: "123456789/1000", - }, + 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", }, - } - return ns + }, }, - Expected: 123456789, - Err: "", - }, - { - Name: "Valid group annotation with comma", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", - Annotations: map[string]string{ - constants.AnnotationOpenShiftGroups: "1234,1000", + Pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "consul-connect-inject-init", }, }, - } - return ns + Containers: []corev1.Container{ + { + Name: "consul-dataplane", + }, + { + Name: "app", + SecurityContext: &corev1.SecurityContext{ + RunAsUser: pointer.Int64(100), + }, + }, + }, + }, }, - Expected: 1234, - Err: "", + ExpectedAvailableUserIDs: []int64{101, 102, 103, 104}, + ExpectedAvailableGroupIDs: []int64{101, 102, 103, 104}, + Err: "", }, { - Name: "Invalid group annotation missing slash or comma", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", - Annotations: map[string]string{ - // annotation should have a slash '/' or comma ',' - constants.AnnotationOpenShiftGroups: "5678", - }, + Name: "Bad annotation format", + Namespace: corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "default", + Annotations: map[string]string{ + constants.AnnotationOpenShiftUIDRange: "100:5", }, - } - return ns + }, }, - Expected: 0, - Err: fmt.Sprintf( - "annotation %s contains an invalid format for value %s", - constants.AnnotationOpenShiftGroups, - "5678", - ), + Pod: corev1.Pod{}, + ExpectedAvailableUserIDs: nil, + Err: "unable to get valid userIDs from namespace annotation: invalid range format: 100:5", }, { - Name: "Missing group annotation, fall back to UID annotation", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", - Annotations: map[string]string{ - // annotation should have a slash '/' or comma ',' - constants.AnnotationOpenShiftUIDRange: "9012/1000", - }, + 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", }, - } - return ns + }, }, - Expected: 9012, - Err: "", + Pod: corev1.Pod{}, + ExpectedAvailableUserIDs: []int64{100, 101, 102, 103, 104}, + ExpectedAvailableGroupIDs: []int64{100, 101, 102, 103, 104, 200, 201, 202, 203, 204}, + Err: "", }, { - Name: "Missing both group and fallback uid annotation", - Namespace: func() *corev1.Namespace { - ns := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - Namespace: "default", + 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", }, - } - return ns + }, }, - Expected: 0, - Err: fmt.Sprintf( - "unable to find annotation %s or %s", - constants.AnnotationOpenShiftGroups, - constants.AnnotationOpenShiftUIDRange, - ), + 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) + 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) + 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 := GetOpenShiftGroup(tt.Namespace(), SelectFirstInRange) + actual, err := getIDsInRange(tt.Annotation) if tt.Err == "" { require.NoError(err) - require.Equal(tt.Expected, actual) + 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) } From 6a995150122e4eaebe7037b5e4d319b8dff93fcf Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Mon, 8 Jul 2024 13:00:50 -0400 Subject: [PATCH 34/38] Updated some tests --- .../connect-inject/common/openshift_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/control-plane/connect-inject/common/openshift_test.go b/control-plane/connect-inject/common/openshift_test.go index a83f64fb6f..a82d717256 100644 --- a/control-plane/connect-inject/common/openshift_test.go +++ b/control-plane/connect-inject/common/openshift_test.go @@ -131,6 +131,8 @@ import ( //} func TestGetDataplaneIDs(t *testing.T) { + dataplaneImage := "consul-dataplane" + k8sImage := "consul-k8s-control-plane" cases := []struct { Name string Namespace corev1.Namespace @@ -219,7 +221,7 @@ func TestGetDataplaneIDs(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { require := require.New(t) // Test UID - actualUIDs, err := GetDataplaneUID(tt.Namespace, tt.Pod) + actualUIDs, err := GetDataplaneUID(tt.Namespace, tt.Pod, dataplaneImage, k8sImage) if tt.Err == "" { require.NoError(err) require.Equal(tt.ExpectedDataplaneUserAndGroupIDs, actualUIDs) @@ -227,7 +229,7 @@ func TestGetDataplaneIDs(t *testing.T) { require.EqualError(err, tt.Err) } // Test GroupID - actualGroupIDs, err := GetDataplaneGroupID(tt.Namespace, tt.Pod) + actualGroupIDs, err := GetDataplaneGroupID(tt.Namespace, tt.Pod, dataplaneImage, k8sImage) if tt.Err == "" { require.NoError(err) require.Equal(tt.ExpectedDataplaneUserAndGroupIDs, actualGroupIDs) @@ -239,6 +241,8 @@ func TestGetDataplaneIDs(t *testing.T) { } func TestGetAvailableIDs(t *testing.T) { + dataplaneImage := "consul-dataplane" + k8sImage := "consul-k8s-control-plane" cases := []struct { Name string Namespace corev1.Namespace @@ -338,14 +342,14 @@ func TestGetAvailableIDs(t *testing.T) { 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) + 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) + actualGroupIDs, err := getAvailableIDs(tt.Namespace, tt.Pod, constants.AnnotationOpenShiftGroups, dataplaneImage, k8sImage) if tt.Err == "" { require.NoError(err) require.Equal(tt.ExpectedAvailableGroupIDs, actualGroupIDs) From a1496617803dfe9f00e4f70338dae27a61988005 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Mon, 8 Jul 2024 14:12:21 -0400 Subject: [PATCH 35/38] Updated some tests --- .../connect-inject/common/openshift_test.go | 301 +++++++++++------- 1 file changed, 185 insertions(+), 116 deletions(-) diff --git a/control-plane/connect-inject/common/openshift_test.go b/control-plane/connect-inject/common/openshift_test.go index a82d717256..dd94519b0d 100644 --- a/control-plane/connect-inject/common/openshift_test.go +++ b/control-plane/connect-inject/common/openshift_test.go @@ -13,122 +13,153 @@ import ( "k8s.io/utils/pointer" ) -// TODO: Melisa add a test for application taking last UID/GroupID in the range (also second to last) - -//func TestGetDatplaneUID(t *testing.T) { -// cases := []struct { -// Name string -// Namespace func() *corev1.Namespace -// Pod func() *corev1.Pod -// Expected int64 -// Err string -// }{ -// { -// Name: "Valid uid annotation with slash", -// Namespace: func() *corev1.Namespace { -// ns := &corev1.Namespace{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "default", -// Namespace: "default", -// Annotations: map[string]string{ -// constants.AnnotationOpenShiftUIDRange: "1000700000/100000", -// }, -// }, -// } -// pod := &corev1.Pod{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "pod", -// } -// } -// return ns -// }, -// Expected: 1000700000, -// Err: "", -// }, -// { -// Name: "Valid uid annotation with dash", -// Namespace: func() *corev1.Namespace { -// ns := &corev1.Namespace{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "default", -// Namespace: "default", -// Annotations: map[string]string{ -// constants.AnnotationOpenShiftUIDRange: "1234-1000", -// }, -// }, -// } -// return ns -// }, -// Expected: 1234, -// Err: "", -// }, -// { -// Name: "Invalid uid annotation missing slash or dash", -// Namespace: func() *corev1.Namespace { -// ns := &corev1.Namespace{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "default", -// Namespace: "default", -// Annotations: map[string]string{ -// // annotation should have a slash '/' or dash '-' -// constants.AnnotationOpenShiftUIDRange: "5678", -// }, -// }, -// } -// return ns -// }, -// Expected: 0, -// Err: fmt.Sprintf( -// "annotation %s contains an invalid format for value %s", -// constants.AnnotationOpenShiftUIDRange, -// "5678", -// ), -// }, -// { -// Name: "Missing uid annotation", -// Namespace: func() *corev1.Namespace { -// ns := &corev1.Namespace{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "default", -// Namespace: "default", -// }, -// } -// return ns -// }, -// Expected: 0, -// Err: fmt.Sprintf("unable to find annotation %s", constants.AnnotationOpenShiftUIDRange), -// }, -// { -// Name: "Empty", -// Namespace: func() *corev1.Namespace { -// ns := &corev1.Namespace{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: "default", -// Namespace: "default", -// Annotations: map[string]string{ -// constants.AnnotationOpenShiftUIDRange: "", -// }, -// }, -// } -// return ns -// }, -// Expected: 0, -// Err: "found annotation openshift.io/sa.scc.uid-range but it was empty", -// }, -// } -// for _, tt := range cases { -// t.Run(tt.Name, func(t *testing.T) { -// require := require.New(t) -// actual, err := GetDataplaneUID(tt.Namespace(), tt.Pod) -// if tt.Err == "" { -// require.NoError(err) -// require.Equal(tt.Expected, actual) -// } else { -// require.EqualError(err, tt.Err) -// } -// }) -// } -//} +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" @@ -179,6 +210,44 @@ func TestGetDataplaneIDs(t *testing.T) { 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{ From d2f1b265da4b561976a4920ed07bf50c2b7d45b5 Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Mon, 8 Jul 2024 14:32:27 -0400 Subject: [PATCH 36/38] Updated sidecar tests --- .../webhook/consul_dataplane_sidecar_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 4bbf24bc45..b217ab1995 100644 --- a/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go +++ b/control-plane/connect-inject/webhook/consul_dataplane_sidecar_test.go @@ -829,8 +829,8 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { tproxyEnabled: false, openShiftEnabled: true, expSecurityContext: &corev1.SecurityContext{ - RunAsUser: pointer.Int64(1000700000), - RunAsGroup: pointer.Int64(1000700000), + RunAsUser: pointer.Int64(1000799998), + RunAsGroup: pointer.Int64(1000799998), RunAsNonRoot: pointer.Bool(true), ReadOnlyRootFilesystem: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), @@ -843,8 +843,8 @@ func TestHandlerConsulDataplaneSidecar_withSecurityContext(t *testing.T) { tproxyEnabled: true, openShiftEnabled: true, expSecurityContext: &corev1.SecurityContext{ - RunAsUser: pointer.Int64(1000700000), - RunAsGroup: pointer.Int64(1000700000), + RunAsUser: pointer.Int64(1000799998), + RunAsGroup: pointer.Int64(1000799998), RunAsNonRoot: pointer.Bool(true), ReadOnlyRootFilesystem: pointer.Bool(true), AllowPrivilegeEscalation: pointer.Bool(false), From 4cf176a24031dba02ae1accca58c4462d08b7fbb Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Mon, 8 Jul 2024 15:11:29 -0400 Subject: [PATCH 37/38] Updated init container test and redirect traffic test for webhook --- control-plane/connect-inject/webhook/container_init_test.go | 4 ++-- control-plane/connect-inject/webhook/redirect_traffic_test.go | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/control-plane/connect-inject/webhook/container_init_test.go b/control-plane/connect-inject/webhook/container_init_test.go index 5896c0c0eb..66e44ba09b 100644 --- a/control-plane/connect-inject/webhook/container_init_test.go +++ b/control-plane/connect-inject/webhook/container_init_test.go @@ -318,8 +318,8 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { } else if c.cniEnabled && c.openShiftEnabled { // When cni + openShift expectedSecurityContext = &corev1.SecurityContext{ - RunAsUser: pointer.Int64(1000700000), - RunAsGroup: pointer.Int64(1000700000), + RunAsUser: pointer.Int64(1000799999), + RunAsGroup: pointer.Int64(1000799999), RunAsNonRoot: pointer.Bool(true), Privileged: pointer.Bool(privileged), Capabilities: &corev1.Capabilities{ 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()) } From 636a8dd300c3be5b9f0dd65b15e222a5364e50bd Mon Sep 17 00:00:00 2001 From: Melisa Griffin Date: Tue, 9 Jul 2024 13:55:41 -0400 Subject: [PATCH 38/38] Un-bump Go version --- control-plane/go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/go.mod b/control-plane/go.mod index 36a4db2f70..33470a6cb5 100644 --- a/control-plane/go.mod +++ b/control-plane/go.mod @@ -164,4 +164,4 @@ require ( sigs.k8s.io/yaml v1.3.0 // indirect ) -go 1.21 +go 1.20