[tempo] Add support for Network Policy.

[Sheikh-Abubaker]

Fixes #2890

[zanhsieh]

I think feature should bump to minor version:

Suggested change

	version: 1.7.2
	version: 1.8.0

[zanhsieh]

Can you get rid of extra comment lines?

[zanhsieh]

Can you get rid of extra comment lines?

[Sheikh-Abubaker]

@zanhsieh please checkout I've done the required changes.

[Sheikh-Abubaker]

@zanhsieh I think I found some missing entries that should be in values.yaml but are not, please correct me if I am wrong, this code section below in charts/grafana/templates/networkpolicy.yaml is defined to include any labels and annotations defined in values.yaml right ? but when I searched through values.yaml I was unable to find any entries for labels though I can see entry for annotations but it is commented out, what do you think about this ?

    {{- with .Values.labels }}
    {{- toYaml . | nindent 4 }}
    {{- end }}
  {{- with .Values.annotations }}
  annotations:
    {{- toYaml . | nindent 4 }}
  {{- end }}

[zanhsieh]

@Sheikh-Abubaker
That's why the code go with {{ with }}.

[hkailantzis]

Hi, is there anything else pending for this ? Or just an additional review required ? Thanks a lot @Sheikh-Abubaker.

[Sheikh-Abubaker]

@hkailantzis I've added this resource from referring to : https://github.com/grafana/helm-charts/blob/main/charts/grafana/templates/networkpolicy.yaml

I have a doubt in this part of the code:

egress:
    {{- if not .Values.networkPolicy.egress.blockDNSResolution }}
    - ports:
        - port: 53
          protocol: UDP
    {{- end }}

As for the grafana networkpolicy the port is defined as 53, which port should be assigned for tempo networkpolicy ? Please correct me if I am wrong but as per my understanding since it a general K8s resource then we should utilize the same port i.e 53 ?

[hkailantzis]

@Sheikh-Abubaker , I'm really not sure about this tbh. I suspect is something general on networking, like when blocking DNS resolution which runs on port 53. is related with Firewalls etc. ie: https://library.netapp.com/ecmdocs/ECMP1155586/html/GUID-D052D155-EF55-4D19-A70F-B9A8FA86A6D3.html#:~:text=The%20Domain%20Name%20System%20(DNS,name%20and%20IP%20address%20lookups.

So, is basically if someone wishes to Block DNS resolution, then the template just says: if blockDNSResolution=true, then do NOT allow outgoing traffic via UDP port 53. So is an generic additional feature, I guess you could include it also here.

[Sheikh-Abubaker]

So, is basically if someone wishes to Block DNS resolution, then the template just says: if blockDNSResolution=true, then do NOT allow outgoing traffic via UDP port 53. So is an generic additional feature, I guess you could include it also here.

@hkailantzis thanks for the additional info, I hope this gets merged after the awaited additional review.

[Sheikh-Abubaker]

@zalegrala can you please approve this PR.

[zalegrala]

I've no way to validate this change and perhaps seems quite site-dependent. Would a generic "extraObjects" allow for the change you're after, similar to what is currently in tempo-distributed? https://github.com/grafana/helm-charts/blob/main/charts/tempo-distributed/values.yaml#L2150C1-L2150C13

[Sheikh-Abubaker]

Would a generic "extraObjects" allow for the change you're after, similar to what is currently in tempo-distributed? https://github.com/grafana/helm-charts/blob/main/charts/tempo-distributed/values.yaml#L2150C1-L2150C13

Yes that would work but the original issue #2890 was about defining a Network Policy to be able to accept incoming traffic from other components, so I went on with defining Network Policy to get rid of manual definition that the user stated about.

[zalegrala]

From my perspective, I think its more important that we have extension points so that site-specific requirements can be made without needing to maintain all those specifics in the chart. Forgive my ignorance around the NetworkPolicy object, since I've not used it. If we create that extension point, then it seems that we empower users to extend the chart in ways that are useful to them, and we can supplement with docs or other ways to demonstrate how some of these specifics were achieved without holding the maintenance burden on the chart. I see the request was also made for the tempo-distributed chart, but considering the extraObjects support there, it would be good to know if this works for their use case.

[Sheikh-Abubaker]

Forgive my ignorance around the NetworkPolicy object, since I've not used it.

@zalegrala No worries we're all here to learn and giveback, let me break it down for you in very simple terms, we have a set of rules(policies) using which we control the traffic flow in/out within our network, you can go through the conversations of this PR for a detailed insight on how it works.

but considering the extraObjects support there, it would be good to know if this works for their use case.

sounds good, what do you suggest now ? should I keep this or change it to support extraObjects I am open to both, let me know your views.

[zalegrala]

I think let's learn if the extraObjects support would solve for the issue at hand. If so, let's line up the charts to use that approach. I'm happy to continue the discussion as well, perhaps others feel strongly. My view is probably rooted more in a desire to keep this particular chart as simple as we can keep it.

[zalegrala]

Sorry, I see now that I'm a little late to the conversation as well.

[Sheikh-Abubaker]

I think let's learn if the extraObjects support would solve for the issue at hand. If so, let's line up the charts to use that approach.

No worries I can proceed with extraObjects part too.

My view is probably rooted more in a desire to keep this particular chart as simple as we can keep it.

It would be simple after Network Policy too because we are not enabling it by default, but if user wants to enable it they can do it on their own as per their requirement just like: https://github.com/grafana/helm-charts/blob/1b48dcc6a74adae15fc3e183858025acc353cc78/charts/grafana/values.yaml#L1240C1-L1243C17

[hkailantzis]

I think let's learn if the extraObjects support would solve for the issue at hand. If so, let's line up the charts to use that approach. I'm happy to continue the discussion as well, perhaps others feel strongly. My view is probably rooted more in a desire to keep this particular chart as simple as we can keep it.

@zalegrala I really fail to see, why Tempo should differ on this, when e.g. grafana, Loki etc. are using NetworkPolicy specific templates already...isn't better to unify the approach, for consistency, common pattern etc ? or perhaps I'm missing something... ? :-)

and as @Sheikh-Abubaker stated, is quite the pain for users to maintain such template on their own...and is kind of clean since is up to a boolean to enable or not...and set to false by default. Aslo, NP, is not some exotic resource, is quite basic k8s resource, used in security context, similar to RBAC, is just network traffic specific. In this case, is may as well be specified and let user enabling or not. And in addition, if done also here, it could make sense to also apply in the Tempo-distributed also (in a separate PR).

https://github.com/grafana/helm-charts/blob/main/charts/loki-distributed/templates/networkpolicy.yaml

https://github.com/grafana/helm-charts/blob/main/charts/grafana/templates/networkpolicy.yaml
https://github.com/grafana/helm-charts/blob/main/charts/grafana/templates/image-renderer-network-policy.yaml

[zalegrala]

Hi @hkailantzis, thank you for leaving your thoughts. I agree that it would be nice to unify certain portions of the charts where we can. Perhaps this is one of those areas. Many of the charts in this repo are entirely community maintained, and so there isn't a unified approach here unless it comes from the community, which arguably hasn't, and Grafana hasn't done much to push standards here as well. I think testing the charts could be a good step, since we have no diff to review in this PR, etc. Those kinds of standards would be good to push on in my oppinion.

My questions around the NetworkPolicy object made no mention of an exotic resource, just one that I'd not used and have no way to validate. As to the other charts containing the this resource already, respectfully, I was not directly pinged to review those PRs.

The part that I struggle to understand is why its better to invent a new yaml struct rather than providing users direct access to render full kubernetes objects in the chart when they aren't specific to Tempo. In the extraObjects case discussed above, it seems to me that there would be no additional template that needed to be managed on the user side, and users would be able to put exactly the details about the NetworkPolicy in the values. This would mean we don't have to wonder about why we are including an option for blocking egress DNS traffic, etc. This goes to the point above I was mentioning creating the extension points, rather than site specifics, though I agree the single boolean covers us in this case. If including the NetworkPolicy here is the right move, I have no intention of standing in the way, but I would like to understand this point.

As someone who gets regularly pinged for review on the charts of which I am not a user, its good for me to have a working understanding of the needs, so thank you for your patience while I work to understand the challenges users face.

[zalegrala]

Are we able to cover this rule with the other options? What's missing if not?

[Sheikh-Abubaker]

egress is an important part of the Network Policy resource it basically defines rules for outgoing traffic from the pods, if I didn't get you wrong, that is something you want to know about ?

[zalegrala]

No sorry, my question is more specific to the blockDnsResolution. If the primitives allow for specifying in the user values "port 53" and "protocol UDP", then this doesn't seem needed, correct?

[zalegrala]

Something like the following in my local values would avoid needing this added option if I understand.

networkPolicy:
  enabled: true
  egress:
    ports: 
      - port: 53
        protocol: UDP

[Sheikh-Abubaker]

yes that would do, but the reason behind going with the below code was to maintain uniformity as the same code in the grafana NP is working fine.

  {{- if .Values.networkPolicy.egress.enabled }}
  egress:
    {{- if not .Values.networkPolicy.egress.blockDNSResolution }}

And also the user have to write the same thing to block DNS so wouldn't it be better to provide it by default ?

[Sheikh-Abubaker]

And as per my understanding we are simplifying it for the users, if they want to block DNS resolution they would simply make it true i.e blockDNSResolution: true, without needing to define this:

networkPolicy:
  enabled: true
  egress:
    ports: 
      - port: 53
        protocol: UDP

and if they don't want to block DNS resolution i.e blockDNSResolution: false the below part would be executed allowing traffic in Port 53:

  {{- if .Values.networkPolicy.egress.enabled }}
  egress:
    {{- if not .Values.networkPolicy.egress.blockDNSResolution }}

In this case they don't have to write a single piece of code to block DNS resolution, it would be just true or false and the rest would follow.

[zalegrala]

Sorry for the delay, I've been traveling. Thanks for the contributions @Sheikh-Abubaker. Keep them coming.

[Sheikh-Abubaker]

Sorry for the delay, I've been traveling.

No worries it's alright 😄!!

Thanks for the contributions @Sheikh-Abubaker. Keep them coming.

You're welcome and thanks for your support too!! I'd definitely keep it coming 🎉!

[Sheikh-Abubaker]

Hey @zalegrala the original issue #2890 also mentioned about having this feature enabled for tempo-distributed, so should I raise a similar PR for tempo-distributed too ??

[zalegrala]

If its valuable to someone, sure feel free. If we are making changes that nobody is using then 🤷 I guess.

[Sheikh-Abubaker]

If its valuable to someone, sure feel free. If we are making changes that nobody is using then 🤷 I guess.

Yes it is, as the original issue #2890 wanted this feature for tempo-distributed too!

[hkailantzis]

hi @Sheikh-Abubaker , thanks for this!. It works like a charm, with the 'client' label etc. Sorry for the delay of the reply. I kinda noticed that 'client' label is basically not taken into account in the cross-namespace selector case. (its '-', so its now an 'OR' operation. e.g:

- podSelector:
    matchLabels:
      tempo-sre-client: 'true'
- namespaceSelector:
    matchLabels:
      env: test

when for cross-namespace, the 'client' label should be also present right ? otherwise any pods from another namespace can access Tempo. e.g: notice the lack of '-' in the second podSelector, which denotes and AND operation. (YAML joy , I know...)

- podSelector:
    matchLabels:
      tempo-sre-client: 'true'
- namespaceSelector:
    matchLabels:
      env: test
  podSelector:
    matchLabels:
      tempo-sre-client: 'true'

Explanation with example here: https://kubernetes.io/docs/concepts/services-networking/network-policies/#behavior-of-to-and-from-selectors

[Sheikh-Abubaker]

Hey there @hkailantzis I have addressed your comment in the PR #3203, checkout and let me know if it is something you were looking for.

[hkailantzis]

@Sheikh-Abubaker looks good! thanks!