Skip to content

ACME: module configuration. #1

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

Closed
wants to merge 7 commits into from
Closed

Conversation

bavshin-f5
Copy link
Member

Module configuration directives only, no other functionality.
See the README.md file for currently implemented directives.

@bavshin-f5 bavshin-f5 requested review from xeioex, avahahn and ensh63 July 16, 2025 07:02
@bavshin-f5 bavshin-f5 self-assigned this Jul 16, 2025
@bavshin-f5 bavshin-f5 added this to the 0.1.0 milestone Jul 16, 2025
@bavshin-f5 bavshin-f5 force-pushed the bavshin/initial-conf branch 9 times, most recently from ad7ceee to e6c8528 Compare July 21, 2025 18:23
@bavshin-f5
Copy link
Member Author

As Pavel suggested offline, changed syntax of the acme_certificate directive to acme_certificate issuer [identifier...] [key=...];, where an empty list of identifiers instructs the module to take the server_name values.

@bavshin-f5 bavshin-f5 force-pushed the bavshin/initial-conf branch 2 times, most recently from 19d4cbc to 9dbfc36 Compare July 21, 2025 19:03
Copy link

@avahahn avahahn left a comment

Choose a reason for hiding this comment

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

The code looks great, with just a few caveats.

src/conf.rs Outdated
conf: *mut c_void,
) -> *mut c_char {
let cf = unsafe { cf.as_mut().unwrap() };
let uri: &mut http::Uri = unsafe { &mut *conf.byte_offset((*cmd).offset as isize).cast() };
Copy link

Choose a reason for hiding this comment

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

In the nginx-wasm crate Pat used a static compile time assert to guarantee that manual access like that was correct before building. I think this crate's use of byte_offset also may call for something similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

The general idea of configuration slot handlers in NGINX is that we don't know the exact type of the configuration structure, but we know that the value at conf + specified offset is of the necessary type. It's on the end user to set the right value in ngx_command_t.offset.
We could try to design a Command builder for the high-level Rust API better, but we don't have that right now.

The generic code looks unnecessary in the current context though, so I will remove byte offsets and replace with direct access to the structure fields.

Copy link

Choose a reason for hiding this comment

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

In the ACME module we are the 'end user' of this API. My concern was whether or not we can validate that we are fetching the right data structure. If we can predict the offset and type size at build time then I think it is worth the assert, but otherwise there is nothing we can do.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only way to validate this that I can imagine is to generate the code to access the fields and never allow the user to interact with the offsets directly.
That would require proc-macros though, and IMO we aren't at the point where we should start introducing these yet.

Copy link

Choose a reason for hiding this comment

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

We can also generate code with the build script.
But if we can generalize it in the future with proc-macros, then maybe this is conversation for ngx-rust.... whenever you see proc-macros as ready.

@bavshin-f5 bavshin-f5 force-pushed the bavshin/initial-conf branch 2 times, most recently from 42e2bdb to d876fd9 Compare July 22, 2025 02:20
Copy link

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

Otherwise looks good.

@bavshin-f5 bavshin-f5 force-pushed the bavshin/initial-conf branch 2 times, most recently from ae81472 to c496119 Compare July 23, 2025 17:31
@xeioex
Copy link

xeioex commented Jul 23, 2025

diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index 773eaba..e7750c0 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -40,7 +40,7 @@ jobs:
           - master
           - stable-1.28
         build:
-          - debug 
+          - debug
           - debug-static
           - release
 
@@ -61,7 +61,7 @@ jobs:
             build: debug
 
     runs-on: ${{ matrix.runner }}-latest
-            
+
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
@@ -103,12 +103,12 @@ jobs:
         if: ${{ !cancelled() && steps.build.outcome == 'success' }}
         run: make BUILD=${{ matrix.build }} check
 
-      - name: run unit-tests 
+      - name: run unit-tests
         # always run if build succeeds
         if: ${{ !cancelled() && steps.build.outcome == 'success' && runner.os != 'macOS' }}
         run: make BUILD=${{ matrix.build }} unittest
 
-      - name: run tests 
+      - name: run tests
         # always run if build succeeds
         if: ${{ !cancelled() && steps.build.outcome == 'success' }}
         run: make BUILD=${{ matrix.build }} TEST_PREREQ= test
diff --git a/Makefile b/Makefile
index b39124a..250c0d2 100644
--- a/Makefile
+++ b/Makefile
@@ -106,5 +106,5 @@ clean: ## Cleanup everything
 build-%: ## Build with the specified configuration. E.g. make build-sanitize.
 	$(MAKE) build BUILD="$*"
 
-test-%: ## Test with the specified configuration. 
+test-%: ## Test with the specified configuration.
 	$(MAKE) test BUILD="$*"

trailing spaces

@bavshin-f5 bavshin-f5 force-pushed the bavshin/initial-conf branch 3 times, most recently from 1d2d6f5 to 2d5895e Compare July 23, 2025 23:26
@bavshin-f5 bavshin-f5 force-pushed the bavshin/initial-conf branch from 2d5895e to 0a72144 Compare July 23, 2025 23:54
Copy link

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

LGTM.

@bavshin-f5 bavshin-f5 closed this Jul 24, 2025
@bavshin-f5 bavshin-f5 deleted the bavshin/initial-conf branch July 24, 2025 01:24
@bavshin-f5
Copy link
Member Author

Manually merged as af6fffb to clean the commit history.

Thanks a lot to all who participated in the review!

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.

4 participants