Simplify workload selectors#4917
Conversation
31e88ed to
a968e08
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
9d128c9 to
c979dda
Compare
254d510 to
4419375
Compare
mtardy
left a comment
There was a problem hiding this comment.
This feel reasonable and would simplify things, we discussed offline maybe merging the table because it's not obvious to me when I read this. Just another comment:
4419375 to
d1bcd95
Compare
There was a problem hiding this comment.
I think that's indeed better.
So as I understand you do not need to revert 564b89a as well?
Maybe we should highlight that the default null/null/null is a special case with a note or caution?
Should you also not merge #4918 and revert the PR now since you shouldn't be able to get any policy not respecting the validation?
Possibly part of that. I will check now. Not everything as this also contains fixes/changes for the policyfilter implementation to support the hostSelector (i.e. an entry in the policy_filter_map with all pod cgroup IDs).
Yes, I can do that.
I have closed #4918 as it does not make sense to merge it now. I have kept the check in the agent but now it is an error. Just in case somehow we overcome the XValidation check. |
#4814 added support for spec.hostSelector. The idea was to have by default `spec.podSelector: {}`, `spec.containerSelector: {}`, and `spec.hostSelector: {}`. But this has some unintended side effects that complicate our codebase (i.e. #4889 and #4896). In order to fix that, we change the defaults of `spec.{pod, container, host}Selector` from `{}` to `null`. This change will also allow us to remove the code introduced in #4889 and be able to check the `hostSelector` value inside `TracingPolicyNamespaced` with a `kubebuilder` `XValidation` (i.e. #4896 (comment)). This patch also reverts part of 564b89a to reflect the new changes in the tests. Signed-off-by: Anastasios Papagiannis <anastasios.papagiannis@isovalent.com>
Signed-off-by: Anastasios Papagiannis <anastasios.papagiannis@isovalent.com>
This reverts commit 6c6cd99. We do not need to specify hostSelector to be null explicitly. Signed-off-by: Anastasios Papagiannis <anastasios.papagiannis@isovalent.com>
This reverts commit 38a6543. As we change the default value of workload selectors from {} to null this code is not required now. Signed-off-by: Anastasios Papagiannis <anastasios.papagiannis@isovalent.com>
d1bcd95 to
f8c6bab
Compare
Most of that was already done in the first patch. I have added a comment in the first patch description.
Done. |
#4814 added support for spec.hostSelector. The idea was to have by default
spec.podSelector: {},spec.containerSelector: {}, andspec.hostSelector: {}. But this has some unintended side effects that complicate our codebase (i.e. #4889 and #4896).In order to fix that, we change the defaults of
spec.{pod, container, host}Selectorfrom{}tonull. The new behaviour is described in the table below:hostSelectorpodSelectorcontainerSelectornull(default)null(default)null(default)null(default)null(default){...}containerSelectoracross all pods. No host workloads are selected.null(default){...}null(default)podSelector. No host workloads are selected.null(default){...}{...}containerSelectorin pods that matchpodSelector. No host workloads are selected.{}null(default)null(default){}null(default){...}containerSelectoracross all pods.{}{...}null(default)podSelector.{}{...}{...}containerSelectorin pods that matchpodSelector.This change will also allow us to remove the code introduced in #4889 and be able to check the
hostSelectorvalue insideTracingPolicyNamespacedwith akubebuilderXValidation(i.e. #4896 (comment)).