Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create new resource policy, "no-upgrade-existing" #5290

Closed
wants to merge 2 commits into from

Conversation

@michaelfig
Copy link

@michaelfig michaelfig commented Feb 10, 2019

Signed-off-by: Michael FIG michael@fig.org

What this PR does / why we need it:

refs charts #5167

This annotation can be added to existing charts that created random passwords, and they will no longer modify the passwords on upgrade.

See the sample secret.yaml below for usage.

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

Not sure how to create a unit test: this is what I used for testing on my machine, after running helm create helm-random-secret

helm-random-secret/templates/secret.yaml

apiVersion: v1
kind: Secret
metadata:
  name: {{ template "helm-random-secret.fullname" . }}
{{- if not .Values.somePassword }}
  annotations:
    "helm.sh/resource-policy": no-upgrade-existing
{{- end }}
  labels:
    app: {{ template "helm-random-secret.name" . }}
    chart: {{ template "helm-random-secret.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
data:
  {{- if .Values.somePassword }}
  some-password: {{ .Values.somePassword | b64enc | quote }}
  {{- else }}
  some-password: {{ randAlphaNum 10 | b64enc | quote }}
  {{- end }}

t.sh:

RELEASE=t
CHART=helm-random-secret
PATH="$HOME/go/src/k8s.io/helm/bin:$PATH"
helm version
helm delete --purge $RELEASE
echo "=== Expect deleted"
kubectl get secret $RELEASE-$CHART -oyaml | sed -ne 's/^ *some-password: //p' | base64 -D
echo; read
helm upgrade -i $RELEASE ./$CHART
echo "=== Expect secret"
kubectl get secret $RELEASE-$CHART -oyaml | sed -ne 's/^ *some-password: //p' | base64 -D
echo; read
helm upgrade -i $RELEASE ./$CHART
echo "=== Expect same"
kubectl get secret $RELEASE-$CHART -oyaml | sed -ne 's/^ *some-password: //p' | base64 -D
echo; read
helm upgrade -i $RELEASE --set somePassword='mystring' ./$CHART
echo "=== Expect mystring"
kubectl get secret $RELEASE-$CHART -oyaml | sed -ne 's/^ *some-password: //p' | base64 -D
echo; read
helm delete --purge $RELEASE
echo "=== Expect missing"
kubectl get secret $RELEASE-$CHART -oyaml | sed -ne 's/^ *some-password: //p' | base64 -D
…ing an existing resource.

Signed-off-by: Michael FIG <michael@fig.org>
@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Feb 10, 2019

Hey there. Thanks for the PR.

I'm curious to know what's the difference between this and wrapping the whole object in a {{ if .Release.IsInstall }}? I'm pretty sure this capability is already possible today without needing to make any changes to Helm.

Loading

@michaelfig
Copy link
Author

@michaelfig michaelfig commented Feb 10, 2019

I tried that, but helm will delete the resource on upgrade.

Loading

@msvticket
Copy link

@msvticket msvticket commented Feb 11, 2019

As a curious bystander I have a question:
Do this PR have any effect when running "helm template"?

Loading

@michaelfig
Copy link
Author

@michaelfig michaelfig commented Feb 11, 2019

@msvticket, helm template --help says:

This does not require Tiller. However, any values that would normally be
looked up or retrieved in-cluster will be faked locally.

The no-upgrade-existing needs to find resources by looking them up in-cluster, and I haven't implemented any "local faking" for it, so no, there is no effect when running helm template.

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Feb 11, 2019

Ah yes, forgot about that. Sorry!

What about a pre-install hook instead? If I recall, this should work without an issue (and solve the same use case as described):

apiVersion: v1
kind: Secret
metadata:
  name: {{ template "helm-random-secret.fullname" . }}
  annotations:
    "helm.sh/hook": "pre-install"
    "helm.sh/hook-delete-policy": "before-hook-creation"
  labels:
    app: {{ template "helm-random-secret.name" . }}
    chart: {{ template "helm-random-secret.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
data:
  some-password: {{ default (randAlphaNum 10) .Values.somePassword | b64enc | quote }}

That hook will render the secret before any resources are created in Kubernetes, and will not be updated during a helm upgrade. As far as I understand, that should be the same behaviour as what's described here.

Have you tried that before, and if so, what difference does the no-upgrade-existing annotation provide vs. the pre-install hook?

Loading

@michaelfig
Copy link
Author

@michaelfig michaelfig commented Feb 11, 2019

@bacongobbler The difference between no-upgrade-existing and the pre-install hook is that the user can explicitly set the password at any time in my example, and that will upgrade the secret (and a subsequent upgrade will not touch the secret unless it is manually deleted or again explicitly set), as well as automatically deleting the secret on a helm delete.

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Feb 11, 2019

The use cases provided in helm/charts#5167 has been

  1. When I install a chart, I want a password generated for me, which is then persisted and should not change.
  2. As a chart user, I want to be able to set that password during helm install, which is then persisted and should not change.

The pre-install + before-hook-creation hook deletion policy example solves those use cases, which is why I'm resistant to merge a PR that seems to already be solved with existing features.

the user can explicitly set the password at any time in my example

Why would you want to install a chart with a password that gets changed every time you call helm upgrade, then once it's override and lock it in place? Why wouldn't you want to just lock it in place the first time the chart is installed?

Loading

@michaelfig
Copy link
Author

@michaelfig michaelfig commented Feb 11, 2019

Why would you want to install a chart with a password that gets changed every time you call helm upgrade, then once it's override and lock it in place? Why wouldn't you want to just lock it in place the first time the chart is installed?

It wouldn't be changed every time you call helm upgrade, just if you wanted to, say change the random password to an explicit one with helm upgrade ... --set somePassword='mychosenpassword'.

Also, the main advantage (that I've perhaps not stated very well) is that the annotation can be added to an existing chart, and if somebody already installed the old chart with a random password, upgrading to the new chart will not overwrite it. With an init hook, the default Helm behaviours will mean that user of the old chart is out of luck: it will still be deleted (or in the best case, overwritten) when they upgrade to the new chart.

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Feb 12, 2019

Okay, I'm starting to see the use case now. So essentially it's kind of "pinning" an object to its state. Thank you. I'll have another look.

Loading

@michaelfig
Copy link
Author

@michaelfig michaelfig commented Feb 12, 2019

Would no-replace-existing be a better name? I would also like to test that it has the same meaning on helm install, not just upgrade.

Also, what if the chart wants both this policy and also keep? Should resource policies be comma-separated?

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Feb 12, 2019

pin might be a better name. It's a common term used in package management to pin an object at a certain point in time.

Loading

@michaelfig
Copy link
Author

@michaelfig michaelfig commented Feb 12, 2019

I think pin is counterintuitive, as it doesn't pin the resource you add the annotation to, rather just prevents the annotated resource from overriding an existing one, just for this particular helm run.

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Feb 15, 2019

related discussion: #3053 (comment)

Loading

Copy link

@FrenchBen FrenchBen left a comment

LGTM! Can't wait to see this in the next release.

Loading

@michaelfig
Copy link
Author

@michaelfig michaelfig commented Feb 19, 2019

@FrenchBen, @bacongobbler will this be merged?

Thanks,
Michael.

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Feb 19, 2019

Right now I'm focusing on getting 2.13.0 out the door as well as Helm 3 core development work. When I have more time I will get back to this, but for now I don't have the time to review. Sorry!

@FrenchBen's input is appreciated, but he is not a core maintainer. It needs an LGTM from a core maintainer before it can be merged. :)

Loading

@cgetzen
Copy link
Contributor

@cgetzen cgetzen commented Mar 26, 2019

Would this work if we wanted to update one field in the resource, but not another (say a randomly generated ID or password)?

Loading

@michaelfig
Copy link
Author

@michaelfig michaelfig commented Mar 27, 2019

@cgetzen, sorry but no, subsets of a resource are not kept.

The current patch is all-or-nothing.

Loading

@asychev
Copy link

@asychev asychev commented Jul 11, 2019

Is there any ETA for merging this?

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Jul 11, 2019

No ETA. I've been focusing more recently on security fixes for Helm 2 as well as ongoing development for Helm 3 so I have not been able to give this PR the time to think through the design.

If others could actually test this PR out and give it some feedback, that would be very helpful. I'm concerned that it might not solve the use cases in helm/charts#5167, so feedback from users running this in staging/production would be more helpful moving this forward.

Loading

@jaygorrell
Copy link

@jaygorrell jaygorrell commented Aug 26, 2019

What are your thoughts on why this doesn't resolve #5167 @bacongobbler? It really seems like it's exactly what's needed. Would love to see this get in.

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Sep 2, 2019

It only checks the "target" (the chart you are upgrading to) for the "pin" annotation. As soon as the annotation is removed from the template on a helm upgrade, the "pin" is lifted. A very simple use case where this won't work is with rollbacks. If you roll back to an older release that did not have that pin annotation, it will roll back rather than ignoring the update. That probably isn't the desired behaviour.

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Sep 2, 2019

IMO it should be a "set and forget" setting that occurs outside of the update logic. In other words the user should mark the existing resource as "pinned" with the annotation, which then Helm will recognize the annotation and ignore the update. The user must remove the annotation from the resource to allow updates. IOW we should be checking the source for the annotation, not the target.

Loading

@bacongobbler
Copy link
Member

@bacongobbler bacongobbler commented Oct 2, 2019

Closing as this is not the right approach for the problem. If someone wants to implement a PR as well as the supporting documentation that covers the approach described here for Helm 3, that'd be great. Helm 2.15.0 is the last feature release for Helm 2 so we are no longer accepting new features in Helm 2.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants