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

Add support for --no-hooks switch in Helm template #545

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

PatTheSilent
Copy link
Contributor

This will allow users disabling hooks when rendering manifests from helm templates.

A use-case is, for example, installing Loki on EKS from https://github.com/grafana/helm-charts/tree/main/charts/loki-stack
When running tk diff you'll get an error

Error from server (BadRequest): admission webhook "iam-for-pods.amazonaws.com" does not support dry run

This isn't an issue in Tanka but the scope of this functionality is not limited to it and I think you can very much find use-cases outside of this example.

A potential improvement here could be to allow passing any args to helm template maybe using an array without any validation.

@sh0rez
Copy link
Member

sh0rez commented Mar 23, 2021

Hey!

From helm template --help:

--no-hooks     prevent hooks from running during install

With "hook" I guess they mean Chart hooks, which appear to be Kubernetes Job resources underneath. Furthermore, these only run during install, but Tanka uses template.

How would disabling hooks aid in Admission controllers not implementing dry run?

@PatTheSilent
Copy link
Contributor Author

@sh0rez it helps by not having anything to validate 😄
For example the test-success hook in loki-stack is a Pod that runs something. The Admission Controller that I've mentioned runs only for Pod objects. By disabling the hook there is no Pod manifest to validate by the Admission Controller and tk diff runs happily.

A problem with hooks implemented as Pods is also that when running with tanka there is nothing to delete that Pod after it's done its supposed job. Like https://github.com/localstack/helm-charts/tree/main/charts/localstack/templates/tests. This one is failing on my clusters for some reason and is running indefinitely, consuming resources.

And, like I've mentioned, I don't think this is a problem with tanka but with chart maintainers and charts in general. But it would be helpful to be able to disable those hooks until they're fixed or if you don't really want them running.

@Duologic
Copy link
Member

Duologic commented Mar 23, 2021

Maybe we can simply hide the resource in jsonnet that is generated by helm template that triggers this behavior.

Can you share a minimal reproducible example? I'd like to try it out and see if how it behaves.

@PatTheSilent
Copy link
Contributor Author

@Duologic yes, you can do that.
This is more or less what I currently have in my project. To get it working with mainline tanka you'd need to add that override that's commented out.

  lokiStack:
    helm.template('loki-stack', 'charts/loki-stack', {
      namespace: 'monitoring',
      NoHooks: true,
      values: {}
    })
//    + {
//      'pod_loki_stack_test': {} //disable the test, otherwise `diff` fails on admission webhooks
//    }

Overriding that in jsonnet is not a problem but it felt somewhat hacky, which motivated me to submit this PR.
I feel that we should have more control over the execution of helm template than we do have now. We already have IncludeCRDs as an option.

@Duologic
Copy link
Member

I would say the jsonnet override is actually a good solution to workaround this.

@sh0rez You were more about tying down on helm template arguments, is that still the case? Are we slowly going to implement every argument? Or maybe just have a list of argurments in jsonnet to add arbitrary arguments (like the original implementation)?

@PatTheSilent
Copy link
Contributor Author

@sh0rez @Duologic if there we some design decisions behind the current implementation, I was not aware of them. I have no intent on insisting on this feature as there is a decent workaround. If you feel it be better to have a list of arguments instead of strict parameters, I'm more than fine with it can can adjust this PR or close it altogether.

@Duologic
Copy link
Member

I have no strong opinion on which direction to choose, I was hoping on some input from @sh0rez when he gets to this.

Copy link
Member

@sh0rez sh0rez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

There was some confusion on my side, I did not expect helm template to include Helm hooks, as that makes limited sense to me, given no one is controlling hook rollout, but so be it.

@Duologic as to whether to open the floodgates on any flag – let's see how this goes. If we exceed like 10 flags or so, sure, let's do that instead then.

@sh0rez sh0rez merged commit 1e4d1dd into grafana:master Mar 30, 2021
@PatTheSilent
Copy link
Contributor Author

Thanks both!

@sh0rez believe me when I say I was no less confused than you by helm template behavior.

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

Successfully merging this pull request may close these issues.

None yet

3 participants