From 6d420e8cce6fec7faa77e2dedeb261ebdfd1d5bc Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Fri, 6 Dec 2024 20:17:07 +0000 Subject: [PATCH 01/10] start helm hip Signed-off-by: Austin Abro --- hips/hip-0999.md | 63 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 hips/hip-0999.md diff --git a/hips/hip-0999.md b/hips/hip-0999.md new file mode 100644 index 00000000..dd011c47 --- /dev/null +++ b/hips/hip-0999.md @@ -0,0 +1,63 @@ +--- +hip: 9999 +title: "Wait With kstatus" +authors: [ "@austinabro321" ] +created: "2024-12-06" +type: "feature" +status: "draft" +--- + +## Abstract + +Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait for all resources to be fully reconciled, and does not wait for custom resources (CRs) at all. By replacing the current wait logic with kstatus Helm will achieve *better* waits, while simplifying it's code and documentation. + +## Motivation + +Clearly explain why the existing design is inadequate to address the problem +that the HIP solves. + +Certain workflows require CRDs to be ready before continuing to the next step. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. + +Certain workflows may want to wait until their resources are fully reconciled before continuing to their next step. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. Ensuring only the new pods are available after waiting for an upgrade can come in handy. + +By introducing kstatus we will lower user friction with the `--wait` flag. + +## Rationale + +Describe why particular design decisions were made. + +## Specification + +Describe the syntax and semantics of any new feature. + +## Backwards compatibility + +Describe potential impact and severity on pre-existing code. + +## Security implications + +No security implications + +## How to teach this + +Replace the [existing wait documentation](https://helm.sh/docs/intro/using_helm/) by explaining that we use kstatus and pointing to the [kstatus documentation](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md). This comes with the added benefit of not needed to maintain Helm specific wait documentation. A separate team has already written extensive documentation on how the wait logic of kstatus works. + +## Reference implementation + +Link to any existing implementation and details about its state, e.g. +proof-of-concept. + +Here is an implementation of waiting for resources using kstatus in a project I maintain called Zarf. I will replace this section with a draft PR showing an example implementation in Helm once I have feedback that this HIP is in the right direction. + +## Rejected ideas + +Why certain ideas that were brought while discussing this HIP were not +ultimately pursued. + +## Open issues + +[8661](https://github.com/helm/helm/issues/8661) + +## References + +A collection of URLs or materials used as references through the HIP. \ No newline at end of file From 39a043c85914f13c5a61e2ef205bf2643097eff5 Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Thu, 12 Dec 2024 17:06:01 +0000 Subject: [PATCH 02/10] updates Signed-off-by: Austin Abro --- hips/hip-0999.md | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index dd011c47..c640dd67 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -9,13 +9,10 @@ status: "draft" ## Abstract -Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait for all resources to be fully reconciled, and does not wait for custom resources (CRs) at all. By replacing the current wait logic with kstatus Helm will achieve *better* waits, while simplifying it's code and documentation. +Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait for all resources to be fully reconciled, and does not wait for custom resources (CRs) at all. By replacing the current wait logic with kstatus, Helm will achieve more intuitive waits, while simplifying it's code and documentation. ## Motivation -Clearly explain why the existing design is inadequate to address the problem -that the HIP solves. - Certain workflows require CRDs to be ready before continuing to the next step. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. Certain workflows may want to wait until their resources are fully reconciled before continuing to their next step. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. Ensuring only the new pods are available after waiting for an upgrade can come in handy. @@ -30,10 +27,20 @@ Describe why particular design decisions were made. Describe the syntax and semantics of any new feature. +From a CLI user's perspective there will be no changes in how waits are called, they will still use the `--wait` flag. + +Kstatus does not output any logs, Helm will supply it's own logs similar to the current logic. Helm with output a log message each second signalling that a resource is not ready. Helm will log one not ready resource at a time to not overwhelm users with output. The resource will be chosen alphabetically by `metadata.name`. + +A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pkc up the custom resources during wait. + + + ## Backwards compatibility Describe potential impact and severity on pre-existing code. +Waiting for custom resources and for reconciliation to complete for every resource could lead to charts timing out that weren't previously. + ## Security implications No security implications @@ -44,15 +51,13 @@ Replace the [existing wait documentation](https://helm.sh/docs/intro/using_helm/ ## Reference implementation -Link to any existing implementation and details about its state, e.g. -proof-of-concept. +Once I have feedback that this HIP is in the right direction I will make a draft PR in Helm implementing this feature. -Here is an implementation of waiting for resources using kstatus in a project I maintain called Zarf. I will replace this section with a draft PR showing an example implementation in Helm once I have feedback that this HIP is in the right direction. +For now, see [healthchecks.go](https://github.com/zarf-dev/zarf/blob/main/src/internal/healthchecks/healthchecks.go) in a project I maintain called Zarf. This is how we implement kstatus. After each Helm install or upgrade in Zarf, we get every resource from the chart and send them into the `WaitForReady` function. ## Rejected ideas -Why certain ideas that were brought while discussing this HIP were not -ultimately pursued. +TBD ## Open issues @@ -60,4 +65,6 @@ ultimately pursued. ## References -A collection of URLs or materials used as references through the HIP. \ No newline at end of file +existing wait documentation - https://helm.sh/docs/intro/using_helm/ +kstatus documentation - https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md +Zarf kstatus implementation - \ No newline at end of file From c9dbf03318515040805bc3f9079df0033f37fd56 Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Thu, 12 Dec 2024 17:18:11 +0000 Subject: [PATCH 03/10] grammar Signed-off-by: Austin Abro --- hips/hip-0999.md | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index c640dd67..70ffcac7 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -9,36 +9,32 @@ status: "draft" ## Abstract -Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait for all resources to be fully reconciled, and does not wait for custom resources (CRs) at all. By replacing the current wait logic with kstatus, Helm will achieve more intuitive waits, while simplifying it's code and documentation. +Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait for all resources to be fully reconciled, and does not wait for custom resources (CRs) at all. By replacing the wait logic with kstatus, Helm will achieve more intuitive waits, while simplifying its code and documentation. ## Motivation -Certain workflows require CRDs to be ready before continuing to the next step. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. +Certain workflows require custom resources to be ready before continuing to the next step. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. -Certain workflows may want to wait until their resources are fully reconciled before continuing to their next step. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. Ensuring only the new pods are available after waiting for an upgrade can come in handy. +Certain workflows may want to wait until their resources are fully reconciled before continuing to their next step. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. This ensures only the new pods are available after waiting for an upgrade can come in handy. By introducing kstatus we will lower user friction with the `--wait` flag. ## Rationale -Describe why particular design decisions were made. +Leveraging a existing status management tool maintained by the Kubernetes team will simplify the code and documentation that Helm needs to maintain and improve the functionality of `--wait`. ## Specification -Describe the syntax and semantics of any new feature. - From a CLI user's perspective there will be no changes in how waits are called, they will still use the `--wait` flag. -Kstatus does not output any logs, Helm will supply it's own logs similar to the current logic. Helm with output a log message each second signalling that a resource is not ready. Helm will log one not ready resource at a time to not overwhelm users with output. The resource will be chosen alphabetically by `metadata.name`. +Kstatus does not output any logs. Helm will output a log message each second signalling that a resource is not ready. Helm will log one not ready resource at a time to not overwhelm users with output. This behavior will look similar to the current `--wait` logging. The resource will be chosen alphabetically by `metadata.name`. -A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pkc up the custom resources during wait. +A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pick up the custom resources during wait. ## Backwards compatibility -Describe potential impact and severity on pre-existing code. - Waiting for custom resources and for reconciliation to complete for every resource could lead to charts timing out that weren't previously. ## Security implications @@ -47,7 +43,7 @@ No security implications ## How to teach this -Replace the [existing wait documentation](https://helm.sh/docs/intro/using_helm/) by explaining that we use kstatus and pointing to the [kstatus documentation](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md). This comes with the added benefit of not needed to maintain Helm specific wait documentation. A separate team has already written extensive documentation on how the wait logic of kstatus works. +Replace the [existing wait documentation](https://helm.sh/docs/intro/using_helm/) by explaining that we use kstatus and pointing to the [kstatus documentation](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md). This comes with the added benefit of not needing to maintain Helm specific wait documentation. A separate team has already written extensive documentation on how the wait logic of kstatus works. ## Reference implementation @@ -67,4 +63,4 @@ TBD existing wait documentation - https://helm.sh/docs/intro/using_helm/ kstatus documentation - https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md -Zarf kstatus implementation - \ No newline at end of file +Zarf kstatus implementation - https://github.com/zarf-dev/zarf/blob/main/src/internal/healthchecks/healthchecks.go \ No newline at end of file From 472f81a82a3e596fb72007af7ce128d4463aaf3e Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Thu, 12 Dec 2024 17:25:01 +0000 Subject: [PATCH 04/10] hip Signed-off-by: Austin Abro --- hips/hip-0999.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index 70ffcac7..9d879c16 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -9,19 +9,19 @@ status: "draft" ## Abstract -Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait for all resources to be fully reconciled, and does not wait for custom resources (CRs) at all. By replacing the wait logic with kstatus, Helm will achieve more intuitive waits, while simplifying its code and documentation. +Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait for all resources to be fully reconciled, and does not wait for custom resources (CRs) at all. By replacing the wait logic with [kstatus](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md), Helm will achieve more intuitive waits, while simplifying its code and documentation. ## Motivation -Certain workflows require custom resources to be ready before continuing to the next step. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. +Certain workflows require custom resources to be ready. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. -Certain workflows may want to wait until their resources are fully reconciled before continuing to their next step. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. This ensures only the new pods are available after waiting for an upgrade can come in handy. +Certain workflows requires resources to be fully reconciled. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. By introducing kstatus we will lower user friction with the `--wait` flag. ## Rationale -Leveraging a existing status management tool maintained by the Kubernetes team will simplify the code and documentation that Helm needs to maintain and improve the functionality of `--wait`. +Leveraging a existing status management library maintained by the Kubernetes team will simplify the code and documentation that Helm needs to maintain and improve the functionality of `--wait`. ## Specification @@ -43,13 +43,13 @@ No security implications ## How to teach this -Replace the [existing wait documentation](https://helm.sh/docs/intro/using_helm/) by explaining that we use kstatus and pointing to the [kstatus documentation](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md). This comes with the added benefit of not needing to maintain Helm specific wait documentation. A separate team has already written extensive documentation on how the wait logic of kstatus works. +Replace the [existing wait documentation](https://helm.sh/docs/intro/using_helm/) by explaining that we use kstatus and pointing to the [kstatus documentation](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md). This comes with the added benefit of not needing to maintain Helm specific wait documentation. ## Reference implementation Once I have feedback that this HIP is in the right direction I will make a draft PR in Helm implementing this feature. -For now, see [healthchecks.go](https://github.com/zarf-dev/zarf/blob/main/src/internal/healthchecks/healthchecks.go) in a project I maintain called Zarf. This is how we implement kstatus. After each Helm install or upgrade in Zarf, we get every resource from the chart and send them into the `WaitForReady` function. +For now, see [healthchecks.go](https://github.com/zarf-dev/zarf/blob/main/src/internal/healthchecks/healthchecks.go) in a project I help maintain called Zarf. After each Helm install or upgrade in Zarf, we get every resource from the chart and send them into the `WaitForReady` function. ## Rejected ideas From dcf6c8b1fc0c4144f8c2116cd99d3ffd25f3eaca Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Sat, 14 Dec 2024 18:21:45 +0000 Subject: [PATCH 05/10] updates Signed-off-by: Austin Abro --- hips/hip-0999.md | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index 9d879c16..e86260f8 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -13,9 +13,9 @@ Currently the `--wait` flag on `helm install` and `helm upgrade` does not wait f ## Motivation -Certain workflows require custom resources to be ready. There is no way to tell Helm to wait for custom resources to be ready, so anyone that has this requirement must write their own logic to wait for their custom resources. +Certain workflows require custom resources to be ready. There is no way to tell Helm to wait for custom resources to be ready, so anyone with this requirement must write their own logic to wait for their custom resources. Kstatus solves this by waiting for custom resources to have [the ready condition](https://github.com/kubernetes-sigs/cli-utils/blob/master/pkg/kstatus/README.md#the-ready-condition) set to true. -Certain workflows requires resources to be fully reconciled. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. +Certain workflows requires resources to be fully reconciled, which does happen in the current `--wait` logic. For example, Helm waits for all new pods in an upgraded deployment to be ready. However, Helm does not wait for the previous pods in that deployment to be removed. Since kstatus waits for all resources to be reconciled, the wait will not finish for a deployment until all of its new pods are ready and all of its old pods have been deleted. By introducing kstatus we will lower user friction with the `--wait` flag. @@ -27,16 +27,35 @@ Leveraging a existing status management library maintained by the Kubernetes tea From a CLI user's perspective there will be no changes in how waits are called, they will still use the `--wait` flag. -Kstatus does not output any logs. Helm will output a log message each second signalling that a resource is not ready. Helm will log one not ready resource at a time to not overwhelm users with output. This behavior will look similar to the current `--wait` logging. The resource will be chosen alphabetically by `metadata.name`. +Kstatus does not output any logs. Helm will output a log message each second signalling that a resource is not ready. Helm will log one of the resources it's waiting on at a time as to not overwhelm users with output. This behavior will look similar to the current `--wait` logging. + + A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pick up the custom resources during wait. - +Kstatus will not be exposed by the Helm SDK, instead an adapter/interface will be created. If kstatus is ever deprecated, unmaintained, or replaced there should be no impact to the public API of the Helm SDK. + + ## Backwards compatibility Waiting for custom resources and for reconciliation to complete for every resource could lead to charts timing out that weren't previously. +Kstatus does not require any general RBAC additions, such as events. This can be tested by following instructions in this repo https://github.com/AustinAbro321/kstatus-rbac-test. + +Kstatus does require some RBAC permissions for child resources. For instance, checking if a deployment is ready requires "list" permissions for the replicaset. Currently, Helm's logic also necessitates RBAC permissions to "list" child replicasets, so this requirement is not new. However, there may be cases where the RBAC requirements for child resources differ between Kstatus and Helm, as an evaluation has not been conducted. + +```yaml +rules: + - apiGroups: ["apps"] + resources: ["deployments"] + verbs: ["list", "watch"] + - apiGroups: ["apps"] + resources: ["replicasets"] + verbs: ["list"] +``` + + ## Security implications No security implications From c049e80c98ad6c83e9363ad337b061836bf4a704 Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Mon, 6 Jan 2025 15:25:00 +0000 Subject: [PATCH 06/10] updates to new architecture Signed-off-by: Austin Abro --- hips/hip-0999.md | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index e86260f8..c5528084 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -33,9 +33,30 @@ Kstatus does not output any logs. Helm will output a log message each second sig A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pick up the custom resources during wait. -Kstatus will not be exposed by the Helm SDK, instead an adapter/interface will be created. If kstatus is ever deprecated, unmaintained, or replaced there should be no impact to the public API of the Helm SDK. +Any functions involving waits will be separated from the `kube.Interface` interface into the `kube.Waiter` interface. The client will embed the `Waiter` interface to allow calls to look like `client.Wait()` instead of `client.waiter.Wait()`. `kube.New()` will accept a wait strategy to decide the wait implementation. Options will be either `HelmWaiter` or `statusWaiter`. `HelmWaiter` is the legacy implementation. The `statusWaiter` will not be public so that is kstatus is ever deprecated or replaced a new implementation can be used without changing the public SDK. + +The new client will look like below: +```go +type Client struct { + Factory Factory + Log func(string, ...interface{}) + Namespace string + kubeClient *kubernetes.Clientset + Waiter +} +type WaitStrategy int +const ( + StatusWaiter WaitStrategy = iota + LegacyWaiter +) +func New(getter genericclioptions.RESTClientGetter, ws WaitStrategy) (*Client, error) +``` + +The Helm CLI will always use the `statusWaiter` implementation, if this is found to be insufficient during Helm 4 testing a new cli flag `wait-strategy` will be introduced with options `status` and `legacy` to allow usage of the `HelmWaiter`. If the `statusWaiter` is found to be sufficient the `HelmWaiter` will be deleted from the public SDK before Helm 4 is released. + +`WaitAndGetCompletedPodPhase` is an exported function that is not called anywhere within the Helm repository. It will be removed. - +The current Helm wait logic does not wait for paused deployments to be ready. This is because if `helm upgrade --wait` is run on a chart with paused deployments they will never be ready, see [#5789](https://github.com/helm/helm/pull/5789). ## Backwards compatibility From 0c7da3fc9373b177be9a1283d412cf5fbfd8174b Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Mon, 6 Jan 2025 15:30:51 +0000 Subject: [PATCH 07/10] updates to new architecture Signed-off-by: Austin Abro --- hips/hip-0999.md | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index c5528084..7d123323 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -33,9 +33,9 @@ Kstatus does not output any logs. Helm will output a log message each second sig A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pick up the custom resources during wait. -Any functions involving waits will be separated from the `kube.Interface` interface into the `kube.Waiter` interface. The client will embed the `Waiter` interface to allow calls to look like `client.Wait()` instead of `client.waiter.Wait()`. `kube.New()` will accept a wait strategy to decide the wait implementation. Options will be either `HelmWaiter` or `statusWaiter`. `HelmWaiter` is the legacy implementation. The `statusWaiter` will not be public so that is kstatus is ever deprecated or replaced a new implementation can be used without changing the public SDK. +Any functions involving waits will be separated from the `kube.Interface` interface into the `kube.Waiter` interface. `kube.Waiter` will be embedded into `kube.Interface`. The client struct will embed the `Waiter` interface to allow calls to look like `client.Wait()` instead of `client.Waiter.Wait()`. `kube.New()` will accept a wait strategy to decide the wait implementation. Options will be either `HelmWaiter` or `statusWaiter`. `HelmWaiter` is the legacy implementation. The `statusWaiter` will not be public so that is kstatus is ever deprecated or replaced a new implementation can be used without changing the public SDK. -The new client will look like below: +The new client will look like: ```go type Client struct { Factory Factory @@ -52,10 +52,21 @@ const ( func New(getter genericclioptions.RESTClientGetter, ws WaitStrategy) (*Client, error) ``` -The Helm CLI will always use the `statusWaiter` implementation, if this is found to be insufficient during Helm 4 testing a new cli flag `wait-strategy` will be introduced with options `status` and `legacy` to allow usage of the `HelmWaiter`. If the `statusWaiter` is found to be sufficient the `HelmWaiter` will be deleted from the public SDK before Helm 4 is released. +The waiter interface will look like: +```go +type Waiter interface { + Wait(resources ResourceList, timeout time.Duration) error + WaitWithJobs(resources ResourceList, timeout time.Duration) error + WaitForDelete(resources ResourceList, timeout time.Duration) error +} +``` `WaitAndGetCompletedPodPhase` is an exported function that is not called anywhere within the Helm repository. It will be removed. +`WatchUntilReady` is used only for hooks. It has custom wait logic different from the Helm 3 general logic. Ideally, this could be replaced with a regular `Wait()` call. If there is any historical context as to why this logic is the way it is, please share. + +The Helm CLI will always use the `statusWaiter` implementation, if this is found to be insufficient during Helm 4 testing a new cli flag `wait-strategy` will be introduced with options `status` and `legacy` to allow usage of the `HelmWaiter`. If the `statusWaiter` is found to be sufficient the `HelmWaiter` will be deleted from the public SDK before Helm 4 is released. + The current Helm wait logic does not wait for paused deployments to be ready. This is because if `helm upgrade --wait` is run on a chart with paused deployments they will never be ready, see [#5789](https://github.com/helm/helm/pull/5789). ## Backwards compatibility From 467dde60506b80aca5726649f3ac0c59bf58eab3 Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Wed, 8 Jan 2025 13:50:38 +0000 Subject: [PATCH 08/10] mention watch and polling Signed-off-by: Austin Abro --- hips/hip-0999.md | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index 7d123323..45f04e43 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -29,8 +29,6 @@ From a CLI user's perspective there will be no changes in how waits are called, Kstatus does not output any logs. Helm will output a log message each second signalling that a resource is not ready. Helm will log one of the resources it's waiting on at a time as to not overwhelm users with output. This behavior will look similar to the current `--wait` logging. - - A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pick up the custom resources during wait. Any functions involving waits will be separated from the `kube.Interface` interface into the `kube.Waiter` interface. `kube.Waiter` will be embedded into `kube.Interface`. The client struct will embed the `Waiter` interface to allow calls to look like `client.Wait()` instead of `client.Waiter.Wait()`. `kube.New()` will accept a wait strategy to decide the wait implementation. Options will be either `HelmWaiter` or `statusWaiter`. `HelmWaiter` is the legacy implementation. The `statusWaiter` will not be public so that is kstatus is ever deprecated or replaced a new implementation can be used without changing the public SDK. @@ -73,15 +71,14 @@ The current Helm wait logic does not wait for paused deployments to be ready. Th Waiting for custom resources and for reconciliation to complete for every resource could lead to charts timing out that weren't previously. -Kstatus does not require any general RBAC additions, such as events. This can be tested by following instructions in this repo https://github.com/AustinAbro321/kstatus-rbac-test. +The kstatus status watcher requires the "list" and "watch" RBAC permissions to watch a resource. The kstatus status poller and current Helm implementation only require "list" permissions for the resources they're watching. Kstatus and Helm require "list" permissions for some child resources. For instance, checking if a deployment is ready requires "list" permissions for the replicaset. There may be cases where the RBAC requirements for child resources differ between Kstatus and Helm, as an evaluation has not been conducted. -Kstatus does require some RBAC permissions for child resources. For instance, checking if a deployment is ready requires "list" permissions for the replicaset. Currently, Helm's logic also necessitates RBAC permissions to "list" child replicasets, so this requirement is not new. However, there may be cases where the RBAC requirements for child resources differ between Kstatus and Helm, as an evaluation has not been conducted. - +Below is the minimal set needed to watch a deployment with the status watcher. This can be verified by following instructions in this repo: https://github.com/AustinAbro321/kstatus-rbac-test. ```yaml rules: - apiGroups: ["apps"] resources: ["deployments"] - verbs: ["list", "watch"] + verbs: ["list", "watch"] # The status poller only requires "list" here - apiGroups: ["apps"] resources: ["replicasets"] verbs: ["list"] @@ -90,7 +87,7 @@ rules: ## Security implications -No security implications +Users will now need "watch" permissions on resources in their chart if `--wait` is used, assuming the status watcher is used over the status poller. ## How to teach this @@ -98,9 +95,7 @@ Replace the [existing wait documentation](https://helm.sh/docs/intro/using_helm/ ## Reference implementation -Once I have feedback that this HIP is in the right direction I will make a draft PR in Helm implementing this feature. - -For now, see [healthchecks.go](https://github.com/zarf-dev/zarf/blob/main/src/internal/healthchecks/healthchecks.go) in a project I help maintain called Zarf. After each Helm install or upgrade in Zarf, we get every resource from the chart and send them into the `WaitForReady` function. +seen here - https://github.com/helm/helm/pull/13604 ## Rejected ideas From bb2b1902c50f203d80cbdcbd667a2cce4ba173bb Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Wed, 8 Jan 2025 15:56:09 +0000 Subject: [PATCH 09/10] mention poller vs watcher Signed-off-by: Austin Abro --- hips/hip-0999.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index 45f04e43..1151e6bd 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -29,7 +29,7 @@ From a CLI user's perspective there will be no changes in how waits are called, Kstatus does not output any logs. Helm will output a log message each second signalling that a resource is not ready. Helm will log one of the resources it's waiting on at a time as to not overwhelm users with output. This behavior will look similar to the current `--wait` logging. -A [dynamic rest mapper](https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/client/apiutil/restmapper.go#L36) will be used, this way if a chart installs a custom resource definition, kstatus will be able to pick up the custom resources during wait. +Kstatus can be used with either a poller or a watcher. The poller runs on a specified interval and only requires "list" RBAC permissions for polled resources. The watcher reacts to [watch events](https://github.com/kubernetes/kubernetes/blob/90a45563ae1bab5868ee888432fec9aac2f7f8b1/staging/src/k8s.io/apimachinery/pkg/watch/watch.go#L55-L61) and requires "list" and "watch" RBAC permission. This proposal uses the watcher as it responds slightly faster when all resources are ready, however, if the additional RBAC permissions are deemed to cause potential issues, we can instead switch to the poller. Any functions involving waits will be separated from the `kube.Interface` interface into the `kube.Waiter` interface. `kube.Waiter` will be embedded into `kube.Interface`. The client struct will embed the `Waiter` interface to allow calls to look like `client.Wait()` instead of `client.Waiter.Wait()`. `kube.New()` will accept a wait strategy to decide the wait implementation. Options will be either `HelmWaiter` or `statusWaiter`. `HelmWaiter` is the legacy implementation. The `statusWaiter` will not be public so that is kstatus is ever deprecated or replaced a new implementation can be used without changing the public SDK. From 5275514523b0ca36ff767c274c3e9ad352d2e810 Mon Sep 17 00:00:00 2001 From: Austin Abro Date: Wed, 22 Jan 2025 17:47:48 +0000 Subject: [PATCH 10/10] update why around watch Signed-off-by: Austin Abro --- hips/hip-0999.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hips/hip-0999.md b/hips/hip-0999.md index 1151e6bd..38e0deba 100644 --- a/hips/hip-0999.md +++ b/hips/hip-0999.md @@ -29,7 +29,7 @@ From a CLI user's perspective there will be no changes in how waits are called, Kstatus does not output any logs. Helm will output a log message each second signalling that a resource is not ready. Helm will log one of the resources it's waiting on at a time as to not overwhelm users with output. This behavior will look similar to the current `--wait` logging. -Kstatus can be used with either a poller or a watcher. The poller runs on a specified interval and only requires "list" RBAC permissions for polled resources. The watcher reacts to [watch events](https://github.com/kubernetes/kubernetes/blob/90a45563ae1bab5868ee888432fec9aac2f7f8b1/staging/src/k8s.io/apimachinery/pkg/watch/watch.go#L55-L61) and requires "list" and "watch" RBAC permission. This proposal uses the watcher as it responds slightly faster when all resources are ready, however, if the additional RBAC permissions are deemed to cause potential issues, we can instead switch to the poller. +Kstatus can be used with either a poller or a watcher. The poller runs on a specified interval and only requires "list" RBAC permissions for polled resources. The watcher reacts to [watch events](https://github.com/kubernetes/kubernetes/blob/90a45563ae1bab5868ee888432fec9aac2f7f8b1/staging/src/k8s.io/apimachinery/pkg/watch/watch.go#L55-L61) and requires "list" and "watch" RBAC permissions. This proposal uses the watcher as it responds slightly faster when all resources are ready, and it is very likely that users applying or deleting resources will have "watch" permissions on their resources. However, if the additional RBAC permissions are deemed to cause potential issues the poller can be used instead. Any functions involving waits will be separated from the `kube.Interface` interface into the `kube.Waiter` interface. `kube.Waiter` will be embedded into `kube.Interface`. The client struct will embed the `Waiter` interface to allow calls to look like `client.Wait()` instead of `client.Waiter.Wait()`. `kube.New()` will accept a wait strategy to decide the wait implementation. Options will be either `HelmWaiter` or `statusWaiter`. `HelmWaiter` is the legacy implementation. The `statusWaiter` will not be public so that is kstatus is ever deprecated or replaced a new implementation can be used without changing the public SDK.