Allow the option to monitor argocd apps in other namespaces

[abhi-kapoor]

By default, we still run kubechecks in cluster mode that means it will work against all applications deployed within the cluster. However, if KUBECHECKS_ALLOWED_NAMESPACES is set with a comma separated list of namespaces, then it would only work against the resources deployed in those namespaces and ignore the other apps.

closes #123

[MeNsaaH]

https://github.com/zapier/kubechecks/blob/main/cmd/container.go#L74

I think in the case of this issue, we're trying to limit the namespace where kubechecks should autodiscover argocd apps and not the namespaces of the argocd deployments themselves.

[abhi-kapoor]

https://github.com/zapier/kubechecks/blob/main/cmd/container.go#L74

I think in the case of this issue, we're trying to limit the namespace where kubechecks should autodiscover argocd apps and not the namespaces of the argocd deployments themselves.

Are you suggesting something this instead? 534e23f

[MeNsaaH]

main/cmd/container.go#L74
I think in the case of this issue, we're trying to limit the namespace where kubechecks should autodiscover argocd apps and not the namespaces of the argocd deployments themselves.

Are you suggesting something this instead? 534e23f

Yeah. You need to limit the namespaces kubechecks monitors for apps

[djeebus]

Yeah. You need to limit the namespaces kubechecks monitors for apps

Kubechecks currently grabs all applications monitored by argocd, then monitors the argocd namespace for Application and ApplicationSet changes. This effectively means we already assume that argocd is in namespace mode, not cluster mode. The first action uses the ArgoCD API to get Applications in whatever scope it's configured for, and the second action is explicitly scoped to the argocd namespace.

We assumed that this request was to monitor explicit namespaces beyond the ArgoCD namespace, so kubechecks would function correctly when ArgoCD is in cluster mode. Did we miss something?

[MeNsaaH]

monitors the argocd namespace

@djeebus Seems I'm out of touch with the current implementation. My original thought was that kubechecks monitors all namespaces. Then, the scope of this issue should bel be to monitor all apps in all namespaces for when argocd runs in cluster mode🤔

[MeNsaaH]

Change is looking better now. Just need to adjust the variable to a single var instead of multiple vars for configuring namespaces

[MeNsaaH]

IMO, I think we should have one variable for configuring namespace, KUBECHECKS_MONITOR_APPS_NAMESPACES? this can default to argocd namespace, but when it's empty it monitors all namespaces. What do you think @djeebus ?

[djeebus]

I'd like to mimic argocd's behavior in this case. According to my reading of the docs, by default argocd will only monitor the argocd installation namespace (which defaults to argocd, but can be changed). You can add --application-namespaces=a,b,c, and it will monitor those namespaces additonally

... although after rereading the docs, it's not clear to me if this is "in addition to" the installation namespace, or "instead of". I suppose some testing is in order. If it's "instead of", then I agree with you, we should skip the argocd installation namespace when this is set. Mind reading the docs linked above and let me know if you agree?

[djeebus]

The docs do point out that those strings (as parsed by argocd) can be:

• A raw namespace: namespace-a
• A glob: namespace-*
• A regex: /^namespace-(\d+)$/

Think we need to support those in order to resolve this issue?

[abhi-kapoor]

My interpretation with the argo docs is that you have the default namespace for applications, which is argocd. If you would like to deploy applications outside of this namespace, then you need to:

• Update appProject with .spec.sourceNamespace with a list of additional namespaces to configure.
• Update the server and controller with --application-namespaces which I assume would include all the namespaces

I am ok with either approach, but wanted to point out that it needs to be configured in multiple places.

[MeNsaaH]

I went through the docs and it's not entirely clear if these namespaces are in addition to or including the argocd namespace. Although, I'm thinking it's the former.

In that case, I'm happy with having KUBECHECKS_ARGOCD_ADDITIONAL_NAMESPACES. We just need to cater for the *, which is when all namespaces are included.

[abhi-kapoor]

@MeNsaaH @djeebus The recent changes I pushed bring us closer to how ArgoCD handles the informers. Rather than checking for each namespace allowed, we now watch for resources in all namespaces and filter for the allowed ones only. This list of additional namespaces supports * or any of the other glob patterns.

changes have been implemented

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of charts/kubechecks/Chart.yaml

@@ -1,7 +1,7 @@
 apiVersion: v2
 name: kubechecks
 description: A Helm chart for kubechecks
-version: 0.5.2
+version: 0.5.3
 type: application
 maintainers:
   - name: zapier

Feedback & Suggestions: The version bump from 0.5.2 to 0.5.3 is appropriate if there are backward-compatible bug fixes or minor improvements. Ensure that the changes in the codebase reflect this version update. 📈

---

😼 Mergecat review of charts/kubechecks/values.yaml

@@ -7,6 +7,7 @@ commonLabels: {}
 configMap:
   create: false
   env: {}
+  # KUBECHECKS_ADDITIONAL_APPS_NAMESPACES: "*"
   # KUBECHECKS_ARGOCD_API_INSECURE: "false"
   # KUBECHECKS_ARGOCD_API_PATH_PREFIX: /
   # KUBECHECKS_ARGOCD_API_NAMESPACE: argocd

Feedback & Suggestions:

1. Security Consideration: The addition of # KUBECHECKS_ADDITIONAL_APPS_NAMESPACES: "*" suggests a wildcard for namespaces. If this is intended to be used, ensure that it is well-documented and that users are aware of the potential security implications of allowing access to all namespaces. Consider providing guidance on how to specify specific namespaces to limit access.

2. Documentation: Since this is a commented-out configuration, it would be helpful to include a brief comment explaining the purpose and potential use cases for KUBECHECKS_ADDITIONAL_APPS_NAMESPACES. This will aid users in understanding when and why they might want to enable or configure this setting.

---

😼 Mergecat review of localdev/kubechecks/values.yaml

@@ -1,6 +1,8 @@
 configMap:
   create: true
   env:
+    GRPC_ENFORCE_ALPN_ENABLED: false
+    KUBECHECKS_ADDITIONAL_APPS_NAMESPACES: "*"
     KUBECHECKS_LOG_LEVEL: debug
     KUBECHECKS_ENABLE_WEBHOOK_CONTROLLER: "false"
     KUBECHECKS_ARGOCD_API_INSECURE: "true"

Feedback & Suggestions:

• The added lines GRPC_ENFORCE_ALPN_ENABLED: false and KUBECHECKS_ADDITIONAL_APPS_NAMESPACES: "*" are already present in the original code. Ensure that these additions are necessary and not duplicating existing entries. Duplicates can lead to confusion and potential misconfigurations. 🧐

---

😼 Mergecat review of cmd/root.go

@@ -118,6 +118,7 @@ func init() {
 	stringFlag(flags, "replan-comment-msg", "comment message which re-triggers kubechecks on PR.",
 		newStringOpts().
 			withDefault("kubechecks again"))
+	stringSliceFlag(flags, "additional-apps-namespaces", "Additional namespaces other than the ArgoCDNamespace to monitor for applications.")
 
 	panicIfError(viper.BindPFlags(flags))
 	setupLogOutput()

Feedback & Suggestions:

1. Documentation Clarity: 📚 The description for the new flag "additional-apps-namespaces" could be more explicit. Consider specifying the format expected for the namespaces (e.g., comma-separated values) to avoid user confusion.

2. Validation: 🔍 Ensure that the values provided for "additional-apps-namespaces" are validated. This could include checking for valid namespace names or ensuring no duplicates are present. This will help prevent runtime errors or unexpected behavior.

3. Security Consideration: 🔒 If the namespaces are being used to access resources, ensure that there are appropriate checks and permissions in place to avoid unauthorized access.

4. Testing: 🧪 Make sure to add tests that cover this new functionality, especially if it impacts critical application behavior. This will help maintain code reliability and prevent future regressions.

---

😼 Mergecat review of cmd/controller.go

@@ -49,13 +49,13 @@ var ControllerCmd = &cobra.Command{
 
 		// watch app modifications, if necessary
 		if cfg.MonitorAllApplications {
-			appWatcher, err := app_watcher.NewApplicationWatcher(ctr)
+			appWatcher, err := app_watcher.NewApplicationWatcher(ctr, ctx)
 			if err != nil {
 				log.Fatal().Err(err).Msg("failed to create watch applications")
 			}
 			go appWatcher.Run(ctx, 1)
 
-			appSetWatcher, err := app_watcher.NewApplicationSetWatcher(ctr)
+			appSetWatcher, err := app_watcher.NewApplicationSetWatcher(ctr, ctx)
 			if err != nil {
 				log.Fatal().Err(err).Msg("failed to create watch application sets")
 			}

Feedback & Suggestions:

1. Context Propagation: 🧠 The change to pass ctx to NewApplicationWatcher and NewApplicationSetWatcher is a good practice for context propagation. Ensure that these functions are designed to handle context properly, including respecting cancellation and deadlines.

2. Error Handling: ⚠️ Consider handling errors more gracefully. Instead of using log.Fatal(), which exits the program, you might want to handle errors in a way that allows for cleanup or retries, depending on the application's requirements.

3. Concurrency: 🚀 When launching goroutines, ensure that the context is used to manage their lifecycle. This helps in gracefully shutting down the application and avoiding resource leaks.

4. Documentation: 📚 Update any relevant documentation or comments to reflect the changes in function signatures, especially if these functions are part of a public API.

---

😼 Mergecat review of pkg/app_watcher/appset_watcher_test.go

@@ -15,7 +15,11 @@ import (
 )
 
 func initTestObjectsForAppSets(t *testing.T) *ApplicationSetWatcher {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	cfg, err := config.New()
+	cfg.AdditionalAppsNamespaces = []string{"*"}
 	// Handle the error appropriately, e.g., log it or fail the test
 	require.NoError(t, err, "failed to create config")
 
@@ -53,10 +57,9 @@ func initTestObjectsForAppSets(t *testing.T) *ApplicationSetWatcher {
 		vcsToArgoMap:         appdir.NewVcsToArgoMap("vcs-username"),
 	}
 
-	appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*1, cfg)
+	appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*1, cfg, ctx)
 	ctrl.appInformer = appInformer
 	ctrl.appLister = appLister
-
 	return ctrl
 }
 

Feedback & Suggestions:

1. Context Usage: The addition of a context (ctx) in initTestObjectsForAppSets is a good practice for managing the lifecycle of operations. However, ensure that the context is actually utilized within newApplicationSetInformerAndLister. If it's not used, passing it might be unnecessary. 🧠

2. Wildcard Namespace: Setting cfg.AdditionalAppsNamespaces = []string{"*"} allows the watcher to monitor all namespaces. This can be powerful but also risky if not intended, as it might lead to performance issues or security concerns by processing more data than necessary. Consider whether this is the desired behavior and document it clearly. 🔍

3. Context Cancellation: The defer cancel() is correctly placed to ensure the context is canceled when initTestObjectsForAppSets exits. This is important to prevent resource leaks. Ensure that any goroutines or operations using this context handle cancellation properly. 🚦

---

😼 Mergecat review of docs/usage.md

@@ -36,6 +36,7 @@ The full list of supported environment variables is described below:
 
 |Env Var|Description|Default Value|
 |-----------|-------------|------|
+|`KUBECHECKS_ADDITIONAL_APPS_NAMESPACES`|Additional namespaces other than the ArgoCDNamespace to monitor for applications.|`[]`|
 |`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`|

Feedback & Suggestions:

1. Clarification on Default Value: The default value for KUBECHECKS_ADDITIONAL_APPS_NAMESPACES is listed as []. It would be helpful to clarify if this means an empty list or if there are specific namespaces that should be included by default. Consider providing an example or further explanation if applicable.

2. Consistency in Descriptions: Ensure that the description for KUBECHECKS_ADDITIONAL_APPS_NAMESPACES is consistent in style and detail with other environment variables. For instance, if other variables include examples or additional context, consider doing the same here.

3. Security Consideration: If KUBECHECKS_ADDITIONAL_APPS_NAMESPACES involves monitoring additional namespaces, it might be worth mentioning any security implications or permissions required to access these namespaces.

---

😼 Mergecat review of pkg/config/config.go

@@ -2,6 +2,7 @@ package config
 
 import (
 	"reflect"
+	"strings"
 	"time"
 
 	"github.com/mitchellh/mapstructure"
@@ -70,6 +71,7 @@ type ServerConfig struct {
 	WorstPreupgradeState pkg.CommitState `mapstructure:"worst-preupgrade-state"`
 
 	// misc
+	AdditionalAppsNamespaces []string      `mapstructure:"additional-apps-namespaces"`
 	FallbackK8sVersion       string        `mapstructure:"fallback-k8s-version"`
 	LabelFilter              string        `mapstructure:"label-filter"`
 	LogLevel                 zerolog.Level `mapstructure:"log-level"`
@@ -107,6 +109,12 @@ func NewWithViper(v *viper.Viper) (ServerConfig, error) {
 				return time.ParseDuration(input)
 			}
 
+			if in.String() == "string" && out.String() == "[]string" {
+				input := value.(string)
+				ns := strings.Split(input, ",")
+				return ns, nil
+			}
+
 			return value, nil
 		}
 	}); err != nil {

Feedback & Suggestions:

1. Type Safety: Ensure that the value is indeed a string before casting it. Consider using a type assertion with a check to avoid potential panics if the type is not as expected. For example:

   input, ok := value.(string)
   if !ok {
       return nil, errors.New("expected a string for AdditionalAppsNamespaces")
   }

2. Trim Whitespace: When splitting the string into a slice, it might be beneficial to trim whitespace from each element to avoid issues with unexpected spaces. You can use:

   ns := strings.Split(input, ",")
   for i := range ns {
       ns[i] = strings.TrimSpace(ns[i])
   }

3. Error Handling: Consider adding error handling for the DecodeHook function. If the conversion fails, it might be useful to return an error with a descriptive message.

These improvements will enhance the robustness and reliability of the code. 🛡️

---

😼 Mergecat review of pkg/config/config_test.go

@@ -19,6 +19,7 @@ func TestNew(t *testing.T) {
 	v.Set("argocd-api-plaintext", "true")
 	v.Set("worst-conftest-state", "warning")
 	v.Set("repo-refresh-interval", "10m")
+	v.Set("additional-apps-namespaces", "default,kube-system")
 
 	cfg, err := NewWithViper(v)
 	require.NoError(t, err)
@@ -27,4 +28,5 @@ func TestNew(t *testing.T) {
 	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)
+	assert.Equal(t, []string{"default", "kube-system"}, cfg.AdditionalAppsNamespaces)
 }

Feedback & Suggestions:

1. Validation of Configuration Values: Ensure that the additional-apps-namespaces configuration is validated within the NewWithViper function. This will help prevent potential issues if the input format changes or if invalid namespaces are provided. 🛡️

2. Test Coverage: Consider adding more test cases to cover edge cases, such as when additional-apps-namespaces is empty or contains invalid namespace names. This will help ensure robustness. 🧪

3. Documentation: If not already done, update any relevant documentation to reflect the addition of the additional-apps-namespaces configuration option. This will help maintain clarity for future developers. 📚

---

😼 Mergecat review of pkg/app_watcher/app_watcher_test.go

@@ -16,7 +16,11 @@ import (
 )
 
 func initTestObjects(t *testing.T) *ApplicationWatcher {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	cfg, err := config.New()
+	cfg.AdditionalAppsNamespaces = []string{"*"}
 	// Handle the error appropriately, e.g., log it or fail the test
 	require.NoError(t, err, "failed to create config")
 
@@ -40,7 +44,7 @@ func initTestObjects(t *testing.T) *ApplicationWatcher {
 		vcsToArgoMap:         appdir.NewVcsToArgoMap("vcs-username"),
 	}
 
-	appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*1, cfg)
+	appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*1, cfg, ctx)
 	ctrl.appInformer = appInformer
 	ctrl.appLister = appLister
 
@@ -254,3 +258,49 @@ func TestCanProcessApp(t *testing.T) {
 		})
 	}
 }
+
+func TestIsAppNamespaceAllowed(t *testing.T) {
+	tests := map[string]struct {
+		expected bool
+		cfg      config.ServerConfig
+		meta     *metav1.ObjectMeta
+	}{
+		"All namespaces for application are allowed": {
+			expected: true,
+			cfg:      config.ServerConfig{AdditionalAppsNamespaces: []string{"*"}},
+			meta:     &metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
+		},
+		"Specific namespaces for application are allowed": {
+			expected: false,
+			cfg:      config.ServerConfig{AdditionalAppsNamespaces: []string{"default", "kube-system"}},
+			meta:     &metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
+		},
+		"Wildcard namespace for application is allowed": {
+			expected: true,
+			cfg:      config.ServerConfig{AdditionalAppsNamespaces: []string{"default-*"}},
+			meta:     &metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
+		},
+		"Invalid characters in namespace for application are not allowed": {
+			expected: false,
+			cfg:      config.ServerConfig{AdditionalAppsNamespaces: []string{"<default-*", "kube-system"}},
+			meta:     &metav1.ObjectMeta{Name: "test-app-1", Namespace: "default-ns"},
+		},
+		"Specific namespace for application set is allowed": {
+			expected: true,
+			cfg:      config.ServerConfig{AdditionalAppsNamespaces: []string{"<default-*>", "kube-system"}},
+			meta:     &metav1.ObjectMeta{Name: "test-appset-1", Namespace: "kube-system"},
+		},
+		"Regex in namespace for application set is allowed": {
+			expected: true,
+			cfg:      config.ServerConfig{AdditionalAppsNamespaces: []string{"/^((?!kube-system).)*$/"}},
+			meta:     &metav1.ObjectMeta{Name: "test-appset-1", Namespace: "kube-namespace"},
+		},
+	}
+
+	for testName, test := range tests {
+		t.Run(testName, func(t *testing.T) {
+			actual := isAppNamespaceAllowed(test.meta, test.cfg)
+			assert.Equal(t, test.expected, actual)
+		})
+	}
+}

Feedback & Suggestions:

1. Context Management: The addition of context management in initTestObjects is a good practice for managing goroutines. However, ensure that the context is used effectively throughout the application to handle cancellations properly. Consider passing the context to other functions that might benefit from it.

2. Configuration Change: The line cfg.AdditionalAppsNamespaces = []string{"*"} sets the configuration to allow all namespaces. Ensure this change aligns with your security policies, as it might introduce unintended access to namespaces.

3. Function Signature Change: The change in the function signature of newApplicationInformerAndLister to include ctx is appropriate given the context management. Ensure that the function implementation is updated to utilize the context for any long-running operations.

4. Test Coverage: The new test TestIsAppNamespaceAllowed is a valuable addition to ensure namespace validation logic. However, consider adding more edge cases, such as empty namespace lists or invalid regex patterns, to further strengthen the test coverage.

5. Security Considerations: Be cautious with regex patterns in namespace configurations, as they can be complex and potentially introduce security vulnerabilities if not handled correctly. Ensure proper validation and sanitization of these patterns.

6. Code Comments: Adding comments to explain the purpose of new test cases and configuration changes can improve code readability and maintainability. 📚

---

😼 Mergecat review of pkg/app_watcher/appset_watcher.go

@@ -8,13 +8,15 @@ import (
 
 	appv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned"
-	informers "github.com/argoproj/argo-cd/v2/pkg/client/informers/externalversions/application/v1alpha1"
 	applisters "github.com/argoproj/argo-cd/v2/pkg/client/listers/application/v1alpha1"
 	"github.com/rs/zerolog/log"
 	"github.com/zapier/kubechecks/pkg/appdir"
 	"github.com/zapier/kubechecks/pkg/config"
 	"github.com/zapier/kubechecks/pkg/container"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	apiruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/tools/cache"
 )
 
@@ -28,7 +30,7 @@ type ApplicationSetWatcher struct {
 }
 
 // NewApplicationSetWatcher creates new instance of ApplicationWatcher.
-func NewApplicationSetWatcher(ctr container.Container) (*ApplicationSetWatcher, error) {
+func NewApplicationSetWatcher(ctr container.Container, ctx context.Context) (*ApplicationSetWatcher, error) {
 	if ctr.KubeClientSet == nil {
 		return nil, fmt.Errorf("kubeCfg cannot be nil")
 	}
@@ -37,7 +39,7 @@ func NewApplicationSetWatcher(ctr container.Container) (*ApplicationSetWatcher,
 		vcsToArgoMap:         ctr.VcsToArgoMap,
 	}
 
-	appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*30, ctr.Config)
+	appInformer, appLister := ctrl.newApplicationSetInformerAndLister(time.Second*30, ctr.Config, ctx)
 
 	ctrl.appInformer = appInformer
 	ctrl.appLister = appLister
@@ -61,12 +63,40 @@ func (ctrl *ApplicationSetWatcher) Run(ctx context.Context) {
 	<-ctx.Done()
 }
 
-func (ctrl *ApplicationSetWatcher) newApplicationSetInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig) (cache.SharedIndexInformer, applisters.ApplicationSetLister) {
-	log.Debug().Msgf("Creating ApplicationSet informer with namespace: %s", cfg.ArgoCDNamespace)
-	informer := informers.NewApplicationSetInformer(ctrl.applicationClientset, cfg.ArgoCDNamespace, refreshTimeout,
+func (ctrl *ApplicationSetWatcher) newApplicationSetInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig, ctx context.Context) (cache.SharedIndexInformer, applisters.ApplicationSetLister) {
+	watchNamespace := cfg.ArgoCDNamespace
+	// If we have at least one additional namespace configured, we need to
+	// watch on them all.
+	if len(cfg.AdditionalAppsNamespaces) > 0 {
+		watchNamespace = ""
+	}
+
+	informer := cache.NewSharedIndexInformer(
+		&cache.ListWatch{
+			ListFunc: func(options metav1.ListOptions) (apiruntime.Object, error) {
+				// We are only interested in apps that exist in namespaces the
+				// user wants to be enabled.
+				appList, err := ctrl.applicationClientset.ArgoprojV1alpha1().ApplicationSets(watchNamespace).List(ctx, options)
+				if err != nil {
+					return nil, err
+				}
+				newItems := []appv1alpha1.ApplicationSet{}
+				for _, appSet := range appList.Items {
+					if isAppNamespaceAllowed(&appSet.ObjectMeta, cfg) {
+						newItems = append(newItems, appSet)
+					}
+				}
+				appList.Items = newItems
+				return appList, nil
+			},
+			WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
+				return ctrl.applicationClientset.ArgoprojV1alpha1().ApplicationSets(watchNamespace).Watch(ctx, options)
+			},
+		},
+		&appv1alpha1.ApplicationSet{},
+		refreshTimeout,
 		cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc},
 	)
-
 	AppSetLister := applisters.NewApplicationSetLister(informer.GetIndexer())
 	if _, err := informer.AddEventHandler(
 		cache.ResourceEventHandlerFuncs{

Feedback & Suggestions:

1. Context Propagation: 👍 It's great that you've added context propagation to the NewApplicationSetWatcher and newApplicationSetInformerAndLister functions. This is important for managing the lifecycle of requests and ensuring that operations can be canceled if needed.

2. Namespace Handling: The logic to handle multiple namespaces by setting watchNamespace to an empty string is a good approach. However, ensure that isAppNamespaceAllowed is well-tested to avoid inadvertently filtering out desired namespaces.

3. Error Handling: Consider adding more detailed logging for errors in the ListFunc and WatchFunc. This will help in diagnosing issues if the list or watch operations fail. For example, you could log the options being used when an error occurs.

4. Performance Consideration: The filtering of appList.Items could potentially be optimized. If the list of applications is large, this operation might become a bottleneck. Consider if there's a way to offload some of this filtering to the API server, if possible.

5. Imports: The removal of the informers import is appropriate since it's no longer used. Ensure that all other imports are necessary to keep the code clean and efficient.

6. Code Comments: The comments added to explain the logic for handling multiple namespaces are helpful. Ensure that these comments are kept up-to-date with any future changes to the logic.

Overall, the changes improve the flexibility and robustness of the application set watcher. Keep up the good work! 🚀

---

😼 Mergecat review of pkg/app_watcher/app_watcher.go

@@ -9,15 +9,18 @@ import (
 
 	appv1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned"
-	informers "github.com/argoproj/argo-cd/v2/pkg/client/informers/externalversions/application/v1alpha1"
 	applisters "github.com/argoproj/argo-cd/v2/pkg/client/listers/application/v1alpha1"
+	"github.com/argoproj/argo-cd/v2/util/glob"
 	"github.com/rs/zerolog/log"
-	"github.com/zapier/kubechecks/pkg/appdir"
-	"github.com/zapier/kubechecks/pkg/container"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	apiruntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/client-go/tools/cache"
 
+	"github.com/zapier/kubechecks/pkg/appdir"
 	"github.com/zapier/kubechecks/pkg/config"
+	"github.com/zapier/kubechecks/pkg/container"
 )
 
 // ApplicationWatcher is the controller that watches ArgoCD Application resources via the Kubernetes API
@@ -34,7 +37,7 @@ type ApplicationWatcher struct {
 //   - kubeCfg is the Kubernetes configuration.
 //   - vcsToArgoMap is the mapping between VCS and Argo applications.
 //   - cfg is the server configuration.
-func NewApplicationWatcher(ctr container.Container) (*ApplicationWatcher, error) {
+func NewApplicationWatcher(ctr container.Container, ctx context.Context) (*ApplicationWatcher, error) {
 	if ctr.KubeClientSet == nil {
 		return nil, fmt.Errorf("kubeCfg cannot be nil")
 	}
@@ -43,7 +46,7 @@ func NewApplicationWatcher(ctr container.Container) (*ApplicationWatcher, error)
 		vcsToArgoMap:         ctr.VcsToArgoMap,
 	}
 
-	appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*30, ctr.Config)
+	appInformer, appLister := ctrl.newApplicationInformerAndLister(time.Second*30, ctr.Config, ctx)
 
 	ctrl.appInformer = appInformer
 	ctrl.appLister = appLister
@@ -116,6 +119,11 @@ func (ctrl *ApplicationWatcher) onApplicationDeleted(obj interface{}) {
 	ctrl.vcsToArgoMap.DeleteApp(app)
 }
 
+// isAppNamespaceAllowed is used by both the ApplicationWatcher and the ApplicationSetWatcher
+func isAppNamespaceAllowed(meta *metav1.ObjectMeta, cfg config.ServerConfig) bool {
+	return meta.Namespace == cfg.ArgoCDNamespace || glob.MatchStringInList(cfg.AdditionalAppsNamespaces, meta.Namespace, glob.REGEXP)
+}
+
 /*
 newApplicationInformerAndLister, is part of the ApplicationWatcher struct. It sets up a Kubernetes SharedIndexInformer
 and a Lister for Argo CD Applications.
@@ -127,12 +135,41 @@ that need to observe the object.
 newApplicationInformerAndLister use the data from the informer's cache to provide a read-optimized view of the cache which reduces
 the load on the API Server and hides some complexity.
 */
-func (ctrl *ApplicationWatcher) newApplicationInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig) (cache.SharedIndexInformer, applisters.ApplicationLister) {
-	log.Debug().Msgf("Creating Application informer with namespace: %s", cfg.ArgoCDNamespace)
-	informer := informers.NewApplicationInformer(ctrl.applicationClientset, cfg.ArgoCDNamespace, refreshTimeout,
+func (ctrl *ApplicationWatcher) newApplicationInformerAndLister(refreshTimeout time.Duration, cfg config.ServerConfig, ctx context.Context) (cache.SharedIndexInformer, applisters.ApplicationLister) {
+
+	watchNamespace := cfg.ArgoCDNamespace
+	// If we have at least one additional namespace configured, we need to
+	// watch on them all.
+	if len(cfg.AdditionalAppsNamespaces) > 0 {
+		watchNamespace = ""
+	}
+
+	informer := cache.NewSharedIndexInformer(
+		&cache.ListWatch{
+			ListFunc: func(options metav1.ListOptions) (apiruntime.Object, error) {
+				// We are only interested in apps that exist in namespaces the
+				// user wants to be enabled.
+				appList, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications(watchNamespace).List(ctx, options)
+				if err != nil {
+					return nil, err
+				}
+				newItems := []appv1alpha1.Application{}
+				for _, app := range appList.Items {
+					if isAppNamespaceAllowed(&app.ObjectMeta, cfg) {
+						newItems = append(newItems, app)
+					}
+				}
+				appList.Items = newItems
+				return appList, nil
+			},
+			WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) {
+				return ctrl.applicationClientset.ArgoprojV1alpha1().Applications(watchNamespace).Watch(ctx, options)
+			},
+		},
+		&appv1alpha1.Application{},
+		refreshTimeout,
 		cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc},
 	)
-
 	lister := applisters.NewApplicationLister(informer.GetIndexer())
 	if _, err := informer.AddEventHandler(
 		cache.ResourceEventHandlerFuncs{

Feedback & Suggestions:

1. Context Usage: 🧠 The addition of ctx context.Context to the NewApplicationWatcher and newApplicationInformerAndLister functions is a good practice for managing cancellation and timeouts. Ensure that this context is consistently used throughout the application to handle any potential long-running operations.

2. Namespace Watching Logic: 🌐 The logic for determining the watchNamespace is a useful enhancement for flexibility. However, consider logging a message when switching to watch all namespaces for better traceability and debugging.

3. Security Consideration: 🔒 When using glob.MatchStringInList, ensure that the patterns in cfg.AdditionalAppsNamespaces are validated to prevent any potential security issues related to improper namespace access.

4. Performance Consideration: 🚀 The filtering of applications based on namespace in the ListFunc is a good optimization. However, if the list of applications is large, consider the performance impact of iterating over all items. Profiling this section could help identify any bottlenecks.

5. Error Handling: ⚠️ The error handling in the ListFunc and WatchFunc is appropriate. Consider adding more context to the error messages to aid in debugging, such as including the namespace being watched.

6. Code Clarity: 📚 The introduction of isAppNamespaceAllowed improves code readability by encapsulating the namespace checking logic. Ensure that this function is well-documented, especially regarding the expected format of cfg.AdditionalAppsNamespaces.

---

---

Dependency Review

Click to read mergecats review!

No suggestions found