feat(argocd client): Add handling of argocd-api-plaintext option

[grzesuav]

fixes #168

[Greyeye]

@grzesuav can you rrun the earthy so the doc will be updated to include the new parameters?

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/config/config_test.go

@@ -16,13 +16,15 @@ func TestNew(t *testing.T) {
 	v := viper.New()
 	v.Set("log-level", "info")
 	v.Set("argocd-api-insecure", "true")
+	v.Set("argocd-api-plaintext", "true")
 	v.Set("worst-conftest-state", "warning")
 	v.Set("repo-refresh-interval", "10m")
 
 	cfg, err := NewWithViper(v)
 	require.NoError(t, err)
 	assert.Equal(t, zerolog.InfoLevel, cfg.LogLevel)
 	assert.Equal(t, true, cfg.ArgoCDInsecure)
+	assert.Equal(t, true, cfg.ArgoCDPlainText)
 	assert.Equal(t, pkg.StateWarning, cfg.WorstConfTestState, "worst states can be overridden")
 	assert.Equal(t, time.Minute*10, cfg.RepoRefreshInterval)
 }

Feedback & Suggestions:

1. Missing Configuration Check: Ensure that the cfg.ArgoCDPlainText field is actually part of the cfg struct. If it is not, this test will always fail.
2. Security Consideration: Setting argocd-api-plaintext to true might have security implications. Ensure that this setting is intended and properly documented.
3. Test Coverage: Consider adding more test cases to cover different scenarios, such as when argocd-api-plaintext is set to false or not set at all. This will ensure that the configuration behaves as expected in all cases.

---

😼 Mergecat review of pkg/config/config.go

@@ -22,6 +22,7 @@ type ServerConfig struct {
 	ArgoCDPathPrefix    string `mapstructure:"argocd-api-path-prefix"`
 	ArgoCDInsecure      bool   `mapstructure:"argocd-api-insecure"`
 	ArgoCDNamespace     string `mapstructure:"argocd-api-namespace"`
+	ArgoCDPlainText     bool   `mapstructure:"argocd-api-plaintext"`
 	KubernetesConfig    string `mapstructure:"kubernetes-config"`
 	KubernetesType      string `mapstructure:"kubernetes-type"`
 	KubernetesClusterID string `mapstructure:"kubernetes-clusterid"`

Feedback & Suggestions:

1. Documentation: Ensure that the new ArgoCDPlainText field is documented properly, both in the code comments and any external documentation. This will help other developers understand its purpose and usage.
2. Validation: Consider adding validation logic to ensure that ArgoCDPlainText is used correctly in conjunction with other related fields. For example, if ArgoCDPlainText is true, certain security checks might need to be bypassed or additional warnings might need to be logged.
3. Security: If ArgoCDPlainText is intended to disable encryption or similar security features, ensure that there are adequate warnings and safeguards in place to prevent accidental misuse. This could include logging warnings or errors if the field is set to true in a production environment.

---

😼 Mergecat review of pkg/argo_client/client.go

@@ -27,6 +27,7 @@ func NewArgoClient(cfg config.ServerConfig) (*ArgoClient, error) {
 		AuthToken:       cfg.ArgoCDToken,
 		GRPCWebRootPath: cfg.ArgoCDPathPrefix,
 		Insecure:        cfg.ArgoCDInsecure,
+		PlainText:       cfg.ArgoCDPlainText,
 	}
 
 	log.Info().

Feedback & Suggestions:

1. Validation of New Configuration Option: Ensure that cfg.ArgoCDPlainText is properly validated before being used. This can prevent potential misconfigurations or security issues.

   if cfg.ArgoCDPlainText != true && cfg.ArgoCDPlainText != false {
       return nil, fmt.Errorf("invalid value for ArgoCDPlainText: %v", cfg.ArgoCDPlainText)
   }

2. Logging the New Option: Include the new PlainText option in the logging statement to ensure that all configuration options are visible in the logs for debugging purposes.

   log.Info().
       Str("server-addr", opts.ServerAddr).
       Int("auth-token", len(opts.AuthToken)).
       Str("grpc-web-root-path", opts.GRPCWebRootPath).
       Bool("insecure", cfg.ArgoCDInsecure).
       Bool("plain-text", cfg.ArgoCDPlainText).
       Msg("ArgoCD client configuration")

3. Security Consideration: If PlainText is related to communication security, ensure that its usage does not inadvertently expose sensitive data or reduce the security of the connection. Consider adding comments or documentation to explain the implications of this setting.

---

😼 Mergecat review of docs/usage.md

@@ -36,8 +36,9 @@ The full list of supported environment variables is described below:
 
 |Env Var|Description|Default Value|
 |-----------|-------------|------|
-|`KUBECHECKS_ARGOCD_API_INSECURE`|Enable to use insecure connections to the ArgoCD API server.|`false`|
+|`KUBECHECKS_ARGOCD_API_INSECURE`|Enable to use insecure connections over TLS to the ArgoCD API server.|`false`|
 |`KUBECHECKS_ARGOCD_API_NAMESPACE`|ArgoCD namespace where the application watcher will read Custom Resource Definitions (CRD) for Application and ApplicationSet resources.|`argocd`|
+|`KUBECHECKS_ARGOCD_API_PLAINTEXT`|Enable to use plaintext connections without TLS.|`false`|
 |`KUBECHECKS_ARGOCD_API_SERVER_ADDR`|ArgoCD API Server Address.|`argocd-server`|
 |`KUBECHECKS_ARGOCD_API_TOKEN`|ArgoCD API token.||
 |`KUBECHECKS_ENABLE_CONFTEST`|Set to true to enable conftest policy checking of manifests.|`false`|

Feedback & Suggestions:

1. Security Concerns: Adding the KUBECHECKS_ARGOCD_API_PLAINTEXT option introduces a potential security risk by allowing plaintext connections. Ensure that users are aware of the security implications of enabling this option. Consider adding a warning or note about the risks of using plaintext connections.

   +|`KUBECHECKS_ARGOCD_API_PLAINTEXT`|Enable to use plaintext connections without TLS. **Warning: This is insecure and not recommended for production environments.**|`false`|

2. Consistency in Descriptions: The description for KUBECHECKS_ARGOCD_API_INSECURE was updated to specify "insecure connections over TLS". Ensure that this terminology is clear and consistent with the rest of the documentation. If "insecure connections over TLS" means using self-signed certificates or ignoring certificate validation, make that explicit.

   +|`KUBECHECKS_ARGOCD_API_INSECURE`|Enable to use insecure connections over TLS (e.g., self-signed certificates or ignoring certificate validation) to the ArgoCD API server.|`false`|

3. Documentation Clarity: Ensure that the new environment variable KUBECHECKS_ARGOCD_API_PLAINTEXT is clearly documented in other relevant sections of the documentation, such as installation and configuration guides, to avoid any confusion for the users.

---

😼 Mergecat review of cmd/root.go

@@ -59,10 +59,11 @@ func init() {
 	stringFlag(flags, "argocd-api-server-addr", "ArgoCD API Server Address.",
 		newStringOpts().
 			withDefault("argocd-server"))
-	boolFlag(flags, "argocd-api-insecure", "Enable to use insecure connections to the ArgoCD API server.")
+	boolFlag(flags, "argocd-api-insecure", "Enable to use insecure connections over TLS to the ArgoCD API server.")
 	stringFlag(flags, "argocd-api-namespace", "ArgoCD namespace where the application watcher will read Custom Resource Definitions (CRD) for Application and ApplicationSet resources.",
 		newStringOpts().
 			withDefault("argocd"))
+	boolFlag(flags, "argocd-api-plaintext", "Enable to use plaintext connections without TLS.")
 	stringFlag(flags, "kubernetes-type", "Kubernetes Type One of eks, or local. Defaults to local.",
 		newStringOpts().
 			withChoices("eks", "local").

Feedback & Suggestions:

1. Security Concern: Adding the argocd-api-plaintext flag introduces a potential security risk by allowing plaintext connections. Ensure that this flag is used cautiously and that users are aware of the security implications. Consider adding a warning in the documentation or logs when this flag is enabled. 🔒

2. Consistency in Flag Descriptions: The description for argocd-api-insecure was updated to specify "insecure connections over TLS," which is good for clarity. Ensure that similar clarity is maintained across all flag descriptions. For example, you might want to specify that argocd-api-plaintext means "connections without TLS" explicitly, which you have done well. 👍

3. Error Handling: When dealing with flags that affect security, it might be beneficial to add validation logic to ensure that mutually exclusive flags (like argocd-api-insecure and argocd-api-plaintext) are not enabled simultaneously. This can prevent misconfigurations. 🚦

4. Logging: Consider adding a log entry when argocd-api-plaintext is enabled to alert users that they are using an insecure connection. This can help in debugging and ensuring that users are aware of the security posture of their setup. 📝

---

---

Dependency Review

Click to read mergecats review!

No suggestions found

[grzesuav]

@Greyeye done, thanks for pointer

[djeebus]

Thanks!