Address review commments - squash before merge

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

BUILD

@@ -14,12 +14,11 @@ exports_files([
     "linux/type_mapper.h",
 ])
 
-envoy_cc_binary(
+cc_binary(
     name = "cilium-envoy-starter",
     deps = [
         "//starter:main_entry_lib",
     ],
-    repository = "@envoy",
 )
 
 envoy_cc_binary(

Makefile

@@ -94,13 +94,15 @@ clang.bazelrc: bazel/setup_clang.sh /usr/lib/llvm-15
 	bazel/setup_clang.sh /usr/lib/llvm-15
 	echo "build --config=clang" >> $@
 
+.PHONY: bazel-bin/cilium-envoy
 bazel-bin/cilium-envoy: $(COMPILER_DEP) SOURCE_VERSION
 	@$(ECHO_BAZEL)
 	$(BAZEL) $(BAZEL_OPTS) build $(BAZEL_BUILD_OPTS) //:cilium-envoy $(BAZEL_FILTER)
 
 cilium-envoy: bazel-bin/cilium-envoy
 	mv $< $@
 
+.PHONY: bazel-bin/cilium-envoy-starter
 bazel-bin/cilium-envoy-starter: $(COMPILER_DEP) SOURCE_VERSION
 	@$(ECHO_BAZEL)
 	$(BAZEL) $(BAZEL_OPTS) build $(BAZEL_BUILD_OPTS) //:cilium-envoy-starter $(BAZEL_FILTER)

cilium/BUILD

@@ -175,7 +175,7 @@ envoy_cc_library(
     ],
     repository = "@envoy",
     deps = [
-        "//starter:privileged_service_client_lib",
+        "privileged_service_client_lib",
         "@envoy//source/common/common:logger_lib",
         "@envoy//source/common/common:utility_lib",
     ],
@@ -299,3 +299,15 @@ envoy_cc_library(
         "@envoy//source/common/router:config_utility_lib",
     ],
 )
+
+envoy_cc_library(
+    name = "privileged_service_client_lib",
+    hdrs = ["privileged_service_client.h"],
+    srcs = ["privileged_service_client.cc"],
+    repository = "@envoy",
+    deps = [
+        "//starter:privileged_service_protocol",
+        "@envoy//envoy/api:os_sys_calls_interface",
+        "@envoy//source/common/singleton:threadsafe_singleton",
+    ],
+)

cilium/bpf.cc

@@ -1,5 +1,5 @@
 #include "cilium/bpf.h"
-#include "starter/privileged_service_client.h"
+#include "cilium/privileged_service_client.h"
 
 #include "source/common/common/utility.h"
 

starter/privileged_service_client.cc → cilium/privileged_service_client.cc

@@ -2,7 +2,7 @@
 #error "Linux platform file is part of non-Linux build."
 #endif
 
-#include "starter/privileged_service_client.h"
+#include "cilium/privileged_service_client.h"
 
 namespace Envoy {
 namespace Cilium {
@@ -16,7 +16,7 @@ ProtocolClient::ProtocolClient() :
 		 "cilium-envoy running with privileges, exiting");
 
   if (!check_privileged_service()) {
-    // Running without Cilium privileged service
+    // No Cilium privileged service detected
     close();
   }
 
@@ -64,7 +64,7 @@ ssize_t ProtocolClient::transact(MessageHeader& req, size_t req_len, const void
 }
 
 bool ProtocolClient::check_privileged_service() {
-  // Get the effective capabilities from the privileged service process
+  // Dump the effective capabilities of the privileged service process
   DumpRequest req;
   Response resp;
   uint8_t buf[1024];
@@ -74,12 +74,12 @@ bool ProtocolClient::check_privileged_service() {
     return false;
   }
   std::string str(reinterpret_cast<char *>(buf), size - sizeof(resp));
-  ENVOY_LOG_MISC(debug, "Running with Cilium privileged service with the following capabilities: {}", str);
+  ENVOY_LOG_MISC(debug, "Cilium privileged service detected with following capabilities: {}", str);
   return true;
 }
 
 Envoy::Api::SysCallIntResult ProtocolClient::bpf_open(const char *path) {
-  if (!running_with_cilium_privileged_service()) {
+  if (!have_cilium_privileged_service()) {
       return {-1, EPERM};
   }
 
@@ -90,40 +90,40 @@ Envoy::Api::SysCallIntResult ProtocolClient::bpf_open(const char *path) {
   int fd = -1;
   ssize_t size = transact(req.hdr_, sizeof(req), path, path_len, &fd, resp);
   RELEASE_ASSERT(size == ssize_t(sizeof(resp)), "invalid received response size");
-  if (resp.ret_.return_value_ == INT_MAX) {
-    resp.ret_.return_value_ = fd;
+  if (resp.return_value_ == INT_MAX) {
+    resp.return_value_ = fd;
   }
-  return resp.ret_;
+  return Envoy::Api::SysCallIntResult{resp.return_value_, resp.errno_};
 }
 
 Envoy::Api::SysCallIntResult ProtocolClient::bpf_lookup(int fd, const void *key,
 							uint32_t key_size, void* value,
 							uint32_t value_size) {
-  if (!running_with_cilium_privileged_service()) {
+  if (!have_cilium_privileged_service()) {
       return {-1, EPERM};
   }
 
   BpfLookupRequest req(value_size);
   Response resp;
   ssize_t size = transact(req.hdr_, sizeof(req), key, key_size, &fd, resp, value, value_size);
-  RELEASE_ASSERT((size == ssize_t(sizeof(resp)) && resp.ret_.return_value_ == -1)
+  RELEASE_ASSERT((size == ssize_t(sizeof(resp)) && resp.return_value_ == -1)
                  || size == ssize_t(sizeof(resp) + value_size),
                  "invalid received bpf lookup value size");
-  return resp.ret_;
+  return Envoy::Api::SysCallIntResult{resp.return_value_, resp.errno_};
 }
 
 Envoy::Api::SysCallIntResult ProtocolClient::setsockopt(int sockfd, int level, int optname,
 							const void *optval,
 							socklen_t optlen) {
-  if (!running_with_cilium_privileged_service()) {
+  if (!have_cilium_privileged_service()) {
       return {-1, EPERM};
   }
 
   SetSockOptRequest req(level, optname, optval, optlen);
   Response resp;
   ssize_t size = transact(req.hdr_, sizeof(req), nullptr, 0, &sockfd, resp);
   RELEASE_ASSERT(size == ssize_t(sizeof(resp)), "invalid received response size");
-  return resp.ret_;
+  return Envoy::Api::SysCallIntResult{resp.return_value_, resp.errno_};
 }
 
 } // namespace PrivilegedService

starter/privileged_service_client.h → cilium/privileged_service_client.h

@@ -9,6 +9,8 @@
 
 #include "starter/privileged_service_protocol.h"
 
+#include "envoy/api/os_sys_calls_common.h"
+
 namespace Envoy {
 namespace Cilium {
 
@@ -38,7 +40,7 @@ class ProtocolClient : public Protocol {
 
 private:
   bool check_privileged_service();
-  bool running_with_cilium_privileged_service() const { return is_open(); }
+  bool have_cilium_privileged_service() const { return is_open(); }
 
   ssize_t transact(MessageHeader& req, size_t req_len, const void *data, size_t datalen, int *fd, Response& resp, void *buf = nullptr, size_t bufsize = 0, bool assert = true);
 

cilium/socket_option.h

@@ -7,7 +7,7 @@
 #include "source/common/common/utility.h"
 
 #include "cilium/conntrack.h"
-#include "starter/privileged_service_client.h"
+#include "cilium/privileged_service_client.h"
 
 namespace Envoy {
 namespace Cilium {

starter/BUILD

@@ -1,49 +1,26 @@
-load(
-    "@envoy//bazel:envoy_build_system.bzl",
-    "envoy_cc_library",
-    "envoy_package",
-)
-
 licenses(["notice"])  # Apache 2
 
-envoy_package()
-
-envoy_cc_library(
+cc_library(
     name = "main_entry_lib",
     srcs = ["main.cc"],
     deps = [
         "privileged_service_server_lib",
     ],
-    repository = "@envoy",
+    visibility = ["//visibility:public"],
 )
 
-envoy_cc_library(
+cc_library(
     name = "privileged_service_protocol",
     srcs = ["privileged_service_protocol.cc"],
     hdrs = ["privileged_service_protocol.h"],
-    repository = "@envoy",
-    deps = [
-        "@envoy//envoy/api:os_sys_calls_interface",
-    ],
+    visibility = ["//visibility:public"],
 )
 
-envoy_cc_library(
+cc_library(
     name = "privileged_service_server_lib",
     hdrs = ["privileged_service_server.h"],
     srcs = ["privileged_service_server.cc"],
-    repository = "@envoy",
-    deps = [
-        "privileged_service_protocol",
-    ],
-)
-
-envoy_cc_library(
-    name = "privileged_service_client_lib",
-    hdrs = ["privileged_service_client.h"],
-    srcs = ["privileged_service_client.cc"],
-    repository = "@envoy",
     deps = [
         "privileged_service_protocol",
-        "@envoy//source/common/singleton:threadsafe_singleton",
     ],
 )

starter/main.cc

@@ -2,10 +2,14 @@
 #error "Linux platform file is part of non-Linux build."
 #endif
 
+#include <errno.h>
 #include <syscall.h>
-#include <sys/prctl.h>
 #include <unistd.h>
 
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
 #include "starter/privileged_service_server.h"
 
 // NOLINT(namespace-envoy)

starter/privileged_service_protocol.cc

@@ -2,6 +2,8 @@
 #error "Linux platform file is part of non-Linux build."
 #endif
 
+#include <errno.h>
+
 #include <sys/unistd.h>
 #include <sys/syscall.h>
 

starter/privileged_service_protocol.h

@@ -5,12 +5,14 @@
 #endif
 
 #include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
 
 #include <linux/capability.h>
+#include <sys/socket.h>
 #include <sys/types.h>
 
-#include "envoy/api/os_sys_calls_common.h"
-
 // Use Envoy version of this if defined, otherwise roll a simple stderr one without further
 // dependencies
 #ifndef RELEASE_ASSERT
@@ -82,7 +84,8 @@ struct Response {
   Response() : hdr_(TYPE_RESPONSE) {}
 
   struct MessageHeader hdr_;
-  Envoy::Api::SysCallIntResult ret_;
+  int return_value_;
+  int errno_;
   uint8_t data_[];
 };
 

starter/privileged_service_server.cc

@@ -2,10 +2,16 @@
 #error "Linux platform file is part of non-Linux build."
 #endif
 
+#include <algorithm>
+
+#include <errno.h>
 #include <syscall.h>
 #include <string.h>
 #include <unistd.h>
 
+#include <sys/types.h>
+#include <sys/wait.h>
+
 #include <linux/bpf.h>
 
 #include "starter/privileged_service_server.h"
@@ -123,11 +129,13 @@ void ProtocolServer::serve() {
     // Form the response in place
     msg.response.hdr_.msg_type_ = TYPE_RESPONSE;
     if (fd_out != -1) {
-      // Pass a poitive but invalid fd in ret_, to be replaced with the passed
+      // Pass a poitive but invalid fd in return_value_, to be replaced with the passed
       // fd by the receiver.
-      msg.response.ret_ = {INT_MAX, 0};
+      msg.response.return_value_ = INT_MAX;
+      msg.response.errno_ = 0;
     } else {
-      msg.response.ret_ = {rc, rc != -1 ? 0 : errno};
+      msg.response.return_value_ = rc;
+      msg.response.errno_ = rc != -1 ? 0 : errno;
     }
     size = send_fd_msg(&msg, sizeof(msg.response), buf, value_len, fd_out);
     if (size < ssize_t(sizeof(msg.response) + value_len)) {

starter/privileged_service_server.h

@@ -4,6 +4,8 @@
 #error "Linux platform file is part of non-Linux build."
 #endif
 
+#include <limits.h>
+
 #include "starter/privileged_service_protocol.h"
 
 namespace Envoy {