Allow the option to run kubechecks in namespaced scope

[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.

[djeebus]

let's pass in the shared ctx for common cancellation. just keep adding ctx context.Context to the function calls until you can find a function that already has one

[djeebus]

Same here

[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 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:

1. Security Concern: The addition of KUBECHECKS_ADDITIONAL_APPS_NAMESPACES: "*" could potentially expose all namespaces to the application. Ensure that this is intentional and consider limiting the scope to only necessary namespaces to adhere to the principle of least privilege. 🔒

2. Boolean Value: The GRPC_ENFORCE_ALPN_ENABLED is set to false without quotes. For consistency and to avoid potential parsing issues, consider using quotes for boolean values, like "false". This can help prevent unexpected behavior in YAML parsing. 🧩

---

😼 Mergecat review of cmd/root.go

+	stringSliceFlag(flags, "additional-apps-namespaces", "Additional namespaces other than the ArgoCDNamespace to monitor for applications.")

Feedback & Suggestions:

1. Validation of Input: Ensure that the input for additional-apps-namespaces is validated. Since this is a list of namespaces, it might be beneficial to check that each entry is a valid Kubernetes namespace name. This can prevent potential issues later in the application flow.

2. Documentation: Consider adding more detailed documentation or comments about the purpose and usage of the additional-apps-namespaces flag. This will help other developers understand its role and how it interacts with the rest of the system.

3. Security Considerations: If these namespaces are used to access resources, ensure that proper permissions and security checks are in place to prevent unauthorized access.

4. Default Value: If there is a common use case or a default set of namespaces that should be monitored, consider providing a default value or example usage in the documentation.

---

😼 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 addition of ctx to the NewApplicationWatcher and NewApplicationSetWatcher functions is a good practice for context propagation. This ensures that the context is consistently passed down, allowing for better control over cancellation and timeouts. Ensure that the functions being called are designed to handle the context properly.

2. Error Handling: ⚠️ While the current error handling with log.Fatal() is straightforward, consider whether this is the best approach for your application. Using log.Fatal() will terminate the program immediately, which might not always be desirable. If you want to allow for graceful shutdowns or retries, consider alternative error handling strategies.

3. Documentation: 📚 If the NewApplicationWatcher and NewApplicationSetWatcher functions have been updated to accept a context, ensure that their documentation is updated accordingly to reflect this change.

---

😼 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: "*" as a commented-out line suggests that there might be an intention to allow access to all namespaces. If this is uncommented and used, it could pose a security risk by granting overly broad access. Ensure that this is only enabled if absolutely necessary and consider specifying only the required namespaces instead of using a wildcard.

2. Documentation: If this configuration option is intended for use, consider adding a comment explaining its purpose and any security implications. This will help future maintainers understand the context and make informed decisions.

3. Consistency: Ensure that any new configuration options added are consistently documented and follow the same commenting style as existing options. This helps maintain readability and consistency across the configuration file.

---

😼 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. Type Safety: Ensure that cfg.AdditionalAppsNamespaces is indeed a []string. If the type is different, the test will fail. Consider adding a type assertion or conversion if necessary. 🛡️

2. Configuration Parsing: The v.Set("additional-apps-namespaces", "default,kube-system") assumes that the NewWithViper function correctly parses the comma-separated string into a slice of strings. Ensure that this parsing logic is implemented and tested separately to avoid unexpected behavior. 🔍

3. Test Coverage: If additional-apps-namespaces is a new feature, consider adding more tests to cover edge cases, such as empty strings or strings with unexpected formats. This will help ensure robustness. 📈

---

😼 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 set to an empty list []. It might be helpful to clarify what this means in practice. For example, does it mean no additional namespaces are monitored by default? Adding a brief note could enhance understanding.

2. Security Consideration: If KUBECHECKS_ADDITIONAL_APPS_NAMESPACES is intended to monitor additional namespaces, ensure that users are aware of any potential security implications, such as inadvertently exposing sensitive data from other namespaces. A note or warning could be beneficial.

3. Consistency Check: Ensure that the description aligns with the functionality. If there are any specific requirements or limitations for the namespaces that can be added, it would be helpful to mention them.

---

😼 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. Security Consideration: When splitting strings to create a slice, ensure that the input is sanitized to prevent potential injection attacks. Consider trimming whitespace from each element in the resulting slice to avoid unexpected behavior due to leading or trailing spaces. You can achieve this by using strings.TrimSpace on each element after splitting.

2. Error Handling: The current implementation assumes that the input value is always a string. It would be safer to check the type assertion when casting value.(string) to prevent potential panics if the input is not of the expected type. You can handle this by using a type assertion with a check, like:

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

3. Documentation: Consider adding comments to explain the purpose of the AdditionalAppsNamespaces field and the logic for converting a comma-separated string into a slice. This will help future maintainers understand the intent and usage of this configuration.

---

😼 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 used effectively throughout the function and any functions it calls. If newApplicationSetInformerAndLister does not utilize the context for cancellation or timeout, consider whether passing it is necessary.

2. Configuration Change: The line cfg.AdditionalAppsNamespaces = []string{"*"} modifies the configuration to include all namespaces. This change could have significant implications, especially in a testing environment. Ensure that this change is intentional and that tests are designed to handle resources across all namespaces. If this is not the intended behavior, consider specifying the namespaces explicitly.

3. Code Clarity: The addition of the context setup at the beginning of the function is clear and follows good practices. Ensure that any resources or operations that rely on this context are aware of its cancellation to prevent resource leaks or unexpected behavior.

4. Testing Implications: With the configuration change to include all namespaces, verify that existing tests do not assume a single namespace context. This could lead to false positives or negatives in test results.

Overall, the changes introduce good practices, but be cautious about the broader implications of the configuration change. 🛠️

---

😼 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 handling cancellation. However, ensure that the context is appropriately used throughout the application lifecycle to avoid potential resource leaks. 🧹

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

3. Function Signature Change: The change in the function signature of newApplicationInformerAndLister to include ctx is a positive step towards better resource management. Ensure that all calls to this function are updated accordingly. 📞

4. Test Coverage: The new test TestIsAppNamespaceAllowed enhances test coverage for namespace validation. Ensure that the test cases cover all edge cases, especially around regex and wildcard handling. Consider adding comments to explain complex test cases for future maintainers. 🧪

5. Regex Handling: In the test case "Regex in namespace for application set is allowed", ensure that the regex is correctly interpreted and does not introduce security vulnerabilities. Regex can be complex and error-prone, so thorough testing is essential. 🔍

---

😼 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: 🧠 The addition of ctx context.Context to the NewApplicationSetWatcher and newApplicationSetInformerAndLister functions is a good move for better context management. Ensure that this context is used consistently throughout the application to handle cancellations and timeouts effectively.

2. Namespace Handling: 🌐 The logic to handle multiple namespaces by setting watchNamespace to an empty string is a good approach. However, consider adding comments or documentation to explain this behavior clearly, as it might not be immediately obvious to all developers.

3. Error Handling: ⚠️ In the ListFunc, if an error occurs while listing ApplicationSets, it is returned immediately. Consider logging this error before returning to provide more visibility into potential issues during runtime.

4. Performance Consideration: 🚀 The filtering of appList.Items to newItems could be optimized by pre-allocating the slice with an estimated capacity to avoid multiple allocations. This can be done using make([]appv1alpha1.ApplicationSet, 0, len(appList.Items)).

5. Code Readability: 📚 The removal of the informers import and the switch to using cache.NewSharedIndexInformer is a significant change. Ensure that this change is well-documented, as it alters the way informers are created and managed.

By addressing these points, the code will be more robust, maintainable, and performant. 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. Namespace Watching Logic: The change to allow watching multiple namespaces is a good addition. However, ensure that the logic for determining watchNamespace is well-tested, especially when cfg.AdditionalAppsNamespaces is empty or contains invalid entries. 🧪

2. Context Usage: The addition of ctx to the NewApplicationWatcher and newApplicationInformerAndLister functions is a positive change for better context management. Ensure that the context is properly propagated and used in all relevant operations to handle cancellations and timeouts effectively. ⏳

3. Security Considerations: When filtering applications based on namespaces, ensure that the isAppNamespaceAllowed function is robust against any potential security issues, such as namespace spoofing. Consider logging or handling cases where namespaces are unexpectedly not allowed. 🔒

4. Performance: The filtering of applications in the ListFunc could potentially be optimized if the number of applications is large. Consider whether this filtering could be done more efficiently, perhaps by leveraging Kubernetes label selectors if applicable. 🚀

5. Error Handling: Ensure that all potential errors, especially those from the ListFunc and WatchFunc, are logged and handled appropriately to avoid silent failures. 🐛

---

---

Dependency Review

Click to read mergecats review!

No suggestions found