helm-release-plugin icon indicating copy to clipboard operation
helm-release-plugin copied to clipboard

allow specifying service account to be used at ttl cleanup time

Open genisd opened this issue 2 years ago • 2 comments

basically the default service account should not have the permissions needed to cleanup the helm release and the cronjob. hence with this PR one can specify which service account is to be used, avoiding the need for the default service account to get higher privileges

additionally don't use kubectl/helm latest versions implicitly, but specify which versions are to be used.

genisd avatar Feb 16 '23 14:02 genisd

Hi @genisd!

First of all I want to say thank you for your contribution to Helm release plugin. I have reviewed your changes. And on my opinion we have little issue that can be easily fixed. When we using does not existing service account we do not getting any warning or error message. I have prepared small MVP how it can be fixed you can use it as starting point:

service_accounts=`kubectl get serviceaccounts --output=yaml | yq  '.items[].metadata.name'`  # Fetching list of service accounts.
array=(${service_accounts})  # Converting it to array.

# Printing service accounts.
for index in ${!array[*]}
do
    printf '==> %d: %s\n' $index ${array[$index]}
done

service_account='default'

if [[ ${array[*]} =~ ${service_account} ]]; then  # 'contains' if condition example.
    printf "Service account list contains '$service_account'.\n"
fi

if [[ ! ${array[*]} =~ ${service_account} ]]; then  # 'not contains' if condition example
    printf "Service account list does not contains '$service_account'.\n"
fi

Another issue that I see is service account exists but don't have enough rights to remove Helm release. But for this issue I don't have easy solution.

Except this two issues I don't see any other blockers why we can't accept this PR. @rtpro?

vadim-zabolotniy avatar Feb 23 '23 16:02 vadim-zabolotniy

I was sick for a bit, I'll hope to pick this up this week

genisd avatar Mar 01 '23 11:03 genisd

Hey @genisd @vadim-zabolotniy @rtpro , is there any chance to have this merged?

duhow avatar Jan 28 '25 13:01 duhow

Yeah we still run the fork which would love not to do actually. The ball was in my court so let's pick it up.

Essentially the motivation is not to give the default service account this kind of privileged access. I'm no expert but it is my understanding that any code execution inside any container would also get those privileges.

Your suggestion @vadim-zabolotniy is kinda valid as I've found out. service accounts are specific to namespaces :melting_face: So it's easy to overlook that a service account doesn't exist in a specific namespace.

Let me see what I can make of it @duhow maybe you can help test/validate it :see_no_evil:

genisd avatar Feb 04 '25 11:02 genisd

@genisd I'm also happy to validate an updated PR and merge.

rtpro avatar Feb 09 '25 11:02 rtpro

When service account doesn't exist output is, as you can see we use htlm-ttl for our service-account

$ helm release ttl health-pr-2359 --namespace=default --service-account='nonexisiting-service-account' --set='1 hour'
Service account list does not contains 'nonexisiting-service-account'.
Available service accounts: default helm-ttl

[help section follows here...]

genisd avatar Feb 10 '25 11:02 genisd

I tested this locally and I think it looks okay. Looking forward to your feedback.

@duhow could you give this a spin as well please? since you're also interessted in having this land upstream :innocent:

genisd avatar Feb 10 '25 11:02 genisd

easiest way to test this I found was this: https://github.com/JovianX/helm-release-plugin/pull/28/files#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055R29

I added this as an alternative approach to testing.

Basically you can run:

helm plugin install https://github.com/gynzy/helm-release-plugin --version SRE-218-upstream

And then run the ttl as specified in the examples I.e.:

helm release ttl <RELEASE_NAME> --namespace=default --service-account='your-privileged-sa' --set='1 minute'

this should purge the release after 1 minute (please use a test helm release, don't get your production purged or anything)

genisd avatar Feb 10 '25 11:02 genisd