tetragon: group enable process ancestors flags

[t0x01]

This PR deprecates 5 enable-process-ancestors boolean flags added in PR 2938 and replaces them with a new --enable-ancestors flag for better UX. All 5 deprecated flags remain functional; the new --enable-ancestors flag will simply override them if both old and new flags are used.

Flags being deprecated:

• --enable-process-ancestors
• --enable-process-kprobe-ancestors
• --enable-process-tracepoint-ancestors
• --enable-process-uprobe-ancestors
• --enable-process-lsm-ancestors

Also, support for the new flag is added in the Tetragon Helm chart via a tetragon.processAncestors.enabled value.

tetragon: Deprecate enable-process-ancestors boolean flags
helm: Add tetragon.processAncestors.enabled value

[netlify]

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	a7d448d
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/67ffa56ae6a417000812cc54
😎 Deploy Preview	https://deploy-preview-3581--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mtardy]

Hey I didn't realized we added so many flags for one feature, would it be possible to group them under --enable-ancestors=kprobe,tracepoints,lsm etc, instead of boolean flags? This PR is fine but I would think that would be a better usability thing. You can use StringSlice in viper flags.

Would you think that would be better maybe? Since this is a recent feature, maybe it's fine to just change the flags. Not sure it's been released already or not, if yes we might need to deprecate them before removing next release.

[t0x01]

Hey.

I think it should be possible to group them, yes. This feature was released with version 1.4.0 not so long ago. I'll see what i can do about it. Should i submit another PR or extend this one?

[mtardy]

Hey.

I think it should be possible to group them, yes. This feature was released with version 1.4.0 not so long ago. I'll see what i can do about it. Should i submit another PR or extend this one?

I think it would be reasonable to merge this, and create another PR with two things:

1. deprecate existing flag and add the list one that groups them.
2. update the helm charts flag so that it uses the new group flag implem with a better UX.

OR

we can make the UX correct in helm and then change the underlying implem.

Any opinion @lambdanis ?

[lambdanis]

would it be possible to group them under --enable-ancestors=kprobe,tracepoints,lsm etc, instead of boolean flags?

I agree, this sounds like a better approach.

I would update this PR to introduce both a new flag and a Helm value like this:

tetragon:
  processAncestors:
    enabled: "base,kprobe,tracepoint,uprobe,lsm"

You can rename and reuse ParseMetricsLabelFilter function from pkg/option, and check how we use it to parse the metrics-label-filter option - it's also passed as a comma-separated list. I would expect the code logic to override the old flags if enable-ancestors is non-empty. Then we can deprecate the old flags (just make sure you add an upgrade note for that).

Speaking more generally, the Helm values don't have to be named exactly like the corresponding flags. If we can group related values or follow Helm conventions, then it makes sense to structure them slightly differently than the corresponding flags. As an alternative to the above, I think Helm values like this would make sense too:

tetragon:
  processAncestors:
    base:
      enabled: true
    kprobe:
      enabled: true
    tracepoint:
      enabled: true
    uprobe:
      enabled: true
    lsm:
      enabled: true

but then translating these to a comma-separated flag is not as straightforward, so if we're changing the flag format then it seems better to keep the Helm value format same.

[kkourt]

but then translating these to a comma-separated flag is not as straightforward, so if we're changing the flag format then it seems better to keep the Helm value format same.

I don't think we need to translate it into a comma-seperated flag. Using multiple flags (--enable-ancstors=kprobe --enable-ancestors=tracepoint) should also work. If we go down this road, I think we would also need a --disable-ancestors flag (?).

[mtardy]

I don't think we need to translate it into a comma-seperated flag. Using multiple flags (--enable-ancstors=kprobe --enable-ancestors=tracepoint) should also work.

Yes iirc viper parses both --flag=value1 --flag=value2 and --flag=value1,value2 by using StringSlice.

If we go down this road, I think we would also need a --disable-ancestors flag (?).

If the default is to be disabled, not sure we need a disable ancestor flags? Otherwise we need to resolve conflict etc, it sounds annoying and unnecessary.

[t0x01]

Hey @mtardy , @lambdanis

Thanks for the help. I added both new --enable-ancestors flag and tetragon.processAncestors.enabled helm value, as you suggested. I also added an upgrade note that indicates which flags are being deprecated. Let me know if these changes are ok.

[mtardy]

Hey thanks a lot for the PR, it looks really clean!

However I think you could simplify by reusing the old variables internally (it's fine to have these variables, I think the important part is just avoid the UX of having all those flags). Also you can reuse the viper.GetStringSlice and just do validation on the values.

[mtardy]

I think internally, it's fine to keep the many config static variables EnableProcessKprobeAncestors and co, so that it avoids these dynamic string to be used with a map. Because otherwise we should use constant string and that for access I'm not sure it would be optimal

[mtardy]

could you maybe use viper.GetStringSlice(key string) : []string here so that you don't need to split and everything but just validate the values?

[t0x01]

viper.GetStringSlice(key string) : []string only splits strings by spaces, if i'm not mistaken. I think i can use pflag.StringSlice and viper.UnmarshalKey instead to correctly parse strings with comma-separated values read from both the cmd flag and the config files. So it would be something like this:

	var err error
	var enableAncestors []string
...
	if err = viper.UnmarshalKey(KeyEnableAncestors, &enableAncestors); err != nil {
		return fmt.Errorf("failed to parse enable-ancestors value: %w", err)
	}

	if slices.Contains(enableAncestors, "base") {
		Config.EnableProcessAncestors = true
		Config.EnableProcessKprobeAncestors = slices.Contains(enableAncestors, "kprobe")
		Config.EnableProcessTracepointAncestors = slices.Contains(enableAncestors, "tracepoint")
		Config.EnableProcessUprobeAncestors = slices.Contains(enableAncestors, "uprobe")
		Config.EnableProcessLsmAncestors = slices.Contains(enableAncestors, "lsm")
	}
...
	flags.StringSlice(KeyEnableAncestors, []string{}, "Comma-separated list of process event types to enable ancestors for. Supported event types are: base, kprobe, tracepoint, uprobe, lsm. Unknown event types will be ignored. Type 'base' enables ancestors for process_exec and process_exit events and is required by all other supported event types for correct reference counting. An empty string disables ancestors completely")

Would this be fine?

[mtardy]

viper.GetStringSlice(key string) : []string only splits strings by spaces

I checked, it works fine when doing --options A --options B but also --options A,B. It doesn't use spaces.

[t0x01]

It works fine with cmd flags, yes. That's because the flag's value goes through pflag first. But, when the value is read from a config file, it doesn't go through pflag.

viper.GetStringSlice calls cast.ToStringSlice. Here.
cast.ToStringSlice then calls cast.ToStringSliceE here.

If an option's value is read from a config file, the value's type will be a string. And cast.ToStringSliceE will call strings.Fields here.

I used this code to test it:

package main

import (
        "fmt"
        "reflect"
        "slices"

        "github.com/spf13/pflag"
        "github.com/spf13/viper"
)

func main() {
        pflag.StringSlice("options", []string{}, "")
        viper.SetConfigName("config")
        viper.SetConfigType("yaml")
        viper.AddConfigPath(".")
        viper.ReadInConfig()

        pflag.Parse()
        viper.BindPFlags(pflag.CommandLine)

        options := viper.GetStringSlice("options")
        fmt.Printf("options type: %s\n", reflect.TypeOf(viper.Get("options")))
        fmt.Printf("options len = %d\n", len(options))
        fmt.Printf("options value = %s\n", options)
        fmt.Printf("options contains 'A': %t\n", slices.Contains(options, "A"))
}

Tests:

$ go run test.go --options A,B
options type: []string
options len = 2
options value = [A B]
options contains 'A': true

$ go run test.go --options A --options B
options type: []string
options len = 2
options value = [A B]
options contains 'A': true

$ echo 'options: A B' > config.yaml && go run test.go
options type: string
options len = 2
options value = [A B]
options contains 'A': true

$ echo 'options: A,B' > config.yaml && go run test.go
options type: string
options len = 1
options value = [A,B]
options contains 'A': false

$ echo 'options: [A,B]' > config.yaml && go run test.go
options type: []interface {}
options len = 2
options value = [A B]
options contains 'A': true

So, then the option's substrings must be separated with spaces or specified as yaml list elements in config files, if we use viper.GetStringSlice to parse it. With cmd flags, comma separated values should work fine. I'm just not sure what to choose here.

[mtardy]

I think it's fine as YAML list are never defined with comma separated values so it feels okay to ask for YAML list for this. Would that be an issue?

[t0x01]

No, i don't think it would be an issue. I'm just trying to clarify it preemptively to make sure i don't commit any unwanted changes. I'll try to update this PR in the next couple of days, thanks.

[mtardy]

yeah sure, I'm sure we can find a way to make this works fine with Helm as well! Cool thank you!

[t0x01]

So, about Helm. Since ConfigMap.data only allows UTF-8 strings, i can't provide a YAML list as a ConfigMap.data field value. I can concatenate list's elements into a string with a space delimiter, though.

In values.yaml i can make tetragon.processAncestors.enabled value be a YAML list, like so:

tetragon:
  processAncestors:
    # -- List of process event types to enable ancestors for.
    # Supported event types are: base, kprobe, tracepoint, uprobe, lsm. Unknown event types will be ignored.
    # Type "base" is required by all other supported event types for correct reference counting.
    # Set it to [] to disable ancestors completely.
    enabled: []

And in the tetragon_configmap.yaml template, i can do this:

data:
{{- if .Values.tetragon.processAncestors.enabled }}
  enable-ancestors: {{ join " " .Values.tetragon.processAncestors.enabled | quote }}
{{- end }}

So, the rendered tetragon configMap will look like this:

$ helm template -s templates/tetragon_configmap.yaml . --set "tetragon.processAncestors.enabled={base,kprobe,lsm}"
---
# Source: tetragon/templates/tetragon_configmap.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  ...
data:
  ...
  enable-ancestors: "base kprobe lsm"
  ...

viper.GetStringSlice should parse that value correctly, but i guess it can be confusing if this is the only place where the value would not be a YAML list, but a string. Not sure if i can pass an actual YAML list through a configMap somehow. Would that be ok?

[mtardy]

so here you could still reuse the existing vars

[mtardy]

you could reuse the tetragon.EventType_PROCESS_EXEC.String() directly, by removing useless suffix, lowercase, etc...

[mtardy]

Maybe add the keyword to add in the alternative?

Suggested change

	flags.Bool(KeyEnableProcessAncestors, false, "Deprecated, please, use --enable-ancestors. Include ancestors in process_exec and process_exit events. Disabled by default. Required by other enable ancestors options for correct reference counting")
	flags.Bool(KeyEnableProcessAncestors, false, "Deprecated, please, use --enable-ancestors=base. Include ancestors in process_exec and process_exit events. Disabled by default. Required by other enable ancestors options for correct reference counting")

[mtardy]

Suggested change

	flags.MarkDeprecated(KeyEnableProcessAncestors, "please use --enable-ancestors")
	flags.MarkDeprecated(KeyEnableProcessAncestors, "please use --enable-ancestors=base")

[mtardy]

I guess you can avoid writing all this by relying on viper.GetStringSlice