Mixing infra code with logic code

[drpaneas]

the file https://github.com/iam-veeramalla/argo-rollouts-manager/blob/main/controllers/default.go has some nasty bad practices such as hardcoding SHA sums and images. This is not ok. These should come as part of the config, and not built-in the go build code.

Also the comments are off.

[jaideepr97]

@drpaneas why do you think hardcoding a SHA there is bad practice? That SHA only represents the default digest for the rollouts controller image that will be deployed as a fallback if the user has not specified their own custom image that they want to run in the CR

This method allows allows us to pin a specific image as the back up option

[drpaneas]

That SHA only represents the default digest for the rollouts controller image that will be deployed as a fallback if the user has not specified their own custom image that they want to run in the CR. This method allows allows us to pin a specific image as the back up option

I totally agree with you. But ... :D

... this is deployment/infra logic. It should be passed to down to the operator via a building pipeline for example, or makefile via ENV variable during building. If you like to hardcode it you can do it in separate file that is not code (can be a makefile, can be just a plaintext) or whatever. Then your building system can pick this up and pass it to the operator during build. This way your infra logic (deployment) is separate and if you need to change it you can do it easily.

Let me give you a simple example:

Suppose you have a argo-rollouts-manager operator deployment that runs a containerized application, and you've hardcoded the SHA of the fallback container image in case of a problem. The deployment is running in production, and everything is working fine. However, a few weeks later, a critical security vulnerability is discovered in the container image that your application is using and you have to patch the CVE. To address the vulnerability, you need to update the container image to a new version that has the fix (including the fallback image). So the engineering team builds the new container and release it. But since you have hardcoded the SHA of the fallback container image in your code, the developer or release manager forgot to update the Go code (didn't even know there is hardcoded SHA lingering in the defaults.go somewhere). As a result problems are slipped through and updating the image becomes much harder. You'll need to update the code to use the new SHA, rebuild the container image, and redeploy the updated image to your Kubernetes cluster. This process could take a significant amount of time, and during this time, your application would still be vulnerable to the security issue.

Additionally, hardcoding the SHA of the fallback container image could make it harder to roll back to an earlier version of the container image if a problem occurs with the updated image. Since the fallback image's SHA is hardcoded, you would need to modify the code again to switch back to the previous version.

In contrast, if you used a configuration file or environment variable or configmap or something that can be kubectl applied to specify the fallback container image, updating or rolling back the image would be much easier. You could update the configuration file or environment variable to point to the new or previous image, respectively, and then restart your application or redeploy it without modifying the code. So let's say the building system calculates the SHA and sets this via build flag. In that case the operator would know what is the fallback image without having this information hardcoded, so nobody from the development team needs to update this ever.

This example highlights the importance of separating infrastructure logic from application code and using configuration files or environment variables instead of hardcoding values. Samething for the other values, not just SHA.

[jgwest]

This should now be handled by https://github.com/argoproj-labs/argo-rollouts-manager/issues/33