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

Allow linking Nginx modules dynamically #258260

Open
KFearsoff opened this issue Sep 30, 2023 · 8 comments
Open

Allow linking Nginx modules dynamically #258260

KFearsoff opened this issue Sep 30, 2023 · 8 comments
Labels
0.kind: enhancement Add something new

Comments

@KFearsoff
Copy link
Contributor

Issue description

Currently, Nginx allows linking modules only statically:

++ map (mod: "--add-module=${mod.src}") modules;

However, Nginx doesn't have feature parity between static and dynamic modules. This means that modules that don't support static linking don't work on NixOS.

Steps to reproduce

  1. Apply a patch that reverts nginx: take care not to pull in module sources as runtime deps #173721 (see reasoning in Certain nginx modules fail to build #182935 )
  2. Build Nginx with opentracing module
  3. Try loading the module

Unless I'm missing something, you can't do that: opentracing-contrib/nginx-opentracing#162
Interestingly, nginx-otel module that we don't have in Nixpkgs also can only be installed dynamically: nginxinc/nginx-otel#9

Technical details

Thankfully, the difference in how dynamic and static compiling is done is minimal. Static compilation uses --add-module, while dynamic compilation uses --add-dynamic-module . I haven't verified if it works yet, but I'll run some tests and add an update in a few days unless someone beats me to it.

For the Nginx package, it only requires adding a new optional argument dynamicModules (similar to modules). For the Nginx NixOS module, it requires adding a new option or extending existing one.

@RaitoBezarius
Copy link
Member

Please consider sending a PR, I don't think the NGINX maintenance team have the bandwidth to drive this change by themselves. We can surely try to review it.

Also please do not use assignment, we are maintainers, we do not owe feature requests. Just mention us.

@KFearsoff
Copy link
Contributor Author

Right. Sorry, the Github UI confused me here for a sec, as there are "assignees" and "reviewers" in the same place.

I'll send a PR as soon as I can.

@osokin
Copy link

osokin commented Oct 5, 2023

Historically www/nginx and www/nginx-devel ports on FreeBSD built all third-party modules statically only, later all of those modules, including nginx-otel, https://github.com/osokin/nginx-otel/tree/no-cmake, can be built both statically and dynamically.

@KFearsoff
Copy link
Contributor Author

So uhh.

I did create a patch that allows dynamic linking. The problem is Nginx modules themselves.

https://github.com/opentracing-contrib/nginx-opentracing is a bit old: OpenTracing was merged into OpenTelemetry last year.
https://github.com/nginxinc/nginx-otel is seemingly packaged in a legacy way, so it requires the --with-contrib flag which I'm too scared to touch as no module in Nixpkgs is packaged like that.
https://github.com/open-telemetry/opentelemetry-cpp-contrib/tree/main/instrumentation/nginx is... tolerable, but it needs to be built with opentracing-cpp which we don't have packaged, and requires and esoteric build flag too.

If anyone is interested in the dynamic linking itself, I'm happy to submit a PR, but my main motivation for getting Opentelemetry thing to completion (which drove me to create this issue) basically vanished lol. Will try to get into Caddy I guess.

@leiserfg
Copy link
Contributor

leiserfg commented Apr 1, 2024

@KFearsoff did you manage to make nginx-otel work with nix?

@KFearsoff
Copy link
Contributor Author

@leiserfg No. Truth be told, I haven't looked at it in more depth after the last comment on this issue. From a brief look, it seems like everything stayed the same way. I have also tried running Nginx with Otel in Kubernetes, but it was an extremely janky setup. I'm guessing nobody actually uses Nginx with Otel.

@ninaforsyth
Copy link

@KFearsoff Hello, I work at NGINX F5 and would love to make the Otel model less janky. Do you have any feedback on the problems or how we might be able to make it better?

@KFearsoff
Copy link
Contributor Author

@ninaforsyth Hey, sorry for the late reply. I was meaning to write a comprehensive reply, but it was really hard for the past two months.

The main issue I've had is no support for static linking. Notably, this is very inconvenient for Nixpkgs. From what I can tell, it is still unsupported. This is also a problem for ingress-nginx project: from what I can tell, instead of modifying the basic build image, they instead do some arcane hackery with building a separate container image that compiles the Otel module into some directory that is assumed to be a volume that will also be mounted by the primary Nginx container: https://github.com/kubernetes/ingress-nginx/blob/57d96128b10f8d24308f7055d8f5ed9b79903ffc/charts/ingress-nginx/values.yaml#L706-L729

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement Add something new
Projects
None yet
Development

No branches or pull requests

7 participants