fix: allow disabling builtin service account

[aslafy-z]

This PR fixes inconsistencies in handling custom service accounts by ensuring serviceAccountName is only set when both rbac.enabled and rbac.serviceAccount.enabled are true across CronJob, Job, and Deployment templates.

Motivation

Previously, templates could reference a service account even when it wasn’t created due to rbac.serviceAccount.enabled being false, causing unpredictable behavior. This change provides consistent, configurable handling.

Potential Impact

• Users relying on referencing non-existent service accounts may need to update configurations. (A future change will allow the use of a pre-existing service account)

References

• Replaces Make cronJob work without a service account #320
• XKCD 1172

[d3adb5]

I don't have strong feelings about which direction we take regarding default behavior. However, I do believe we should avoid major bumps when they are unnecessary. I seem to be missing the reasoning behind this.

[d3adb5]

I understand the changes in the templates that fixes service accounts being set without .Values.rbac.serviceAccount.enabled being true, but I don't understand why we're now defaulting to true for that value.

I believe setting this to true is what introduces a breaking change, no? What is the reasoning behind this? Can we simply introduce a patch bump by removing this?

[aslafy-z]

The primary issue is that the previous checks around rbac.serviceAccount.enabled were inconsistent, leading to unexpected behavior. Here’s a breakdown of how the behavior shifts based on the value of rbac.serviceAccount.enabled:

1. When rbac.serviceAccount.enabled defaults to false:

   • CronJobs and Jobs:
     • Previous behavior: If rbac.enabled was true and rbac.serviceAccount.enabled was false, a custom service account was still used, which was inconsistent.
     • Current behavior: No service account is created, aligning more logically with the intended purpose of the flag.
   • Deployments: There is no change in behavior; they remain unaffected.

2. When rbac.serviceAccount.enabled defaults to true:

   • CronJobs and Jobs: No impact as the custom service account would already have been created.
   • Deployments: Deployments will now automatically have a custom service account, which may seem like a breaking change but aligns more consistently with our intended functionality.

I don’t have a strong preference between these two values. We'll see a breaking change anyway.
Open to further discussion and suggestions on how to best balance these changes!

cc @MuneebAijaz @AsfaMumtaz @karl-johan-grahn

[d3adb5]

I'm with you on not having a strong preference, but I'll leave my 2 cents here:

We'll see a breaking change anyway.

I don't agree with this 100%. Let me explain:

1. When rbac.serviceAccount.enabled defaults to false:
   • CronJobs and Jobs:
     • Previous behavior: If rbac.enabled was true and rbac.serviceAccount.enabled was false, a custom service account was still used, which was inconsistent.

The service account that would be associated with these is not created by this chart, because templates/serviceaccount.yaml yields an empty document if rbac.serviceAccount.enabled is false. As a result, what is wrong is that we're referencing a service account that does not in theory exist.

To me, this is a bug in how we render our CronJob and Job templates, NOT intended behavior, and consequently I see this as NOT something we maintain or guarantee as part of version 5.x.

I understand that fixing this could break some chart users' setups, but they will have been leveraging the consequences of a bug, not a feature offered by the chart maintainers. XKCD 1172 comes to mind.

Meanwhile, by changing the default value to true, we are changing functionality that was previously offered: that of not creating a service account by default, because perhaps users don't have those permissions or don't want any token that could authenticate against the API server.

[aslafy-z]

Thanks for the thoughtful feedback, you make a great point.

This is indeed a bug, not intended behavior. Fixing this will break setups that relied on this inconsistency, but as you said, it's not a feature we guaranteed.
I updated the default value to false.

[aslafy-z]

@d3adb5

Following our discussion and your feedback, I opened a new PR: #363. This PR not only addresses the inconsistency with service accounts but also properly implements the feature for using an existing service account when specified.

Please have a look and let me know