Add helm chart - Githubissues

digitalrayne commented 2 years ago

Hello,

Thank you for writing k8s-ecr-login-renew, and sharing it! It definitely saved me some heartache today.

Noticing #23 - and having decent experience with writing helm charts - and needing to deploy this via Helm myself, I thought I'd write and contribute a chart to this project. As noted in #23 - people who are maintaining complex Kubernetes deployments are quite commonly using Helm due to the templating and secret-handling utility is has, as well as companion tools such as helmfile to help keep multi-environment deployments as DRY as possible. This was my use case, so I hope this chart will help others.

I've tested this chart myself and it is working well.

I've tried to keep it as minimal and secure as possible following the good example in the examples folder. I have not added the ability to specify multiple repositories to the chart, but that would be quite easy to add later if someone needed it in a non-breaking way.

Let me know if any questions or if you have any changes you'd like to see.

nabsul commented 2 years ago

Thanks for the contribution @devec0 ! As someone who doesn't use it at all, I'm hoping looking forward to using your PR to learn more about Helm.

I'll try to review this ASAP, but please be patient. If you don't hear back from me in a couple of weeks, please feel free to ping me with a reminder.

Thanks again!

xavidop commented 2 years ago

thanks for this contribution @devec0 ! this is so much needed!

armenr commented 2 years ago

I was about to add the same thing today, @xavidop -

The templates are dangerous without the explicit .Release.Namespace

Have a look at mine. Thank you BOTH for YOUR hard work!

https://github.com/armenr/helm-k8s-ecr-login-renew

digitalrayne commented 2 years ago

I've pushed some updates including -

Passing the deployment namespace to the job by default
Allowing override of the namespace from helm configuration explicitly
Adding a pre-install hook that creates the job for immediate renewal on install

Thanks @PawelLipski and @xavidop for the helpful review and kind words.

digitalrayne commented 2 years ago

Additionally, if anyone has changes they would like to make, and would like push access to the forked version I have attached to this PR, either open a PR on my repo and I'll merge and push it - or just please let me know, I will grant push access if needed.

nabsul commented 2 years ago

I did start learning Helm yesterday finally :-). Hopefully in the next week or two I'll be in a position to review and merge this.

digitalrayne commented 2 years ago

Please take your time @nabsul - helm takes some time to get used to, but it is worth it, and thanks for considering this PR 🙂

PawelLipski commented 2 years ago

Hey there... any updates here? ;)

nabsul commented 2 years ago

My update from the last three months:

[x] Prepare for arrival of baby (never completely finished, but as much as possible)
[x] Prepare for parental leave by wrapping up / handing over work projects
[x] Baby arrives
[ ] Get used to new life with baby
[ ] Catch up on open source projects (this PR is the first priority)
[ ] Go back to work (mid-september)

I make no promises, but I really do want to get this done, and I think I'll have the energy (and need the distraction from baby-work) as I adjust to baby life. 🤞 🤞 😄

digitalrayne commented 2 years ago

My update from the last three months:
* [x]  Prepare for arrival of baby (never completely finished, but as much as possible)

* [x]  Prepare for parental leave by wrapping up / handing over work projects

* [x]  Baby arrives

* [ ]  Get used to new life with baby

* [ ]  Catch up on open source projects (this PR is the first priority)

* [ ]  Go back to work (mid-september)
I make no promises, but I really do want to get this done, and I think I'll have the energy (and need the distraction from baby-work) as I adjust to baby life. 🤞 🤞 😄

Congratulations @nabsul 🎉 🎊 It looks to me like your priorities are correct 👍🏻

PawelLipski commented 2 years ago

Wow congrats indeed! No rush, take a good care of the baby 👶🏻 Girl or boy btw? ;)

armenr commented 2 years ago

@nabsul - Congratulations on the birth of your child! I think everyone on this thread/PR agrees that your priorities and your heart are in the right place. <3

nabsul commented 2 years ago

Boy :-) And thank you all for all the kind words ❤️

So many decisions to make with this new child, like: Do I teach him Go or C# first? 😆

digitalrayne commented 2 years ago

I suggest not creating the secret via values to preserve confidentiality. A note could be added to the readme explaining how to create the secret
# secrets.nocommit.yaml
apiVersion: v1
kind: Secret
metadata:
  name: aws-secrets
  namespace: xxx
type: Opaque
data:
  aws-acces-key-id: xxx=
  aws-secret-access-key: xxx=

# kubectl apply -f ./secrets.nocommit.yaml
What do you think ?

Thank you for your fantastic improvements and suggestions @Startouf - I've reviewed them and merged them into this PR.

Startouf commented 2 years ago

Note for @nabsul in the incoming PR that was merged, I have added configuration to serve the helm chart via github pages. See my commit there with information so it should behave like https://itzmanish.github.io/ecr-token-renew

nabsul commented 2 years ago

I suggest not creating the secret via values to preserve confidentiality. A note could be added to the readme explaining how to create the secret
# secrets.nocommit.yaml
apiVersion: v1
kind: Secret
metadata:
  name: aws-secrets
  namespace: xxx
type: Opaque
data:
  aws-acces-key-id: xxx=
  aws-secret-access-key: xxx=

# kubectl apply -f ./secrets.nocommit.yaml
What do you think ?

I agree with this by the way. The secret should be a prerequisite, not a configuration parameter.

Startouf commented 2 years ago

@devec0 can you handle nabsul's review requested changes ?

digitalrayne commented 2 years ago

I suggest not creating the secret via values to preserve confidentiality. A note could be added to the readme explaining how to create the secret
# secrets.nocommit.yaml
apiVersion: v1
kind: Secret
metadata:
  name: aws-secrets
  namespace: xxx
type: Opaque
data:
  aws-acces-key-id: xxx=
  aws-secret-access-key: xxx=

# kubectl apply -f ./secrets.nocommit.yaml
What do you think ?
I agree with this by the way. The secret should be a prerequisite, not a configuration parameter.

Also, I wanted to add to this specifically - the values support is important when working with other tools and things like the helm-secrets plugin to do proper secrets management. I think having both is the right call if you want to be able to supply the secret as a pre-existing definition configured by some other tool (like terraform). Additionally, helm3 stores the values as secrets so there is no downside from a security perspective either.

nabsul commented 2 years ago

Heyo! I'm going to take this into a local branch on this repo to make it easier to continue the work.

nabsul commented 2 years ago

https://github.com/nabsul/k8s-ecr-login-renew/pull/38

nabsul commented 2 years ago

Thanks again for your contributions here! I believe I have https://github.com/nabsul/k8s-ecr-login-renew/pull/38 working the way I want it to work. This PR was a great base to start from, but I decided to deviate a bit in the following ways:

Avoid pre-hooks and just have all resource created in the main event
Skip the one-off job creation in favor of giving the user the one-liner kubectl command needed to run the cronjob once manually (in the NOTE.txt that is displayed after a successful install)
I tried Helm's recommended GH action for publishing the repo, but the more I understand how repos work, the more I feel like it's not a great solution. The main reasons being:
- The Helm chart for this tool should live in the same repo as the actual tool/code
- The repo index however can accommodate multiple tools/projects (I plan to add a Helm chart for KCert), so the index.yaml file should NOT live in this repo. I have moved it to nabsul/helm
- I don't believe in checking tgz files into the git repo. Right now I'm manually creating the tgz file and uploading it as a release. In a later step, I'll create a GH action that does this automatically.
- Automating the index.yaml file would be nice, but for now I'll manually edit it whenever there's a new release.
I still would like to provide a raw-yaml approach for people who prefer kubectl apply, but I'll have that code auto-generated from Helm's --debug and dry-run commands. This will eliminate the need to maintain two separate ways to install the tool.

digitalrayne commented 2 years ago

@nabsul - I'll close this one out in favour of #38

nabsul / k8s-ecr-login-renew

Add helm chart #28