Container api port watcher

[Nino-K]

Subscribe to the Docker API to monitor container creation and deletion events. Upon receiving these events, the event monitor immediately sends them as API events down the aggregated event channel.

[jandubois]

Why is this necessary? There should be no brackets in the strings.

[jandubois]

Keep them on separate lines:

Suggested change

	command_background=true pidfile="/run/lima-guestagent.pid"
	command_background=true
	pidfile="/run/lima-guestagent.pid"

[jandubois]

strip_array should not be necessary. And the app should do the right thing when called with an empty argument --docker-sockets=. So I think this can be just

Suggested change

	docker_args=""
	docker_items="$(strip_array "${LIMA_CIDATA_DOCKER_PORT_MONITOR_SOCKETS}")"
	if [ -n "$docker_items" ]; then
	docker_args="--docker-sockets=${LIMA_CIDATA_DOCKER_PORT_MONITOR_SOCKETS}"
	fi
	docker_args="--docker-sockets=${LIMA_CIDATA_DOCKER_PORT_MONITOR_SOCKETS}"

Same argument applies to the next 2 conditionals.

[jandubois]

They are never empty, so this could just be:

Suggested change

	${docker_args:+${docker_args}} \
	${containerd_args:+${containerd_args}} \
	${kubernetes_args:+${kubernetes_args}}
	${docker_args} \
	${containerd_args} \
	${kubernetes_args}

Same in the next 2 blocks.

[jandubois]

The naming convention is to call this file containerd_others.go and not _stub.

[jandubois]

Maybe simplify:

Suggested change

	if out, err := executeGuestTemplate(y.PortMonitors.Docker.Sockets[i], instDir, y.User, y.Param); err == nil {
	y.PortMonitors.Docker.Sockets[i] = out.String()
	} else {
	logrus.WithError(err).Warnf("Couldn't process Docker socket %q as a template", y.PortMonitors.Docker.Sockets[i])
	}
	socket := &y.PortMonitors.Docker.Sockets[i]
	if out, err := executeGuestTemplate(*socket, instDir, y.User, y.Param); err == nil {
	*socket = out.String()
	} else {
	logrus.WithError(err).Warnf("Couldn't process Docker socket %q as a template", *socket)
	}

[jandubois]

There are no variables in the filename, so no need to run it through the templating.

[jandubois]

The comment sounds wrong. Engine is a list of socket locations, not the name of the container engine.

[jandubois]

The normal format of validation errors would look more like

field `portMonitor.docker.sockets[1]` must not be empty

You will need to pass in the index too.

[Nino-K]

The validateSocket gets called in a loop, so no need for the index. The socket is individual and not a list.

[jandubois]

No, the sockets are a list, so you should provide the list index for the error message. You can pass in the field name instead of the index if you want the caller to assemble it. But since it is called from 2 places, it would be simpler to pass in the index.

Suggested change

	return fmt.Errorf("%s socket path must not be empty", engine)
	return fmt.Errorf("field `portMonitor.%s.sockets[%d]` must not be empty", engine, i)

[jandubois]

You don't really need to put quotes around paths that don't contain template variables. But I guess it doesn't hurt either.

[Nino-K]

I just wanted to keep it consistent with the reset of the file e.g:

portForwards:
- guestSocket: "/var/run/docker.sock"
  hostSocket: "{{.Dir}}/sock/docker.sock"