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

PHP-FPM options via environment variables #394

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions builders/php/acceptance/flex_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@ func TestAcceptance(t *testing.T) {
VersionInclusionConstraint: "< 8.2.0",
MustUse: []string{flex, composer},
},
{
Name: "overridden document root",
App: "front_controller",
VersionInclusionConstraint: "< 8.2.0",
MustUse: []string{flex, composer},
Env: []string{"NGINX_DOCUMENT_ROOT=public"},
Path: "/",
MustMatch: "from public dir",
},
{
Name: "php ini override",
App: "php_ini",
Expand Down
1 change: 1 addition & 0 deletions builders/testdata/php/flex/front_controller/public/app.php
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from public dir
21 changes: 20 additions & 1 deletion cmd/php/supervisor/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ func buildFn(ctx *gcp.Context) error {
overrides := webconfig.OverriddenProperties(ctx, runtimeConfig)
webconfig.SetEnvVariables(l, overrides)

err = overrides.UpdateFromEnvironment(ctx)
if err != nil {
return err
}

fpmConfFile, err := writeFpmConfig(l.Path, overrides)
if err != nil {
return err
Expand Down Expand Up @@ -164,11 +169,17 @@ func nginxConfig(layer string, overrides webconfig.OverrideProperties) nginx.Con
frontController = overrides.FrontController
}

root := defaultRoot
if overrides.DocumentRoot != "" {
root = filepath.Join(defaultRoot, overrides.DocumentRoot)
}

nginx := nginx.Config{
Port: defaultNginxPort,
FrontControllerScript: frontController,
Root: filepath.Join(defaultRoot, overrides.DocumentRoot),
Root: root,
AppListenAddress: defaultAddress,
ServesStaticFiles: overrides.NginxServesStaticFiles,
}

if overrides.NginxServerConfInclude {
Expand Down Expand Up @@ -205,6 +216,14 @@ func fpmConfig(layer string, overrides webconfig.OverrideProperties) (nginx.FPMC
AddNoDecorateWorkers: true,
}

if overrides.PHPFPMDynamicWorkers {
fpm.DynamicWorkers = overrides.PHPFPMDynamicWorkers
}

if overrides.PHPFPMWorkers > 0 {
fpm.NumWorkers = overrides.PHPFPMWorkers
}

if overrides.PHPFPMOverride {
fpm.ConfOverride = overrides.PHPFPMOverrideFileName
}
Expand Down
16 changes: 9 additions & 7 deletions cmd/php/webconfig/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,10 @@ func buildFn(ctx *gcp.Context) error {
webconfig.SetEnvVariables(l, overrides)
}

if customNginxConf, present := os.LookupEnv(php.CustomNginxConfig); present {
overrides.NginxConfOverride = true
overrides.NginxConfOverrideFileName = filepath.Join(defaultRoot, customNginxConf)
}

nginxServesStaticFiles, err := env.IsPresentAndTrue(php.NginxServesStaticFiles)
err = overrides.UpdateFromEnvironment(ctx)
if err != nil {
return err
}
overrides.NginxServesStaticFiles = nginxServesStaticFiles

fpmConfFile, err := writeFpmConfig(ctx, l.Path, overrides)
if err != nil {
Expand Down Expand Up @@ -206,6 +200,14 @@ func fpmConfig(layer string, addNoDecorateWorkers bool, overrides webconfig.Over
fpm.ListenAddress = defaultFlexAddress
}

if overrides.PHPFPMDynamicWorkers {
fpm.DynamicWorkers = overrides.PHPFPMDynamicWorkers
}

if overrides.PHPFPMWorkers > 0 {
fpm.NumWorkers = overrides.PHPFPMWorkers
}

if overrides.PHPFPMOverride {
fpm.ConfOverride = overrides.PHPFPMOverrideFileName
}
Expand Down
15 changes: 15 additions & 0 deletions pkg/env/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -194,3 +194,18 @@ func IsPresentAndTrue(varName string) (bool, error) {

return parsed, nil
}

// IsPresentAndInt returns true and the integer value if the environment variable evaluates exists and contains an integer.
func IsPresentAndInt(varName string) (bool, int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not a big deal, but I would simplify this to return 2 values instead of 3:

func GetIfInt(varName string) (int, bool) {
...
}

We lost the error indication, but simplify things a bit. Your call.

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, I initially wrote it this way then backtracked, as everywhere else is very heavy on catching the errors & throwing them properly, so I didn't want to implement something that would silently fail (and revert to the default) if someone makes a mistake in the environment variable and it doesn't parse properly.

I'm happy either way though - if someone from Google can weigh in on which one you'd be more likely to accept for merge, I'll make it so.

varValue, present := os.LookupEnv(varName)
if !present {
return false, 0, nil
}

parsed, err := strconv.Atoi(varValue)
if err != nil {
return false, 0, fmt.Errorf("parsing %s: %v", varName, err)
}

return true, parsed, nil
}
11 changes: 10 additions & 1 deletion pkg/php/php.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,17 @@ post_max_size = 32M
// CustomNginxConfig is an environment variable to pass a custom nginx configuration.
CustomNginxConfig = "GOOGLE_CUSTOM_NGINX_CONFIG"

// NginxServesStaticFiles is an environment variable to configure Nginx to serve static files.
// NginxServesStaticFiles is an environment variable to configure nginx to serve static files.
NginxServesStaticFiles = "NGINX_SERVES_STATIC_FILES"

// NginxDocumentRoot is an environment variable to configure the document root of nginx.
NginxDocumentRoot = "NGINX_DOCUMENT_ROOT"

// PHPFPMDynamicWorkers is an environment variable to enable dynamic workers for php-fpm.
PHPFPMDynamicWorkers = "PHP_FPM_DYNAMIC_WORKERS"

// PHPFPMWorkerCount is an environment variable to configure the number of php-fpm workers.
PHPFPMWorkerCount = "PHP_FPM_WORKER_COUNT"
)

type composerScriptsJSON struct {
Expand Down
1 change: 1 addition & 0 deletions pkg/webconfig/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ go_library(
],
deps = [
"//pkg/appyaml",
"//pkg/env",
"//pkg/gcpbuildpack",
"//pkg/php",
"@com_github_buildpacks_libcnb//:go_default_library",
Expand Down
45 changes: 45 additions & 0 deletions pkg/webconfig/webconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@
package webconfig

import (
"fmt"
"os"
"path/filepath"

"github.com/GoogleCloudPlatform/buildpacks/pkg/appyaml"
"github.com/GoogleCloudPlatform/buildpacks/pkg/env"
gcp "github.com/GoogleCloudPlatform/buildpacks/pkg/gcpbuildpack"
"github.com/GoogleCloudPlatform/buildpacks/pkg/php"
"github.com/buildpacks/libcnb"
Expand Down Expand Up @@ -44,6 +47,10 @@ type OverrideProperties struct {
NginxHTTPInclude bool
// NginxHTTPIncludeFileName name of the partial nginx config to be included in the http section.
NginxHTTPIncludeFileName string
// PHPFPMDynamicWorkers boolean to toggle dynamic workers in the php-fpm config file.
PHPFPMDynamicWorkers bool
// PHPFPMWorkers integer to specify the worker thread count in the php-fpm config file.
PHPFPMWorkers int
// PHPFPMOverride boolean to check if user-provided php-fpm config exists.
PHPFPMOverride bool
// PHPFPMOverrideFileName name of the user-provided php-fpm config file.
Expand All @@ -56,6 +63,44 @@ type OverrideProperties struct {
NginxServesStaticFiles bool
}

func (overrides *OverrideProperties) UpdateFromEnvironment(ctx *gcp.Context) error {
if customNginxConf, present := os.LookupEnv(php.CustomNginxConfig); present {
overrides.NginxConfOverride = true
overrides.NginxConfOverrideFileName = filepath.Join(defaultRoot, customNginxConf)
}

nginxServesStaticFiles, err := env.IsPresentAndTrue(php.NginxServesStaticFiles)
if err != nil {
return err
}
overrides.NginxServesStaticFiles = nginxServesStaticFiles

nginxDocumentRoot := os.Getenv(php.NginxDocumentRoot)
if nginxDocumentRoot != "" {
overrides.DocumentRoot = nginxDocumentRoot
}

phpFPMDynamicWorkers, err := env.IsPresentAndTrue(php.PHPFPMDynamicWorkers)
if err != nil {
return err
}
overrides.PHPFPMDynamicWorkers = phpFPMDynamicWorkers

phpFPMWorkerCountPresent, phpFPMWorkerCount, err := env.IsPresentAndInt(php.PHPFPMWorkerCount)
if err != nil {
return err
}
if phpFPMWorkerCountPresent {
if phpFPMWorkerCount > 0 {
overrides.PHPFPMWorkers = phpFPMWorkerCount
} else {
return fmt.Errorf("invalid %s value, must be greater than 0", php.PHPFPMWorkerCount)
}
}

return nil
}

// OverriddenProperties returns whether the property has been overridden and the path to the file.
func OverriddenProperties(ctx *gcp.Context, runtimeConfig appyaml.RuntimeConfig) OverrideProperties {
phpIniOverride, phpIniOverrideFileName := overrideProperties(ctx, runtimeConfig.PHPIniOverride, defaultPHPIni)
Expand Down