WIP: multiple changes by ivanitskiy · Pull Request #43 · nginx/ngx-rust

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


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


Does this need rebasing from the commits yesterday?

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


+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

 * `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

-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

 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

-        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

-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

 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

-
+    // 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

 /// 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

    }
-    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

+        // 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

+            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

+        });
+    }
+
+    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

+        }
+    }
+
+    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

ivanitskiy closed this May 10, 2025

Conversation

ivanitskiy commented Aug 28, 2023

Proposed changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanitskiy Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ivanitskiy Aug 29, 2023 •

edited

Loading