Skip to content

Commit e8ee7c1

Browse files
sjbermansjberman
authored
Fix hostNetwork data plane pod connection issue (#4481)
Problem: When hostNetwork was enabled, the data plane pod would connect and give us its hostname, which was the name of the node instead of the pod. This would cause issues internally with both tracking and acquiring the pod's owner so we know which config to send to it. Solution: Add the owner's name and type via labels in the agent config. These labels are then sent to the control plane when an agent connects, and can be used to determine which configuration to send. Updated all trackin to use the UUID instead of pod name.
1 parent e8f29de commit e8ee7c1

File tree

12 files changed

+259
-436
lines changed

12 files changed

+259
-436
lines changed

‎internal/controller/nginx/agent/command.go‎

Lines changed: 126 additions & 122 deletions
Large diffs are not rendered by default.

‎internal/controller/nginx/agent/command_test.go‎

Lines changed: 58 additions & 260 deletions
Large diffs are not rendered by default.

‎internal/controller/nginx/agent/file.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func (fs *fileService) GetFile(
5656
ctx context.Context,
5757
req *pb.GetFileRequest,
5858
) (*pb.GetFileResponse, error) {
59-
gi, ok := grpcContext.FromContext(ctx)
59+
grpcInfo, ok := grpcContext.FromContext(ctx)
6060
if !ok {
6161
return nil, agentgrpc.ErrStatusInvalidConnection
6262
}
@@ -65,7 +65,7 @@ func (fs *fileService) GetFile(
6565
return nil, status.Error(codes.InvalidArgument, "invalid request")
6666
}
6767

68-
contents, err := fs.getFileContents(req, gi.UUID)
68+
contents, err := fs.getFileContents(req, grpcInfo.UUID)
6969
if err != nil {
7070
return nil, err
7171
}
@@ -84,7 +84,7 @@ func (fs *fileService) GetFileStream(
8484
req *pb.GetFileRequest,
8585
server grpc.ServerStreamingServer[pb.FileDataChunk],
8686
) error {
87-
gi, ok := grpcContext.FromContext(server.Context())
87+
grpcInfo, ok := grpcContext.FromContext(server.Context())
8888
if !ok {
8989
return agentgrpc.ErrStatusInvalidConnection
9090
}
@@ -93,7 +93,7 @@ func (fs *fileService) GetFileStream(
9393
return status.Error(codes.InvalidArgument, "invalid request")
9494
}
9595

96-
contents, err := fs.getFileContents(req, gi.UUID)
96+
contents, err := fs.getFileContents(req, grpcInfo.UUID)
9797
if err != nil {
9898
return err
9999
}
@@ -133,11 +133,11 @@ func (fs *fileService) GetFileStream(
133133

134134
func (fs *fileService) getFileContents(req *pb.GetFileRequest, connKey string) ([]byte, error) {
135135
conn := fs.connTracker.GetConnection(connKey)
136-
if conn.PodName == "" {
136+
if conn.InstanceID == "" {
137137
return nil, status.Errorf(codes.NotFound, "connection not found")
138138
}
139139

140-
deployment := fs.nginxDeployments.Get(conn.Parent)
140+
deployment := fs.nginxDeployments.Get(conn.ParentName)
141141
if deployment == nil {
142142
return nil, status.Errorf(codes.NotFound, "deployment not found in store")
143143
}
@@ -187,17 +187,17 @@ func (fs *fileService) UpdateOverview(
187187
ctx context.Context,
188188
req *pb.UpdateOverviewRequest,
189189
) (*pb.UpdateOverviewResponse, error) {
190-
gi, ok := grpcContext.FromContext(ctx)
190+
grpcInfo, ok := grpcContext.FromContext(ctx)
191191
if !ok {
192192
return &pb.UpdateOverviewResponse{}, agentgrpc.ErrStatusInvalidConnection
193193
}
194194

195-
conn := fs.connTracker.GetConnection(gi.UUID)
196-
if conn.PodName == "" {
195+
conn := fs.connTracker.GetConnection(grpcInfo.UUID)
196+
if conn.InstanceID == "" {
197197
return &pb.UpdateOverviewResponse{}, status.Errorf(codes.NotFound, "connection not found")
198198
}
199199

200-
deployment := fs.nginxDeployments.Get(conn.Parent)
200+
deployment := fs.nginxDeployments.Get(conn.ParentName)
201201
if deployment == nil {
202202
return &pb.UpdateOverviewResponse{}, status.Errorf(codes.NotFound, "deployment not found in store")
203203
}

‎internal/controller/nginx/agent/file_test.go‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,8 @@ func TestGetFile(t *testing.T) {
4343

4444
connTracker := &agentgrpcfakes.FakeConnectionsTracker{}
4545
conn := agentgrpc.Connection{
46-
PodName: "nginx-pod",
4746
InstanceID: "12345",
48-
Parent: deploymentName,
47+
ParentName: deploymentName,
4948
}
5049
connTracker.GetConnectionReturns(conn)
5150

@@ -109,9 +108,8 @@ func TestGetFile_InvalidRequest(t *testing.T) {
109108
deploymentName := types.NamespacedName{Name: "nginx-deployment", Namespace: "default"}
110109
connTracker := &agentgrpcfakes.FakeConnectionsTracker{}
111110
conn := agentgrpc.Connection{
112-
PodName: "nginx-pod",
113111
InstanceID: "12345",
114-
Parent: deploymentName,
112+
ParentName: deploymentName,
115113
}
116114
connTracker.GetConnectionReturns(conn)
117115

@@ -165,9 +163,8 @@ func TestGetFile_DeploymentNotFound(t *testing.T) {
165163

166164
connTracker := &agentgrpcfakes.FakeConnectionsTracker{}
167165
conn := agentgrpc.Connection{
168-
PodName: "nginx-pod",
169166
InstanceID: "12345",
170-
Parent: deploymentName,
167+
ParentName: deploymentName,
171168
}
172169
connTracker.GetConnectionReturns(conn)
173170

@@ -198,9 +195,8 @@ func TestGetFile_FileNotFound(t *testing.T) {
198195

199196
connTracker := &agentgrpcfakes.FakeConnectionsTracker{}
200197
conn := agentgrpc.Connection{
201-
PodName: "nginx-pod",
202198
InstanceID: "12345",
203-
Parent: deploymentName,
199+
ParentName: deploymentName,
204200
}
205201
connTracker.GetConnectionReturns(conn)
206202

@@ -234,9 +230,8 @@ func TestGetFileStream(t *testing.T) {
234230

235231
connTracker := &agentgrpcfakes.FakeConnectionsTracker{}
236232
conn := agentgrpc.Connection{
237-
PodName: "nginx-pod",
238233
InstanceID: "12345",
239-
Parent: deploymentName,
234+
ParentName: deploymentName,
240235
}
241236
connTracker.GetConnectionReturns(conn)
242237

@@ -312,9 +307,8 @@ func TestGetFileStream_InvalidRequest(t *testing.T) {
312307
deploymentName := types.NamespacedName{Name: "nginx-deployment", Namespace: "default"}
313308
connTracker := &agentgrpcfakes.FakeConnectionsTracker{}
314309
conn := agentgrpc.Connection{
315-
PodName: "nginx-pod",
316310
InstanceID: "12345",
317-
Parent: deploymentName,
311+
ParentName: deploymentName,
318312
}
319313
connTracker.GetConnectionReturns(conn)
320314

@@ -369,9 +363,8 @@ func TestUpdateOverview(t *testing.T) {
369363

370364
connTracker := &agentgrpcfakes.FakeConnectionsTracker{}
371365
conn := agentgrpc.Connection{
372-
PodName: "nginx-pod",
373366
InstanceID: "12345",
374-
Parent: deploymentName,
367+
ParentName: deploymentName,
375368
}
376369
connTracker.GetConnectionReturns(conn)
377370

@@ -504,9 +497,8 @@ func TestUpdateOverview_DeploymentNotFound(t *testing.T) {
504497

505498
connTracker := &agentgrpcfakes.FakeConnectionsTracker{}
506499
conn := agentgrpc.Connection{
507-
PodName: "nginx-pod",
508500
InstanceID: "12345",
509-
Parent: deploymentName,
501+
ParentName: deploymentName,
510502
}
511503
connTracker.GetConnectionReturns(conn)
512504

‎internal/controller/nginx/agent/grpc/connections.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ type ConnectionsTracker interface {
2121

2222
// Connection contains the data about a single nginx agent connection.
2323
type Connection struct {
24-
PodName string
2524
InstanceID string
26-
Parent types.NamespacedName
25+
ParentType string
26+
ParentName types.NamespacedName
2727
}
2828

2929
// Ready returns if the connection is ready to be used. In other words, agent

‎internal/controller/nginx/agent/grpc/connections_test.go‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,8 @@ func TestGetConnection(t *testing.T) {
1616
tracker := agentgrpc.NewConnectionsTracker()
1717

1818
conn := agentgrpc.Connection{
19-
PodName: "pod1",
2019
InstanceID: "instance1",
21-
Parent: types.NamespacedName{Namespace: "default", Name: "parent1"},
20+
ParentName: types.NamespacedName{Namespace: "default", Name: "parent1"},
2221
}
2322
tracker.Track("key1", conn)
2423

@@ -34,9 +33,8 @@ func TestConnectionIsReady(t *testing.T) {
3433
g := NewWithT(t)
3534

3635
conn := agentgrpc.Connection{
37-
PodName: "pod1",
3836
InstanceID: "instance1",
39-
Parent: types.NamespacedName{Namespace: "default", Name: "parent1"},
37+
ParentName: types.NamespacedName{Namespace: "default", Name: "parent1"},
4038
}
4139

4240
g.Expect(conn.Ready()).To(BeTrue())
@@ -47,8 +45,7 @@ func TestConnectionIsNotReady(t *testing.T) {
4745
g := NewWithT(t)
4846

4947
conn := agentgrpc.Connection{
50-
PodName: "pod1",
51-
Parent: types.NamespacedName{Namespace: "default", Name: "parent1"},
48+
ParentName: types.NamespacedName{Namespace: "default", Name: "parent1"},
5249
}
5350

5451
g.Expect(conn.Ready()).To(BeFalse())
@@ -60,8 +57,7 @@ func TestSetInstanceID(t *testing.T) {
6057

6158
tracker := agentgrpc.NewConnectionsTracker()
6259
conn := agentgrpc.Connection{
63-
PodName: "pod1",
64-
Parent: types.NamespacedName{Namespace: "default", Name: "parent1"},
60+
ParentName: types.NamespacedName{Namespace: "default", Name: "parent1"},
6561
}
6662
tracker.Track("key1", conn)
6763

@@ -81,9 +77,8 @@ func TestRemoveConnection(t *testing.T) {
8177

8278
tracker := agentgrpc.NewConnectionsTracker()
8379
conn := agentgrpc.Connection{
84-
PodName: "pod1",
8580
InstanceID: "instance1",
86-
Parent: types.NamespacedName{Namespace: "default", Name: "parent1"},
81+
ParentName: types.NamespacedName{Namespace: "default", Name: "parent1"},
8782
}
8883
tracker.Track("key1", conn)
8984

‎internal/controller/nginx/agent/grpc/interceptor/interceptor.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,12 @@ func (c ContextSetter) Unary(logger logr.Logger) grpc.UnaryServerInterceptor {
8484
// validateConnection checks that the connection is valid and returns a new
8585
// context containing information used by the gRPC command/file services.
8686
func (c ContextSetter) validateConnection(ctx context.Context) (context.Context, error) {
87-
gi, err := getGrpcInfo(ctx)
87+
grpcInfo, err := getGrpcInfo(ctx)
8888
if err != nil {
8989
return nil, err
9090
}
9191

92-
return c.validateToken(ctx, gi)
92+
return c.validateToken(ctx, grpcInfo)
9393
}
9494

9595
func getGrpcInfo(ctx context.Context) (*grpcContext.GrpcInfo, error) {
@@ -114,11 +114,11 @@ func getGrpcInfo(ctx context.Context) (*grpcContext.GrpcInfo, error) {
114114
}, nil
115115
}
116116

117-
func (c ContextSetter) validateToken(ctx context.Context, gi *grpcContext.GrpcInfo) (context.Context, error) {
117+
func (c ContextSetter) validateToken(ctx context.Context, grpcInfo *grpcContext.GrpcInfo) (context.Context, error) {
118118
tokenReview := &authv1.TokenReview{
119119
Spec: authv1.TokenReviewSpec{
120120
Audiences: []string{c.audience},
121-
Token: gi.Token,
121+
Token: grpcInfo.Token,
122122
},
123123
}
124124

@@ -169,5 +169,5 @@ func (c ContextSetter) validateToken(ctx context.Context, gi *grpcContext.GrpcIn
169169
return nil, status.Error(codes.Unauthenticated, msg)
170170
}
171171

172-
return grpcContext.NewGrpcContext(ctx, *gi), nil
172+
return grpcContext.NewGrpcContext(ctx, *grpcInfo), nil
173173
}

‎internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ func TestValidateToken_PodListOptions(t *testing.T) {
253253

254254
testCases := []struct {
255255
pod *corev1.Pod
256-
gi *grpcContext.GrpcInfo
256+
grpcInfo *grpcContext.GrpcInfo
257257
name string
258258
shouldErr bool
259259
}{
@@ -269,7 +269,7 @@ func TestValidateToken_PodListOptions(t *testing.T) {
269269
},
270270
Status: corev1.PodStatus{Phase: corev1.PodRunning},
271271
},
272-
gi: &grpcContext.GrpcInfo{Token: "dummy-token"},
272+
grpcInfo: &grpcContext.GrpcInfo{Token: "dummy-token"},
273273
shouldErr: false,
274274
},
275275
{
@@ -284,7 +284,7 @@ func TestValidateToken_PodListOptions(t *testing.T) {
284284
},
285285
Status: corev1.PodStatus{Phase: corev1.PodRunning},
286286
},
287-
gi: &grpcContext.GrpcInfo{Token: "dummy-token"},
287+
grpcInfo: &grpcContext.GrpcInfo{Token: "dummy-token"},
288288
shouldErr: true,
289289
},
290290
{
@@ -299,7 +299,7 @@ func TestValidateToken_PodListOptions(t *testing.T) {
299299
},
300300
Status: corev1.PodStatus{Phase: corev1.PodRunning},
301301
},
302-
gi: &grpcContext.GrpcInfo{Token: "dummy-token"},
302+
grpcInfo: &grpcContext.GrpcInfo{Token: "dummy-token"},
303303
shouldErr: true,
304304
},
305305
{
@@ -312,7 +312,7 @@ func TestValidateToken_PodListOptions(t *testing.T) {
312312
},
313313
Status: corev1.PodStatus{Phase: corev1.PodRunning},
314314
},
315-
gi: &grpcContext.GrpcInfo{Token: "dummy-token"},
315+
grpcInfo: &grpcContext.GrpcInfo{Token: "dummy-token"},
316316
shouldErr: true,
317317
},
318318
{
@@ -327,7 +327,7 @@ func TestValidateToken_PodListOptions(t *testing.T) {
327327
},
328328
Status: corev1.PodStatus{Phase: corev1.PodPending},
329329
},
330-
gi: &grpcContext.GrpcInfo{Token: "dummy-token"},
330+
grpcInfo: &grpcContext.GrpcInfo{Token: "dummy-token"},
331331
shouldErr: true,
332332
},
333333
}
@@ -352,7 +352,7 @@ func TestValidateToken_PodListOptions(t *testing.T) {
352352
patchedClient := &patchClient{fakeClient}
353353
csPatched := NewContextSetter(patchedClient, "ngf-audience")
354354

355-
resultCtx, err := csPatched.validateToken(t.Context(), tc.gi)
355+
resultCtx, err := csPatched.validateToken(t.Context(), tc.grpcInfo)
356356
if tc.shouldErr {
357357
g.Expect(err).To(HaveOccurred())
358358
g.Expect(err.Error()).To(ContainSubstring("no running pods"))

‎internal/controller/nginx/types/types.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,14 @@ const (
44
// Nginx503Server is used as a backend for services that cannot be resolved (have no IP address).
55
Nginx503Server = "unix:/var/run/nginx/nginx-503-server.sock"
66
)
7+
8+
const (
9+
// AgentOwnerNameLabel is the label key used to store the owner name of the nginx agent.
10+
AgentOwnerNameLabel = "owner-name"
11+
// AgentOwnerTypeLabel is the label key used to store the owner type of the nginx agent.
12+
AgentOwnerTypeLabel = "owner-type"
13+
// DaemonSetType is the value used to represent a DaemonSet owner type.
14+
DaemonSetType = "DaemonSet"
15+
// DeploymentType is the value used to represent a Deployment owner type.
16+
DeploymentType = "Deployment"
17+
)

‎internal/controller/provisioner/objects.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
ngfAPIv1alpha2 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha2"
2626
"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/config"
27+
nginxTypes "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/types"
2728
"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/dataplane"
2829
"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/graph"
2930
"github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller"
@@ -451,6 +452,14 @@ func (p *NginxProvisioner) buildNginxConfigMaps(
451452
metricsPort = *port
452453
}
453454

455+
depType := nginxTypes.DeploymentType
456+
if nProxyCfg != nil && nProxyCfg.Kubernetes != nil && nProxyCfg.Kubernetes.DaemonSet != nil {
457+
depType = nginxTypes.DaemonSetType
458+
}
459+
460+
p.cfg.AgentLabels[nginxTypes.AgentOwnerNameLabel] = fmt.Sprintf("%s_%s", objectMeta.Namespace, objectMeta.Name)
461+
p.cfg.AgentLabels[nginxTypes.AgentOwnerTypeLabel] = depType
462+
454463
agentFields := map[string]interface{}{
455464
"Plus": p.cfg.Plus,
456465
"ServiceName": p.cfg.GatewayPodConfig.ServiceName,
@@ -865,10 +874,8 @@ func (p *NginxProvisioner) buildNginxPodTemplateSpec(
865874
tokenAudience := fmt.Sprintf("%s.%s.svc", p.cfg.GatewayPodConfig.ServiceName, p.cfg.GatewayPodConfig.Namespace)
866875

867876
clusterID := "unknown"
868-
if p.cfg.AgentLabels != nil {
869-
if val, ok := p.cfg.AgentLabels["cluster-id"]; ok {
870-
clusterID = val
871-
}
877+
if val, ok := p.cfg.AgentLabels["cluster-id"]; ok {
878+
clusterID = val
872879
}
873880

874881
spec := corev1.PodTemplateSpec{

0 commit comments

Comments
 (0)