[stable/jenkins] Add support for idleMinutes and serviceAccount

[jbussdieker]

What this PR does / why we need it:

This PR allows setting the idleMinutes and serviceAccount attributes used for provisioning build agents.

Which issue this PR fixes

Special notes for your reviewer:

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

[k8s-ci-robot]

Hi @jbussdieker. 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.

[jbussdieker]

/assign @torstenwalter

[torstenwalter]

Thnks for the PR. What do you think about managing the service account used by the Jenkins agents in a similar way as the service account for Jenkins itself?

https://helm.sh/docs/chart_best_practices/#using-rbac-resources

So having an option to create it etc.

[jbussdieker]

That's a good idea. I'll incorporate https://helm.sh/docs/chart_best_practices/#using-rbac-resources into this PR.

[jbussdieker]

Actually now that I think about it the agent should default to not creating rbac even if rbac.create is set to true.

Typically users won't want to set a service account for the build agents unless they are using Jenkins to manage and deploy to the cluster hosting Jenkins so the default rules to create is a bit of a grey area.

What might make sense is to the have agent.rbac.create in addition to rbac.create but I'm not sure if that's a good pattern.

On a side note I have noticed that since Jenkins will be the one launching and assigning the service account to the agents, the permissions on the agent service account need to be equal to or less privileged than the master's service account.

I'll think about it more as I work with the chart. Thanks for the feedback!

[torstenwalter]

@jbussdieker I agree that it's unclear how rbac for the service account of Jenkins agents should look like. Especially since there are many diffrent ways how people are using it.
It heavily depends on their setup e.g.

• do they want to use it to deploy to the same or a different kubernetes cluster
• should agents have permission to deploy to the whole cluster or just specific namespaces
• ...

Helm best practices suggest to split between service account and rbac setup:

rbac:
  # Specifies whether RBAC resources should be created
  create: true

serviceAccount:
  # Specifies whether a ServiceAccount should be created
  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:

That's what we do for the Jenkins master. Even if we don't know how to setup rbac we could still manage the creation of the service account. e.g. introducing something like:

serviceAccountAgent:
  # Specifies whether a ServiceAccount should be created
  create: false
  # The name of the ServiceAccount to use.
  # If not set and create is true, a name is generated using the fullname template
  name:

And use a template like:

{{/*
Create the name of the service account for Jenkins agents to use
*/}}
{{- define "jenkins.serviceAccountAgentName" -}}
{{- if .Values.serviceAccountAgent.create -}}
    {{ default (include "jenkins.fullname" .) .Values.serviceAccountAgent.name }}
{{- else -}}
    {{ default "default" .Values.serviceAccountAgent.name }}
{{- end -}}
{{- end -}}

When creating the service account we could also ensure that it is done in the correct namespace if master.slaveKubernetesNamespace is set.

[jbussdieker]

@torstenwalter thanks that makes sense.

[torstenwalter]

/ok-to-test

[torstenwalter]

I think we have a problem here. If neither Values.serviceAccount.name nor .Values.serviceAccountAgent.name is set then we are trying two service accounts with the same name.

jenkins.serviceAccountName: 
{{ default (include "jenkins.fullname" .) .Values.serviceAccount.name }}

jenkins.serviceAccountAgentName
{{ default (include "jenkins.fullname" .) .Values.serviceAccountAgent.name }}

We could add the "-agent" suffix:

{{ default (printf "%s-%s" (include "jenkins.fullname" .) "agent") .Values.serviceAccountAgent.name }}

[jbussdieker]

Good point. I thought there might have been a collision somewhere but I was hung up on there being two different bindings to default but that's not an issue.

[torstenwalter]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbussdieker, torstenwalter

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:

• stable/jenkins/OWNERS [torstenwalter]

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

[torstenwalter]

@jbussdieker Thank you for this improvement!