fix(health): only consider non-empty health checks (argoproj#20232)

* fix(health): only consider non-empty health checks

For wildcard health checks, only consider wildcards with a non-empty
health check. Fixes argoproj#16905 (at least partially).

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* test: renaming test case for clarity

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* refactor: add clarity as to what the function is supposed to do

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* Update docs/operator-manual/health.md

Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* test: add test case for `*/*` override with empty healthcheck

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

---------

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: austin5219 <3936059+austin5219@users.noreply.github.com>

docs/operator-manual/health.md

@@ -98,20 +98,27 @@ data:
     return hs
 ```
 
-In order to prevent duplication of the custom health check for potentially multiple resources, it is also possible to specify a wildcard in the resource kind, and anywhere in the resource group, like this:
+In order to prevent duplication of custom health checks for potentially multiple resources, it is also possible to
+specify a wildcard in the resource kind, and anywhere in the resource group, like this:
 
 ```yaml
-  resource.customizations.health.ec2.aws.crossplane.io_*: |
-    ...
+  resource.customizations: |
+    ec2.aws.crossplane.io/*:
+      health.lua: |
+        ...
 ```
 
 ```yaml
-  resource.customizations.health.*.aws.crossplane.io_*: |
-    ...
+  # If a key _begins_ with a wildcard, please ensure that the GVK key is quoted.
+  resource.customizations: |
+    "*.aws.crossplane.io/*":
+      health.lua: |
+        ...
 ```
 
 !!!important
-    Please, note that there can be ambiguous resolution of wildcards, see [#16905](https://github.com/argoproj/argo-cd/issues/16905)
+    Please, note that wildcards are only supported when using the `resource.customizations` key, the `resource.customizations.health.<group>_<kind>`
+style keys do not work since wildcards (`*`) are not supported in Kubernetes configmap keys.
 
 The `obj` is a global variable which contains the resource. The script must return an object with status and optional message field.
 The custom health check might return one of the following health statuses:
@@ -121,7 +128,7 @@ The custom health check might return one of the following health statuses:
   * `Degraded` - the resource is degraded
   * `Suspended` - the resource is suspended and waiting for some external event to resume (e.g. suspended CronJob or paused Deployment)
 
-By default health typically returns `Progressing` status.
+By default, health typically returns a `Progressing` status.
 
 NOTE: As a security measure, access to the standard Lua libraries will be disabled by default. Admins can control access by
 setting `resource.customizations.useOpenLibs.<group>_<kind>`. In the following example, standard libraries are enabled for health check of `cert-manager.io/Certificate`.

util/lua/lua.go

@@ -141,13 +141,11 @@ func (vm VM) GetHealthScript(obj *unstructured.Unstructured) (string, bool, erro
 		return script.HealthLua, script.UseOpenLibs, nil
 	}
 
-	// if not found as is, perhaps it matches wildcard entries in the configmap
-	wildcardKey := GetWildcardConfigMapKey(vm, obj.GroupVersionKind())
+	// if not found as is, perhaps it matches a wildcard entry in the configmap
+	getWildcardHealthOverride, useOpenLibs := getWildcardHealthOverrideLua(vm.ResourceOverrides, obj.GroupVersionKind())
 
-	if wildcardKey != "" {
-		if wildcardScript, ok := vm.ResourceOverrides[wildcardKey]; ok && wildcardScript.HealthLua != "" {
-			return wildcardScript.HealthLua, wildcardScript.UseOpenLibs, nil
-		}
+	if getWildcardHealthOverride != "" {
+		return getWildcardHealthOverride, useOpenLibs, nil
 	}
 
 	// if not found in the ResourceOverrides at all, search it as is in the built-in scripts
@@ -426,15 +424,18 @@ func GetConfigMapKey(gvk schema.GroupVersionKind) string {
 	return fmt.Sprintf("%s/%s", gvk.Group, gvk.Kind)
 }
 
-func GetWildcardConfigMapKey(vm VM, gvk schema.GroupVersionKind) string {
+// getWildcardHealthOverrideLua returns the first encountered resource override which matches the wildcard and has a
+// non-empty health script. Having multiple wildcards with non-empty health checks that can match the GVK is
+// non-deterministic.
+func getWildcardHealthOverrideLua(overrides map[string]appv1.ResourceOverride, gvk schema.GroupVersionKind) (string, bool) {
 	gvkKeyToMatch := GetConfigMapKey(gvk)
 
-	for key := range vm.ResourceOverrides {
-		if glob.Match(key, gvkKeyToMatch) {
-			return key
+	for key, override := range overrides {
+		if glob.Match(key, gvkKeyToMatch) && override.HealthLua != "" {
+			return override.HealthLua, override.UseOpenLibs
 		}
 	}
-	return ""
+	return "", false
 }
 
 func (vm VM) getPredefinedLuaScripts(objKey string, scriptFile string) (string, error) {

util/lua/lua_test.go

@@ -786,6 +786,11 @@ return hs`
 	const healthWildcardOverrideScript = `
  hs = {}
  hs.status = "Healthy"
+ return hs`
+
+	const healthWildcardOverrideScriptUnhealthy = `
+ hs = {}
+ hs.status = "UnHealthy"
  return hs`
 
 	getHealthOverride := func(openLibs bool) ResourceHealthOverrides {
@@ -803,6 +808,21 @@ return hs`
 		},
 	}
 
+	getMultipleWildcardHealthOverrides := ResourceHealthOverrides{
+		"*.aws.crossplane.io/*": appv1.ResourceOverride{
+			HealthLua: "",
+		},
+		"*.aws*": appv1.ResourceOverride{
+			HealthLua: healthWildcardOverrideScriptUnhealthy,
+		},
+	}
+
+	getBaseWildcardHealthOverrides := ResourceHealthOverrides{
+		"*/*": appv1.ResourceOverride{
+			HealthLua: "",
+		},
+	}
+
 	t.Run("Enable Lua standard lib", func(t *testing.T) {
 		testObj := StrToUnstructured(testSA)
 		overrides := getHealthOverride(true)
@@ -836,6 +856,23 @@ return hs`
 		assert.Equal(t, expectedStatus, status)
 	})
 
+	t.Run("Get resource health for wildcard override with non-empty health.lua", func(t *testing.T) {
+		testObj := StrToUnstructured(ec2AWSCrossplaneObjJson)
+		overrides := getMultipleWildcardHealthOverrides
+		status, err := overrides.GetResourceHealth(testObj)
+		require.NoError(t, err)
+		expectedStatus := &health.HealthStatus{Status: "Unknown", Message: "Lua returned an invalid health status"}
+		assert.Equal(t, expectedStatus, status)
+	})
+
+	t.Run("Get resource health for */* override with empty health.lua", func(t *testing.T) {
+		testObj := StrToUnstructured(ec2AWSCrossplaneObjJson)
+		overrides := getBaseWildcardHealthOverrides
+		status, err := overrides.GetResourceHealth(testObj)
+		require.NoError(t, err)
+		assert.Nil(t, status)
+	})
+
 	t.Run("Resource health for wildcard override not found", func(t *testing.T) {
 		testObj := StrToUnstructured(testSA)
 		overrides := getWildcardHealthOverride