Moving on with dataplane

Makefile

@@ -80,6 +80,19 @@ image: clean-image ## Build image for a particular arch.
 		--build-arg BUILD_ID="$(BUILD_ID)" \
 		-t $(REGISTRY)/controller:$(TAG) rootfs
 
+.PHONY: image-dataplane
+image-dataplane: clean-dataplane-image ## Build image for a particular arch.
+	echo "Building docker image dataplane ($(ARCH))..."
+	docker build \
+		${PLATFORM_FLAG} ${PLATFORM} \
+		--no-cache \
+		--build-arg BASE_IMAGE="$(BASE_IMAGE)" \
+		--build-arg VERSION="$(TAG)" \
+		--build-arg TARGETARCH="$(ARCH)" \
+		--build-arg COMMIT_SHA="$(COMMIT_SHA)" \
+		--build-arg BUILD_ID="$(BUILD_ID)" \
+		-t $(REGISTRY)/dataplane:$(TAG) -f rootfs/Dockerfile.dataplane rootfs
+
 .PHONY: gosec
 gosec:
 	docker run --rm -it -w /source/ -v "$(pwd)"/:/source securego/gosec:2.11.0 -exclude=G109,G601,G104,G204,G304,G306,G307 -tests=false -exclude-dir=test -exclude-dir=images/  -exclude-dir=docs/ /source/...
@@ -101,12 +114,15 @@ clean-image: ## Removes local image
 	echo "removing old image $(REGISTRY)/controller:$(TAG)"
 	@docker rmi -f $(REGISTRY)/controller:$(TAG) || true
 
-
 .PHONY: clean-chroot-image
 clean-chroot-image: ## Removes local image
 	echo "removing old image $(REGISTRY)/controller-chroot:$(TAG)"
 	@docker rmi -f $(REGISTRY)/controller-chroot:$(TAG) || true
 
+.PHONY: clean-dataplane-image
+clean-dataplane-image: ## Removes local image
+	echo "removing old image $(REGISTRY)/dataplane:$(TAG)"
+	@docker rmi -f $(REGISTRY)/dataplane:$(TAG) || true
 
 .PHONY: build
 build:  ## Build ingress controller, debug tool and pre-stop hook.

build/build.sh

@@ -48,6 +48,16 @@ ${GO_BUILD_CMD} \
   -buildvcs=false \
   -o "${TARGETS_DIR}/nginx-ingress-controller" "${PKG}/cmd/nginx"
 
+echo "Building ${PKG}/cmd/dataplane"
+
+${GO_BUILD_CMD} \
+  -trimpath -ldflags="-buildid= -w -s \
+  -X ${PKG}/version.RELEASE=${TAG} \
+  -X ${PKG}/version.COMMIT=${COMMIT_SHA} \
+  -X ${PKG}/version.REPO=${REPO_INFO}" \
+  -buildvcs=false \
+  -o "${TARGETS_DIR}/nginx-ingress-dataplane" "${PKG}/cmd/dataplane"
+
 echo "Building ${PKG}/cmd/dbg"
 
 ${GO_BUILD_CMD} \

cmd/dataplane/main.go

@@ -61,8 +61,11 @@ func main() {
 	signal.Notify(signalChan, syscall.SIGTERM)
 	// TODO: Turn delay configurable
 	n := dataplanenginx.NewNGINXExecutor(mux, 10, errCh, stopCh)
-	//go executor.Start()
+	err := n.Start()
-	go metrics.StartHTTPServer("127.0.0.1", 12345, mux)
+	if err != nil {
+		klog.Fatalf("error starting nginx process: %s", err)
+	}
+	go metrics.StartHTTPServer("0.0.0.0", 12345, mux)
 
 	// TODO: deal with OS signals
 	select {

cmd/dataplane/pkg/nginx/nginx.go

@@ -3,6 +3,7 @@ package nginx
 import (
 	"fmt"
 	"net/http"
+	"path/filepath"
 
 	nginxdataplane "k8s.io/ingress-nginx/internal/dataplane/nginx"
 	"k8s.io/klog/v2"
@@ -26,8 +27,12 @@ func NewNGINXExecutor(mux *http.ServeMux, stopdelay int, errch chan error, stopc
 	return n
 }
 
-func (n *nginxExecutor) Start() {
+func (n *nginxExecutor) Start() error {
-	n.cmd.Start(n.errch)
+	err := n.cmd.Start(n.errch)
+	if err != nil {
+		return err
+	}
+	return nil
 }
 
 func (n *nginxExecutor) Stop() error {
@@ -81,7 +86,10 @@ func (n *nginxExecutor) handleTest(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	o, err := n.cmd.Test(testFile)
+	// Avoid transversal path. We will get the final file from /etc/ingress-controller/tempconf
+	tmpFileFinal := filepath.Base(testFile)
+
+	o, err := n.cmd.Test(tmpFileFinal)
 	if err != nil {
 		w.WriteHeader(http.StatusInternalServerError)
 		w.Write([]byte(err.Error() + "\n"))

internal/dataplane/nginx/nginx.go

@@ -3,6 +3,7 @@ package nginx
 import (
 	"os"
 	"os/exec"
+	"path/filepath"
 	"syscall"
 
 	"k8s.io/klog/v2"
@@ -11,6 +12,7 @@ import (
 const (
 	defBinary = "/usr/bin/nginx"
 	CfgPath   = "/etc/nginx/conf/nginx.conf"
+	TempDir = "/etc/ingress-controller/tempconf"
 )
 
 // NginxExecTester defines the interface to execute
@@ -19,7 +21,7 @@ type NginxExecutor interface {
 	Reload() ([]byte, error)
 	Test(cfg string) ([]byte, error)
 	Stop() error
-	Start(chan error)
+	Start(chan error) error
 }
 
 // NginxCommand stores context around a given nginx executable path
@@ -57,18 +59,18 @@ func (nc NginxCommand) execCommand(args ...string) *exec.Cmd {
 	return executor
 }
 
-func (nc NginxCommand) Start(errch chan error) {
+func (nc NginxCommand) Start(errch chan error) error {
 	cmd := nc.execCommand()
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	if err := cmd.Start(); err != nil {
-		klog.Fatalf("NGINX error: %v", err)
+		klog.ErrorS(err, "NGINX error")
-		errch <- err
+		return err
-		return
 	}
 	go func() {
 		errch <- cmd.Wait()
 	}()
+	return nil
 }
 
 func (nc NginxCommand) Reload() ([]byte, error) {
@@ -86,5 +88,6 @@ func (nc NginxCommand) Stop() error {
 // Test checks if config file is a syntax valid nginx configuration
 func (nc NginxCommand) Test(cfg string) ([]byte, error) {
 	//nolint:gosec // Ignore G204 error
+	cfg = filepath.Join(TempDir, cfg)
 	return exec.Command(nc.Binary, "-c", cfg, "-t").CombinedOutput()
 }

internal/dataplane/nginx/remote.go

@@ -0,0 +1,103 @@
+package nginx
+
+import (
+	"fmt"
+	"io"
+	"net/http"
+	"net/url"
+	"strings"
+)
+
+// NginxCommand stores context around a given nginx executable path
+type NginxRemote struct {
+	host string
+}
+
+// NewNginxCommand returns a new NginxCommand from which path
+// has been detected from environment variable NGINX_BINARY or default
+func NewNginxRemote(host string) NginxExecutor {	
+	return NginxRemote{
+		host: host,
+	}
+}
+
+func (nc NginxRemote) Start(errch chan error) error {
+	getStart, err := url.JoinPath(nc.host, "start") // TODO: Turn this path a constant on dataplane
+	if err != nil {
+		return err
+	}
+	resp, err := http.Get(getStart)
+	if err != nil {
+		return err
+	}
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return err
+	}
+	if resp.StatusCode != http.StatusOK {
+		return fmt.Errorf("error executing start: %s", string(body))
+	}
+
+	// TODO: Add a ping/watcher to backend and populate error channel
+	return nil
+}
+
+func (nc NginxRemote) Reload() ([]byte, error) {
+	getReload, err := url.JoinPath(nc.host, "reload") // TODO: Turn this path a constant on dataplane
+	if err != nil {
+		return nil, err
+	}
+	resp, err := http.Get(getReload)
+	if err != nil {
+		return nil, err
+	}
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, err
+	}
+	if resp.StatusCode != http.StatusOK {
+		return nil, fmt.Errorf("error executing reload: %s", string(body))
+	}
+	return body, nil
+}
+
+func (nc NginxRemote) Stop() error {
+	getStop, err := url.JoinPath(nc.host, "stop") // TODO: Turn this path a constant on dataplane
+	if err != nil {
+		return err
+	}
+	resp, err := http.Get(getStop)
+	if err != nil {
+		return err
+	}
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return err
+	}
+	if resp.StatusCode != http.StatusOK {
+		return fmt.Errorf("error executing stop: %s", string(body))
+	}
+	return nil
+}
+
+// Test checks if config file is a syntax valid nginx configuration
+func (nc NginxRemote) Test(cfg string) ([]byte, error) {
+	form := url.Values{}
+	form.Add("testfile", cfg)
+	getStop, err := url.JoinPath(nc.host, "test") // TODO: Turn this path a constant on dataplane
+	if err != nil {
+		return nil, err
+	}
+	resp, err := http.Post(getStop, "application/x-www-form-urlencoded", strings.NewReader(form.Encode()))
+	if err != nil {
+		return nil, err
+	}
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, err
+	}
+	if resp.StatusCode != http.StatusOK {
+		return body, fmt.Errorf("error executing stop: %s", string(body))
+	}
+	return body, nil
+}

internal/ingress/controller/nginx.go

@@ -109,7 +109,9 @@ func NewNGINXController(config *Configuration, mc metric.Collector) *NGINXContro
 
 		metricCollector: mc,
 
-		command: nginxdataplane.NewNginxCommand(),
+		command: nginxdataplane.NewNginxRemote("http://127.0.0.1:12345"),
+		//command: nginxdataplane.NewNginxCommand(),
+
 	}
 
 	if n.cfg.ValidationWebhook != "" {
@@ -298,9 +300,12 @@ func (n *NGINXController) Start() {
 		})
 	}
 
+	// TODO: SSLPassthrough is not yet supported on dataplane mode
+	/*
 	if n.cfg.EnableSSLPassthrough {
 		n.setupSSLProxy()
 	}
+	*/
 
 	klog.InfoS("Starting NGINX process")
 	n.start()
@@ -331,6 +336,7 @@ func (n *NGINXController) Start() {
 
 	for {
 		select {
+		// TODO: create a separate error channel for the remote executor
 		case err := <-n.ngxErrCh:
 			if n.isShuttingDown {
 				return
@@ -414,7 +420,11 @@ func (n *NGINXController) Stop() error {
 }
 
 func (n *NGINXController) start() {
-	n.command.Start(n.ngxErrCh)
+	// TODO: do a better retry of start before failing 
+	if err := n.command.Start(n.ngxErrCh); err != nil {
+		n.stopCh <- struct{}{}
+		klog.Fatalf("error starting NGINX: %s", err)
+	}
 }
 
 // DefaultEndpoint returns the default endpoint to be use as default server that returns 404.
@@ -618,7 +628,7 @@ func (n *NGINXController) testTemplate(cfg []byte) error {
 	if len(cfg) == 0 {
 		return fmt.Errorf("invalid NGINX configuration (empty)")
 	}
-	tmpDir := os.TempDir() + "/nginx"
+	tmpDir := nginxdataplane.TempDir
 	tmpfile, err := os.CreateTemp(tmpDir, tempNginxPattern)
 	if err != nil {
 		return err
@@ -628,7 +638,8 @@ func (n *NGINXController) testTemplate(cfg []byte) error {
 	if err != nil {
 		return err
 	}
-	out, err := n.command.Test(tmpfile.Name())
+	tmpfileName := filepath.Base(tmpfile.Name())
+	out, err := n.command.Test(tmpfileName)
 	if err != nil {
 		// this error is different from the rest because it must be clear why nginx is not working
 		oe := fmt.Sprintf(`
@@ -1021,7 +1032,7 @@ func createOpentelemetryCfg(cfg *ngx_config.Configuration) error {
 func cleanTempNginxCfg() error {
 	var files []string
 
-	err := filepath.Walk(os.TempDir(), func(path string, info os.FileInfo, err error) error {
+	err := filepath.Walk(nginxdataplane.TempDir, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}

rootfs/Dockerfile

@@ -12,9 +12,9 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-ARG BASE_IMAGE
+#ARG BASE_IMAGE
 
-FROM ${BASE_IMAGE}
+FROM alpine:3.19
 
 ARG TARGETARCH
 ARG VERSION
@@ -31,13 +31,13 @@ LABEL org.opencontainers.image.revision="${COMMIT_SHA}"
 
 LABEL build_id="${BUILD_ID}"
 
-WORKDIR  /etc/nginx
-
 RUN apk update \
   && apk upgrade \
   && apk add --no-cache \
-    diffutils \
+    diffutils bash \
-  && rm -rf /var/cache/apk/*
+  && rm -rf /var/cache/apk/* \
+  && adduser -S -D -H -u 101 -h /usr/local/nginx \
+  -s /sbin/nologin -G www-data -g www-data www-data
 
 COPY --chown=www-data:www-data etc /etc
 
@@ -53,6 +53,7 @@ RUN bash -xeu -c ' \
     /etc/ingress-controller/auth \
     /etc/ingress-controller/geoip \
     /etc/ingress-controller/telemetry \
+    /etc/ingress-controller/tempconf \
     /etc/nginx/conf \
     /var/log \
     /var/log/nginx \
@@ -71,18 +72,11 @@ RUN bash -xeu -c ' \
 RUN apk add --no-cache libcap \
   && setcap    cap_net_bind_service=+ep /nginx-ingress-controller \
   && setcap -v cap_net_bind_service=+ep /nginx-ingress-controller \
-  && setcap    cap_net_bind_service=+ep /usr/local/nginx/sbin/nginx \
+  && apk del libcap
-  && setcap -v cap_net_bind_service=+ep /usr/local/nginx/sbin/nginx \
-  && setcap    cap_net_bind_service=+ep /usr/bin/dumb-init \
-  && setcap -v cap_net_bind_service=+ep /usr/bin/dumb-init \
-  && apk del libcap \
-  && ln -sf /usr/local/nginx/sbin/nginx /usr/bin/nginx
-
 USER www-data
 
 # Create symlinks to redirect nginx logs to stdout and stderr docker log collector
 RUN  ln -sf /dev/stdout /var/log/nginx/access.log \
   && ln -sf /dev/stderr /var/log/nginx/error.log
 
-ENTRYPOINT ["/usr/bin/dumb-init", "--"]
+ENTRYPOINT ["/nginx-ingress-controller"]
-CMD ["/nginx-ingress-controller"]

rootfs/Dockerfile.dataplane

@@ -54,6 +54,7 @@ RUN bash -xeu -c ' \
     /etc/ingress-controller/ssl \
     /etc/ingress-controller/auth \
     /etc/ingress-controller/geoip \
+    /etc/ingress-controller/tempconf \
     /etc/ingress-controller/telemetry \
     /etc/nginx/conf \
     /var/log \
@@ -71,8 +72,8 @@ RUN bash -xeu -c ' \
   
 
 RUN apk add --no-cache libcap \
-  && setcap    cap_net_bind_service=+ep /nginx-ingress-controller \
+  && setcap    cap_net_bind_service=+ep /nginx-ingress-dataplane \
-  && setcap -v cap_net_bind_service=+ep /nginx-ingress-controller \
+  && setcap -v cap_net_bind_service=+ep /nginx-ingress-dataplane \
   && setcap    cap_net_bind_service=+ep /usr/local/nginx/sbin/nginx \
   && setcap -v cap_net_bind_service=+ep /usr/local/nginx/sbin/nginx \
   && setcap    cap_net_bind_service=+ep /usr/bin/dumb-init \