Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add complete k8s object validation and defaults on standalone #1521

Merged
merged 9 commits into from
Oct 4, 2023

Conversation

mtardy
Copy link
Member

@mtardy mtardy commented Sep 28, 2023

See individual commits.

This PR actually uncovered a few issues/bugs in type declaration and TracingPolicy tests. We will now have the same custom resource validation and defaults when deploying on k8s using CRD mechanisms and in standalone, this should unify the behavior.

Code addition is huge because we needed to make a binary an independent Go module because of a version mismatch in a library and I vendored the dependencies.

This might break user workflow if they relied on the fact that the validation was not properly done on standalone.

@mtardy mtardy added the release-note/bug This PR fixes an issue in a previous release of Tetragon. label Sep 28, 2023
@mtardy mtardy linked an issue Sep 28, 2023 that may be closed by this pull request
@mtardy mtardy force-pushed the pr/mtardy/validation branch 2 times, most recently from d1ce74c to b1ccbc6 Compare October 3, 2023 11:01
@cilium cilium deleted a comment from netlify bot Oct 3, 2023
@mtardy mtardy force-pushed the pr/mtardy/validation branch 3 times, most recently from f835b9e to 4baded7 Compare October 3, 2023 15:42
@mtardy mtardy changed the title Add proper k8s validation on standalone and defaults Add complete k8s object validation and defaults on standalone Oct 3, 2023
@mtardy mtardy marked this pull request as ready for review October 3, 2023 15:44
@mtardy mtardy requested a review from a team as a code owner October 3, 2023 15:44
@mtardy mtardy requested a review from kkourt October 3, 2023 15:44
@mtardy mtardy linked an issue Oct 3, 2023 that may be closed by this pull request
@mtardy
Copy link
Member Author

mtardy commented Oct 3, 2023

I linked this PR to close #1318 as well because trying to decode the content of examples/tracingpolicy is already done here:

func TestExamplesSmoke(t *testing.T) {
_, filename, _, _ := runtime.Caller(0)
examplesDir := filepath.Join(filepath.Dir(filename), "../../examples/tracingpolicy")
err := filepath.Walk(examplesDir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
// Skip non-directories
if info.IsDir() {
return nil
}
// Skip non-yaml files with a warning
if !strings.HasSuffix(info.Name(), "yaml") || strings.HasSuffix(info.Name(), "yml") {
logger.GetLogger().WithField("path", path).Warn("skipping non-yaml file")
return nil
}
// Fill this in with template data as needed
data := map[string]string{
"Pid": fmt.Sprint(os.Getpid()),
}
// Attempt to parse the file
_, err = fileConfigWithTemplate(path, data)
assert.NoError(t, err, "example %s must parse correctly", info.Name())
return nil
})
assert.NoError(t, err, "failed to walk examples directory")
}

But with this PR it will actually now pass a proper validation step while decoding FromYAML.

@mtardy mtardy force-pushed the pr/mtardy/validation branch 2 times, most recently from f15f66f to 1378eb6 Compare October 4, 2023 11:15
@mtardy
Copy link
Member Author

mtardy commented Oct 4, 2023

Followups would be:

  • Improve the state of PreValidate for kprobes and add something more standard that we can share for advanced validation between kprobes/tracepoints/uprobes with specific validation for each of them, outputting format similar to what k8s validation does.
  • Add advanced pre validation in smoke test

@mtardy mtardy force-pushed the pr/mtardy/validation branch from 1378eb6 to df8fc28 Compare October 4, 2023 15:11
mtardy and others added 9 commits October 4, 2023 15:25
Use native k8s types in order to not redeclare existing struct and be
compatible when calling k8s functions.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
Before that, validation was only applied via the JSON unmarshalling, so
mostly type validation. Only the k8s API server was checking the CRD
schema validation thus making it possible to load a Tracing Policy in
standalone that was an invalid k8s object.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
Before we had a discrepancy between using custom resources Tracing
Policies on k8s and in standalone mode. If you went via the API server,
the validation and default was applied but going straight via gRPC or
--tracing-policy flag allowed to circumvent validation and most
importantly defaulting. Creating potentially different behavior loading
on k8s and in standalone.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
To avoid having FromYAML and FromYAMLNamespaced, and similar for
FromFile, use a common interface between GenericTracingPolicy and
GenericTracingPolicyNamespaced.

Note we could get rid of one type as they represent the same object
internally, just keep GenericTracingPolicy and remove that interface. We
can then distinguish between Namespaced or not by reading the Kind of
the resource. That's a matter of preference between type casting and
calling a method to distinguish which kind is it really.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
Missing omitempty JSON tag was making validation requiring that empty
JSON arrays should be indicated explicitly with `array: null` to be a
valid object. I think it was forgotten by the authors.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
Testing cases were often invalids, which was not particularly an issue
given what was tested, but the new mandatory validation step detected
many issues. Most of that was missing `kind: TracingPolicy`. We had
many, around 50 so I injected that automatically, it might have been
overlooking some `kind: TracingPolicyNamespaced`.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
New validation made some Tracing Policy containing tracepoints without
types on args invalid. Tracepoints are stables and well defined so we
parse the format files to retrieve the args if we don't specify them in
the Tracing Policy. Unfortunately, we could not just add a Optional
Kubebuilder tag on the field because Enum still need to match the exact
list, which cannot contain an empty value. So we define the new 'auto'
type which is the default can be used for autodetection on tracepoint.

We prevent the use of 'auto' on kprobe but we could implement similar
auto detection with BTF support.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
- Make tetragon-oci-hook its own module so that we can use an old cel
  version. Google CEL dependency was in conflict with the version used
  in the k8s API server that was needed to perform defaulting.
- Add a replace directive for the tetragon/api pkg for the new
  tetragon-oci-hook module.
- Add vendoring of the new module in the Makefile vendor command
- Remove cilium replacement. I'd expect this to be fine. The replacement
  is also removed from cilium's main branch.

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Add the new entry for the tetragon OCI hook Go module and try to remove
useless packageRules by adding the postUpgradeTasks and
postUpdateOptions directly to all Go updates.

Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
@mtardy mtardy force-pushed the pr/mtardy/validation branch from df8fc28 to d08f4e3 Compare October 4, 2023 15:29
Copy link
Contributor

@kevsecurity kevsecurity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's all green, then I think it's good to go.

@kkourt kkourt merged commit 6a0bc0e into main Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/bug This PR fixes an issue in a previous release of Tetragon.
Projects
None yet
3 participants