diff --git a/internal/controller/nginx/agent/grpc/interceptor/interceptor.go b/internal/controller/nginx/agent/grpc/interceptor/interceptor.go index 0a32234400..7732adbe6e 100644 --- a/internal/controller/nginx/agent/grpc/interceptor/interceptor.go +++ b/internal/controller/nginx/agent/grpc/interceptor/interceptor.go @@ -16,6 +16,7 @@ import ( authv1 "k8s.io/api/authentication/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/labels" "sigs.k8s.io/controller-runtime/pkg/client" grpcContext "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/agent/grpc/context" @@ -160,35 +161,25 @@ func (c ContextSetter) validateToken(ctx context.Context, gi *grpcContext.GrpcIn var podList corev1.PodList opts := &client.ListOptions{ FieldSelector: fields.SelectorFromSet(fields.Set{"status.podIP": gi.IPAddress}), + Namespace: usernameItems[2], + LabelSelector: labels.Set(map[string]string{ + controller.AppNameLabel: usernameItems[3], + }).AsSelector(), } if err := c.k8sClient.List(getCtx, &podList, opts); err != nil { return nil, status.Error(codes.Internal, fmt.Sprintf("error listing pods: %s", err.Error())) } - if len(podList.Items) != 1 { - msg := fmt.Sprintf("expected one Pod to have IP address %s, found %d", gi.IPAddress, len(podList.Items)) - return nil, status.Error(codes.Internal, msg) - } - - podNS := podList.Items[0].GetNamespace() - if podNS != usernameItems[2] { - msg := fmt.Sprintf( - "token user namespace %q does not match namespace of requesting pod %q", usernameItems[2], podNS, - ) - return nil, status.Error(codes.Unauthenticated, msg) - } - - scName, ok := podList.Items[0].GetLabels()[controller.AppNameLabel] - if !ok { - msg := fmt.Sprintf("could not get app name from %q label; unable to authenticate token", controller.AppNameLabel) - return nil, status.Error(codes.Unauthenticated, msg) + runningCount := 0 + for _, pod := range podList.Items { + if pod.Status.Phase == corev1.PodRunning { + runningCount++ + } } - if scName != usernameItems[3] { - msg := fmt.Sprintf( - "token user name %q does not match service account name of requesting pod %q", usernameItems[3], scName, - ) + if runningCount != 1 { + msg := fmt.Sprintf("expected a single Running pod with IP address %q, but found %d", gi.IPAddress, runningCount) return nil, status.Error(codes.Unauthenticated, msg) } diff --git a/internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go b/internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go index d334c9a465..c31a56c241 100644 --- a/internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go +++ b/internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go @@ -17,7 +17,9 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + grpcContext "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/agent/grpc/context" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller" ) @@ -67,6 +69,7 @@ func (m *mockClient) List(_ context.Context, obj client.ObjectList, _ ...client. Namespace: m.podNamespace, Labels: labels, }, + Status: corev1.PodStatus{Phase: corev1.PodRunning}, }, } @@ -204,38 +207,6 @@ func TestInterceptor(t *testing.T) { expErrCode: codes.Unauthenticated, expErrMsg: "must be of the format", }, - { - name: "mismatched namespace in username", - md: validMetadata, - peer: validPeerData, - username: "system:serviceaccount:invalid:gateway-nginx", - appName: "gateway-nginx", - podNamespace: "default", - authenticated: true, - expErrCode: codes.Unauthenticated, - expErrMsg: "does not match namespace", - }, - { - name: "mismatched name in username", - md: validMetadata, - peer: validPeerData, - username: "system:serviceaccount:default:invalid", - appName: "gateway-nginx", - podNamespace: "default", - authenticated: true, - expErrCode: codes.Unauthenticated, - expErrMsg: "does not match service account name", - }, - { - name: "missing app name label", - md: validMetadata, - peer: validPeerData, - username: "system:serviceaccount:default:gateway-nginx", - podNamespace: "default", - authenticated: true, - expErrCode: codes.Unauthenticated, - expErrMsg: "could not get app name", - }, } streamHandler := func(_ any, _ grpc.ServerStream) error { @@ -261,7 +232,7 @@ func TestInterceptor(t *testing.T) { } cs := NewContextSetter(mockK8sClient, "ngf-audience") - ctx := context.Background() + ctx := t.Context() if test.md != nil { peerCtx := context.Background() if test.peer != nil { @@ -290,3 +261,147 @@ func TestInterceptor(t *testing.T) { }) } } + +type patchClient struct { + client.Client +} + +func (p *patchClient) Create(_ context.Context, obj client.Object, _ ...client.CreateOption) error { + tr, ok := obj.(*authv1.TokenReview) + if ok { + tr.Status.Authenticated = true + tr.Status.User.Username = "system:serviceaccount:default:gateway-nginx" + } + return nil +} + +func TestValidateToken_PodListOptions(t *testing.T) { + t.Parallel() + + testCases := []struct { + pod *corev1.Pod + gi *grpcContext.GrpcInfo + name string + shouldErr bool + }{ + { + name: "all match", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-pod", + Namespace: "default", + Labels: map[string]string{ + controller.AppNameLabel: "gateway-nginx", + }, + }, + Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning}, + }, + gi: &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"}, + shouldErr: false, + }, + { + name: "ip matches, namespace does not", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-pod", + Namespace: "other-namespace", + Labels: map[string]string{ + controller.AppNameLabel: "gateway-nginx", + }, + }, + Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning}, + }, + gi: &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"}, + shouldErr: true, + }, + { + name: "ip matches, label value does not match", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-pod", + Namespace: "default", + Labels: map[string]string{ + controller.AppNameLabel: "not-gateway-nginx", + }, + }, + Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning}, + }, + gi: &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"}, + shouldErr: true, + }, + { + name: "ip matches, label does not exist", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-pod", + Namespace: "default", + Labels: map[string]string{}, + }, + Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning}, + }, + gi: &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"}, + shouldErr: true, + }, + { + name: "ip does not match", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-pod", + Namespace: "default", + Labels: map[string]string{ + controller.AppNameLabel: "gateway-nginx", + }, + }, + Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning}, + }, + gi: &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "9.9.9.9"}, + shouldErr: true, + }, + { + name: "all match but pod not running", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-pod", + Namespace: "default", + Labels: map[string]string{ + controller.AppNameLabel: "gateway-nginx", + }, + }, + Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodPending}, + }, + gi: &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"}, + shouldErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + fakeClient := fake.NewClientBuilder(). + WithObjects(tc.pod). + WithIndex(&corev1.Pod{}, "status.podIP", func(obj client.Object) []string { + pod, ok := obj.(*corev1.Pod) + g.Expect(ok).To(BeTrue()) + if pod.Status.PodIP != "" { + return []string{pod.Status.PodIP} + } + return nil + }). + Build() + + patchedClient := &patchClient{fakeClient} + csPatched := NewContextSetter(patchedClient, "ngf-audience") + + resultCtx, err := csPatched.validateToken(t.Context(), tc.gi) + if tc.shouldErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("expected a single Running pod")) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(resultCtx).ToNot(BeNil()) + } + }) + } +}