policy: Change abort to an error log

[ upstream commit 7b1dc6c ] Log an error instead of crashing when Cilium NetworkPolicy resources are destructed in a worker thread. Remove stacktrace that is not useful at all with release builds. Prior to enabling use of SDS secrets running NetworkPolicy destructors in a worker thread did not cause visible problems, even though we had taken measures to not do that a long time ago. Now that references to the policy have been removed from the connection metadata (Cilium SocketOption) this should no longer trigger. Nonetheless, crashing Envoy is too drastic as the event may be survivable (as it apparently has been for a long time when running without SDS references in the policy). Enable trace level logging for troubleshooting if this error ever occurs. Signed-off-by: Jarno Rajahalme <[email protected]> Signed-off-by: Tam Mach <[email protected]>
cilium · Dec 3, 2024 · e8071c1 · e8071c1
1 parent 161656a
commit e8071c1
Show file tree

Hide file tree

Showing 2 changed files with 246 additions and 6 deletions.
diff --git a/cilium/network_policy.cc b/cilium/network_policy.cc
@@ -608,12 +608,8 @@ class PortNetworkPolicyRules : public Logger::Loggable<Logger::Id::config> {
 
   ~PortNetworkPolicyRules() {
     if (!Thread::MainThread::isMainOrTestThread()) {
-      ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::envoy_bug), error,
-                          "envoy bug failure: !Thread::MainThread::isMainOrTestThread()");
-      Envoy::Assert::EnvoyBugStackTrace st;
-      st.capture();
-      st.logStackTrace();
-      ::abort();
+      ENVOY_LOG(error, "PortNetworkPolicyRules: Destructor executing in a worker thread, while "
+                       "only main thread should destruct xDS resources");
     }
   }
 

diff --git a/tests/cilium_network_policy_test.cc b/tests/cilium_network_policy_test.cc
@@ -875,6 +875,250 @@ TEST_F(CiliumNetworkPolicyTest, HttpPolicyUpdate) {
   EXPECT_FALSE(EgressAllowed("10.1.2.3", 43, 80, {{":path", "/publicz"}}));
 }
 
+TEST_F(CiliumNetworkPolicyTest, HttpOverlappingPortRanges) {
+  std::string version;
+  EXPECT_NO_THROW(version = updateFromYaml(R"EOF(version_info: "0"
+)EOF"));
+  EXPECT_EQ(version, "0");
+  EXPECT_FALSE(policy_map_->exists("10.1.2.3"));
+  // No policy for the pod
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 80, {{":path", "/allowed"}}));
+
+  // 1st update
+  EXPECT_NO_THROW(version = updateFromYaml(R"EOF(version_info: "1"
+resources:
+- "@type": type.googleapis.com/cilium.NetworkPolicy
+  endpoint_ips:
+  - "10.1.2.3"
+  endpoint_id: 42
+  ingress_per_port_policies:
+  - port: 80
+    rules:
+    - remote_policies: [ 43 ]
+      http_rules:
+        http_rules:
+        - headers:
+          - name: ':path'
+            exact_match: '/allowed'
+  - port: 80
+    rules:
+    - remote_policies: [ 43 ]
+      http_rules:
+        http_rules:
+        - headers:
+          - name: ':method'
+            exact_match: 'GET'
+)EOF"));
+  EXPECT_EQ(version, "1");
+  EXPECT_TRUE(policy_map_->exists("10.1.2.3"));
+
+  std::string expected = R"EOF(ingress:
+  rules:
+    [80-80]:
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":method"
+            value: "GET"
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":path"
+            value: "/allowed"
+  wildcard_rules: []
+egress:
+  rules: []
+  wildcard_rules: []
+)EOF";
+
+  EXPECT_TRUE(Validate("10.1.2.3", expected));
+
+  // Allowed remote ID, port, & method OR path:
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 80, {{":method", "PUSH"}, {":path", "/allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 80, {{":method", "GET"}, {":path", "/also_allowed"}}));
+  // Wrong remote ID:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 40, 80, {{":path", "/allowed"}}));
+  // Wrong port:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 8080, {{":path", "/allowed"}}));
+  // Wrong path:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 80, {{":path", "/notallowed"}}));
+
+  // No egress is allowed:
+  EXPECT_FALSE(EgressAllowed("10.1.2.3", 43, 80, {{":path", "/public"}}));
+
+  // 2nd update with overlapping port range and a single port
+  EXPECT_NO_THROW(version = updateFromYaml(R"EOF(version_info: "2"
+resources:
+- "@type": type.googleapis.com/cilium.NetworkPolicy
+  endpoint_ips:
+  - "10.1.2.3"
+  endpoint_id: 42
+  ingress_per_port_policies:
+  - port: 70
+    end_port: 90
+    rules:
+    - remote_policies: [ 43 ]
+      http_rules:
+        http_rules:
+        - headers:
+          - name: ':path'
+            exact_match: '/allowed'
+  - port: 80
+    rules:
+    - remote_policies: [ 43 ]
+      http_rules:
+        http_rules:
+        - headers:
+          - name: ':method'
+            exact_match: 'GET'
+)EOF"));
+  EXPECT_EQ(version, "2");
+  EXPECT_TRUE(policy_map_->exists("10.1.2.3"));
+
+  expected = R"EOF(ingress:
+  rules:
+    [70-79]:
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":path"
+            value: "/allowed"
+    [80-80]:
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":method"
+            value: "GET"
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":path"
+            value: "/allowed"
+    [81-90]:
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":path"
+            value: "/allowed"
+  wildcard_rules: []
+egress:
+  rules: []
+  wildcard_rules: []
+)EOF";
+
+  EXPECT_TRUE(Validate("10.1.2.3", expected));
+
+  // Allowed remote ID, port, & method OR path:
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 70, {{":method", "PUSH"}, {":path", "/allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 80, {{":method", "PUSH"}, {":path", "/allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 90, {{":method", "PUSH"}, {":path", "/allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 80, {{":method", "GET"}, {":path", "/also_allowed"}}));
+  // wrong port for GET
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 70, {{":method", "GET"}, {":path", "/also_allowed"}}));
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 90, {{":method", "GET"}, {":path", "/also_allowed"}}));
+  // Wrong remote ID:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 40, 80, {{":path", "/allowed"}}));
+  // Wrong port:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 8080, {{":path", "/allowed"}}));
+  // Wrong path:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 80, {{":path", "/notallowed"}}));
+
+  // No egress is allowed:
+  EXPECT_FALSE(EgressAllowed("10.1.2.3", 43, 80, {{":path", "/public"}}));
+
+  // 3rd update with overlapping port ranges
+  EXPECT_NO_THROW(version = updateFromYaml(R"EOF(version_info: "2"
+resources:
+- "@type": type.googleapis.com/cilium.NetworkPolicy
+  endpoint_ips:
+  - "10.1.2.3"
+  endpoint_id: 42
+  ingress_per_port_policies:
+  - port: 70
+    end_port: 90
+    rules:
+    - remote_policies: [ 43 ]
+      http_rules:
+        http_rules:
+        - headers:
+          - name: ':path'
+            exact_match: '/allowed'
+  - port: 80
+    end_port: 8080
+    rules:
+    - remote_policies: [ 43 ]
+      http_rules:
+        http_rules:
+        - headers:
+          - name: ':method'
+            exact_match: 'GET'
+)EOF"));
+  EXPECT_EQ(version, "2");
+  EXPECT_TRUE(policy_map_->exists("10.1.2.3"));
+
+  expected = R"EOF(ingress:
+  rules:
+    [70-79]:
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":path"
+            value: "/allowed"
+    [80-90]:
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":path"
+            value: "/allowed"
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":method"
+            value: "GET"
+    [91-8080]:
+    - rules:
+      - remotes: [43]
+        http_rules:
+        - headers:
+          - name: ":method"
+            value: "GET"
+  wildcard_rules: []
+egress:
+  rules: []
+  wildcard_rules: []
+)EOF";
+
+  EXPECT_TRUE(Validate("10.1.2.3", expected));
+
+  // Allowed remote ID, port, & method OR path:
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 70, {{":method", "PUSH"}, {":path", "/allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 80, {{":method", "PUSH"}, {":path", "/allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 90, {{":method", "PUSH"}, {":path", "/allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 80, {{":method", "GET"}, {":path", "/also_allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 90, {{":method", "GET"}, {":path", "/also_allowed"}}));
+  EXPECT_TRUE(IngressAllowed("10.1.2.3", 43, 8080, {{":method", "GET"}, {":path", "/also_allowed"}}));
+  // wrong port for GET
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 70, {{":method", "GET"}, {":path", "/also_allowed"}}));
+  // Wrong remote ID:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 40, 80, {{":path", "/allowed"}}));
+  // Wrong port:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 8080, {{":path", "/allowed"}}));
+  // Wrong path:
+  EXPECT_FALSE(IngressAllowed("10.1.2.3", 43, 80, {{":path", "/notallowed"}}));
+
+  // No egress is allowed:
+  EXPECT_FALSE(EgressAllowed("10.1.2.3", 43, 80, {{":path", "/public"}}));
+}
+
 TEST_F(CiliumNetworkPolicyTest, TcpPolicyUpdate) {
   std::string version;
   EXPECT_NO_THROW(version = updateFromYaml(R"EOF(version_info: "0"