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

Helm chart update #695

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Helm chart update #695

merged 2 commits into from
Sep 27, 2024

Conversation

EronWright
Copy link
Contributor

@EronWright EronWright commented Sep 27, 2024

Proposed changes

Updates the Helm chart to install PKOv2, as similarly as possible to operator/config/default.

Details:

  • adds an aggregation role (view/edit) for the Pulumi API groups
  • tweaks the controller's resources such that limits equals resources to have "guaranteed" qos
  • exposes the metrics port and the fileserver port
  • supports two rbac modes for the controller - ClusterRole and Role

To install:

helm upgrade --install pulumi-kubernetes-operator ./deploy/helm/pulumi-operator

Related issues (optional)

Closes #684

@EronWright EronWright added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Sep 27, 2024
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 49.23%. Comparing base (3c0c7a9) to head (fb5b455).
Report is 1 commits behind head on v2.

Files with missing lines Patch % Lines
...r/internal/controller/pulumi/program_controller.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##               v2     #695   +/-   ##
=======================================
  Coverage   49.23%   49.23%           
=======================================
  Files          26       26           
  Lines        2890     2890           
=======================================
  Hits         1423     1423           
  Misses       1290     1290           
  Partials      177      177           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@EronWright EronWright marked this pull request as ready for review September 27, 2024 06:56
version: 0.9.0
appVersion: 1.14.0
version: 2.0.0
appVersion: "v2.0-devel"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rationale for changing the version info:

  • appVersion is used as the image tag in deployment.yaml and should match our normal version string. The prerelease target will replace the value with the tag.
  • version is the chart version, and I would advocate for doing a major bump.

createClusterRole: false

## -- Enable this and set the rules: to whatever custom rules you want for the Cluster Role resource.
clusterRoleRules:
Copy link
Contributor Author

@EronWright EronWright Sep 27, 2024

Choose a reason for hiding this comment

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

Update: I think I know the original purpose of the ClusterRole, it was to grant admin-level access to the Pulumi program and its Kubernetes Provider, not simply to grant the requisite access to the operator. Now I have repurposed it to support a cluster-wide installation of the operator. The Pulumi program runs in its own service account and user-controlled bindings.

So createClusterRole and clusterRoleRules are obsolete. Under the new rbac section there's some flags related to the operator's rbac.

Comment on lines +34 to +37
# -- Specifies whether cluster roles and bindings should be created
createClusterRole: true
# -- Specifies whether namespaced roles and bindings should be created
createRole: false
Copy link
Contributor Author

@EronWright EronWright Sep 27, 2024

Choose a reason for hiding this comment

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

one either creates a clusterrole or a role for the controller manager, depending on whether you want a namepaced installation of the operator. There's still some follow-up work needed (#690), but the helm piece is in place.

Comment on lines -105 to +98
cpu: 500m
memory: 5123Mi
cpu: 200m
memory: 128Mi
requests:
cpu: 100m
cpu: 200m
Copy link
Contributor Author

@EronWright EronWright Sep 27, 2024

Choose a reason for hiding this comment

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

Rationale for changing the resources is to ensure that the pod is assigned the "Guaranteed" QoS class.

Copy link
Member

@rquitales rquitales left a comment

Choose a reason for hiding this comment

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

LGTM

I assume users will also need to manually apply the new CRDs first before running: helm upgrade --install pulumi-kubernetes-operator ./deploy/helm/pulumi-operator?

Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Awesome!

Obviously not something that we need to resolve now but it would be nice to more easily keep this in sync with the generated configs. There's kubernetes-sigs/kubebuilder#3074 which took me to https://github.com/arttor/helmify which looks interesting.

deploy/helm/pulumi-operator/values.yaml Show resolved Hide resolved
@EronWright EronWright merged commit 388a7ee into v2 Sep 27, 2024
9 checks passed
@EronWright EronWright deleted the issue-684-pt3 branch September 27, 2024 17:50
@rquitales
Copy link
Member

Awesome!

Obviously not something that we need to resolve now but it would be nice to more easily keep this in sync with the generated configs. There's kubernetes-sigs/kubebuilder#3074 which took me to https://github.com/arttor/helmify which looks interesting.

@blampe the original helm chart for v1 was actually scaffolded using helmify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants