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 ordering of resources within a chart for Custom Resources #8439

Open
ashokponkumar opened this issue Jul 13, 2020 · 72 comments · May be fixed by #9534, #12541 or #8448
Open

Add support for ordering of resources within a chart for Custom Resources #8439

ashokponkumar opened this issue Jul 13, 2020 · 72 comments · May be fixed by #9534, #12541 or #8448

Comments

@ashokponkumar
Copy link

ashokponkumar commented Jul 13, 2020

Background

Custom Resources need a way to be ordered when deployed in certain cases. For example, When creating a SCC and service account as part of a helm chart, since it is not ordered to be created before deployment and other resources, the scc does not have an effect on the pods.

Solution 1

The simplest fix might be to just add kind "SecurityContextConstraints" just after "ServiceAccount" in the kind_sorter.go file.
https://github.com/helm/helm/blob/release-3.0/pkg/releaseutil/kind_sorter.go

Solution 2 - Generic for all Custom Resources

Similar to how we have an ordering requirement for known resources in the InstallOrder, users want the capability to order custom resources too. For example a custom resource like SCC need to be applied before creation of any pods.

To enable this capability we can follow the same approach we use for hooks. Essentially add a annotation "helm.sh/order-weight" which can take an integer value (+ve or -ve). This will behave the same way as "helm.sh/hook-weight", but for resources not covered by hooks.

Here is how it will work:

The first resources to be installed will be all resources that have negative weights, and then those with zero or no weights, and then those with positive weights. All in ascending order.
For resources which have same weight, Install order is preserved for known resources.

PR here showing the solution.

ashokponkumar added a commit to ashokponkumar/helm that referenced this issue Jul 13, 2020
ashokponkumar added a commit to ashokponkumar/helm that referenced this issue Jul 13, 2020
Fixes helm#8439

Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
@hickeyma
Copy link
Contributor

@ashokponkumar The kind sorter is for Kubernetes resources only. Is "SecurityContextConstraints" an Open Shift object?

@ashokponkumar
Copy link
Author

ashokponkumar commented Jul 13, 2020

@hickeyma I am not sure of the difference between a kubernetes resource and an Openshift object. Isn't everything a Kubernetes resource, even if it is added as a CRD. Each flavour/version of kubernetes has its own set of kinds depending on the operators that are installed in it.

For example, kubectl api-resources does lists SecurityContextConstraints along with every other resource supported in that cluster.

@liuming-dev
Copy link
Contributor

cc #8292 , and this need reviewers.

@liuming-dev
Copy link
Contributor

whether this can be solved via using hook?

@ashokponkumar
Copy link
Author

hooks don't solve this issue for couple of reasons:

  1. Conceptually, hooks are things that need to be done outside the chart context. SCC's and many other CRs are part of the chart.
  2. If a scc is created as part of the chart, then it should be deleted when the chart is deleted. Hooks don't get deleted as far as I understand.

Technically speaking, the right way to handle any CR would be by using the "weight" concept that is used in the helm-hooks. That way it would be very flexible.

@liuming-dev I looked at #8292, and if I read it right, it is using alphabetical ordering or ordering in the manifest. I would assume both are of no significance when it comes to Kind names. Isn't it? Why can't we use weight annotation similar to that used by hooks?

@mattfarina
Copy link
Collaborator

@ashokponkumar There is a difference between conformant Kubernetes and Kubernetes with customizations. Kubernetes conformance is something handled by the CNCF and something that can install to a conformant Kubernetes should be able to be installed into any conformant Kubernetes system.

SecurityContextConstraints is an OpenShift addition that's not part of the conformance.

Helm sorting targets conformant Kubernetes. If the sorting targeted every possible CRD variation it would be a fair amount of work to just handle the public ones. There are many private and proprietary ones that Helm could not handle because it does not have the option of knowing about them.

The best way would be for Kubernetes to hold an ordering but the Kubernetes project considers things to be declarative so that order should not matter. I would not expect to get one from the Kubernetes project.

So, how can Helm handle this in a manner that does not hard code CRD ordering and is backwards compatible in v3? This is an honest question.

@ashokponkumar
Copy link
Author

Thanks @mattfarina. I completely agree with you. Just for my own understanding, I had been trying to understand the Kubernetes flavors, and it would really help me, if you can point to the components that are part of the conformant Kubernetes distribution.

If I were to propose a generic solution that covers all CRs, then it would go as follows:

  1. Have an annotation by the name "helm.sh/weight", which takes an Integer value (-ve or +ve). This a concept that Helm anyway uses for hooks ("helm.sh/hook-weight").
  2. We retain the InstallOrder. We install CRs which have a negative weight in ascending order before those in InstallOrder. Then those with InstallOrder, and then Install those with +ve weights in ascending order.
  3. If anyone specifies a weight for a resource in Install order, which is +ve or -ve, it goes with that batch.
  4. If the weight is 0, then it happens just before those in the install order. So if there is a resource which in InstallOrder, but has a weight of 0, it will be applied before any other resource.

@mattfarina What do you think about this proposal?

@ashokponkumar
Copy link
Author

The best way would be for Kubernetes to hold an ordering but the Kubernetes project considers things to be declarative so that order should not matter. I would not expect to get one from the Kubernetes project.

Technically speaking I would agree with you that the order should not matter. But not all objects were implemented so cleanly for the order to not matter, and I would assume that is the reason we even have InstallOrder in Helm. Kubernetes can not have InstallOrder, since for it each artifact is a discrete artifact. But when it comes to helm, we have the advantage of seeing the application as a whole. So we can technically enforce some order.

For aspects like scc, which influences the roles applied to a ServiceName. Lets say a pod/deployment/statefulset is created before a scc is applied, that pod would not have the right roles assigned by the scc. And the pod till it gets restarted at some later point in time, will never see the changes in the roles.

@hickeyma
Copy link
Contributor

@ashokponkumar Would it be possible to use a separate chart for the SCC and deploy it prior to the app chart?

@ashokponkumar
Copy link
Author

@ashokponkumar Would it be possible to use a separate chart for the SCC and deploy it prior to the app chart?

Technically yes, but it is not ideal for two reasons.

  1. If I have 3 such resources which are dependent on order, then I would need to use 3 charts, which is not scalable.
  2. I would always love to have all the resources in a single folder for a single application for context, and this would make it a bit more messy.

@paulczar
Copy link
Contributor

Regardless of Helm's ability to determine when to apply an SCC I would have some concerns about putting them in the same chart as deployment. Clearly I am not you and don't know your full context and reasoning ... but these are my thoughts on wrapping stuff like SCC or PSP up in a helm chart.

Reading the documentation for Security Context Constraints, it seems to me they're designed to be set by the cluster administration level, not the application deployer. Setting SCC as part of a Helm install is like giving someone sudo access to only run a script they control. Ideally RBAC would be set so that a regular app deployer doesn't even have access to create/modify SCC.

I would second @hickeyma that SCC should be done in a separate chart and run as part of preparing a cluster for applications to be deployed. If a given Chart requires special SCC then that should be handled seperately, otherwise you're effectively allowing an application deployer to set an SCC that allows pretty much anything.

As an example of how I solve similar, I have a set of resources that I always deploy as the cluster admin, things like prometheus, fluentd, cert-manager, etc, as well as all my default Pod Security Policies etc, These are all separate charts that I wrap up in a helmfile chart. If anyone requires a resource that is blocked by my PSP I decide if they should be able to do it and update my charts/helmfile appropriately at the cluster level.

@ashokponkumar
Copy link
Author

Regardless of Helm's ability to determine when to apply an SCC I would have some concerns about putting them in the same chart as deployment. Clearly I am not you and don't know your full context and reasoning ... but these are my thoughts on wrapping stuff like SCC or PSP up in a helm chart.

Valid points. Just to give some more context, I am trying to create a helm chart based operator. The servicename used by the operator generally runs with more permissions than required by the application. So I am trying to let the helm chart create a scc that is limited in permission, and use it for its resources. When I have to add a serviceName to a scc, I have to edit the scc. So instead of polluting other scc's for each of my deployment, I am creating a new scc with only the relevant servicename for each deployment. This is more to limit the permissions I give to my resources/pods.

If I were to propose a generic solution that covers all CRs, then it would go as follows:

  1. Have an annotation by the name "helm.sh/weight", which takes an Integer value (-ve or +ve). This a concept that Helm anyway uses for hooks ("helm.sh/hook-weight").
  2. We retain the InstallOrder. We install CRs which have a negative weight in ascending order before those in InstallOrder. Then those with InstallOrder, and then Install those with +ve weights in ascending order.
  3. If anyone specifies a weight for a resource in Install order, which is +ve or -ve, it goes with that batch.
  4. If the weight is 0, then it happens just before those in the install order. So if there is a resource which in InstallOrder, but has a weight of 0, it will be applied before any other resource.

Do you think it makes sense to use this as a generic CR implementation, so that it can handle such resources, which are dependent on others? Users can fall back to the dual chart approach based on their need and usage pattern. Its gives a choice.

@paulczar
Copy link
Contributor

@ashokponkumar there have been some attempts in the past (example #5214) to implement chart template ordering, but they haven't been followed through to a mergeable state. I would suggest we'd be open to the idea, but I don't see it as a roadmapped item that the maintainers are actively working on.

@ashokponkumar
Copy link
Author

@ashokponkumar there have been some attempts in the past (example #5214) to implement chart template ordering, but they haven't been followed through to a mergeable state. I would suggest we'd be open to the idea, but I don't see it as a roadmapped item that the maintainers are actively working on.

Yes. I too noticed it. Example #3635 seems pretty close to this suggestion. I did not see any objection to this idea though. If the commenters in this thread see this as an useful idea, I will create a new issue for this one.

@ashokponkumar
Copy link
Author

Closing this in favor of #8446, since it covers a more generic solution to this issue.

@hickeyma hickeyma reopened this Jul 14, 2020
@hickeyma hickeyma changed the title Openshift SCC should be installed before any resource which creates a pod Add support for ordering of resources within a chart for Custom Resources Jul 14, 2020
@hickeyma
Copy link
Contributor

Re-opened as it contains comments so far. Updated as proposal.

@ashokponkumar ashokponkumar linked a pull request Jul 14, 2020 that will close this issue
3 tasks
@hickeyma
Copy link
Contributor

hickeyma commented Jul 14, 2020

@ashokponkumar This proposal will be discussed at the Helm dev weekly meeting on this Thursday, the 16th. It would be great if you could attend to pitch the proposal.

@abdennour
Copy link

the only way as per today is to delegate helm to argocd & use sync-wave annotations there.

@andrejusc
Copy link

so on original CRDs flexible sorting aspect support for deployment needs via Helm - is there agreement/disagreement in Helm community? As this issue by now open for 2+ years. Implementation of such support doesn't look to be complex, but is that then affects any other pattern to be aware about?

@joejulian joejulian added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Oct 12, 2022
@joejulian
Copy link
Contributor

Happy to have a contribution to address this. Probably should start with a HIP also take a look at the contributing doc.

@github-actions
Copy link

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Jan 10, 2023
@pawellegowski89
Copy link

Solution is here:
#9534
helm/community#230

@NiklasWagner please fix this PR according to the reviewer's comments.
We are waiting for this solution!

@github-actions github-actions bot removed the Stale label Jan 11, 2023
@joejulian joejulian added feature in progress and removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. proposal labels Jan 25, 2023
@sergey-kudriavtsev
Copy link

sergey-kudriavtsev commented Feb 21, 2023

Hi guys!

I have an alternative suggestion..

  1. I am a diagram developer, so I understand exactly in what order they should be installed.
  2. Offering to simply create an flow folder, where all the contents, level 1, will be based on alphabetical sorting.
    flow/
    1-example.yaml
    ./alpha crd
    ./betta-crd

But I personally think the solution is even simpler

It is strange that we are in 2023, when AI draws and programs, we are trying to indicate the order. You can just use a heuristic algorithm.
Simple loop example:

  1. If we are repeatedly in a cycle, check the allowable number of cycles, take a pause.
  2. Take a resource from the queue for installation.
  3. Attempt to install/update a resource,
    If:
    3.1 - resource installed
    3.2 - the resource is not installed, - Move the resource to the next cycle of the queue.
  4. If the queue is not empty then return to Step 3 Else run the next cycle (Step 1)

@aladdin-atypon
Copy link

This feature should be intuitively provided long ago! I have ExternalSecret objects and these are used by statefulset, the statefulset can't be ready without these secrets created by ExternalSecret.

It's not like we need to create separate chart for such case!

@Dave-c-Ross
Copy link

I faced another use case today, in a unique chart, I have openshift CRDs MachineSet ClusterAutoscaler and MachineAutoscaler. MachineSet has to be created before MachineAutoscaler, since MachineAutoscaler reference MachineSet.

So I had to create different charts for each kind

@TOHUHM
Copy link

TOHUHM commented Aug 2, 2023

I met the some issue when I'm trying to deploy externalSecret, any solution for now?

@aladdin-atypon
Copy link

No progress or update?

@motabass
Copy link

motabass commented Oct 2, 2023

We are still using multiple Helm-Charts only to get the ordering right. This approach seems so wrong because the idea seemed to be 1 chart for 1 application...now i have to use 2 just to have some CRs being in place before Kubernetes-Kinds. I mean, is having CustomResources in an helm-chart really that much of an edge case, especially when there seem to be first class support for CRDs? All efforts already made seem to be stale...are there any plans for fixing this?

@corinz
Copy link

corinz commented Oct 25, 2023

My use case for this feature would be to only update Istio DestinationRule and VirtualService after any new replicasets/pods are ready. Today, they are created together, before pods are ready (even when using --wait). Hooks are undesirable because these resources are orphaned/distinct from the release.

Does anyone have a suggestion for a workaround? Could we not allow the sort order slice to be merged with a user input?

@karmops
Copy link

karmops commented Feb 26, 2024

Regarding this, maybe it could be solved using (kustomize), instead of helm.

@owais
Copy link

owais commented Feb 29, 2024

I have another unrelated use case custom ordering could help with. I implemented a post-renderer for our internal usage. The post-renderer is quite important and we cannot afford a person or system not using it as it can result in very significant diffs that can be destructive. We also need to provide some additional context to the post-renderer. Usually this can be done via post renderers args but the data we need to provide is contextual and usually is stored in different values files for different environments. We solved both of these issues by adding a custom template that an unsupported resource called "HelmContext". The resource uses data from values to render itself. The post renderer program extracts information it needs from this helm context resource and operates on the rest of the resources based on this info. The post renderer program removes this helm context resource from the output so it is never installed in k8s. This gives us a very nice side-effect i.e, the release install would fail without going through the post renderer implicitly enforcing using post-renderer.

Now the issue we face is that custom resources are applied after everything else so this works only when CI runs helm diff or helm template before install. Someone manaully running install would still see the failure but other resources would be applied before the error would be thrown.

Being able to tell helm to always install our custom resource before anything else would very elegantly solve this issue for us :)

@hajdukda
Copy link

Just hit the same issue with Custom Resources for Vault secrets :).. Custom Resource should be installed first coz it's a dependency for everything else.

@brc
Copy link

brc commented Aug 6, 2024

🍿 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment