helm fixes

[bwagner5]

I did some manual testing and found a few things.

• Added inferring of KARPENTER_IAM_ROLE_ARN in Makefile so that devs don't have to set that in an env var
• Fixed HELM_OPTS serviceAccount annotations --set (needed an extra \ for escaping .
• There were a few missing {{- end }}
• Although namespace may not be required when using helm install or helm upgrade --install (didn't try that), but it is when using helm template, so when I originally tried make apply which does a helm template ... | ko apply it installed karpenter in the default namespace.
• The webhook certificate was inferring karpenter-webhook for the domain so changed to just karpenter
• There was a missing - matchExpressions: on the nodeAffinity in the values.yaml file.

[stevehipwell]

Thanks @bwagner5, I don't think this PR was my best work; comments below and inline.

• That make change looks much better
• I did wonder why helm template was used but completely forgot to add the --namespace flag to the apply
• It's not idiomatic to add namespace in chart metadata as it's not needed, but we could change that here if we want to
• I thought I'd added the \. to the annotations
• I have no idea how the {{- end }} tags were broken, or how I missed that
• Likewise no idea how I missed the matchExpressions

[bwagner5]

Thanks @bwagner5, I don't think this PR was my best work; comments below and inline.

• That make change looks much better

• I did wonder why helm template was used but completely forgot to add the --namespace flag to the apply

• It's not idiomatic to add namespace in chart metadata as it's not needed, but we could change that here if we want to

• I thought I'd added the \. to the annotations

• I have no idea how the {{- end }} tags were broken, or how I missed that

• Likewise no idea how I missed the matchExpressions

The helm template before actually did include the namespace and it still doesn't add it to the metadata. It does seem to add it when you do a regular helm install, not sure why they would work differently... After noticing the namespace weirdness on the template here, I confirmed that NTH is also missing the namespace on the generated plain yaml files we distribute with the release. Until we figure out why, let's just keep the explicit namespace in the metadata.

And seriously, no worries on some of these small issues that slipped through the cracks! It was a huge change and is in a much much cleaner state than we had it before. We really appreciate all the time you put into making karpenter and node termination handlers helm chart better!!

I was super excited to see a single karpenter pod when testing yesterday!

[stevehipwell]

The helm template before actually did include the namespace and it still doesn't add it to the metadata. It does seem to add it when you do a regular helm install, not sure why they would work differently... After noticing the namespace weirdness on the template here, I confirmed that NTH is also missing the namespace on the generated plain yaml files we distribute with the release. Until we figure out why, let's just keep the explicit namespace in the metadata.

The helm template command doesn't add the namespace to metadata by design, the following two commands are comparable helm upgrade --install --namespace test mychart myrepo/mychart & helm template --namespace test mychart myrepo/mychart | kubectl apply --namespace test -f -

The idiomatic Helm pattern breaks down when doing things like RBAC so I'd be happy to break this and always set namespace to release namespace.

[stevehipwell]

Sorry, I thought I'd pushed these for discussion.

[bwagner5]

Ok, I think that addresses everything! Let me know if you're cool with everything and we can get this merged today! :)

[stevehipwell]

@bwagner5 that looks great, shall I merge your branch into mine so you can take both commits in the original PR?

As a side note would you like a PR to NTH to add namespace to all of the resources for consistency?

[bwagner5]

Sure, if you merge this PR into your branch, I can approve the original PR and merge it there.

We were planning on releasing the NTH helm chart today and add the release namespace in the metadata. If you want to PR the addition, that would be great. But I know our timezones are off by quite a bit, so no worries if you can't get to it.

[stevehipwell]

Do you want to structure your commits more or are you happy with them as they are?

[bwagner5]

I think they're fine, they'll get squashed anyways :D