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) }