incubator/kube-downscaler - Expand configuration options for service account

[patrungel]

Is this a new chart

No

What this PR does / why we need it:

The present introduces support for imagePullSecrets, annotations, and extra labels for ServiceAccount in use

I moved SA options out of RBAC tree to decouple SA configuration from the one for roles.

I took the liberty of incrementing minor version of the chart as the change is a feature increment.

Special notes for your reviewer:

None

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• DCO signed
• Chart Version bumped
• Variables are documented in the README.md
• Title of the PR starts with chart name (e.g. [stable/mychartname])

[k8s-ci-robot]

Hi @patrungel. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[patrungel]

/assign @Pluies

[Pluies]

Hi @patrungel!

This is a breaking change by removing rbac.serviceAccountName, so it'd need to be a major version change if we go this route.

Looking at other popular charts (like cluster-autoscaler, coredns or prometheus-operator), it looks like the usual approach to do that would be with:

• serviceaccount.create controls whether a new serviceAccount will be created; sometimes just rbac.create
• serviceaccount.name can be set to an existing serviceAccount name, created outside the chart for full manual control
  ^ These two appear pretty standard across charts, and I think (if we do this :) ) we should follow this unwritten convention.
• serviceAccount.annotations, sometimes serviceAccountAnnotations to add annotations on the newly-created serviceAccount

But I haven't seen any other chart that define either labels or pull secrets for automatically-created serviceaccounts. I'd argue that if someone needs that level of customisation, they should probably create the serviceAccount separately and define it with serviceAccount.name 👍

What do you think?

cc @Rowern

[patrungel]

Thanks for the review, @Pluies .

• Agree on the breaking character of the change. Is it okay ot up major version, considering the chart is in pre-release (major version 0)? https://semver.org/#how-do-i-know-when-to-release-100
• Confirmed the more adopted layout for serviceAccount to be

create: true
  # The name of the ServiceAccount to use.
  # If not set and create is true, a name is generated using the fullname template
  # name:

I'll comply with the above.

• I'd favour serviceAccount.annotations over serviceAccountAnnotations since we have other serviceAccount.* variables.
• Confirmed: annotations are rare, labels are virtually non-existent (spotted just once really) . I'll drop labels, and leave annotations.
• imagepullsecrets is widely supported feature (83 charts seem to have it), but rather as a top-level variable. I can move the variable up the structure and add image pull secrets to the ServiceAccount if created, otherwise to the Deployment. wdyt, @Pluies ?

UPD: all items but the version are now committed.

[Pluies]

Agree on the breaking character of the change. Is it okay ot up major version, considering the chart is in pre-release (major version 0)? https://semver.org/#how-do-i-know-when-to-release-100

I guess that makes sense, especially because we're in the incubator part of the charts :)

The imagePullSecrets part looks fine. 👍

Just a couple of notes on this PR and this is mergeable!

[Pluies]

The default here should be true

[patrungel]

Fixed that one, thanks!

[Pluies]

Is this line needed? 🤔 I would think it creates resources in the release namespace by default.

[patrungel]

That's an ongoing discussion for about a year or so: helm/helm#5465
While in general helm is supposed to place items into the correct namespace, there are use-cases depending on namespace field to be present. I err on the side of caution here.

[Pluies]

🤷 fine; probably should be added to other resources for consistency, but I won't hold the PR based on this!

[patrungel]

I guess that makes sense, especially because we're in the incubator part of the charts :)

Just for the sake of clarity, is it going to be 0.5.0 or 1.0.0?

[Pluies]

@patrungel let's keep 0.5.0 👍

/ok-to-test

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrungel, Pluies

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• incubator/kube-downscaler/OWNERS [Pluies]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

[scottrigby]

Edit: deleted comment

[scottrigby]

Edit: deleted comment

[Pluies]

@scottrigby uh, this PR is for the kube-downscaler, as far as I know it has nothing to do with Prometheus – did you actually mean to close it or was it a mistake?

[scottrigby]

@Pluies you're right 👌 This was my mistake somehow from the previous comment, then last night my search picked it up again. Thanks 🙂

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

[stale]

This issue is being automatically closed due to inactivity.