Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple Service account creation from PodSecurityPolicy #387

Merged
merged 7 commits into from
Dec 21, 2023

Conversation

frankjkelly
Copy link
Contributor

@frankjkelly frankjkelly commented Aug 24, 2023

Motivation

Primary reason: We need component-specific service accounts (for our Istio Authz roles) but cannot deploy the current 3.0.0 version of the Helm Chart as is with psp: true since K8S/EKS 1.25+ does not support the PodSecurityPolicy API any longer.

Secondary reason: I am not aware of any reason not to give the pods specific service accounts even if pod security is disabled. So seems like a reasonable idea to decouple the two.

Modifications

Removed the helm templating rules around serviceAccountName

Local Testing

Using Minikube

First with rbac.enabled: false and rbac.psp: false

kubectl get serviceaccount
NAME                 SECRETS   AGE
default              1         8m32s
pulsar-bookkeeper    1         3m57s
pulsar-broker-acct   1         3m57s
pulsar-proxy         1         3m57s
pulsar-recovery      1         3m57s
pulsar-toolset       1         3m57s
pulsar-zookeeper     1         3m57s

and then with rbac.enabled: true and rbac.psp: true

kubectl get serviceaccount
NAME                 SECRETS   AGE
default              1         11m
pulsar-bookkeeper    1         44s
pulsar-broker-acct   1         44s
pulsar-proxy         1         44s
pulsar-recovery      1         44s
pulsar-toolset       1         44s
pulsar-zookeeper     1         44s

Verifying this change

  • Make sure that the change passes the CI checks.

@eolivelli eolivelli changed the title Proposal: service accounts creation should be decoupled from PodSecur… Proposal: service accounts creation should be decoupled from PodSecurityPolicy Sep 5, 2023
@frankjkelly frankjkelly changed the title Proposal: service accounts creation should be decoupled from PodSecurityPolicy Service accounts creation can be decoupled from PodSecurityPolicy Sep 15, 2023
@frankjkelly
Copy link
Contributor Author

@tisonkun can I ask if you can PTAL?

@tisonkun
Copy link
Member

Sorry I don't have the related tech knowledge to review this patch. Perhaps @michaeljmarshall can help here?

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @frankjkelly! I made some further changes:

  • renamed *-rbac.yaml to *-psp.yaml so that PSP changes are completely separated from other parts
  • moved service account creation to *-service-account.yaml files
  • disabled PSP by default on k8s >= 1.25
  • added new CI test to ensure that chart can be deployed on both 1.21 and 1.27 with rbac.enabled and rbac.psp enabled (would ignore rbac.psp on 1.27)

@lhotari lhotari modified the milestone: 3.0.0 Dec 21, 2023
@lhotari lhotari changed the title Service accounts creation can be decoupled from PodSecurityPolicy Decouple Service account creation from PodSecurityPolicy Dec 21, 2023
@lhotari lhotari merged commit 0b2d9b4 into apache:master Dec 21, 2023
26 checks passed
@frankjkelly
Copy link
Contributor Author

@lhotari Somehow I missed the email with your changes and the merge. Just wanted to say thanks so much!

@frankjkelly frankjkelly deleted the seperate-rbac-service-account branch January 11, 2024 18:56
@Mortom123
Copy link
Contributor

Mortom123 commented Jan 15, 2024

Thanks for the work @lhotari and @frankjkelly. I am trying to implement #424 and stumbled upon your recent work.
I understand separating PSP related stuff from the rest, as it will be deprecated soon. However, as far as I understand, when using RBAC one still needs to define Roles and RoleBindings. Is there any reason why one needs to toggle PSP for these things to become active (e.g. here)?
Shouldn't we split the old setup into 3 parts:

  1. PSP-related stuff (soon to be removed) (.Values.rbac.psp == True)
  2. Service Account creation (.Values.rbac.enabled == True)
  3. Roles and RoleBindings, used by 1. and 2. (.Values.rbac.enabled == True)

@lhotari
Copy link
Member

lhotari commented Jan 15, 2024

Good questions @Mortom123 .

Thanks for the work @lhotari and @frankjkelly. I am trying to implement #424 and stumbled upon your recent work. I understand separating PSP related stuff from the rest, as it will be deprecated soon. However, as far as I understand, when using RBAC one still needs to define Roles and RoleBindings. Is there any reason why one needs to toggle PSP for these things to become active (e.g. here)? Shouldn't we split the old setup into 3 parts:

  1. PSP-related stuff (soon to be removed) (.Values.rbac.psp == True)
  2. Service Account creation (.Values.rbac.enabled == True)
  3. Roles and RoleBindings, used by 1. and 2. (.Values.rbac.enabled == True)

However, as far as I understand, when using RBAC one still needs to define Roles and RoleBindings. Is there any reason why one needs to toggle PSP for these things to become active (e.g. here)?

The referred roles and role bindings defined in broker-psp.yaml are tightly coupled to PSP. That's why they are in the broker-psp.yaml file and require toggling PSP.

  1. PSP-related stuff (soon to be removed) (.Values.rbac.psp == True)

That is already split into the *-psp.yaml templates

  1. Service Account creation (.Values.rbac.enabled == True)
  2. Roles and RoleBindings, used by 1. and 2. (.Values.rbac.enabled == True)

If we would start from a clean slate with the Pulsar Helm Chart, we would most likely do it this way. It seems that .Values.rbac.enabled hasn't has a clear meaning. IIRC, there are currently templates that create service accounts regardless of .Values.rbac.enabled.

@Mortom123 The Apache Pulsar PMC is looking for maintainers for this repository so if you'd like to show some care for this project, it would be appreciated. We can introduce breaking changes in major releases so if it would make sense to organize things differently, that could be done as long as the breaking changes are documented and we bump the major version number.

@Mortom123
Copy link
Contributor

Mhh. I see the problem. Historic growth of repos always comes at a price..., I guess for now I will settle to just be able to toggle account creation and leave the rest for the psp-Flag to toggle.

While I appreciate the offer @lhotari , I do not feel confident in my documentation skills and just started working with kubernetes-clusters recently - however the company I am working at plans to introduce Pulsar as major infrastructure building block. Once the projects responsibilities and the way we use Pulsar is settled, I might be able to revise my position or even recommend someone whose daily business will be pulsar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants