WIP: multiple changes #43

ivanitskiy · 2023-08-28T15:34:02Z

Proposed changes

This is a temporary PR to share changes. I intend to break it down into a few separated PRs

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have written my commit messages in the Conventional Commits format.
I have read the CONTRIBUTING doc
I have added tests (when possible) that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto master
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

f5yacobucci · 2023-08-29T15:14:32Z

Cargo.toml

@@ -6,7 +6,7 @@ members = [

 [package]
 name = "ngx"
-version = "0.3.0-beta"


Does this need rebasing from the commits yesterday?

f5yacobucci · 2023-08-29T15:16:19Z

README.md

@@ -14,23 +14,29 @@ dynamic modules completely in Rust.

 In short, this SDK allows writing NGINX modules using the Rust language.

+It contains the following Rust crates:
+ * [nginx-sys](./nginx-sys) - allows to use ngx_* C functions via FFI when implementing modules. The `-sys` is used to follow a [naming convention](https://doc.rust-lang.org/cargo/reference/build-scripts.html#-sys-packages) to link with a C library.
+ * [ngx](./src) - it an opinionated SDK to make it a bit easer to use [nginx-sys](./nginx-sys) crate. Implements `macro_rules`, provides a way to build a dynamic module without any C code (see `ngx_modules!` macro_rule).


it an opinionated SDK

f5yacobucci · 2023-08-29T15:17:41Z

README.md

 * `PCRE2_VERSION` (default 10.42)
 * `OPENSSL_VERSION` (default 3.0.7)
 * `NGX_VERSION` (default 1.23.3) - NGINX OSS version
 * `NGX_DEBUG` (default to false)-  if set to true, then will compile NGINX `--with-debug` option
+* `NGX_SRC_DIR` (default not set) - When the value is set, then use this NGINX source code folder to build bindings from
+* `NGX_CONFIGURE_ARGS` (default not set) - When the value is set, then run the NGINX configure script with


Suggest: When the value is set, use these arguments with the NGINX configure script

f5yacobucci · 2023-08-29T15:22:47Z

README.md

-This SDK is currently unstable. Right now, our primary goal is collect feedback and stabilize it be before
-offering support. Feel free [contributing](CONTRIBUTING.md) by creating issues, PRs, or github discussions.
+This SDK is currently unstable. Right now, our primary goal is to collect feedback and stabilize it
+before offering support. Feel free to [contribute](CONTRIBUTING.md) by creating issues, PRs, or GitHub discussions.


Do you know if we link to the community slack anywhere? That would be a convenient way for people to ask question.

f5yacobucci · 2023-08-29T15:24:47Z

examples/README.md

@@ -28,7 +28,7 @@ cargo build --package=examples --examples

 This module demonstrates how to create a minimal dynamic module with `http_request_handler`, that checks for User-Agent headers and returns status code 403 if UA starts with `curl`, if a module is disabled then uses `core::Status::NGX_DECLINED` to indicate the operation is rejected, for example, because it is disabled in the configuration (`curl off`). Additionally, it demonstrates how to write a defective parser.

-An example of nginx configuration file that uses that module can be found at [curl.conf](./curl.conf).
+An example of an Nginx configuration file that uses that module can be found at [curl.conf](./curl.conf).


I haven't been checking these elsewhere, but I'm pretty sure the convention is to all-caps NGINX.

I'll confirm with Jodie.

Confirmed with Jodie, NGINX should be all capitalized according to our style guide.

f5yacobucci · 2023-08-29T15:31:26Z

examples/awssig.rs

-        ngx_log_debug_http!(request, "headers_in  {}: {}", name, value);
+    for mut h in request.headers_in_iterator() {
+        ngx_log_debug_http!(request, "headers_in  {}", h);
+        if h.lowercase_key() == Some("foo") {


Is this testing code? What's the 'foo' header in this context?

f5yacobucci · 2023-08-29T15:39:37Z

examples/curl.rs

-use ngx::{core, core::Status, http, http::HTTPModule};
-use ngx::{http_request_handler, ngx_log_debug_http, ngx_modules, ngx_null_command, ngx_string};
+use ngx::{core, core::Status, http, http::HTTPModule, http::MergeConfigError};
+use ngx::{http_request_handler, ngx_log_debug, ngx_modules, ngx_null_command, ngx_string};


This is really just stylistic, but why not all in one:

use ngx::{core, core::Status, http, http::{HTTPModule, MergeConfigError}, http_request_handler, ngx_log_debug, ngx_modules, ngx_null_command, ngx_string};

f5yacobucci · 2023-08-29T15:45:45Z

examples/curl.rs

@@ -104,18 +104,30 @@ impl http::Merge for ModuleConfig {
 }

 http_request_handler!(curl_access_handler, |request: &mut http::Request| {
+    // we can check if a request is internal and disable handler
+    let log = request.log();
+    ngx_log_debug!(log, "is internal: {}", request.is_internal());


When this is printed will there be enough context for a user to know what's internal? Is there a convention around module identification for context?

f5yacobucci · 2023-08-29T15:47:27Z

examples/curl.rs

-
+    // check if module is enabled based on the location config
+    ngx_log_debug!(log, "curl module enabled: {}", co.enable);
+    if request.is_internal() {


I think we'll have already bailed out at line 111.

f5yacobucci · 2023-08-29T17:19:17Z

nginx-sys/build.rs

@@ -89,14 +90,14 @@ const ENV_VARS_TRIGGERING_RECOMPILE: [&str; 9] = [
 /// Function invoked when `cargo build` is executed.
 /// This function will download NGINX and all supporting dependencies, verify their integrity,
 /// extract them, execute autoconf `configure` for NGINX, compile NGINX and finally install
-/// NGINX in a subdirectory with the project.
+/// NGINX in a subdirectory with the project or use provided NGNINX source path


NGNINX -> NGINX

f5yacobucci · 2023-08-29T17:48:44Z

nginx-sys/build.rs

    }
-    Ok((nginx_install_dir, nginx_src_dir.to_owned()))
+
+    // TODO: a problem why we need to ran build - openssl needs to be configured if it is used,


Is this, or should this, be filed as a github issue?

f5yacobucci · 2023-08-29T17:56:36Z

src/core/string.rs

+        // as the lifetime of NgxStr is properly managed, this should be safe.
+        unsafe {
+            // let slice = std::slice::from_raw_parts(self.0.data, self.0.len);
+            std::str::from_utf8_unchecked(&self.0)


Can we be guaranteed this is valid UTF-8? to_str is using the checked version now.

f5yacobucci · 2023-08-29T20:21:21Z

src/http/headers.rs

+            table.value.data = crate::ffi::str_to_uchar(self.1, value);
+        });
+
+        // Alternative way is using CString and transfer ownership.


Any speculation which one might be more efficient?

I'm wagering CString will put memory management under Rust semantics and reference counting. Where pool will allocate from nginx, could this mean it gets held longer in the pool?

f5yacobucci · 2023-08-29T20:40:53Z

src/http/headers.rs

+        });
+    }
+
+    pub fn set_hash(&mut self, hash: ngx_uint_t) {


Overall comment, we'll need doc strings for the API. I don't get what this is doing, is it setting the hash algorithm? Is hash a closed set of values, if so, an enum could be more expressive.

This is magic from the C guide:

from https://nginx.org/en/docs/dev/development_guide.html#list

For example, to mark HTTP output headers (which are stored as ngx_table_elt_t objects) as missing, set the hash field in ngx_table_elt_t to zero. Items marked in this way are explicitly skipped when the headers are iterated over.

it is trick to "delete" header (by marking hash =0 for that element)

basically set_key, set_hash, set_value mimic the NGINX's API.

f5yacobucci · 2023-08-29T20:44:17Z

src/http/headers.rs

+        }
+    }
+
+    pub fn key(&self) -> &str {


This looks like a collections (map) API. Is there a collection trait we can implement for a consistent Rust idiomatic API?

Does this follow NGINX patterns?

I think this should either be clearly recognizable as a Rust set of functions or an NGINX set of functions, my opinion leans more towards something Rust-like, especially if there's a trait.

there is no trait in Rust. there is pub struct HeaderMap<T = HeaderValue> { /* private fields */ } which is widely used, here is:
https://docs.rs/http/latest/http/header/struct.HeaderMap.html#

Unfortunately, we are constrained with NGINX types and implementations.

so I'm following Nginx way. in Rust's Header is an abstraction that encapsulates table_elt and pool: pub struct Header(*mut ngx_table_elt_t, *mut ngx_pool_t); and it represents a single Header (a pair of key and value).
in nginx you don't remove headers, you set hash to 0 ( tl;dr - headers are lists of lists ).

the next thing is to have a HeaderMap (constructed from headers_in.headers or headers_out.headers which has a set of methods similar to hyperium'shttp crate.

Anyway, seems like I don't like what I've done here at all too. if you have some ideas please share

Allows providing NGX_SRC_DIR, NGX_CONFIGURE_ARGS via env variables. This enables to run bingen against provided NGINX source code and use non-default arguments for configure script

ivanitskiy force-pushed the wip branch from a49e275 to f90a298 Compare August 28, 2023 18:57

f5yacobucci reviewed Aug 29, 2023

View reviewed changes

ivanitskiy force-pushed the wip branch 3 times, most recently from a439847 to d4aef0f Compare August 31, 2023 00:04

ivanitskiy added 10 commits September 19, 2023 15:31

doc: updated docker command to use named container

5a5ff5e

fix: fixed curl module nginx config example

5754568

ci: address some clippy warnings

1931ef2

feat: add cmp_ignore_case_utf8 and as_str for NgxStr

6ef3eb4

feat: is_internal method for request

413aca4

feat!: owned header iterator

3490d39

update default zlib version to 1.3

8b5e172

feat!: support NGX_SRC_DIR and NGX_CONFIGURE_ARGS via env vars

7809674

Allows providing NGX_SRC_DIR, NGX_CONFIGURE_ARGS via env variables. This enables to run bingen against provided NGINX source code and use non-default arguments for configure script

chore: run cargo clean insted of rm -fr target

cc4a5ba

ci: Bump nginx-sys to 0.3.0 and ngx to 0.5.0

8ceb8b6

ivanitskiy force-pushed the wip branch from d4aef0f to 8ceb8b6 Compare September 19, 2023 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: multiple changes #43

WIP: multiple changes #43

ivanitskiy commented Aug 28, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

f5yacobucci Aug 29, 2023

ivanitskiy Oct 18, 2023

ivanitskiy Oct 18, 2023

f5yacobucci Aug 29, 2023

ivanitskiy Aug 29, 2023 •

edited

Loading

WIP: multiple changes #43

Are you sure you want to change the base?

WIP: multiple changes #43

Conversation

ivanitskiy commented Aug 28, 2023

Proposed changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanitskiy Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

ivanitskiy Aug 29, 2023 •

edited

Loading