[GitHub] [trafficserver] cmcfarlen commented on a diff in pull request #9845: Extract apply_ip_allow_filter

["cmcfarlen (via GitHub)" <gi...@apache.org>]

cmcfarlen commented on code in PR #9845:
URL: https://github.com/apache/trafficserver/pull/9845#discussion_r1229964012


##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,68 @@ HttpSM::get_outbound_sni() const
   return zret;
 }
 
+bool
+HttpSM::apply_ip_allow_filter()
+{
+  // Method allowed on dest IP address check
+  IpAllow::ACL acl = IpAllow::match(server_ip(), IpAllow::DST_ADDR);
+
+  if (ip_allow_is_request_forbidden(acl)) {
+    ip_allow_deny_request(acl);
+    return false;
+  }
+
+  if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+      t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+    call_transact_and_set_next_state(HttpTransact::OriginDown);
+    return false;
+  }
+
+  return true;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+  if (acl.isValid()) {
+    if (acl.isDenyAll()) {
+      return true;

Review Comment:
   IMO, style-wise, I like to see a result variable with the default and then a single return after calculating the answer.



##########
proxy/http/HttpSM.cc:
##########
@@ -5258,6 +5258,68 @@ HttpSM::get_outbound_sni() const
   return zret;
 }
 
+bool
+HttpSM::apply_ip_allow_filter()
+{
+  // Method allowed on dest IP address check
+  IpAllow::ACL acl = IpAllow::match(server_ip(), IpAllow::DST_ADDR);
+
+  if (ip_allow_is_request_forbidden(acl)) {
+    ip_allow_deny_request(acl);
+    return false;
+  }
+
+  if (HttpTransact::is_server_negative_cached(&t_state) == true &&
+      t_state.txn_conf->connect_attempts_max_retries_down_server <= 0) {
+    call_transact_and_set_next_state(HttpTransact::OriginDown);
+    return false;
+  }
+
+  return true;
+}
+
+bool
+HttpSM::ip_allow_is_request_forbidden(const IpAllow::ACL &acl)
+{
+  if (acl.isValid()) {
+    if (acl.isDenyAll()) {
+      return true;
+    } else if (!acl.isAllowAll()) {
+      if (method() != -1) {
+        return !acl.isMethodAllowed(method());
+      } else {
+        int method_str_len{};
+        auto method_str = t_state.hdr_info.server_request.method_get(&method_str_len);
+        return !acl.isNonstandardMethodAllowed(std::string_view(method_str, method_str_len));
+      }
+    }
+  }
+
+  return false;
+}
+
+void
+HttpSM::ip_allow_deny_request(const IpAllow::ACL &acl)
+{
+  if (is_debug_tag_set("ip_allow")) {
+    ip_text_buffer ipb;
+    const char *method_str;
+    int method_str_len{};
+    if (method() != -1) {
+      method_str     = hdrtoken_index_to_wks(method());
+      method_str_len = strlen(method_str);
+    } else {
+      method_str = t_state.hdr_info.client_request.method_get(&method_str_len);
+    }
+    Warning("server '%s' prohibited by ip-allow policy at line %d", ats_ip_ntop(server_ip(), ipb, sizeof(ipb)), acl.source_line());
+    SMDebug("ip_allow", "Line %d denial for '%.*s' from %s", acl.source_line(), method_str_len, method_str,
+            ats_ip_ntop(server_ip(), ipb, sizeof(ipb)));

Review Comment:
   Since you are cleaning this up you could pull the `ats_ip_ntop` call out here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org