Add release name to CRD spec

vmware-archive / helm-crd

Experimental CRD controller for managing Helm releases

Apache License 2.0

100 stars 13 forks source link

Add release name to CRD spec #29

Closed andresmgot closed 6 years ago

andresmgot commented 6 years ago

This PR adds the possibility of specifying a release name. If not given it will use namespace-name by default.

Commits:

839882d - Codegen updates. I needed to update update-codegen.sh to generate the proper copyright.
1aca4f6 - Spec and controller changes.
b73c2a0 - Added a finalizer to be able to clean Tiller releases.

andresmgot commented 6 years ago

I think you have another commit that adds finalizers bundled with this one, although the two are unrelated afaics.

I added the finalizer after realizing that the delete flow was still using a hard-coded namespace-name (and we need the finalizer to know the name of the Tiller release before the custom resource gets deleted).

Thanks for the review!

anguslees commented 6 years ago

Aha, makes sense. Yes, we need reliable deletion with finalizers, or some sort of more complex mark+sweep global garbage collector.

andresmgot commented 6 years ago

I agree with your concern. Releases have a field Namespace that we can use to avoid HelmReleases affecting other namespaces. We can open a different issue for that if you agree.

anguslees commented 6 years ago

The release namespace doesn't do what you think it does...

The release's "namespace" value is the k8s namespace that resources will be installed into (assuming a well-behaved chart) - and helm-crd always sets this to the same namespace as the HelmRelease CRD.

The issue here however, is that the release name is global (per tiller). So by allowing it to be specified arbitrarily, we have allowed users to affect charts belonging to other users.

andresmgot commented 6 years ago

Yes, I know that it refers to the k8s namespace but it is true that nothing prevents a chart to ignore that value and install stuff in other namespaces.