Warn on HostSelector != nil in TracingPolicyNamespaced#4896
Conversation
be8a7e5 to
088d6ce
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| if tp.TpNamespace() != "" && tp.TpSpec().HostSelector != nil { | ||
| logger.GetLogger().Warn("TracingPolicyNamespaced cannot match host workloads. Explicitly set `spec.hostSelector: null` to remove this warning. Overrided spec.hostSelector with null before applying this policy.") | ||
| hostSelector = nil | ||
| } |
There was a problem hiding this comment.
What's the default? Will this warning be emitted for namespaced policies taht do not define a HostSelector?
There was a problem hiding this comment.
The default is {} as defined a previous PR. The user will get this warning, but the policy will load and the hostSelector will be removed in TracingPolicyNamespaced.
To remove that warning users need to define spec.hostSelector: null explicitly.
There was a problem hiding this comment.
OK, can we then have the warning show only a single time in the agent's lifetime?
mtardy
left a comment
There was a problem hiding this comment.
I know the why might be obvious for someone with context but would be worth adding (to the commit descr as well as the docs)
| // _, err if an error occurred | ||
| func (h *handler) updatePolicyFilter(tp tracingpolicy.TracingPolicy, tpID uint64) (policyfilter.PolicyID, error) { | ||
| hostSelector := tp.TpSpec().HostSelector | ||
| if tp.TpNamespace() != "" && tp.TpSpec().HostSelector != nil { |
There was a problem hiding this comment.
Idea: couldn't we make this impossible by design having different kubebuilder validation on the namespaced version? Or adding X-Validation?
There was a problem hiding this comment.
I have done several attends to fix that in a cleaner way.
Possibly a better way is to have different defaults for TracingPolicy and TracingPolicyNamespaced for the spec.hostSelector . I can do that with having separate TracingPolicySpec for each of these policies. For the global selectors this will be not a big deal regarding code duplication. But I am thinking also for #4833 that will have the same issues. In that case we need to create a lot of duplicate structs which does not sound ideal to me.
Regarding X-Validation, I tried something like:
diff --git a/pkg/k8s/apis/cilium.io/v1alpha1/tracing_policy_types.go b/pkg/k8s/apis/cilium.io/v1alpha1/tracing_policy_types.go
index 6f2376ab4..e189786bd 100644
--- a/pkg/k8s/apis/cilium.io/v1alpha1/tracing_policy_types.go
+++ b/pkg/k8s/apis/cilium.io/v1alpha1/tracing_policy_types.go
@@ -50,6 +50,7 @@ type TracingPolicy struct {
// +genclient:noStatus
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +kubebuilder:resource:categories={tetragon},singular="tracingpolicynamespaced",path="tracingpoliciesnamespaced",scope="Namespaced",shortName={tgtpn}
+// +kubebuilder:validation:XValidation:rule="spec.hostSelector == null",message="TracingPolicyNamespaced should have null spec.hostSelector."
type TracingPolicyNamespaced struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata"`But this also does not work as expected. Possibly related to #4889 (review) but I haven't spend more time on that yet to be honest.
I just created this RP to make sure that things work as expected and I will continue with a cleaner solution on a followup PR if this makes sense.
There was a problem hiding this comment.
oh okay cool :) all right indeed that looks easier like this!
TracingPolicyNamespaced policies can be applied only on specific
namespaces. This means that it generates events only for the specific
namespace that they are applied.
They can be used in cases where a user has permissions to access only a
specific namespace. If we allow users to specify spec.hostSelecector: {}
they will also get events from the host, and this is something that can
cause security issues.
To overcome this issue, when a TracingPolicyNamespaced policy is
applied with spec.hostSelecector != null we print a warning and we override
HostSelector to be nil. This is needed because the default value is
{} and it will complicate things to reject those policies.
This warning is only generated once for all policies that have those
issues.
Signed-off-by: Anastasios Papagiannis <anastasios.papagiannis@isovalent.com>
Signed-off-by: Anastasios Papagiannis <anastasios.papagiannis@isovalent.com>
088d6ce to
99a718a
Compare
| hostSelector := tp.TpSpec().HostSelector | ||
| if tp.TpNamespace() != "" && tp.TpSpec().HostSelector != nil { | ||
| warnNonNilHostSelector.Do(func() { | ||
| logger.GetLogger().Warn("TracingPolicyNamespaced cannot match host workloads. Explicitly set `spec.hostSelector: null` to remove this warning. Overrided spec.hostSelector with null before applying this policy.") |
There was a problem hiding this comment.
How about:
TracingPolicyNamespaced cannot match host workloads. All namespaced policies will ignore this field. Explicitly set
spec.hostSelector: nullto remove this warning.
#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)). Signed-off-by: Anastasios Papagiannis <anastasios.papagiannis@isovalent.com>
#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>
#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>
No description provided.