add agent Role object

[paulfantom]

I was investigating this helm chart from the security perspective and I noticed few things, one of which is lack of agent Role allowing agent to use PSP (included in this PR).

Additionally this helm chart looks like it is a bit more relaxed in terms of security when compared to jsonnet code in parca and parca-agent repositories. The jsonnet code has 2 main advantages:

• it ships PSP for parca server
• there is no sharing of SA between parca server and agent

I tried to include those things, but it turned out to be a task longer than anticipated. IMHO it would make sense to consider splitting agent and server into 2 helm charts and is necessary provide a 3rd helm chart joining both into one.

FWIW I combined pros of this helm chart (scrape configuration enabled by default) with jsonnet code and generated manifests are available at https://github.com/thaum-xyz/ankhmorpork/tree/master/apps/parca/manifests

[rlex]

I hasn't included PSP because at the time of writing it was already deprecated https://kubernetes.io/blog/2021/04/06/podsecuritypolicy-deprecation-past-present-and-future/

And it's being removed in next major release. Not sure if this is needed?

[paulfantom]

Then the solution is either to remove RoleBinding and PSP shipped with the chart or adding the Role. Right now it is broken as the chart ships PSP and RoleBinding but there is no Role.

As for the removal of PSP from the chart - it depends on which kubernets versions this chart is supposed to support. For example, right now the newest kubernetes version on Amazon EKS is 1.22.

[rlex]

@brancz @kakkoyun your ideas on what k8s versions this chart should support?

Splitting charts into two should not be that hard, but it's not something i want to do right now.

[brancz]

Our take with the manifests was that until 1.25 is actually released we will include PSPs. Once 1.25 is released we will remove them from the standard manifests and make it an optional addition in jsonnet. I think we should do the same thing in helm, once 1.25 ships we release a new version of the chart that does not include PSPs by default anymore but allow them to be turned on via configuration.

[paulfantom]

it's not something i want to do right now.

If that's due to time constrains then I can offer my help as we are exploring this deployment method at timescale.

[rlex]

Both time and some issues i see with that approach, ie inability to test agent at CI stage.

Current setup (single-chart) deploys both parca-server and parca-agent, checks that agent can contact server and completes test.

In case of separate chart i'm not sure how test can be invoked. In theory, it may be possible to add server as dependency for agent and invoke it only when specific variable is set, but it sounds like one giant kludge. Helm dependencies are not very pleasant to use.

[paulfantom]

In case of separate chart i'm not sure how test can be invoked.

That's why I was suggesting a third chart that uses both, agent and server helm charts as dependencies. In such case we can do the same test as currently because both applications would be installed. Additionally it allows to easily include other types of tests that are relevant only to server or only to agent in their respective helm charts.

Basically my suggestion is to do the following:

• extract server parts from the current chart and move it into a new parca-sever helm chart
• extract agent parts from the current chart and move it into a new parca-agent helm chart
• use existing parca chart to be an overarching chart which uses parca-server and parca-agent as dependencies.

Later on I would suggest to also add ct at least for chart linting and ideally for chart testing.

In theory, it may be possible to add server as dependency for agent

I wouldn't go this route. It has a potential to cause UX issues (consider how values.yaml would look like).

Helm dependencies are not very pleasant to use.

Their not that bad when there isn't too many charts and they are structured correctly ;)

[rlex]

current helm chart already have separate agent and server values blocks, plus i'm gonna add server.enabled: true/false in main chart too, so structure should be OK. Yeah, i think splitting charts will be best option.

[rlex]

Later on I would suggest to also add ct at least for chart linting and ideally for chart testing.

https://github.com/parca-dev/helm-charts/blob/master/.github/workflows/lint-test.yaml

It's already done, on pull requests. Some checks were disabled, because i was too lazy to fully fix yaml syntax.

Some overrides are needed for agent tho, because k3s which is used in k3d have non-standard CRI socket path: https://github.com/parca-dev/helm-charts/blob/master/charts/parca/ci/k3s-values.yaml

[paulfantom]

Some overrides are needed for agent tho, because k3s which is used in k3d have non-standard CRI socket path: https://github.com/parca-dev/helm-charts/blob/master/charts/parca/ci/k3s-values.yaml

AFAIR that should be already handled by the agent as part of parca-dev/parca-agent#90

[paulfantom]

Either way, can we merge or close this particular PR?

[rlex]

Can you please rebase it on latest and add psp.enabled variable to allow disabling psp deployment? Or i can do it, if you allow me to commit to that PR.

[paulfantom]

Sorry for the late replay, I was unavailable.

Weird, I have "Allow edits and access to secrets by maintainers" checked so you should be able to modify the PR 🤔 Either way I rebased it.