cloudfoundry / php-buildpack

A Cloud Foundry Buildpack for PHP.
Apache License 2.0
142 stars 346 forks source link

Php-buildpack extension supplied by extension buildpacks #221

Closed gberche-orange closed 5 years ago

gberche-orange commented 7 years ago

As part of Multi buildpack design proposal discussion, @sclevine suggested to express php-builpack use-cases as github issues.

In addition to be able to inject binaries and libraries in the path (as covered in story 142852347, we would also like to 1- be able to inject loading of libraries and their config into the http engine managed by the php-buildpack. For instance, inject mod_security module into the apache httpd engine (see https://github.com/cloudfoundry/php-buildpack/issues/144 and https://github.com/cloudfoundry/php-buildpack/compare/master...orange-cloudfoundry:modsecurity_rebased) 2- be able to inject side-car processes into php_buildpack, such as https://github.com/orange-cloudfoundry/gobis which acts as a reverse proxy in front of the httpd process.

The current generic contract in the proposal doc reproduced below does not seem to cover the 1st use case.

The config.yml file should be formatted as such:

name: config: {}

In addition, the following directories may be created inside of /tmp/deps// to provide dependencies to subsequent buildpacks:

/bin: contains binaries intended for $PATH during staging/launch /lib: contains libraries intended for $LD_LIBRARY_PATH during staging/launch /include: contains header files intended for compilation during staging /pkgconfig: contains pkgconfig files intended for compilation during staging /env: contains env vars intended for staging, loaded as FILENAME=FILECONTENTS /profile.d: contains scripts intended for /app/.profile.d (sourced before launch)

One possible improvement could be that the php-buildpack would load php-buildpack extensions that extension buildpacks would supply. This would enable extensions buildpack authors to leverage existing well defined extension mechanism to hook into the php-buildpack. For instance, the config.yml file config key could contain a list of extensions to load, with possible associated meta data such as path in the deps dir.

For the 2nd use-case of launching side car, we were considering an extension buildpack that would supply a profile.d script that would start a background command as well are modify the PORT variable. We're eager to have feedback from the buildpack team as whether this would be the right approach (and possibly whether it'd work for other core buildpacks)

Thanks in advance for your feedback

/CC @ArthurHlt

cf-gitbot commented 7 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/150361099

The labels on this github issue will be updated when the story is started.

sclevine commented 7 years ago

I think the existing config object covers (1):

config: {<YAML object containing buildpack-specific configuration>}

Apache modules are "buildpack-specific," so the PHP buildpack should define its contract for accepting Apache modules via the config object. If you submit a PR that enables this contract, we would definitely consider merging it. Otherwise, we'll get to it when priorities allow.

For (2), I worry about introducing side-car processes into buildpacks because it makes memory management more difficult (and thus breaks the Java buildpack). I'm open to it for the PHP buildpack specifically if you explain why your use case isn't covered by container-to-container networking. (Side note: I see no reason why your /profile.d solution wouldn't work.)

(CC: @dgodd @dmikusa-pivotal)

dmikusa commented 7 years ago

1- be able to inject loading of libraries and their config into the http engine managed by the php-buildpack. For instance, inject mod_security module into the apache httpd engine (see #144 and master...orange-cloudfoundry:modsecurity_rebased)

Is it just mod_security that you need or is it a general need to add, enable and configure HTTPD modules? If it's just mod_security, I would suggest a PR to binary-builder & perhaps a PR to the php-buildpack with a new PHP build pack extension to enable support for mod_security. I think that mod_security would be generally useful to all users of the build pack so this seems like a good path.

2- be able to inject side-car processes into php_buildpack, such as https://github.com/orange-cloudfoundry/gobis which acts as a reverse proxy in front of the httpd process.

You could also handle this with a PHP build pack extension. At the moment you would have to either fork the build pack to pull in the extension or include the extension with all the apps that want to use it.

I'm not understanding the use case for this, so I'm not sure a PR would be appropriate. If it supports some general use case that is applicable to a lot of PHP build pack users then I could see that being a possibility.

One possible improvement could be that the php-buildpack would load php-buildpack extensions that extension buildpacks would supply.

I'm not sure about this. I know you could technically do it, but it seems to cross a border that I'm not sure is good to cross. Specifically that one build pack could break another build pack.

I think I would rather see the decomposition of the PHP build pack such that it's easier for users to compose what they need together. For example, you're talking about bringing a custom reverse proxy and customizing how HTTPD is setup. I think it would be better if you had your own build pack that brought this stuff and you could tell the PHP build pack to just not install any of that. Then it would just setup PHP & PHP-FPM in a consistent way that your build pack can depend on. i.e. it's always setup to listen on port 9000 (or a file socket) for FastCGI traffic. I think you might be able to get close to this if you set the WEB_SERVER option to none, but I haven't tried it.

For the 2nd use-case of launching side car, we were considering an extension buildpack that would supply a profile.d script that would start a background command as well are modify the PORT variable. We're eager to have feedback from the buildpack team as whether this would be the right approach (and possibly whether it'd work for other core buildpacks)

If it were me, I would look at a PHP build pack extension. You can use the configure method to adjust the PORT & you can use the service_commands method to instruct the build pack to launch your additional service. The benefit of this is that it will monitor this command and if it exits for some reason, it will make sure everything else cleans up. If you start your service in a profile.d script and it exits, it's just going to exit and what happens to the rest of the processes running in that container is anybody's guess (hint - they may not crash and the container may just keep running in a broken state).

https://github.com/cloudfoundry/php-buildpack#methods

gberche-orange commented 7 years ago

thanks for your answers and sorry for my late follow up

@sclevine:

Apache modules are "buildpack-specific," so the PHP buildpack should define its contract for accepting Apache modules via the config object. If you submit a PR that enables this contract, we would definitely consider merging it.

I was thinking about the PHP buildpack accepting PHP buildpack python extensions instead of a focused solely on apache modules.

I'm open to it for the PHP buildpack specifically if you explain why your use case isn't covered by container-to-container networking.

Mod security is a web firewall with OWASP rules that send warnings for suspicious incoming traffic (e.g. with payload containing SQL injection or XSS queries). It is thus applicable to application ingress through through their mapped routes (unlike container 2 container networking which is filtering app to app traffic at the TCP level without inspecting http payload)

@dmikusa-pivotal :

If it's just mod_security, I would suggest a PR to binary-builder & perhaps a PR to the php-buildpack with a new PHP build pack extension to enable support for mod_security. I think that mod_security would be generally useful to all users of the build pack so this seems like a good path.

That's what we had proposed in https://github.com/cloudfoundry/php-buildpack/issues/144 back in 2016, but there was not yet enough interest to merge such PR at that time. @sclevine would you now consider merging a native mod_security support in php-buildpack ?

Note we also have other use-cases (e.g. customizing the log format by importing an apache module).

For the 2nd use-case of launching side car, [...] If it were me, I would look at a PHP build pack extension.

Definitely. Our requirement is that these two use cases get configured by admins and applied to all apps without requiring developers to embed PHP buildpack extension into their code. That's why we were considering embedding some php buildpack extensions into extension buildpacks configured in the future as mandatory buildpacks when the features comes out.

dmikusa commented 7 years ago

If it's just mod_security, I would suggest a PR to binary-builder & perhaps a PR to the php-buildpack with a new PHP build pack extension to enable support for mod_security. I think that mod_security would be generally useful to all users of the build pack so this seems like a good path.

That's what we had proposed in #144 back in 2016, but there was not yet enough interest to merge such PR at that time. @sclevine would you now consider merging a native mod_security support in php-buildpack ?

+1 for this. I know mod_security is fairly common to use in conjunction with PHP apps. I suspect other users will find this helpful to have in the build pack.

sclevine commented 7 years ago

@gberche-orange I would accept a PR to both repos. I agree with @dmikusa-pivotal, this is generally useful.

zmackie commented 5 years ago

Closing as stale. @dmikusa-pivotal @sclevine feel free to re-open