build: support clang tidy

[jrajahalme]

Add new clang-tidy make targets tidy and tidy-fix. Rename existing clang-format targets from check and fix to format and format-fix, respectively.

Add a new GH workflow to run clang-tidy on changed files.
Bazel build options are passed into gen_compilation_database.py so that the analysis cache is not invalidated between make tidy and make tests targets, for example.

.clang-tidy is copied from upstream Envoy, with addition of misc-include-cleaner IgnoreHeaders options adapted from .clangd.

By default make tidy will tidy up all sources in tests and cilium directories, which takes a long time. Defining the variable TIDY_SOURCES can be used to specify a subset, e.g.,:

$ TIDY_SOURCES=cilium/network_policy.cc make tidy

Please note that make tidy-fix is prone to producing duplicated includes and non-compiling code, so all edits made with it must be manually inspected.

For the tidy targets to work, the host must have the package clang-tidy-17 installed.

.clang-tidy is copied from upstream Envoy with the addition of linting local variables to lower_case, which found out a few places where the "member variable suffix" _ was used on a local variable, in addition to some camelCase local variables that are now changed to lower_case.

[mhofstetter]

Nice, thanks a lot for this Jarno! Left some nits inline.

1. What about linting and fixing the files in starter too? There seem to be a lot of issues left.
2. Going over the code base with your changes applied, i discovered the following additional changes that might be worth to change. Maybe you want to apply them.

diff --git a/cilium/accesslog.h b/cilium/accesslog.h
index 2dd4af8f..b744720a 100644
--- a/cilium/accesslog.h
+++ b/cilium/accesslog.h
@@ -53,7 +53,7 @@ public:
     void addRejected(absl::string_view key, absl::string_view value);
     void addMissing(absl::string_view key, absl::string_view value);
 
-    ::cilium::LogEntry entry_{};
+    ::cilium::LogEntry entry_;
     bool request_logged_ = false;
   };
 
diff --git a/cilium/bpf_metadata.h b/cilium/bpf_metadata.h
index 277359af..42dc36b8 100644
--- a/cilium/bpf_metadata.h
+++ b/cilium/bpf_metadata.h
@@ -149,10 +149,10 @@ public:
   Network::Address::InstanceConstSharedPtr ipv6_source_address_;
   bool enforce_policy_on_l7lb_;
 
-  std::shared_ptr<const Cilium::NetworkPolicyMap> npmap_{};
-  Cilium::CtMapSharedPtr ct_maps_{};
-  Cilium::IPCacheSharedPtr ipcache_{};
-  std::shared_ptr<const Cilium::PolicyHostMap> hosts_{};
+  std::shared_ptr<const Cilium::NetworkPolicyMap> npmap_;
+  Cilium::CtMapSharedPtr ct_maps_;
+  Cilium::IPCacheSharedPtr ipcache_;
+  std::shared_ptr<const Cilium::PolicyHostMap> hosts_;
 
 private:
   uint32_t resolveSourceIdentity(const PolicyInstance& policy, const Network::Address::Ip* sip,
diff --git a/cilium/network_filter.h b/cilium/network_filter.h
index 0add9f96..a27003dd 100644
--- a/cilium/network_filter.h
+++ b/cilium/network_filter.h
@@ -66,10 +66,10 @@ private:
   Network::ReadFilterCallbacks* callbacks_ = nullptr;
   uint32_t remote_id_ = 0;
   uint16_t destination_port_ = 0;
-  std::string l7proto_{};
+  std::string l7proto_;
   bool should_buffer_ = false;
   Buffer::OwnedImpl buffer_; // Buffer for initial connection data
-  Cilium::GoFilter::InstancePtr go_parser_{};
+  Cilium::GoFilter::InstancePtr go_parser_;
   Cilium::AccessLog::Entry log_entry_{};
 };
 
diff --git a/cilium/network_policy.cc b/cilium/network_policy.cc
index a4c7e095..0a6e0739 100644
--- a/cilium/network_policy.cc
+++ b/cilium/network_policy.cc
@@ -619,7 +619,7 @@ public:
   // Use std::less<> to allow heterogeneous lookups (with string_view).
   std::set<std::string, std::less<>> allowed_snis_; // All SNIs allowed if empty.
   std::vector<HttpNetworkPolicyRule> http_rules_; // Allowed if empty, but remote is checked first.
-  std::string l7_proto_{};
+  std::string l7_proto_;
   std::vector<L7NetworkPolicyRule> l7_allow_rules_;
   std::vector<L7NetworkPolicyRule> l7_deny_rules_;
 };
@@ -1077,7 +1077,7 @@ public:
   }
 
   PolicyMap rules_;
-  RulesList wildcard_rules_{};
+  RulesList wildcard_rules_;
 };
 
 // Construction is single-threaded, but all other use is from multiple worker threads using const
@@ -1363,7 +1363,7 @@ void NetworkPolicyMap::runAfterAllThreads(std::function<void()> cb) const {
   //
   // For now we rely on the implementation dependent fact that the reference returned by
   // context_.threadLocal() actually is a ThreadLocal::Instance reference, where
-  // runOnAllWorkerThreads() is exposed. Withtout this cast we'd need to use a dummy thread local
+  // runOnAllWorkerThreads() is exposed. Without this cast we'd need to use a dummy thread local
   // variable that would take a thread local slot for no other purpose than to avoid this type cast.
   dynamic_cast<ThreadLocal::Instance&>(context_.threadLocal()).runOnAllWorkerThreads([]() {}, cb);
 }
diff --git a/cilium/network_policy.h b/cilium/network_policy.h
index a19405bc..6c1a2baa 100644
--- a/cilium/network_policy.h
+++ b/cilium/network_policy.h
@@ -129,8 +129,8 @@ public:
       : ipv4_(ipv4), ipv6_(ipv6){};
   IPAddressPair(const cilium::NetworkPolicy& proto);
 
-  Network::Address::InstanceConstSharedPtr ipv4_{};
-  Network::Address::InstanceConstSharedPtr ipv6_{};
+  Network::Address::InstanceConstSharedPtr ipv4_;
+  Network::Address::InstanceConstSharedPtr ipv6_;
 };
 
 class PolicyInstance {
@@ -287,7 +287,7 @@ private:
   //   written, and emits CPU instructions to also make the CPU out-of-order-execution logic to not
   //   reorder any write operations to happen after the pointer itself is written. This guarantees
   //   that the map is not modified after the point when the worker threads can observe the new
-  //   pointer value, i.e., the map is actaully immutable (const) from that point forward.
+  //   pointer value, i.e., the map is actually immutable (const) from that point forward.
   // - when the pointer is read (by a worker thread) 'std::memory_order_acquire' in the load
   //   operation informs the compiler to emit CPU instructions to make the CPU
   //   out-of-order-execution logic to not reorder any reads from the new map to happen before the
diff --git a/cilium/secret_watcher.h b/cilium/secret_watcher.h
index a0811ee8..474251b8 100644
--- a/cilium/secret_watcher.h
+++ b/cilium/secret_watcher.h
@@ -81,7 +81,7 @@ public:
 private:
   Ssl::ServerContextConfigPtr server_config_;
   std::vector<std::string> server_names_;
-  Ssl::ServerContextSharedPtr server_context_ ABSL_GUARDED_BY(ssl_context_mutex_){};
+  Ssl::ServerContextSharedPtr server_context_ ABSL_GUARDED_BY(ssl_context_mutex_);
 };
 using DownstreamTLSContextPtr = std::unique_ptr<DownstreamTLSContext>;
 
@@ -98,7 +98,7 @@ public:
 
 private:
   Ssl::ClientContextConfigPtr client_config_;
-  Ssl::ClientContextSharedPtr client_context_ ABSL_GUARDED_BY(ssl_context_mutex_){};
+  Ssl::ClientContextSharedPtr client_context_ ABSL_GUARDED_BY(ssl_context_mutex_);
 };
 using UpstreamTLSContextPtr = std::unique_ptr<UpstreamTLSContext>;
 
diff --git a/cilium/websocket_codec.h b/cilium/websocket_codec.h
index 67d3f3ba..817940d5 100644
--- a/cilium/websocket_codec.h
+++ b/cilium/websocket_codec.h
@@ -63,7 +63,7 @@ private:
 
     Codec& parent_;
     bool end_stream_{false};
-    Buffer::OwnedImpl encoded_{}; // Buffer for encoded websocket frames
+    Buffer::OwnedImpl encoded_; // Buffer for encoded websocket frames
   };
 
   class Decoder : Logger::Loggable<Logger::Id::filter> {
@@ -79,8 +79,8 @@ private:
 
     Codec& parent_;
     bool end_stream_{false};
-    Buffer::OwnedImpl buffer_{};  // Buffer for partial websocket frames
-    Buffer::OwnedImpl decoded_{}; // Buffer for decoded websocket frames
+    Buffer::OwnedImpl buffer_;  // Buffer for partial websocket frames
+    Buffer::OwnedImpl decoded_; // Buffer for decoded websocket frames
 
     bool unmasking_{false};
     uint8_t mask_[4];
@@ -124,7 +124,7 @@ private:
   uint64_t ping_count_{0};
 
   Event::TimerPtr handshake_timer_{nullptr};
-  Buffer::OwnedImpl handshake_buffer_{};
+  Buffer::OwnedImpl handshake_buffer_;
   bool accepted_{false};
 };
 using CodecPtr = std::unique_ptr<Codec>;
diff --git a/tests/cilium_tcp_integration_test.cc b/tests/cilium_tcp_integration_test.cc
index 24ca3817..1f7f0775 100644
--- a/tests/cilium_tcp_integration_test.cc
+++ b/tests/cilium_tcp_integration_test.cc
@@ -279,7 +279,7 @@ TEST_P(CiliumTcpProxyIntegrationTest, CiliumTcpProxyUpstreamFlushEnvoyExit) {
   ASSERT_TRUE(fake_upstream_connection->close());
   ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
 
-  // Success criteria is that no ASSERTs fire and there are no leaks.
+  // Success criteria is that no ASSERT's fire and there are no leaks.
 }
 
 //
diff --git a/tests/cilium_tls_tcp_integration_test.cc b/tests/cilium_tls_tcp_integration_test.cc
index 84052f13..7e7e58db 100644
--- a/tests/cilium_tls_tcp_integration_test.cc
+++ b/tests/cilium_tls_tcp_integration_test.cc
@@ -500,7 +500,7 @@ TEST_P(CiliumTLSProxyIntegrationTest, CiliumTLSProxyUpstreamFlushEnvoyExit) {
   ASSERT_TRUE(fake_upstream_connection->close());
   ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
 
-  // Success criteria is that no ASSERTs fire and there are no leaks.
+  // Success criteria is that no ASSERT's fire and there are no leaks.
 }
 
 //
diff --git a/tests/cilium_websocket_codec_integration_test.cc b/tests/cilium_websocket_codec_integration_test.cc
index b93f8702..adf51935 100644
--- a/tests/cilium_websocket_codec_integration_test.cc
+++ b/tests/cilium_websocket_codec_integration_test.cc
@@ -338,7 +338,7 @@ TEST_P(CiliumWebSocketIntegrationTest, CiliumWebSocketUpstreamFlushEnvoyExit) {
   ASSERT_TRUE(fake_upstream_connection->close());
   ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
 
-  // Success criteria is that no ASSERTs fire and there are no leaks.
+  // Success criteria is that no ASSERT's fire and there are no leaks.
 }
 
 } // namespace Envoy
diff --git a/tests/cilium_websocket_encap_integration_test.cc b/tests/cilium_websocket_encap_integration_test.cc
index 126a857a..fe4ff67b 100644
--- a/tests/cilium_websocket_encap_integration_test.cc
+++ b/tests/cilium_websocket_encap_integration_test.cc
@@ -555,7 +555,7 @@ TEST_P(CiliumWebSocketIntegrationTest, CiliumWebSocketUpstreamFlushEnvoyExit) {
   ASSERT_TRUE(fake_upstream_connection->close());
   ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
 
-  // Success criteria is that no ASSERTs fire and there are no leaks.
+  // Success criteria is that no ASSERT's fire and there are no leaks.
 }
 
 } // namespace Envoy

[mhofstetter]

rename struct to IPCacheKey? (getting Invalid case style for struct 'ipcache_key' (fix available))

[jrajahalme]

Are you getting this from clangd or from running make tidy?

Applied.

[mhofstetter]

clangd - but i assume that it now respects the clang-tidy config too

[mhofstetter]

this include is marked as unused in my IDE 🫤

[jrajahalme]

I think clang-tidy asked me to add these. If we add the clang tidy CI check, then we'll have to keep these.

We are using Http::CodecClient::Type::HTTP1, which is defined in source/common/http/codec_client.h as:

class CodecClient ... {
  using Type = Envoy::Http::CodecType;

So we are "using" the definition in envoy/http/codec.h.

[mhofstetter]

maybe we need a // IWYU pragma: keep here?

[mhofstetter]

same here - include marked as unused

[jrajahalme]

tested removing this, clang-tidy complains:

$ TIDY_SOURCES=tests/cilium_http_integration.cc make tidy
BUILDING on arm64 for arm64 using //bazel:linux_aarch64
Skip registering C++ toolchains via BAZEL_BUILD_OPTS. Local C++ toolchain needs to be available!
Using Docker Buildx builder "" with build flags "--output type=docker ".
BAZEL_STARTUP_OPTION_LIST="" BAZEL_BUILD_OPTION_LIST="--platforms=//bazel:linux_aarch64 --config=release" tools/gen_compilation_database.py --include_all //cilium/... //starter/... //tests/... @com_google_absl//absl/...
INFO: Analyzed 614 targets (0 packages loaded, 0 targets configured).
INFO: Found 614 targets...
INFO: Elapsed time: 0.375s, Critical Path: 0.01s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
run-clang-tidy-17 -quiet -extra-arg="-Wno-unknown-pragmas" -checks=misc-include-cleaner -j $(( 16205592 / 4500000 )) tests/cilium_http_integration.cc
clang-tidy-17 -checks=misc-include-cleaner -extra-arg=-Wno-unknown-pragmas -p=/Volumes/dev/cilium/proxy -quiet /home/jarnor.linux/.cache/bazel/_bazel_jarnor/ccfd0d975635a3b9fe8b2f331622c68a/execroot/cilium/tests/cilium_http_integration.cc
tests/cilium_http_integration.cc:27:52: error: no header providing "Envoy::Http::CodecType::HTTP1" is directly included [misc-include-cleaner,-warnings-as-errors]
   11 |     : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam(), config),
      |                                                    ^
72098 warnings generated.
144174 warnings generated.
make: *** [Makefile.dev:63: tidy] Error 1

[mhofstetter]

unused

[mhofstetter]

in this file i get an additional lint issue, maybe worth to fix?

payload_reader_.reset(p: new WaitForPayloadReader(&dispatcher: *dispatcher_));     ● clang-tidy: Use std::make_shared instead (fix available)

[jrajahalme]

reset only takes bare pointers, so maybe the proposed fix is to use assignment instead? I'll leave this for later, unless you have make tidy complaining about this?