client: 404 when accessing files for GC'ed alloc (#18232)

When an allocation is garbage collected from the client, but not from the servers, the API request is routed to the client and the client does attempt to read the file, but the alloc dir has already been deleted, resulting in a 500 error. This happens because the client GC only destroys the alloc runner (deleting the alloc dir), but it keeps a reference to the alloc runner until the alloc is garbage collected from the servers as well. This commit adjusts this logic by checking if the alloc runner (and the alloc files) has been destroyed, returning a 404 if so.
hashicorp · Aug 21, 2023 · 14a38be · 14a38be
1 parent 9fa39eb
commit 14a38be
Show file tree

Hide file tree

Showing 3 changed files with 234 additions and 28 deletions.
diff --git a/.changelog/18232.txt b/.changelog/18232.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+client: return 404 instead of 500 when trying to access logs and files from allocations that have been garbage collected
+```
diff --git a/client/fs_endpoint.go b/client/fs_endpoint.go
@@ -169,48 +169,58 @@ func (f *FileSystem) stream(conn io.ReadWriteCloser) {
 	encoder := codec.NewEncoder(conn, structs.MsgpackHandle)
 
 	if err := decoder.Decode(&req); err != nil {
-		handleStreamResultError(err, pointer.Of(int64(500)), encoder)
+		handleStreamResultError(err, pointer.Of(int64(http.StatusInternalServerError)), encoder)
 		return
 	}
 
 	if req.AllocID == "" {
-		handleStreamResultError(allocIDNotPresentErr, pointer.Of(int64(400)), encoder)
+		handleStreamResultError(allocIDNotPresentErr, pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
-	alloc, err := f.c.GetAlloc(req.AllocID)
+
+	ar, err := f.c.getAllocRunner(req.AllocID)
 	if err != nil {
-		handleStreamResultError(structs.NewErrUnknownAllocation(req.AllocID), pointer.Of(int64(404)), encoder)
+		handleStreamResultError(structs.NewErrUnknownAllocation(req.AllocID), pointer.Of(int64(http.StatusNotFound)), encoder)
+		return
+	}
+	if ar.IsDestroyed() {
+		handleStreamResultError(
+			fmt.Errorf("state for allocation %s not found on client", req.AllocID),
+			pointer.Of(int64(http.StatusNotFound)),
+			encoder,
+		)
 		return
 	}
+	alloc := ar.Alloc()
 
 	// Check read permissions
 	if aclObj, err := f.c.ResolveToken(req.QueryOptions.AuthToken); err != nil {
-		handleStreamResultError(err, pointer.Of(int64(403)), encoder)
+		handleStreamResultError(err, pointer.Of(int64(http.StatusForbidden)), encoder)
 		return
 	} else if aclObj != nil && !aclObj.AllowNsOp(alloc.Namespace, acl.NamespaceCapabilityReadFS) {
-		handleStreamResultError(structs.ErrPermissionDenied, pointer.Of(int64(403)), encoder)
+		handleStreamResultError(structs.ErrPermissionDenied, pointer.Of(int64(http.StatusForbidden)), encoder)
 		return
 	}
 
 	// Validate the arguments
 	if req.Path == "" {
-		handleStreamResultError(pathNotPresentErr, pointer.Of(int64(400)), encoder)
+		handleStreamResultError(pathNotPresentErr, pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
 	switch req.Origin {
 	case "start", "end":
 	case "":
 		req.Origin = "start"
 	default:
-		handleStreamResultError(invalidOrigin, pointer.Of(int64(400)), encoder)
+		handleStreamResultError(invalidOrigin, pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
 
 	fs, err := f.c.GetAllocFS(req.AllocID)
 	if err != nil {
-		code := pointer.Of(int64(500))
+		code := pointer.Of(int64(http.StatusInternalServerError))
 		if structs.IsErrUnknownAllocation(err) {
-			code = pointer.Of(int64(404))
+			code = pointer.Of(int64(http.StatusNotFound))
 		}
 
 		handleStreamResultError(err, code, encoder)
@@ -220,13 +230,13 @@ func (f *FileSystem) stream(conn io.ReadWriteCloser) {
 	// Calculate the offset
 	fileInfo, err := fs.Stat(req.Path)
 	if err != nil {
-		handleStreamResultError(err, pointer.Of(int64(400)), encoder)
+		handleStreamResultError(err, pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
 	if fileInfo.IsDir {
 		handleStreamResultError(
 			fmt.Errorf("file %q is a directory", req.Path),
-			pointer.Of(int64(400)), encoder)
+			pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
 
@@ -328,7 +338,7 @@ OUTER:
 	}
 
 	if streamErr != nil {
-		handleStreamResultError(streamErr, pointer.Of(int64(500)), encoder)
+		handleStreamResultError(streamErr, pointer.Of(int64(http.StatusInternalServerError)), encoder)
 		return
 	}
 }
@@ -344,19 +354,29 @@ func (f *FileSystem) logs(conn io.ReadWriteCloser) {
 	encoder := codec.NewEncoder(conn, structs.MsgpackHandle)
 
 	if err := decoder.Decode(&req); err != nil {
-		handleStreamResultError(err, pointer.Of(int64(500)), encoder)
+		handleStreamResultError(err, pointer.Of(int64(http.StatusInternalServerError)), encoder)
 		return
 	}
 
 	if req.AllocID == "" {
-		handleStreamResultError(allocIDNotPresentErr, pointer.Of(int64(400)), encoder)
+		handleStreamResultError(allocIDNotPresentErr, pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
-	alloc, err := f.c.GetAlloc(req.AllocID)
+
+	ar, err := f.c.getAllocRunner(req.AllocID)
 	if err != nil {
-		handleStreamResultError(structs.NewErrUnknownAllocation(req.AllocID), pointer.Of(int64(404)), encoder)
+		handleStreamResultError(structs.NewErrUnknownAllocation(req.AllocID), pointer.Of(int64(http.StatusNotFound)), encoder)
+		return
+	}
+	if ar.IsDestroyed() {
+		handleStreamResultError(
+			fmt.Errorf("state for allocation %s not found on client", req.AllocID),
+			pointer.Of(int64(http.StatusNotFound)),
+			encoder,
+		)
 		return
 	}
+	alloc := ar.Alloc()
 
 	// Check read permissions
 	if aclObj, err := f.c.ResolveToken(req.QueryOptions.AuthToken); err != nil {
@@ -373,29 +393,29 @@ func (f *FileSystem) logs(conn io.ReadWriteCloser) {
 
 	// Validate the arguments
 	if req.Task == "" {
-		handleStreamResultError(taskNotPresentErr, pointer.Of(int64(400)), encoder)
+		handleStreamResultError(taskNotPresentErr, pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
 	switch req.LogType {
 	case "stdout", "stderr":
 	default:
-		handleStreamResultError(logTypeNotPresentErr, pointer.Of(int64(400)), encoder)
+		handleStreamResultError(logTypeNotPresentErr, pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
 	switch req.Origin {
 	case "start", "end":
 	case "":
 		req.Origin = "start"
 	default:
-		handleStreamResultError(invalidOrigin, pointer.Of(int64(400)), encoder)
+		handleStreamResultError(invalidOrigin, pointer.Of(int64(http.StatusBadRequest)), encoder)
 		return
 	}
 
 	fs, err := f.c.GetAllocFS(req.AllocID)
 	if err != nil {
-		code := pointer.Of(int64(500))
+		code := pointer.Of(int64(http.StatusInternalServerError))
 		if structs.IsErrUnknownAllocation(err) {
-			code = pointer.Of(int64(404))
+			code = pointer.Of(int64(http.StatusNotFound))
 		}
 
 		handleStreamResultError(err, code, encoder)
@@ -404,9 +424,9 @@ func (f *FileSystem) logs(conn io.ReadWriteCloser) {
 
 	allocState, err := f.c.GetAllocState(req.AllocID)
 	if err != nil {
-		code := pointer.Of(int64(500))
+		code := pointer.Of(int64(http.StatusInternalServerError))
 		if structs.IsErrUnknownAllocation(err) {
-			code = pointer.Of(int64(404))
+			code = pointer.Of(int64(http.StatusNotFound))
 		}
 
 		handleStreamResultError(err, code, encoder)
@@ -418,15 +438,15 @@ func (f *FileSystem) logs(conn io.ReadWriteCloser) {
 	if taskState == nil {
 		handleStreamResultError(
 			fmt.Errorf("unknown task name %q", req.Task),
-			pointer.Of(int64(400)),
+			pointer.Of(int64(http.StatusBadRequest)),
 			encoder)
 		return
 	}
 
 	if taskState.StartedAt.IsZero() {
 		handleStreamResultError(
 			fmt.Errorf("task %q not started yet. No logs available", req.Task),
-			pointer.Of(int64(404)),
+			pointer.Of(int64(http.StatusNotFound)),
 			encoder)
 		return
 	}
@@ -512,7 +532,7 @@ OUTER:
 
 	if streamErr != nil {
 		// If error has a Code, use it
-		var code int64 = 500
+		var code int64 = http.StatusInternalServerError
 		if codedErr, ok := streamErr.(interface{ Code() int }); ok {
 			code = int64(codedErr.Code())
 		}