Custom resource health checks randomly not evaluated when using glob pattern

[duizabojul]

Describe the bug

When utilizing custom resource health checks with glob patterns, Lua scripts are inconsistently invoked, resulting in the application controller randomly failing to report resource health.

Upon investigation of the codebase, I identified the underlying issue:

To locate the health Lua script associated with a resource, the controller iterates over all resource override keys until it finds a key matching the resource group/version.

The primary problem here lies in the fact that map keys are not ordered. Consequently, if a resource group/version matches multiple resource override glob keys, the controller randomly selects one of these keys based on the order of iteration. This leads to the random usage of one of the Lua scripts if they exist.

While it might seem improbable to define multiple globs matching the same group/kind resource, another issue arises. The controller adds default resource overrides under the */* key.

Due to this additional resource override, which matches all resources, the iteration over map keys may randomly match this key instead of the user-defined key. The */* resource override lacks a Lua script, causing the resource to have no health check evaluated.

To address this, I propose a fix that involves sorting keys by string length (from longer key to shorter):

func GetWildcardConfigMapKey(vm VM, gvk schema.GroupVersionKind) string {
	gvkKeyToMatch := GetConfigMapKey(gvk)

	resourceOverridesKeys := make([]string, len(vm.ResourceOverrides))

	i := 0
	for k := range vm.ResourceOverrides {
		resourceOverridesKeys[i] = k
		i++
	}

	// Sort the keys so that the longest key is first
	sort.Slice(resourceOverridesKeys, func(i, j int) bool {
		return len(resourceOverridesKeys[i]) > len(resourceOverridesKeys[j])
	})

	for _, key := range resourceOverridesKeys {
		if glob.Match(key, gvkKeyToMatch) {
			return key
		}
	}

	return ""
}

By sorting the keys, the */* key is consistently evaluated last. This not only addresses the issue with the random evaluation but also provides an added benefit: it allows the definition of a health check script with a glob and its subsequent override with a longer glob. For instance, defining *.upbound.io/* and *.aws.upbound.io/* health checks ensures that the health evaluation for the s3.aws.upbound.io group will use the *.aws.upbound.io/* Lua script.

Version

argocd: v2.9.3+6eba5be
  BuildDate: 2023-12-01T23:05:50Z
  GitCommit: 6eba5be864b7e031871ed7698f5233336dfe75c7
  GitTreeState: clean
  GoVersion: go1.21.3
  Compiler: gc
  Platform: linux/amd64

[amanfredi]

Wow, nice find! I was wondering why the custom crossplane health checks appeared to be randomly toggling on and off.

[mbbush]

I like this solution, especially as it provides a place to make "find the best match" smarter than just length-based. Length-checking is a clever shortcut, and might be entirely sufficient if argo only supports globs, but I wonder if it might be a good idea to treat the group and kind match sections separately, and/or use a metric that evaluates the length of non-glob sections of the pattern, in order.

[mbbush]

I think given the following globs, I'd want them applied in this order:

1. */ProviderConfigUsage
2. */ProviderConfig
3. s3.aws.upbound.io/*
4. *.aws.upbound.io/*
5. *.upbound.io/*
6. */*

So maybe a good metric would be to sort first by the length of the kind pattern, then by the length of the group pattern?

It would probably be good for someone with a non-crossplane use case to also chime in on their preferences.

[jan-mrm]

nice find! There is no workaround or is there until the issue is fixed?

[mbbush]

I'm not aware of any workaround, and as I'm about to start relying on argocd healthchecks as part of my CD pipeline, I really want them to be more stable.

@jan-mrm @amanfredi @duizabojul does my suggestion of sorting first by the length of the kind pattern, then by the length of the group pattern seem like a good idea to you?

[jan-mrm]

@mbbush I personally don't think that I have a preference between "overall" length vs "kind-then-group" length.
I don't see a downside for the "kind-then-group" length sorting at the moment.
From a non-crossplane perspective I'm not sure how applicable/beneficial it is but I probably don't have a good overview here about how many equal kinds from different groups there are, that follow the same schema so that the health check can be equal so that this sorting benefits them. But I guess even if not and you then wanted to differentiate between them you could always define multiple checks again anyways.
So no preference. I could work with both :)

[llavaud]

any updates ?

[mbbush]

I've got a local branch that does this, I just haven't written the tests yet.

[agaudreault]

I don't think the length by itself is very deterministic. You basically want to use the most specific configuration, but this is very subjective using the name.

I think the algorithm could be
For GK1 and GK2

1. If the first glob (*) starting from the end occurs earlier in G1, G1 has lower priority
2. If G1 has more *, G1 is lower priority
3. If G1 is shorter, it is lower priority
4. If G1 is smaller lexicographically, it is lower priority
5. If G1 and G2 are equal, evaluate K1 and K2
6. If the first glob (*) occurs earlier in K1, K1 has lower priority
7. If K1 ends in * and K2 doesn't, K1 is lower priority
8. If K1 has more *, K1 is lower priority
9. If K1 is shorter, it is lower priority
10. If K1 is smaller lexicographically, it is lower priority

You would end up with this order. The logical reasoning behind this would be that if you can specify the resource Kind without wildcard, then you are able to specify the group.

• s3.aws.upbound.io/Something
• s3.aws.upbound.io/Some*
• s3.aws.upbound.io/*
• *.aws.upbound.io/Something
• *.aws.upbound.io/Something*
• *.aws.upbound.io/Some*
• *.aws.upbound.io/*thing
• *.aws.upbound.io/*m*th
• *.aws.upbound.io/*
• *.upbound.io/*
• */ProviderConfigUsage
• */ProviderConfig
• */*

Perhaps adding a priority integer would help create a generic deterministic order in a clearer and more objective way?

[duizabojul]

I don't think we should overcomplicate things. Sorting by the full pattern size or by kind size, followed by group size as suggested by @mbbush, is sufficient to fix this bug. This approach will add consistency across runs and remain easy for everyone to understand.

[blakepettersson]

I don't think the length by itself is very deterministic. You basically want to use the most specific configuration, but this is very subjective using the name.
....

Perhaps adding a priority integer would help create a generic deterministic order in a clearer and more objective way?

Could a simpler variant be to sort by the full pattern size as @duizabojul is suggesting, and also to enforce that an asterisk (*) can only be the first or last character? That's a potentially breaking change though...

[crenshaw-dev]

Going back to the original bug description:

While it might seem improbable to define multiple globs matching the same group/kind resource, another issue arises. The controller adds default resource overrides under the / key.

Due to this additional resource override, which matches all resources, the iteration over map keys may randomly match this key instead of the user-defined key. The / resource override lacks a Lua script, causing the resource to have no health check evaluated.

It seems to me that the immediate problem isn't the nondeterministic selection of wildcard overrides: it's that overrides with no health checks are being considered at all.

Here's the logic to select a wildcard-based health override:

	wildcardKey := GetWildcardConfigMapKey(vm, obj.GroupVersionKind())

	if wildcardKey != "" {
		if wildcardScript, ok := vm.ResourceOverrides[wildcardKey]; ok && wildcardScript.HealthLua != "" {
			return wildcardScript.HealthLua, wildcardScript.UseOpenLibs, nil
		}
	}

	builtInScript, err := vm.getPredefinedLuaScripts(key, healthScriptFile)

In the case where GetWildCardConfigMapKey randomly encounters a */* wildcard first, we'll evaluate the inner if condition to be false, because wildcardScript.HealthLua == "". Then we'll fall back to vm.getPredefinedLuaScripts, which presumably will yield no health check for the custom resource.

imo, instead of developing some algorithm to select the desired health check first, we should fix the immediate bug: don't consider overrides which have wildcardScript.HealthLua == "" as candidates for wildcard matches. In other words, instead of GetWildcardConfigMapKey, use this:

// GetWildcardHealthOverride returns the first encountered resource override which matches the wildcard
// and has a health script.
// If multiple wildcards match and have a health check, the selection from those matches is non-deterministic.
func GetWildcardHealthOverrideLua(vm VM, gvk schema.GroupVersionKind) string {
	gvkKeyToMatch := GetConfigMapKey(gvk)

	for key, override := range vm.ResourceOverrides {
		if glob.Match(key, gvkKeyToMatch) && override.HealthLua != "" {
			return override.HealthLua
		}
	}
	return ""
}

This solves the immediate problem of matching irrelevant wildcards.

If we feel this doesn't go far enough, we can come up with a sorting algorithm for the relevant overrides.

[blakepettersson]

@crenshaw-dev that does quite simplify things. I'm happy to amend the PR and take a rain check on the priority field.

[crenshaw-dev]

So after Blake's fix, the order is:

1. Custom health check with exact group/kind match
2. Custom health check with matching glob
   A) ignoring non-health overrides (Blake's bugfix)
   B) randomly selecting one if multiple match
3. Built-in health checks

If 2.A is unacceptable, then we can open a new issue to track defining a selection strategy.