brefphp / extra-php-extensions

Community-maintained extra PHP extensions usable in AWS Lambda with the Bref PHP runtimes.
https://bref.sh/docs/environment/php.html#extra-extensions
213 stars 110 forks source link

Add ddappsec.so to datadog extension #457

Open annuh opened 1 year ago

annuh commented 1 year ago

Fixes https://github.com/brefphp/extra-php-extensions/issues/456.

Proposed fix: Include the ddappsec.so module in the Datadog layer.

Without this fix, I'm getting the following errors:

Warning: PHP Startup: Unable to load dynamic library 'ddappsec.so' (tried: /opt/bref/extensions/ddappsec.so (/opt/bref/extensions/ddappsec.so: cannot open shared object file: No such file or directory), /opt/bref/extensions/ddappsec.so.so (/opt/bref/extensions/ddappsec.so.so: cannot open shared object file: No such file or directory)) in Unknown on line 0
annuh commented 1 year ago

I don't think we should fix this here, and instead wait for the upstream fix to be completed.

Based on that ticket (https://github.com/DataDog/dd-trace-php/issues/2014), it's not clear to me what exactly will be fixed. Maybe this fix (or: work-around) will still be needed, because the datadog-php-trace setup script assumes ddappsec.so is always present, in a normal installation.

EDIT: this is also described in the documentation:

When you do not specify --enable-appsec, the AppSec extension loads shortly at startup, and is not enabled by default. It immediately short-circuits, causing negligible performance overhead. Source: https://docs.datadoghq.com/tracing/trace_collection/dd_libraries/php/?tab=otherenvironments

Nyholm commented 1 year ago

Hm... I think this is reasonable. Anything happened for DataDog the past few months?

annuh commented 1 year ago

Hm... I think this is reasonable. Anything happened for DataDog the past few months?

Not as far I know. The Datadog docs still mention that “the AppSec extension loads shortly at startup”. Also, the Unable to load dynamic library 'ddappsec.so' warning is still logged. To avoid this warning, it would be nice if this PR could be merged 🙂