Skip to content

Commit f46c67f

Browse files
BenTheElderBenTheElder
authored
ateom: avoid unix socket path length limits (agent-substrate#100)
Pod names alone can easily exceed the max length (253 > 107). Add a symlink with the full pod name for debugging. Follow-up agent-substrate#92 > It's a good idea to open an issue first for discussion. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR --- TODO: I'm not sure about the symlink. It's annoying mapping pod => hash(pod) otherwise though. We could put a file inside the directory with the pod name instead, but I think we should probably put the full pod name _somewhere_ in the structure so it's easier to inspect these.
1 parent 4cbac18 commit f46c67f

13 files changed

Lines changed: 119 additions & 177 deletions

File tree

‎cmd/ateapi/internal/controlapi/functional_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -671,7 +671,7 @@ func TestListWorkers(t *testing.T) {
671671
},
672672
}
673673

674-
if diff := cmp.Diff(want, filteredWorkers, protocmp.Transform()); diff != "" {
674+
if diff := cmp.Diff(want, filteredWorkers, protocmp.Transform(), protocmp.IgnoreFields(&ateapipb.Worker{}, "worker_pod_uid")); diff != "" {
675675
t.Errorf("ListWorkers response mismatch (-want +got):\n%s", diff)
676676
}
677677
}
@@ -733,7 +733,7 @@ func TestResumeActor(t *testing.T) {
733733
AteomPodIp: "127.0.0.1",
734734
},
735735
}
736-
if diff := cmp.Diff(want, getResp, protocmp.Transform(), protocmp.IgnoreFields(&ateapipb.Actor{}, "version")); diff != "" {
736+
if diff := cmp.Diff(want, getResp, protocmp.Transform(), protocmp.IgnoreFields(&ateapipb.Actor{}, "version"), protocmp.IgnoreFields(&ateapipb.Actor{}, "ateom_pod_uid")); diff != "" {
737737
t.Errorf("GetActor response mismatch (-want +got):\n%s", diff)
738738
}
739739

@@ -763,7 +763,7 @@ func TestResumeActor(t *testing.T) {
763763
Ip: "127.0.0.1",
764764
}
765765

766-
if diff := cmp.Diff(wantWorker, actorWorker, protocmp.Transform(), protocmp.IgnoreFields(&ateapipb.Worker{}, "version")); diff != "" {
766+
if diff := cmp.Diff(wantWorker, actorWorker, protocmp.Transform(), protocmp.IgnoreFields(&ateapipb.Worker{}, "version"), protocmp.IgnoreFields(&ateapipb.Worker{}, "worker_pod_uid")); diff != "" {
767767
t.Errorf("Worker state mismatch (-want +got):\n%s", diff)
768768
}
769769
}
@@ -943,7 +943,7 @@ func TestSuspendActor(t *testing.T) {
943943
},
944944
}
945945

946-
if diff := cmp.Diff(want, getResp, protocmp.Transform(), protocmp.IgnoreFields(&ateapipb.Actor{}, "version"), protocmp.IgnoreFields(&ateapipb.Actor{}, "last_snapshot")); diff != "" {
946+
if diff := cmp.Diff(want, getResp, protocmp.Transform(), protocmp.IgnoreFields(&ateapipb.Actor{}, "version"), protocmp.IgnoreFields(&ateapipb.Actor{}, "last_snapshot"), protocmp.IgnoreFields(&ateapipb.Actor{}, "ateom_pod_uid")); diff != "" {
947947
t.Errorf("GetActor response mismatch (-want +got):\n%s", diff)
948948
}
949949

‎cmd/ateapi/internal/controlapi/syncer.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ func (s *WorkerPoolSyncer) syncWorkerToStore(ctx context.Context, pod *corev1.Po
119119
WorkerPool: pod.Labels[workerPodLabel],
120120
WorkerPod: pod.Name,
121121
Ip: pod.Status.PodIP,
122+
WorkerPodUid: string(pod.UID),
122123
})
123124
if err != nil && !errors.Is(err, store.ErrAlreadyExists) {
124125
slog.ErrorContext(ctx, "Failed to create worker in store", slog.Any("err", err))

‎cmd/ateapi/internal/controlapi/workflow_resume.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ func (s *AssignWorkerStep) Execute(ctx context.Context, input *ResumeInput, stat
123123
state.Actor.AteomPodNamespace = assignedWorker.GetWorkerNamespace()
124124
state.Actor.AteomPodName = assignedWorker.GetWorkerPod()
125125
state.Actor.AteomPodIp = assignedWorker.GetIp()
126+
state.Actor.AteomPodUid = assignedWorker.GetWorkerPodUid()
126127

127128
if err := s.store.UpdateActor(ctx, state.Actor, state.Actor.GetVersion()); err != nil {
128129
return err
@@ -213,8 +214,7 @@ func (s *CallAteletRestoreStep) Execute(ctx context.Context, input *ResumeInput,
213214
slog.InfoContext(ctx, "Actor has snapshot; Restoring from snapshot")
214215

215216
req := &ateletpb.RestoreRequest{
216-
TargetAteomNamespace: state.Actor.GetAteomPodNamespace(),
217-
TargetAteomName: state.Actor.GetAteomPodName(),
217+
TargetAteomUid: state.Actor.GetAteomPodUid(),
218218
ActorTemplateNamespace: state.Actor.GetActorTemplateNamespace(),
219219
ActorTemplateName: state.Actor.GetActorTemplateName(),
220220
ActorId: state.Actor.GetActorId(),
@@ -233,8 +233,7 @@ func (s *CallAteletRestoreStep) Execute(ctx context.Context, input *ResumeInput,
233233
snapshot := state.ActorTemplate.Status.GoldenSnapshot
234234

235235
req := &ateletpb.RestoreRequest{
236-
TargetAteomNamespace: state.Actor.GetAteomPodNamespace(),
237-
TargetAteomName: state.Actor.GetAteomPodName(),
236+
TargetAteomUid: state.Actor.GetAteomPodUid(),
238237
ActorTemplateNamespace: state.Actor.GetActorTemplateNamespace(),
239238
ActorTemplateName: state.Actor.GetActorTemplateName(),
240239
ActorId: state.Actor.GetActorId(),
@@ -250,8 +249,7 @@ func (s *CallAteletRestoreStep) Execute(ctx context.Context, input *ResumeInput,
250249
} else {
251250
slog.InfoContext(ctx, "Actor has no snapshot; ActorTemplate has no golden snapshot; Booting from ActorTemplate spec")
252251
req := &ateletpb.RunRequest{
253-
TargetAteomNamespace: state.Actor.GetAteomPodNamespace(),
254-
TargetAteomName: state.Actor.GetAteomPodName(),
252+
TargetAteomUid: state.Actor.GetAteomPodUid(),
255253
ActorTemplateNamespace: state.Actor.GetActorTemplateNamespace(),
256254
ActorTemplateName: state.Actor.GetActorTemplateName(),
257255
ActorId: state.Actor.GetActorId(),

‎cmd/ateapi/internal/controlapi/workflow_suspend.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,7 @@ func (s *CallAteletSuspendStep) Execute(ctx context.Context, input *SuspendInput
136136
}
137137

138138
req := &ateletpb.CheckpointRequest{
139-
TargetAteomNamespace: state.Actor.GetAteomPodNamespace(),
140-
TargetAteomName: state.Actor.GetAteomPodName(),
139+
TargetAteomUid: state.Actor.GetAteomPodUid(),
141140
ActorTemplateNamespace: state.Actor.GetActorTemplateNamespace(),
142141
ActorTemplateName: state.Actor.GetActorTemplateName(),
143142
ActorId: state.Actor.GetActorId(),

‎cmd/atelet/main.go‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -280,12 +280,12 @@ func (s *AteomHerder) Run(ctx context.Context, req *ateletpb.RunRequest) (*atele
280280

281281
if err := s.prepareOCIBundles(ctx,
282282
req.GetActorTemplateNamespace(), req.GetActorTemplateName(), req.GetActorId(),
283-
req.GetSpec(), req.GetTargetAteomNamespace(), req.GetTargetAteomName(),
283+
req.GetSpec(), req.GetTargetAteomUid(),
284284
); err != nil {
285285
return nil, err
286286
}
287287

288-
client, err := s.dialAteom(ctx, req.GetTargetAteomNamespace(), req.GetTargetAteomName())
288+
client, err := s.dialAteom(ctx, req.GetTargetAteomUid())
289289
if err != nil {
290290
return nil, err
291291
}
@@ -313,7 +313,7 @@ func (s *AteomHerder) Checkpoint(ctx context.Context, req *ateletpb.CheckpointRe
313313

314314
checkpointDir := ateompath.CheckpointStateDir(req.GetActorTemplateNamespace(), req.GetActorTemplateName(), req.GetActorId())
315315

316-
client, err := s.dialAteom(ctx, req.GetTargetAteomNamespace(), req.GetTargetAteomName())
316+
client, err := s.dialAteom(ctx, req.GetTargetAteomUid())
317317
if err != nil {
318318
return nil, err
319319
}
@@ -401,12 +401,12 @@ func (s *AteomHerder) Restore(ctx context.Context, req *ateletpb.RestoreRequest)
401401
}
402402

403403
if err := s.prepareOCIBundles(ctx, ns, tmpl, actorID,
404-
req.GetSpec(), req.GetTargetAteomNamespace(), req.GetTargetAteomName(),
404+
req.GetSpec(), req.GetTargetAteomUid(),
405405
); err != nil {
406406
return nil, err
407407
}
408408

409-
client, err := s.dialAteom(ctx, req.GetTargetAteomNamespace(), req.GetTargetAteomName())
409+
client, err := s.dialAteom(ctx, req.GetTargetAteomUid())
410410
if err != nil {
411411
return nil, err
412412
}
@@ -446,9 +446,9 @@ func (s *AteomHerder) prepareOCIBundles(
446446
ctx context.Context,
447447
actorTemplateNamespace, actorTemplateName, actorID string,
448448
spec *ateletpb.WorkloadSpec,
449-
targetAteomNamespace, targetAteomName string,
449+
targetAteomUid string,
450450
) error {
451-
netnsPath := ateompath.AteomNetNSPath(targetAteomNamespace, targetAteomName)
451+
netnsPath := ateompath.AteomNetNSPath(targetAteomUid)
452452

453453
g, gCtx := errgroup.WithContext(ctx)
454454

@@ -507,10 +507,10 @@ func (s *AteomHerder) prepareOCIBundles(
507507

508508
// dialAteom opens (or reuses) the gRPC connection to the target ateom
509509
// pod and returns an ateom client.
510-
func (s *AteomHerder) dialAteom(ctx context.Context, namespace, name string) (ateompb.AteomClient, error) {
511-
conn, err := s.ateomDialer.DialAteomPod(ctx, namespace, name)
510+
func (s *AteomHerder) dialAteom(ctx context.Context, targetAteomUid string) (ateompb.AteomClient, error) {
511+
conn, err := s.ateomDialer.DialAteomPod(ctx, targetAteomUid)
512512
if err != nil {
513-
return nil, fmt.Errorf("while getting ateom conn for %s/%s: %w", namespace, name, err)
513+
return nil, fmt.Errorf("while getting ateom conn for %s: %w", targetAteomUid, err)
514514
}
515515
return ateompb.NewAteomClient(conn), nil
516516
}
@@ -542,16 +542,16 @@ type AteomDialer struct {
542542
conns *lru.Cache
543543
}
544544

545-
func (d *AteomDialer) DialAteomPod(ctx context.Context, namespace, name string) (*grpc.ClientConn, error) {
546-
key := namespace + "/" + name
545+
func (d *AteomDialer) DialAteomPod(ctx context.Context, podUID string) (*grpc.ClientConn, error) {
546+
key := podUID
547547

548548
connAny, ok := d.conns.Get(key)
549549
if ok {
550550
return connAny.(*grpc.ClientConn), nil
551551
}
552552

553553
conn, err := grpc.NewClient(
554-
"unix://"+ateompath.AteomSocketPath(namespace, name),
554+
"unix://"+ateompath.AteomSocketPath(podUID),
555555
grpc.WithTransportCredentials(insecure.NewCredentials()),
556556
grpc.WithStatsHandler(otelgrpc.NewClientHandler()),
557557
)

‎cmd/ateom-gvisor/main.go‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ import (
4545
)
4646

4747
var (
48-
podNamespace = flag.String("pod-namespace", "", "The namespace of the current pod")
49-
podName = flag.String("pod-name", "", "The name of the current pod")
48+
podUID = flag.String("pod-uid", "", "The UID of the current pod")
5049

5150
showVersion = flag.Bool("version", false, "Print version and exit.")
5251

@@ -89,7 +88,7 @@ func do(ctx context.Context) error {
8988
defer serverboot.ShutdownProvider("TracerProvider", tp.Shutdown)
9089

9190
// Create ateom dir
92-
ateomDir := ateompath.AteomPath(*podNamespace, *podName)
91+
ateomDir := ateompath.AteomPath(*podUID)
9392
if err := os.MkdirAll(ateomDir, 0o700); err != nil {
9493
return fmt.Errorf("in os.MkdirAll(%q): %w", ateomDir, err)
9594
}
@@ -101,14 +100,8 @@ func do(ctx context.Context) error {
101100
go reap.ReapChildren(nil, nil, nil, &reapLock)
102101
slog.InfoContext(ctx, "Child process reaper launched")
103102

104-
// Validate before opening the socket so the operator sees a clear
105-
// message rather than the kernel's cryptic "bind: invalid argument".
106-
if err := ateompath.ValidateAteomSocketPath(*podNamespace, *podName); err != nil {
107-
return err
108-
}
109-
110103
// Clean up any old socket.
111-
sockPath := ateompath.AteomSocketPath(*podNamespace, *podName)
104+
sockPath := ateompath.AteomSocketPath(*podUID)
112105
if err := os.RemoveAll(sockPath); err != nil {
113106
return fmt.Errorf("while removing %q: %w", sockPath, err)
114107
}
@@ -137,7 +130,7 @@ func do(ctx context.Context) error {
137130
// read the addresses and routes off of every link in the namespace, then
138131
// remove all the addresses and handle injecting packets into the interfaces
139132
// using AF_PACKET.
140-
interiorNetNS, err := createNetNSWithoutSwitching(ctx, ateompath.AteomNetNSName(*podNamespace, *podName))
133+
interiorNetNS, err := createNetNSWithoutSwitching(ctx, ateompath.AteomNetNSName(*podUID))
141134
if err != nil {
142135
return fmt.Errorf("while creating ateom-interior netns: %w", err)
143136
}

‎internal/ateompath/ateompath.go‎

Lines changed: 8 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,9 @@
1616
package ateompath
1717

1818
import (
19-
"fmt"
2019
"path/filepath"
2120
)
2221

23-
// MaxUnixSocketPathLen is the practical Linux limit for unix-domain socket
24-
// paths. The kernel's sockaddr_un.sun_path is 108 bytes including the trailing
25-
// NUL, leaving 107 usable bytes. Bind fails with EINVAL above this.
26-
const MaxUnixSocketPathLen = 107
27-
2822
const (
2923
// The base path. This is both the path of the root shared folder on the
3024
// host filesystem, and when it is mounted into ateom and atelet containers.
@@ -40,45 +34,29 @@ func RunSCBinaryPath(sha256 string) string {
4034
return filepath.Join(StaticFilesDir, "runsc-"+sha256)
4135
}
4236

43-
func AteomPath(ateomNamespace, ateomName string) string {
37+
func AteomPath(podUID string) string {
4438
return filepath.Join(
4539
BasePath,
4640
"ateoms",
47-
ateomNamespace+":"+ateomName,
41+
podUID,
4842
)
4943
}
5044

51-
func AteomSocketPath(ateomNamespace, ateomName string) string {
45+
func AteomSocketPath(podUID string) string {
5246
return filepath.Join(
53-
AteomPath(ateomNamespace, ateomName),
47+
AteomPath(podUID),
5448
"ateom.sock",
5549
)
5650
}
5751

58-
// ValidateAteomSocketPath returns a descriptive error when the socket path
59-
// derived from ateomNamespace and ateomName would exceed Linux's unix-socket
60-
// limit. Calling net.Listen("unix", ...) with an over-limit path otherwise
61-
// fails with the cryptic "bind: invalid argument".
62-
func ValidateAteomSocketPath(ateomNamespace, ateomName string) error {
63-
p := AteomSocketPath(ateomNamespace, ateomName)
64-
if len(p) > MaxUnixSocketPathLen {
65-
return fmt.Errorf(
66-
"ateom socket path %q is %d bytes, exceeds Linux unix-socket limit of %d: shorten the namespace or pod name (%d + %d = %d chars used for namespace + name)",
67-
p, len(p), MaxUnixSocketPathLen,
68-
len(ateomNamespace), len(ateomName), len(ateomNamespace)+len(ateomName),
69-
)
70-
}
71-
return nil
72-
}
73-
74-
func AteomNetNSName(ateomNamespace, ateomName string) string {
75-
return "ateom:" + ateomNamespace + ":" + ateomName
52+
func AteomNetNSName(podUID string) string {
53+
return "ateom:" + podUID
7654
}
7755

78-
func AteomNetNSPath(ateomNamespace, ateomName string) string {
56+
func AteomNetNSPath(podUID string) string {
7957
return filepath.Join(
8058
"/run/netns",
81-
AteomNetNSName(ateomNamespace, ateomName),
59+
AteomNetNSName(podUID),
8260
)
8361
}
8462

‎internal/ateompath/ateompath_test.go‎

Lines changed: 36 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -19,53 +19,42 @@ import (
1919
"testing"
2020
)
2121

22-
func TestValidateAteomSocketPath(t *testing.T) {
23-
tests := []struct {
24-
name string
25-
namespace string
26-
podName string
27-
wantErr bool
28-
}{
29-
{
30-
name: "short names well under the limit",
31-
namespace: "ate-demo-counter",
32-
podName: "counter-deployment-abcd1234-xyzw1",
33-
wantErr: false,
34-
},
35-
{
36-
name: "exactly at the limit",
37-
namespace: strings.Repeat("a", 25),
38-
podName: strings.Repeat("b", 45),
39-
wantErr: false,
40-
},
41-
{
42-
name: "one byte over the limit",
43-
namespace: strings.Repeat("a", 25),
44-
podName: strings.Repeat("b", 46),
45-
wantErr: true,
46-
},
47-
{
48-
name: "the reproducer from the original bug report",
49-
namespace: "ate-demo-lovable-sandbox",
50-
podName: "lovable-sandbox-pool-deployment-5797879cd7-2n7wb",
51-
wantErr: true,
52-
},
22+
func TestAteomPath(t *testing.T) {
23+
podUID := "123e4567-e89b-12d3-a456-426614174000"
24+
25+
path := AteomPath(podUID)
26+
expectedSuffix := "/ateoms/" + podUID
27+
if !strings.HasSuffix(path, expectedSuffix) {
28+
t.Errorf("expected path to end with %s, got %s", expectedSuffix, path)
29+
}
30+
}
31+
32+
func TestAteomSocketPathLimits(t *testing.T) {
33+
podUID := "123e4567-e89b-12d3-a456-426614174000"
34+
35+
sockPath := AteomSocketPath(podUID)
36+
37+
// Unix domain socket path limit is 107 bytes (108 with NUL terminator)
38+
const maxUnixSocketLen = 107
39+
if len(sockPath) > maxUnixSocketLen {
40+
t.Errorf("socket path length %d exceeds max allowed length %d: %q", len(sockPath), maxUnixSocketLen, sockPath)
5341
}
54-
for _, tt := range tests {
55-
t.Run(tt.name, func(t *testing.T) {
56-
err := ValidateAteomSocketPath(tt.namespace, tt.podName)
57-
if (err != nil) != tt.wantErr {
58-
t.Errorf("ValidateAteomSocketPath(%q, %q) err=%v, wantErr=%v",
59-
tt.namespace, tt.podName, err, tt.wantErr)
60-
}
61-
if tt.wantErr && err != nil {
62-
// Error message should mention the limit so an operator can
63-
// figure out by how much to shorten.
64-
msg := err.Error()
65-
if !strings.Contains(msg, "107") {
66-
t.Errorf("error message %q does not reference the limit (107)", msg)
67-
}
68-
}
69-
})
42+
43+
// Verify it is deterministic
44+
sockPath2 := AteomSocketPath(podUID)
45+
if sockPath != sockPath2 {
46+
t.Errorf("expected deterministic socket paths, got %q and %q", sockPath, sockPath2)
47+
}
48+
}
49+
50+
func TestAteomPathUniqueness(t *testing.T) {
51+
uid1 := "123e4567-e89b-12d3-a456-426614174000"
52+
uid2 := "987f6543-e21b-32d1-b654-246614174111"
53+
54+
path1 := AteomPath(uid1)
55+
path2 := AteomPath(uid2)
56+
57+
if path1 == path2 {
58+
t.Errorf("expected different paths for different pod UIDs, got %q", path1)
7059
}
7160
}

0 commit comments

Comments
 (0)