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.

Sign up for GitHub

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

Jump to bottom

Replace ClusterRole with Role only for posthog namespace #632

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Replace ClusterRole with Role only for posthog namespace #632

wants to merge 1 commit into from

Conversation

SuperTux88
Copy link

Description

According to Altinity/clickhouse-operator#994 (comment) it isn't required to give clickhouse-operator permissions for the whole cluster if the clickhouse-operator is only deployed into a single namespace, which is the case for PostHog. So it's better for security reasons to only give the permissions which are required and not just to everything.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How has this been tested?

Unit tests and installation in our cluster.

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@guidoiaquinti
Copy link
Contributor

👋 @SuperTux88 and thank you for your contribution! We currently vendor everything in charts/posthog/templates/clickhouse-operator via scripts/clickhouse_operator_sync.sh as there isn't an Helm chart for the clickhouse-operator. Due to this we usually highly discourage to directly edit templates in that folder.

@SuperTux88
Copy link
Author

Thanks for your comment, I didn't know about this script. I also updated the script now to use MANIFEST_PRINT_RBAC_NAMESPACED to generate the roles, so there are no manual changes required for the templates in that folder.

@SuperTux88
Use MANIFEST_PRINT_RBAC_NAMESPACED to generate clickhouse-operator files
5934470 
This replaces the `ClusterRole` with a `Role` only for posthog
namespace. If the clickhouse-operator is only deployed into a single
namespace, it only needs access to this namespace and not the whole
cluster.
guidoiaquinti
guidoiaquinti reviewed Nov 28, 2022
View reviewed changes
scripts/clickhouse_operator_sync.sh
@@ -18,13 +18,13 @@ TMP_FOLDER="$(mktemp -d)"
trap 'rm -rf -- "$TMP_FOLDER"' EXIT

CLICKHOUSE_OPERATOR_TAG="0.18.4"
URL="https://raw.githubusercontent.com/Altinity/clickhouse-operator/${CLICKHOUSE_OPERATOR_TAG}/deploy/operator/clickhouse-operator-install-template.yaml"
REPO_URL="https://github.com/Altinity/clickhouse-operator.git"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

Because running cat-clickhouse-operator-install-yaml.sh requires multiple template files, so instead of downloading all of them separately, it's easier to just clone the repo and have everything that is required there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guidoiaquinti guidoiaquinti guidoiaquinti left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SuperTux88 @guidoiaquinti