diff --git a/build/build.sh b/build/build.sh index bbcaf78e8..f3504eada 100755 --- a/build/build.sh +++ b/build/build.sh @@ -13,7 +13,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - +set -x GO_BUILD_CMD="go build" #if [ -n "$DEBUG" ]; then @@ -66,4 +66,4 @@ ${GO_BUILD_CMD} \ -X ${PKG}/version.COMMIT=${COMMIT_SHA} \ -X ${PKG}/version.REPO=${REPO_INFO}" \ -buildvcs=false \ - -o "${TARGETS_DIR}/wait-shutdown" "${PKG}/cmd/waitshutdown" \ No newline at end of file + -o "${TARGETS_DIR}/wait-shutdown" "${PKG}/cmd/waitshutdown" diff --git a/cmd/dataplane/main.go b/cmd/dataplane/main.go index 65f898df6..b587528d9 100644 --- a/cmd/dataplane/main.go +++ b/cmd/dataplane/main.go @@ -29,9 +29,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/controller" "k8s.io/ingress-nginx/internal/ingress/metric" "k8s.io/ingress-nginx/internal/nginx" - ingressflags "k8s.io/ingress-nginx/pkg/flags" "k8s.io/ingress-nginx/pkg/metrics" - "k8s.io/ingress-nginx/pkg/util/file" "k8s.io/ingress-nginx/pkg/util/process" "k8s.io/ingress-nginx/version" ) @@ -41,19 +39,6 @@ func main() { fmt.Println(version.String()) var err error - showVersion, conf, err := ingressflags.ParseFlags() - if showVersion { - os.Exit(0) - } - - if err != nil { - klog.Fatal(err) - } - - err = file.CreateRequiredDirectories() - if err != nil { - klog.Fatal(err) - } reg := prometheus.NewRegistry() @@ -64,13 +49,7 @@ func main() { })) mc := metric.NewDummyCollector() - if conf.EnableMetrics { - // TODO: Ingress class is not a part of dataplane anymore - mc, err = metric.NewCollector(conf.MetricsPerHost, conf.ReportStatusClasses, reg, conf.IngressClassConfiguration.Controller, *conf.MetricsBuckets, conf.ExcludeSocketMetrics) - if err != nil { - klog.Fatalf("Error creating prometheus collector: %v", err) - } - } + // Pass the ValidationWebhook status to determine if we need to start the collector // for the admissionWebhook // TODO: Dataplane does not contain validation webhook so the MetricCollector should not receive diff --git a/docs/enhancements/20231001-split-containers.md b/docs/enhancements/20231001-split-containers.md index 3c2e85094..9c397d6c2 100644 --- a/docs/enhancements/20231001-split-containers.md +++ b/docs/enhancements/20231001-split-containers.md @@ -27,6 +27,29 @@ to start, stop and reload NGINX * /test - (POST) Test the configuration of a given file location * "config" argument is the location of temporary file that should be tested +## Implementation details +### Control Plane + +This container will have the following functionality: +* Watch / Map changes on Kubernetes API Server +* Write the configuration files on the shared directory +* Call the Openresty endpoint to do the dynamic configuration +* Trigger reload, when required, on the dataplane container + +Open ports: +* Metrics port + +### Data Plane +This container will have the following functionality + +* Provide Access between users and Pods + * (TODO): Provide TLS Passthrough + +Open ports: +* HTTP/HTTPs Ports +* Localhost only - Openresty configuration port +* (TODO): Metrics port + ### Mounting empty SA on controller container ```yaml diff --git a/images/nginx-1.25/TAG b/images/nginx-1.25/TAG index 254a9f20d..04eddb26b 100644 --- a/images/nginx-1.25/TAG +++ b/images/nginx-1.25/TAG @@ -1 +1 @@ -v0.0.6 \ No newline at end of file +v0.0.7 \ No newline at end of file diff --git a/images/nginx-1.25/rootfs/build.sh b/images/nginx-1.25/rootfs/build.sh index 0d7328d81..0c11a1cb8 100755 --- a/images/nginx-1.25/rootfs/build.sh +++ b/images/nginx-1.25/rootfs/build.sh @@ -625,13 +625,13 @@ done rm -rf /etc/nginx/owasp-modsecurity-crs/.git rm -rf /etc/nginx/owasp-modsecurity-crs/util/regression-tests -rm -rf /etc/nginx/fastcgi.conf.* +rm -rf /etc/nginx/fastcgi.conf* rm -rf /etc/nginx/koi* rm -rf /etc/nginx/mime.types.default rm -rf /etc/nginx/nginx.conf.default rm -rf /etc/nginx/opentracing.json rm -rf /etc/nginx/scgi* -rm -rf /etc/nginx/uwscgi* +rm -rf /etc/nginx/uwsgi* rm -rf /etc/nginx/win-utf # remove .a files diff --git a/internal/dataplane/nginx/nginx.go b/internal/dataplane/nginx/nginx.go new file mode 100644 index 000000000..5feb3be5f --- /dev/null +++ b/internal/dataplane/nginx/nginx.go @@ -0,0 +1,90 @@ +package nginx + +import ( + "os" + "os/exec" + "syscall" + + "k8s.io/klog/v2" +) + +const ( + defBinary = "/usr/bin/nginx" + CfgPath = "/etc/nginx/conf/nginx.conf" +) + +// NginxExecTester defines the interface to execute +// command like reload or test configuration +type NginxExecutor interface { + Reload() ([]byte, error) + Test(cfg string) ([]byte, error) + Stop() error + Start(chan error) +} + +// NginxCommand stores context around a given nginx executable path +type NginxCommand struct { + Binary string +} + +// NewNginxCommand returns a new NginxCommand from which path +// has been detected from environment variable NGINX_BINARY or default +func NewNginxCommand() NginxCommand { + command := NginxCommand{ + Binary: defBinary, + } + + binary := os.Getenv("NGINX_BINARY") + if binary != "" { + command.Binary = binary + } + + return command +} + +// ExecCommand instanciates an exec.Cmd object to call nginx program +func (nc NginxCommand) execCommand(args ...string) *exec.Cmd { + cmdArgs := []string{} + + cmdArgs = append(cmdArgs, "-c", CfgPath) + cmdArgs = append(cmdArgs, args...) + //nolint:gosec // Ignore G204 error + executor := exec.Command(nc.Binary, cmdArgs...) + executor.SysProcAttr = &syscall.SysProcAttr{ + Setpgid: true, + Pgid: 0, + } + return executor +} + +func (nc NginxCommand) Start(errch chan error) { + cmd := nc.execCommand() + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + if err := cmd.Start(); err != nil { + klog.Fatalf("NGINX error: %v", err) + errch <- err + return + } + go func() { + errch <- cmd.Wait() + }() +} + +func (nc NginxCommand) Reload() ([]byte, error) { + cmd := nc.execCommand("-s", "reload") + return cmd.CombinedOutput() +} + +func (nc NginxCommand) Stop() error { + cmd := nc.execCommand("-s", "quit") + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + return cmd.Run() +} + +// Test checks if config file is a syntax valid nginx configuration +func (nc NginxCommand) Test(cfg string) ([]byte, error) { + //nolint:gosec // Ignore G204 error + return exec.Command(nc.Binary, "-c", cfg, "-t").CombinedOutput() +} \ No newline at end of file diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go index e257dd1f1..4977d6fdc 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -25,7 +25,6 @@ import ( "fmt" "io" "os" - "os/exec" "path/filepath" "strings" "testing" @@ -42,6 +41,7 @@ import ( "k8s.io/ingress-nginx/pkg/apis/ingress" + nginxdataplane "k8s.io/ingress-nginx/internal/dataplane/nginx" "k8s.io/ingress-nginx/internal/ingress/annotations" "k8s.io/ingress-nginx/internal/ingress/annotations/canary" "k8s.io/ingress-nginx/internal/ingress/annotations/ipallowlist" @@ -134,8 +134,17 @@ type testNginxTestCommand struct { err error } -func (ntc testNginxTestCommand) ExecCommand(_ ...string) *exec.Cmd { - return nil +// TODO: Implement tests below and move to dataplane package +func (ntc testNginxTestCommand) Stop() error { + panic("not implemented") +} + +func (ntc testNginxTestCommand) Start(errch chan error) { + panic("not implemented") +} + +func (ntc testNginxTestCommand) Reload() ([]byte, error) { + panic("not implemented") } func (ntc testNginxTestCommand) Test(cfg string) ([]byte, error) { @@ -2543,7 +2552,7 @@ func newNGINXController(t *testing.T) *NGINXController { return &NGINXController{ store: storer, cfg: config, - command: NewNginxCommand(), + command: nginxdataplane.NewNginxCommand(), } } @@ -2608,7 +2617,7 @@ func newDynamicNginxController(t *testing.T, setConfigMap func(string) *corev1.C return &NGINXController{ store: storer, cfg: config, - command: NewNginxCommand(), + command: nginxdataplane.NewNginxCommand(), metricCollector: metric.DummyCollector{}, } } diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index b81734154..320ac1b49 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -60,6 +60,7 @@ import ( "k8s.io/ingress-nginx/internal/task" "k8s.io/ingress-nginx/pkg/apis/ingress" + nginxdataplane "k8s.io/ingress-nginx/internal/dataplane/nginx" "k8s.io/ingress-nginx/pkg/util/file" utilingress "k8s.io/ingress-nginx/pkg/util/ingress" @@ -108,7 +109,7 @@ func NewNGINXController(config *Configuration, mc metric.Collector) *NGINXContro metricCollector: mc, - command: NewNginxCommand(), + command: nginxdataplane.NewNginxCommand(), } if n.cfg.ValidationWebhook != "" { @@ -259,7 +260,7 @@ type NGINXController struct { validationWebhookServer *http.Server - command NginxExecTester + command nginxdataplane.NginxExecutor } // Start starts a new NGINX master process running in the foreground. @@ -297,21 +298,12 @@ func (n *NGINXController) Start() { }) } - cmd := n.command.ExecCommand() - - // put NGINX in another process group to prevent it - // to receive signals meant for the controller - cmd.SysProcAttr = &syscall.SysProcAttr{ - Setpgid: true, - Pgid: 0, - } - if n.cfg.EnableSSLPassthrough { n.setupSSLProxy() } klog.InfoS("Starting NGINX process") - n.start(cmd) + n.start() go n.syncQueue.Run(time.Second, n.stopCh) // force initial sync @@ -403,13 +395,10 @@ func (n *NGINXController) Stop() error { // send stop signal to NGINX klog.InfoS("Stopping NGINX process") - cmd := n.command.ExecCommand("-s", "quit") - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - err := cmd.Run() - if err != nil { + if err := n.command.Stop(); err != nil { return err } + // wait for the NGINX process to terminate timer := time.NewTicker(time.Second * 1) @@ -424,18 +413,8 @@ func (n *NGINXController) Stop() error { return nil } -func (n *NGINXController) start(cmd *exec.Cmd) { - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - if err := cmd.Start(); err != nil { - klog.Fatalf("NGINX error: %v", err) - n.ngxErrCh <- err - return - } - - go func() { - n.ngxErrCh <- cmd.Wait() - }() +func (n *NGINXController) start() { + n.command.Start(n.ngxErrCh) } // DefaultEndpoint returns the default endpoint to be use as default server that returns 404. @@ -692,11 +671,12 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { } if klog.V(2).Enabled() { - src, err := os.ReadFile(cfgPath) + src, err := os.ReadFile(nginxdataplane.CfgPath) if err != nil { return err } if !bytes.Equal(src, content) { + // TODO: This test should run on dataplane side tmpfile, err := os.CreateTemp("", "new-nginx-cfg") if err != nil { return err @@ -707,7 +687,7 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { return err } //nolint:gosec //Ignore G204 error - diffOutput, err := exec.Command("diff", "-I", "'# Configuration.*'", "-u", cfgPath, tmpfile.Name()).CombinedOutput() + diffOutput, err := exec.Command("diff", "-I", "'# Configuration.*'", "-u", nginxdataplane.CfgPath, tmpfile.Name()).CombinedOutput() if err != nil { if exitError, ok := err.(*exec.ExitError); ok { ws, ok := exitError.Sys().(syscall.WaitStatus) @@ -728,12 +708,12 @@ func (n *NGINXController) OnUpdate(ingressCfg ingress.Configuration) error { } } - err = os.WriteFile(cfgPath, content, file.ReadWriteByUser) + err = os.WriteFile(nginxdataplane.CfgPath, content, file.ReadWriteByUser) if err != nil { return err } - o, err := n.command.ExecCommand("-s", "reload").CombinedOutput() + o, err := n.command.Reload() if err != nil { return fmt.Errorf("%v\n%v", err, string(o)) } diff --git a/internal/ingress/controller/util.go b/internal/ingress/controller/util.go index 8851f323f..2fbdbfef5 100644 --- a/internal/ingress/controller/util.go +++ b/internal/ingress/controller/util.go @@ -19,7 +19,6 @@ package controller import ( "fmt" "os" - "os/exec" "path" "strconv" "strings" @@ -97,53 +96,7 @@ func rlimitMaxNumFiles() int { return int(rLimit.Max) } -const ( - defBinary = "/usr/bin/nginx" - cfgPath = "/etc/nginx/nginx.conf" -) -// NginxExecTester defines the interface to execute -// command like reload or test configuration -type NginxExecTester interface { - ExecCommand(args ...string) *exec.Cmd - Test(cfg string) ([]byte, error) -} - -// NginxCommand stores context around a given nginx executable path -type NginxCommand struct { - Binary string -} - -// NewNginxCommand returns a new NginxCommand from which path -// has been detected from environment variable NGINX_BINARY or default -func NewNginxCommand() NginxCommand { - command := NginxCommand{ - Binary: defBinary, - } - - binary := os.Getenv("NGINX_BINARY") - if binary != "" { - command.Binary = binary - } - - return command -} - -// ExecCommand instanciates an exec.Cmd object to call nginx program -func (nc NginxCommand) ExecCommand(args ...string) *exec.Cmd { - cmdArgs := []string{} - - cmdArgs = append(cmdArgs, "-c", cfgPath) - cmdArgs = append(cmdArgs, args...) - //nolint:gosec // Ignore G204 error - return exec.Command(nc.Binary, cmdArgs...) -} - -// Test checks if config file is a syntax valid nginx configuration -func (nc NginxCommand) Test(cfg string) ([]byte, error) { - //nolint:gosec // Ignore G204 error - return exec.Command(nc.Binary, "-c", cfg, "-t").CombinedOutput() -} // getSysctl returns the value for the specified sysctl setting func getSysctl(sysctl string) (int, error) { diff --git a/rootfs/Dockerfile b/rootfs/Dockerfile index a04cfe3de..0fe3a55f9 100644 --- a/rootfs/Dockerfile +++ b/rootfs/Dockerfile @@ -53,6 +53,7 @@ RUN bash -xeu -c ' \ /etc/ingress-controller/auth \ /etc/ingress-controller/geoip \ /etc/ingress-controller/telemetry \ + /etc/nginx/conf \ /var/log \ /var/log/nginx \ /tmp/nginx \ diff --git a/rootfs/etc/nginx/nginx.conf b/rootfs/etc/nginx/conf/nginx.conf similarity index 100% rename from rootfs/etc/nginx/nginx.conf rename to rootfs/etc/nginx/conf/nginx.conf