Questions/Feedback for the setup process with podman

[SchoolGuy]

Over the weekend, I had a deeper look into this repository and came up with a bunch of questions.

1. Why is the podman socket enabled during the installation, but not being used for communication with podman?
2. Since there is a lot of communication with Podman, why are the official Podman Go bindings not used?
3. Why is the setup script not inside the container as the entrypoint script? Normally, I would expect the script to be supplied with only environment variables.
4. Why is Apache started during setup? Couldn't we talk to Tomcat directly instead?
5. Why are the SSL certificates ignored during setup? Just skip SSL entirely if we don't check the validity of the certs.
6. Why are the entrypoints for mgradm, mgrctl and mgrpxy not under <repo>/cmd? This is what I would expect in a standard Go project.
7. During setup, I encountered multiple errors, and the error messages were not very descriptive. Sometimes all I got was a red "exit 1".
8. Why do I find snake_case imports in some modules? I would expect CamelCase for everything.
9. Much of the information is being gathered with exec calls and corresponding generated commands. Why is there no network-based internal API that is being called? (Sidecar-Pattern)
10. Why is the setup not implemented as an init-container but as fully dedicated pods?

All of this together made it quite challenging to get comfortable with the codebase on the weekend. I am not saying we should refactor everything, but a few notes on the design decisions - if intentional - would be nice so people can follow the style of the project more easily.

[cbosdo]

1. Why is the podman socket enabled during the installation, but not being used for communication with podman?

This was added to use podman-remote IIRC. We could probably not enable it.

2. Since there is a lot of communication with Podman, why are the official Podman Go bindings not used?

We used the podman go bindings in a very early version but that pulls a lot of other dependencies and added a significant complexity (by then). May be this would be better now, but we don't have much time to spend on refactoring it, as it just works. I also fail to see what value we would get from that.

3. Why is the setup script not inside the container as the entrypoint script? Normally, I would expect the script to be supplied with only environment variables.

Some of the scripts are being moved in the images as we speak.

4. Why is Apache started during setup? Couldn't we talk to Tomcat directly instead?

That was the easy way to go. We would happily review PRs on this.

5. Why are the SSL certificates ignored during setup? Just skip SSL entirely if we don't check the validity of the certs.

From the top of my head, SSL certificates are used after the setup, but we need to set them up early for the database.

6. Why are the entrypoints for mgradm, mgrctl and mgrpxy not under `<repo>/cmd`? This is what I would expect in a standard Go project.

This is clearly what a Go beginner (me) did by then. That could probably be improved as I have no experience of other repos with multiple commands.

7. During setup, I encountered multiple errors, and the error messages were not very descriptive. Sometimes all I got was a red "exit 1".

Some of them are due to the use of an old wrapper around the Exec() function that doesn't get the stderr. Precise examples would help to asnwer that one.

8. Why do I find snake_case imports in some modules? I would expect CamelCase for everything.

Error from a past time that stayed until now?

9. Much of the information is being gathered with exec calls and corresponding generated commands. Why is there no network-based internal API that is being called? (Sidecar-Pattern)

I don't get the problem here…

10. Why is the setup not implemented as an init-container but as fully dedicated pods?

This is only on Kubernetes I suppose since podman has no init containers. This has been changed very recently: the Kubernetes code is moving away from this repo to the helm charts… and there the setup is a Job.

All of this together made it quite challenging to get comfortable with the codebase on the weekend. I am not saying we should refactor everything, but a few notes on the design decisions - if intentional - would be nice so people can follow the style of the project more easily.

This organically grew from a super simple pair of tools to a complicated beast over the course of multiple iterations. There is no documented design decision, and we carry over some old things (Exec wrapper) due to the lack of time to make things better. Getting familiar with the whole code base over a week-end was clearly never a target here. Things will improve as we drop migration and kubernetes from here.

[SchoolGuy]

We used the podman go bindings in a very early version but that pulls a lot of other dependencies and added a significant complexity (by then). May be this would be better now, but we don't have much time to spend on refactoring it, as it just works. I also fail to see what value we would get from that.

Using exec calls always means that you are responsible for handling signals correctly, which gets complicated fast. For that reason alone (and the cleaner code flow), I prefer to use as few exec calls as possible. This would also potentially simplify SELinux permissions, as there is no need to touch some of the binaries that are used.

Some of the scripts are being moved in the images as we speak.

Great to hear!

From the top of my head, SSL certificates are used after the setup, but we need to set them up early for the database.

I found them used during the setup of the first user as well. There are three https://localhost calls in the mgrScriptSetupTemplate that I am thinking of.

This is clearly what a Go beginner (me) did by then. That could probably be improved as I have no experience of other repos with multiple commands.

Well: <repo>/cmd/cmd1.go, <repo>/cmd/cmd2.go, <repo>/cmd/cmd3.go, ... (or you use subfolders). Example can be found in cert-manager... - https://github.com/cert-manager/cert-manager/tree/master/cmd

Some of them are due to the use of an old wrapper around the Exec() function that doesn't get the stderr. Precise examples would help to answer that one.

It was late in the evening. I will try to reproduce them this weekend again. But since you are cleaning up the scripts currently in the repo, maybe I should wait? Whatever you prefer.

Error from a past time that stayed until now?

Want a PR?

I don't get the problem here…

I assume that most administrators actually don't have access to their container hosts due to reasons I am not speculating. Tools like Portainer don't exist for fun and the laziness of people. Securing the host in shared environments is best-practice and revoking access for application admins seems like a very reasonable thing to do as an easy win. Furthermore this should enhance the setup process for k8s-based environments as well.

This is only on Kubernetes I suppose since podman has no init containers. This has been changed very recently: the Kubernetes code is moving away from this repo to the helm charts… and there the setup is a Job.

Podman has init containers since 2021 (podman-container-tools/podman#6480). As such, this is of course possible? You can even apply kube yaml files to podman in part: https://docs.podman.io/en/latest/markdown/podman-kube-apply.1.html

This organically grew from a super simple pair of tools to a complicated beast over the course of multiple iterations. There is no documented design decision, and we carry over some old things (Exec wrapper) due to the lack of time to make things better. Getting familiar with the whole code base over a week-end was clearly never a target here. Things will improve as we drop migration and kubernetes from here.

This can always be said about anything. I understand that there are deadlines, but using that as an excuse to ruin a tool written from scratch, meant to be maintained for years to come, seems shortsighted to me. Even more so, since this means that the foundation for refactoring (which is inevitable for new features) will become harder, and the likelihood of new contributions is definitely low. Lastly, since everything is about AI... This also means that those tools will generate less accurate results because there is little data they can draw from.