feat(helm): initial try at making a helm chart #62
Conversation
|
Welcome @hutchic! |
|
Hi @hutchic. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-ok-to-test
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hutchic The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/XL
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
/check-cla |
k8s-ci-robot
added
cncf-cla: yes
| @@ -0,0 +1,62 @@ | |||
| {{/* | |||
There was a problem hiding this comment.
Out of interest, is there a canonical version of this file? If so, it might be useful to link to it in a comment.
There was a problem hiding this comment.
this is the current latest that gets generated by helm create chart
There was a problem hiding this comment.
Ah got it. Maybe add a "Generated by 'helm create chart'" comment unless that'll be overwritten in the future?
| @@ -0,0 +1,15 @@ | |||
| {{- if .Values.serviceAccount.create -}} | |||
There was a problem hiding this comment.
Why not use the default SA in the hnc-system directory? That would eliminate this and (I think) the cluster role binding.
There was a problem hiding this comment.
that would work but my understanding of the hnc docs is we should/must apply the labels first before installing the controller right? If that's the case we need to pull some things out of band specifically
- install the sa / rolebinging
- run the job that adds the label
- now install the controller et al
There was a problem hiding this comment.
Ah, I see. In that case, can we call it something like hnc-installer so that it's clear what this is for? Ditto for the cluster role binding.
Also, if it's essentially a mandatory part of installing HNC, why are we giving users the option to disable it?
As a side note, you actually can apply the labels after installing HNC, but that will mean that there's a period of time during which the namespaces are not actually excluded. Worse, while HNC is starting up, it may forbid changes to the namespaces, so it could take a while to add the labels. So it's definitely better to label them before installing HNC.
| @@ -0,0 +1,721 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
Instead of just checking this in by hand, can you please update the makefile to regenerate it? It's ok to check this in (I think Helm requires that it's checked in, right?), but other than committing it, a developer shouldn't have to do something other than (say) make manifests. Otherwise this will quickly drift away from the regular version.
Also, can you please add a comment at the top of this file (automatically inserted, of course) saying something like "Do not edit this file by hand; run 'make manifests' [or whatever] to update."
Thanks!
There was a problem hiding this comment.
that's one of the todo items / why it's a draft PR. Wanted some initial feedback before proceeding down the wrong routes :)
There was a problem hiding this comment.
So we should discuss what to generate and how.
-
Helm standards / common practices for example is to split a file like this up such that there's a
deployment.yamlfile aserviceaccount.yamlfile and so on. Is it feasible to do this / do we want this? -
Helm provides a mechanism for namespace creation and targetting. So in theory namespace creation shouldn't be part of the helm chart and anywhere there's a namespace defined it should be a variable
namespace: {{ .Release.Namespace }} -
Helm provides a mechanism for label definition again how challenging will this be technically / how badly do we want or need it ie
apiVersion: apps/v1
kind: Deployment
metadata:
name: {{ include "temp.fullname" . }}
labels:
{{- include "temp.labels" . | nindent 4 }}
spec:
{{- if not .Values.autoscaling.enabled }}
replicas: {{ .Values.replicaCount }}
{{- end }}
selector:
matchLabels:
{{- include "temp.selectorLabels" . | nindent 6 }}
I'm would prefer incremental improvement so a first pass of "an entire static yaml file is generated" with backlog tickets for further helm-ification would be my personal preference but worth discussing before assuming that route is suitable.
There was a problem hiding this comment.
Thanks! And yes, I'm just giving early feedback, I realize this is a draft.
I'd personally prioritize "ease of automation" over "following all the Helm guidelines" for now. I'd rather have something up-to-date and a little nonstandard than a perfect but obsolete chart.
For deployment.yaml vs serviceaccount.yaml - I think I'd say that if you can generate them from anything in config/ or from the output of kustomize, then go for it. We can also iterate on that - start as a monolithic file and then use something like the Gatekeeper tooling to split it up.
re namespace - I don't think we use the string hnc-system anywhere else, so you could totally add a sed command in the makefile to replace it with {{ .Release.Namespace }}. Note that there might be strings that include hnc-system but that should be pretty easy to check and/or exclude. But I'm totally ok with not doing any of that as well. I think the number of people who would object to hnc-system as well as the other names and labels inside that namespace should be vanishingly small, and I'd be very happy with never adding that flexibility unless someone asks for it and/or is willing to do it themselves.
| # This is the chart version. This version number should be incremented each time you make changes | ||
| # to the chart and its templates, including the app version. | ||
| # Versions are expected to follow Semantic Versioning (https://semver.org/) | ||
| version: 0.1.0 |
There was a problem hiding this comment.
I'm not clear on what this is supposed to be. E.g. if we update HNC from v0.8.0 to v0.9.0, should this go from v0.1.0 to v0.2.0?
There was a problem hiding this comment.
my understanding which could be incorrect:
- chart version changes when the helm chart itself changes
- app version changes when the application itself changes
so appVersion should be part of the release process to auto bump it. I guess chart version bumps handled in the PR template as process(?)
There was a problem hiding this comment.
We don't really have an "auto bump" anywhere but it's fine to just document the process and expect the releaser to follow it. That's what we do for the version numbers in our docs.
As for when we increase the minor vs patch version of the chart, I'm happy to just explain it in the comments right here. E.g.: the chart major/minor/patch versions must increment whenever the corresponding app major/minor/patch version changes, but never needs to be the same as those numbers. For example, if we have chart=0.1.0 and app=0.8.0, and we change the app to 0.9.0, we should change the chart to 0.2.0. But if we change it to 0.8.1, we should change the chart to 0.1.1. In addition, we can increment that chart versions for any other reason at any time.
| @@ -0,0 +1,15 @@ | |||
| {{- if .Values.serviceAccount.create -}} | |||
There was a problem hiding this comment.
Ah, I see. In that case, can we call it something like hnc-installer so that it's clear what this is for? Ditto for the cluster role binding.
Also, if it's essentially a mandatory part of installing HNC, why are we giving users the option to disable it?
As a side note, you actually can apply the labels after installing HNC, but that will mean that there's a period of time during which the namespaces are not actually excluded. Worse, while HNC is starting up, it may forbid changes to the namespaces, so it could take a while to add the labels. So it's definitely better to label them before installing HNC.
hack/helm/values.yaml
Outdated
| nameOverride: "" | ||
| fullnameOverride: "" |
There was a problem hiding this comment.
Comment these, i.e. what are the default values ("hnc"?) and what do they control?
There was a problem hiding this comment.
added a comment. This is the current default that get's provided when one does a helm create which I'll explain in more detail here and you can tell me if the comment is suitable.
helm install 123 . sensible defaults apply and objects get named hnc-123 which is a combination of Chart.yaml name and the release name
helm install --set nameOverride=foo 123 objects get named foo-123
helm install --set fullnameOverride=bar 123 objects get named bar
hack/helm/values.yaml
Outdated
| # If not set and create is true, a name is generated using the fullname template | ||
| name: "" | ||
|
|
||
| hncIgnoreNamespaces: |
There was a problem hiding this comment.
hncExcludeNamespaces would be better, and you can also point to the right part of the docs in a comment (as you can tell, I'm pretty big on comments!).
However, simply adding a namespace here will not actually exclude the namespace; you'd also have to add the ----excluded-namespace option to the HNC deployment. So maybe it's better to hardcode these for now, unless you want to somehow add it to the Deployment in a template?
There was a problem hiding this comment.
renamed.
However, simply adding a namespace here will not actually exclude the namespace
Really? My understanding of the docs was labelling namespaces appropriately would tell the HNC controller to ignore them.
There was a problem hiding this comment.
No, it's a two-step process:
- Why not just let people add the label to the namespace (after all, this is what Kubernetes itself cares about)? Because if we did, anyone who could edit a namespace could exempt themselves from HNC's oversight simply by adding the label, and we don't want to allow that. As a result, if you try to add this label to any normal namespace, HNC will stop you.
- Why not just add the namespace to the
--exclude-namespaceflag, and ask HNC itself to label the namespace on our behalf? Because we don't want HNC to automatically be making changes to critical namespaces likekube-system, and users may not want to give it permission to do so anyway.
As a result, we require both. The admin of HNC needs to say --exclude-namespace, which (now that I think about it) probably should have been called --allow-namespace-exclusion, because it gives us permission to add the label to the namespace. And then, someone with edit access to that namespace needs to actually exclude it. Or, if you haven't installed HNC yet, you can do them in the reverse order.
Does that make sense?
There was a problem hiding this comment.
NB - this is no longer true in the next release. Simply updating --exclude-namespace will be enough.
…espace to a variable
| kind: "" | ||
| plural: "" | ||
| conditions: [] | ||
| storedVersions: [] |
There was a problem hiding this comment.
Can you drop status: part from CRDs as those make this chart incompatible with https://fleet.rancher.io as it update that part to contains data like:
status:
acceptedNames:
kind: HierarchyConfiguration
listKind: HierarchyConfigurationList
plural: hierarchyconfigurations
singular: hierarchyconfiguration
conditions:
- lastTransitionTime: "2021-09-09T08:15:55Z"
message: no conflicts found
reason: NoConflicts
status: "True"
type: NamesAccepted
- lastTransitionTime: "2021-09-09T08:15:55Z"
message: the initial names have been accepted
reason: InitialNamesAccepted
status: "True"
type: Established
storedVersions:
- v1alpha2and deployments end up to "Modified" status.
| kind: "" | ||
| plural: "" | ||
| conditions: [] | ||
| storedVersions: [] |
There was a problem hiding this comment.
Can you drop status: from here too.
| kind: "" | ||
| plural: "" | ||
| conditions: [] | ||
| storedVersions: [] |
There was a problem hiding this comment.
Can you drop status: from here too.
|
Hi @hutchic |
|
@SydMusavi I am not. It was mentioned somewhere on an issue that if someone wishes to take over to feel free. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
@adrianludwin looks like this has also been dropped. Shall we close? |
I would like to see a Helm chart to install HNC! Any chance someone could pick this up? |
|
The funny thing is, with all the variants we're producing, that just makes
Helm charts _more_ difficult. The real challenge here will be taking the
manifests that are produced by the HNC build process and autogenerating the
charts from them. Gatekeeper's found a way to do this but it would be
nontrivial to adopt it.
I don't want anyone to just build a Helm chart by hand, because that will
quickly become unsupported IMO.
…On Tue, Mar 29, 2022 at 5:23 PM Erik Godding Boye ***@***.***> wrote:
@adrianludwin <https://github.com/adrianludwin> looks like this has also
been dropped. Shall we close?
I would like to see a Helm chart to install HNC! Any chance someone could
pick this up?
—
Reply to this email directly, view it on GitHub
<#62 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE43PZDSVMUQQH5OCLZMRMTVCNYFVANCNFSM5AN6O5FQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I'm going to close this PR, since with the new variant manifests, it's even more infeasible to follow this approach than it was when there was only one. If someone wants to revive the overall idea that's great, but it needs to be automated and based off of the output of /close |
|
@adrianludwin: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Agreed, we can always reopen
/close
…On Wed, Mar 23, 2022 at 5:56 PM Ryan Bezdicek ***@***.***> wrote:
@adrianludwin <https://github.com/adrianludwin> looks like this has also
been dropped. Shall we close?
—
Reply to this email directly, view it on GitHub
<#62 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE43PZCT2WVNYNV66PPGG7TVBOHQNANCNFSM5AN6O5FQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
related to #46
This is a rough draft of adding a helm chart. Opening as a draft as it will require changes:
hack/templates/hnc-manager.yaml