[tempo] Add support for Network Policy. #2922

Sheikh-Abubaker · 2024-01-21T20:11:39Z

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

zanhsieh · 2024-01-22T01:12:32Z

charts/tempo/Chart.yaml

@@ -2,7 +2,7 @@ apiVersion: v2
 name: tempo
 description: Grafana Tempo Single Binary Mode
 type: application
-version: 1.7.1
+version: 1.7.2


I think feature should bump to minor version:

Suggested change

version: 1.7.2

version: 1.8.0

zanhsieh · 2024-01-22T01:13:42Z

charts/tempo/values.yaml

+  ##
+  ##
+  ##
+  ##
+  ##


Can you get rid of extra comment lines?

zanhsieh · 2024-01-22T01:13:59Z

charts/tempo/values.yaml

+  ##
+  ##
+  ##
+  ##


Can you get rid of extra comment lines?

…into tempo-pr

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Sheikh-Abubaker · 2024-01-22T09:58:03Z

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

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Sheikh-Abubaker · 2024-01-22T11:01:42Z

@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 · 2024-01-24T07:34:36Z

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

…into tempo-pr

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

…into tempo-pr

hkailantzis · 2024-03-12T15:19:24Z

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

Sheikh-Abubaker · 2024-03-13T10:29:52Z

@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 · 2024-03-13T17:04:29Z

@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 · 2024-03-14T09:33:14Z

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.

…into tempo-pr

…arts into tempo-pr

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Sheikh-Abubaker · 2024-06-03T14:57:58Z

@zalegrala can you please approve this PR.

…into tempo-pr

zalegrala · 2024-06-03T20:59:34Z

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

…into tempo-pr

Sheikh-Abubaker · 2024-06-04T10:58:55Z

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 · 2024-06-04T14:34:35Z

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 · 2024-06-04T15:11:14Z

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 · 2024-06-04T15:15:35Z

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 · 2024-06-04T15:17:38Z

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

Sheikh-Abubaker · 2024-06-04T15:26:24Z

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 · 2024-06-06T00:17:46Z

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

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

zalegrala · 2024-06-07T20:14:59Z

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 · 2024-06-07T20:15:41Z

charts/tempo/templates/networkpolicy.yaml

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


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

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 ?

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?

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

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 ?

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 · 2024-06-17T14:51:15Z

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 · 2024-06-17T14:56:31Z

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 · 2024-06-20T17:47:26Z

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

Sheikh-Abubaker · 2024-06-21T12:41:49Z

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 · 2024-06-25T07:21:03Z

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 · 2024-07-03T10:50:22Z

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 · 2024-07-04T09:52:53Z

@Sheikh-Abubaker looks good! thanks!

Added Network Policy for tempo

7862f07

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Sheikh-Abubaker requested review from dgzlopes, swartz-k, BitProcessor and faustodavid as code owners January 21, 2024 20:11

Sheikh-Abubaker changed the title ~~[Tempo]: Added Network Policy capability~~ [tempo] Added Network Policy capability Jan 21, 2024

Sheikh-Abubaker added 3 commits January 22, 2024 01:46

Updated README.md

5d91088

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Updated README.md

27d7a46

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Updated README.md

b39a68b

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

zanhsieh requested changes Jan 22, 2024

View reviewed changes

Sheikh-Abubaker added 2 commits January 22, 2024 13:53

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

593209b

…into tempo-pr

Updated values.yaml

813d4cf

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Updated values.yaml

389462d

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Sheikh-Abubaker added 2 commits January 25, 2024 01:36

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

ab4a11a

…into tempo-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

ee5e4cb

…into tempo-pr

zanhsieh approved these changes Feb 4, 2024

View reviewed changes

Sheikh-Abubaker added 5 commits February 7, 2024 23:08

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

9009b0a

…into tempo-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

30b2ca3

…into tempo-pr

Resolved conflicts

3e50a89

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

566d824

…into tempo-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

8f15ccc

…into tempo-pr

Sheikh-Abubaker added 2 commits March 14, 2024 22:15

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

922e85b

…into tempo-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

ad6b11b

…into tempo-pr

Sheikh-Abubaker added 3 commits May 5, 2024 18:25

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

e4ae24f

…into tempo-pr

Merge branch 'tempo-pr' of https://github.com/Sheikh-Abubaker/helm-ch…

b107a90

…arts into tempo-pr

Resolve Merge Conflicts

b73c61a

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

d7acb92

…into tempo-pr

Merge branch 'main' of https://github.com/Sheikh-Abubaker/helm-charts …

87f9249

…into tempo-pr

Trigger DCO

8a5b021

Signed-off-by: Sheikh-Abubaker <sheikhabubaker761@gmail.com>

zalegrala reviewed Jun 7, 2024

View reviewed changes

Merge branch 'main' into tempo-pr

c2c8dcf

Sheikh-Abubaker changed the title ~~[tempo] Add Network Policy capability~~ [tempo] Add support for Network Policy. Jun 8, 2024

zalegrala approved these changes Jun 17, 2024

View reviewed changes

Merge branch 'main' into tempo-pr

d040e96

zalegrala merged commit 0259e4b into grafana:main Jun 17, 2024
6 checks passed

Sheikh-Abubaker mentioned this pull request Jul 2, 2024

[tempo] Remove multiple - (dash) entries under from within ingress #3203

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tempo] Add support for Network Policy. #2922

[tempo] Add support for Network Policy. #2922

Sheikh-Abubaker commented Jan 21, 2024 •

edited

Loading

zanhsieh Jan 22, 2024

zanhsieh Jan 22, 2024

zanhsieh Jan 22, 2024

Sheikh-Abubaker commented Jan 22, 2024

Sheikh-Abubaker commented Jan 22, 2024

zanhsieh commented Jan 24, 2024

hkailantzis commented Mar 12, 2024

Sheikh-Abubaker commented Mar 13, 2024

hkailantzis commented Mar 13, 2024 •

edited

Loading

Sheikh-Abubaker commented Mar 14, 2024

Sheikh-Abubaker commented Jun 3, 2024

zalegrala commented Jun 3, 2024

Sheikh-Abubaker commented Jun 4, 2024

zalegrala commented Jun 4, 2024

Sheikh-Abubaker commented Jun 4, 2024 •

edited

Loading

zalegrala commented Jun 4, 2024

zalegrala commented Jun 4, 2024

Sheikh-Abubaker commented Jun 4, 2024

hkailantzis commented Jun 6, 2024 •

edited

Loading

zalegrala commented Jun 7, 2024

zalegrala Jun 7, 2024

Sheikh-Abubaker Jun 7, 2024 •

edited

Loading

zalegrala Jun 7, 2024

zalegrala Jun 7, 2024

Sheikh-Abubaker Jun 7, 2024 •

edited

Loading

Sheikh-Abubaker Jun 7, 2024 •

edited

Loading

zalegrala left a comment

Sheikh-Abubaker commented Jun 17, 2024

Sheikh-Abubaker commented Jun 17, 2024 •

edited

Loading

zalegrala commented Jun 20, 2024

Sheikh-Abubaker commented Jun 21, 2024 •

edited

Loading

hkailantzis commented Jun 25, 2024 •

edited

Loading

Sheikh-Abubaker commented Jul 3, 2024

hkailantzis commented Jul 4, 2024 •

edited

Loading

[tempo] Add support for Network Policy. #2922

[tempo] Add support for Network Policy. #2922

Conversation

Sheikh-Abubaker commented Jan 21, 2024 • edited Loading

zanhsieh Jan 22, 2024

Choose a reason for hiding this comment

zanhsieh Jan 22, 2024

Choose a reason for hiding this comment

zanhsieh Jan 22, 2024

Choose a reason for hiding this comment

Sheikh-Abubaker commented Jan 22, 2024

Sheikh-Abubaker commented Jan 22, 2024

zanhsieh commented Jan 24, 2024

hkailantzis commented Mar 12, 2024

Sheikh-Abubaker commented Mar 13, 2024

hkailantzis commented Mar 13, 2024 • edited Loading

Sheikh-Abubaker commented Mar 14, 2024

Sheikh-Abubaker commented Jun 3, 2024

zalegrala commented Jun 3, 2024

Sheikh-Abubaker commented Jun 4, 2024

zalegrala commented Jun 4, 2024

Sheikh-Abubaker commented Jun 4, 2024 • edited Loading

zalegrala commented Jun 4, 2024

zalegrala commented Jun 4, 2024

Sheikh-Abubaker commented Jun 4, 2024

hkailantzis commented Jun 6, 2024 • edited Loading

zalegrala commented Jun 7, 2024

zalegrala Jun 7, 2024

Choose a reason for hiding this comment

Sheikh-Abubaker Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

zalegrala Jun 7, 2024

Choose a reason for hiding this comment

zalegrala Jun 7, 2024

Choose a reason for hiding this comment

Sheikh-Abubaker Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Sheikh-Abubaker Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

zalegrala left a comment

Choose a reason for hiding this comment

Sheikh-Abubaker commented Jun 17, 2024

Sheikh-Abubaker commented Jun 17, 2024 • edited Loading

zalegrala commented Jun 20, 2024

Sheikh-Abubaker commented Jun 21, 2024 • edited Loading

hkailantzis commented Jun 25, 2024 • edited Loading

Sheikh-Abubaker commented Jul 3, 2024

hkailantzis commented Jul 4, 2024 • edited Loading

Sheikh-Abubaker commented Jan 21, 2024 •

edited

Loading

hkailantzis commented Mar 13, 2024 •

edited

Loading

Sheikh-Abubaker commented Jun 4, 2024 •

edited

Loading

hkailantzis commented Jun 6, 2024 •

edited

Loading

Sheikh-Abubaker Jun 7, 2024 •

edited

Loading

Sheikh-Abubaker Jun 7, 2024 •

edited

Loading

Sheikh-Abubaker Jun 7, 2024 •

edited

Loading

Sheikh-Abubaker commented Jun 17, 2024 •

edited

Loading

Sheikh-Abubaker commented Jun 21, 2024 •

edited

Loading

hkailantzis commented Jun 25, 2024 •

edited

Loading

hkailantzis commented Jul 4, 2024 •

edited

Loading