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

redpanda: add rack awareness feature #219

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

joejulian
Copy link
Contributor

@joejulian joejulian commented Dec 1, 2022

Set redpanda.rack to the value of the Kubernetes Node's topology.kubernetes.io/zone annotation.

Fixes #86

@joejulian joejulian self-assigned this Dec 1, 2022
@joejulian joejulian added enhancement New feature or request blocked and removed blocked labels Dec 2, 2022
@joejulian joejulian force-pushed the helm-charts-racks branch 2 times, most recently from cff86c6 to 25acdaa Compare December 2, 2022 22:26
@joejulian joejulian force-pushed the helm-charts-racks branch 3 times, most recently from bf2dfb4 to b578a41 Compare December 5, 2022 22:36
@joejulian joejulian marked this pull request as ready for review December 6, 2022 13:36
Copy link
Member

@vuldin vuldin left a comment

Choose a reason for hiding this comment

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

One comment around rbac.enabled existing and/or it's default being false. I think it would be better to leave this out entirely and then enable it automatically when this (and future features) are enabled by the user. Including this option introduces another (likely) way to end up with a broken cluster, and makes it difficult to enable future features that require this by default.

# Role Based Access Control
rbac:
# enable this for features that need extra priveleges
enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

Should this be enabled by default? I feel that it should as it is required to enable other features out of the box. For instance, we enable external access out of the box, but if we want it to work then they would need to flip this flag (eventually). The same applies to rack awareness, in that they would need to enable rack awareness and also rbac in order to get just rack awareness.

Another option may be to do a check that doesn't allow users to enable either rack awareness or external access without also enabling rbac. I this would work to ensure clusters work as expected, but at the same time making rbac be enabled automatically when these other features are enabled would be a better default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The principal of least privilege suggests that of a privilege is not needed it should not be added. Unless rack awareness is enabled, there's no need for the container to be able to get the Node.

Copy link
Member

@vuldin vuldin Dec 6, 2022

Choose a reason for hiding this comment

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

That's true for rack awareness. I guess another approach would be to enable rbac by default in another incoming PR that fixes external access by making use of this (given that external access is enabled by default).

@joejulian
Copy link
Contributor Author

joejulian commented Dec 6, 2022

One comment around rbac.enabled existing and/or it's default being false. I think it would be better to leave this out entirely and then enable it automatically when this (and future features) are enabled by the user.

I made this choice for a couple reasons. One, to ensure that adding privilege is a conscious choice.

The other reason is to allow this chart to be used by unprivileged users. In an enterprise environment, organization units are given a namespace but they don't have cluster-level privileges. In this scenario, the ServiceAccount, ClusterRole, and ClusterRoleBinding will need to be created by the cluster administrator. The chart user will still be able to use the chart by using the SA assigned to them and not enabling the rbac components.

@vuldin vuldin self-requested a review December 6, 2022 15:54
Copy link
Member

@vuldin vuldin left a comment

Choose a reason for hiding this comment

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

LGTM

@joejulian joejulian merged commit c2145b5 into redpanda-data:main Dec 6, 2022
@joejulian joejulian deleted the helm-charts-racks branch December 6, 2022 16:13
RafalKorepta pushed a commit to redpanda-data/redpanda-operator that referenced this pull request Dec 3, 2024
…charts-racks

redpanda: add rack awareness feature
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable rack awareness
2 participants