Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: multiple changes #43

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

WIP: multiple changes #43

wants to merge 10 commits into from

Conversation

ivanitskiy
Copy link
Contributor

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

Cargo.toml Outdated
@@ -6,7 +6,7 @@ members = [

[package]
name = "ngx"
version = "0.3.0-beta"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need rebasing from the commits yesterday?

@@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it an opinionated SDK

* `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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I'll confirm with Jodie.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NGNINX -> NGINX

}
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

table.value.data = crate::ffi::str_to_uchar(self.1, value);
});

// Alternative way is using CString and transfer ownership.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

});
}

pub fn set_hash(&mut self, hash: ngx_uint_t) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

pub fn key(&self) -> &str {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ivanitskiy ivanitskiy Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ivanitskiy ivanitskiy force-pushed the wip branch 3 times, most recently from a439847 to d4aef0f Compare August 31, 2023 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants