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

helm/cli: don't swallow error in BuildChartDependencies #9851

Merged
merged 1 commit into from May 18, 2022

Conversation

vgramer
Copy link
Contributor

@vgramer vgramer commented May 18, 2022

What does this PR do / Why do we need it:

In BuildChartDependencies function, we add the repositories, build the dependencies and thanks to a defer function, we remove the repository.

The problem is that if the helm repo add fails, the error is swallowed by the defer function. (doing helm repo remove with no repository added triggers the following error Error: no repositories configured. )

as you can see in this the root cause ( the error from the helm repo add ) is hidden.

INFO[10:05:22]    📦 Deploying nginx-ingress-controller…     
INFO[10:05:23]       Deploying Helm chart…                  
ERRO[10:05:23] ❌ Operation failed: failed to deploy nginx-ingress-controller: failed to deploy Helm release: failed to download dependencies: Error: no repositories configured. 

This PR moves the defer function after the helm add repo so we don't try to remove a repository that does not exist. It also combines previous errors (e.g. error from helm dependency build ) with error from helm repo remove repository.

Does this PR close any issues?:
Fixes #

Special notes for your reviewer:
Example of output for the different cases:

the case where add repo fails

INFO[17:38:35]    📦 Deploying nginx-ingress-controller…     
DEBU[17:38:35]       Ensuring namespace…                     namespace=nginx-ingress-controller
DEBU[17:38:35]       Checking for release…                   name=nginx-ingress-controller namespace=nginx-ingress-controller
DEBU[17:38:35] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace nginx-ingress-controller list --all -o json 
INFO[17:38:35]       Deploying Helm chart…                  
DEBU[17:38:35]       Installing…                            
DEBU[17:38:35] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default repo add dep-nginx-ingress-controller-0 https://kubernetes.github.io/ingress-nginx 
ERRO[17:38:35] ❌ Operation failed: failed to deploy nginx-ingress-controller: failed to deploy Helm release: failed to download dependencies: Error: open Library/Preferences/helm/repositories.yaml: permission denied. 
Exiting.

the case where build dependencies fails

INFO[17:40:27]    📦 Deploying nginx-ingress-controller…     
DEBU[17:40:27]       Ensuring namespace…                     namespace=nginx-ingress-controller
DEBU[17:40:27]       Checking for release…                   name=nginx-ingress-controller namespace=nginx-ingress-controller
DEBU[17:40:27] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace nginx-ingress-controller list --all -o json 
INFO[17:40:27]       Deploying Helm chart…                  
DEBU[17:40:27]       Installing…                            
DEBU[17:40:27] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default repo add dep-nginx-ingress-controller-0 https://kubernetes.github.io/ingress-nginx 
DEBU[17:40:35] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default dependency build charts/nginx-ingress-controller 
DEBU[17:40:38] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default repo remove dep-nginx-ingress-controller-0 
ERRO[17:40:38] ❌ Operation failed: failed to deploy nginx-ingress-controller: failed to deploy Helm release: failed to download dependencies: Error: could not download https://github.com/kubernetes/ingress-nginx/releases/download/helm-chart-4.1.0/ingress-nginx-4.1.0.tgz: Get "https://github.com/kubernetes/ingress-nginx/releases/download/helm-chart-4.1.0/ingress-nginx-4.1.0.tgz": dial tcp: lookup github.com on [::1]:53: read udp [::1]:57345->[::1]:53: read: connection refused. 
Exiting.

the case where build dependencies and remove repo fail

INFO[17:42:17]    📦 Deploying nginx-ingress-controller…     
DEBU[17:42:17]       Ensuring namespace…                     namespace=nginx-ingress-controller
DEBU[17:42:17]       Checking for release…                   name=nginx-ingress-controller namespace=nginx-ingress-controller
DEBU[17:42:17] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace nginx-ingress-controller list --all -o json 
INFO[17:42:17]       Deploying Helm chart…                  
DEBU[17:42:17]       Installing…                            
DEBU[17:42:17] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default repo add dep-nginx-ingress-controller-0 https://kubernetes.github.io/ingress-nginx 
DEBU[17:42:27] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default dependency build charts/nginx-ingress-controller 
DEBU[17:42:40] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default repo remove dep-nginx-ingress-controller-0 
ERRO[17:42:47] ❌ Operation failed: failed to deploy nginx-ingress-controller: failed to deploy Helm release: failed to download dependencies: Error: could not download https://github.com/kubernetes/ingress-nginx/releases/download/helm-chart-4.1.0/ingress-nginx-4.1.0.tgz: Get "https://github.com/kubernetes/ingress-nginx/releases/download/helm-chart-4.1.0/ingress-nginx-4.1.0.tgz": dial tcp: lookup github.com on [::1]:53: read udp [::1]:57919->[::1]:53: read: connection refused; error: clean up resources failed: can not remove remopository: Error: open Library/Preferences/helm/repositories.yaml: permission denied. 
Exiting.

the case where remove repo fails

INFO[17:43:36]    📦 Deploying nginx-ingress-controller…     
DEBU[17:43:36]       Ensuring namespace…                     namespace=nginx-ingress-controller
DEBU[17:43:36]       Checking for release…                   name=nginx-ingress-controller namespace=nginx-ingress-controller
DEBU[17:43:36] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace nginx-ingress-controller list --all -o json 
INFO[17:43:36]       Deploying Helm chart…                  
DEBU[17:43:36]       Installing…                            
DEBU[17:43:36] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default repo add dep-nginx-ingress-controller-0 https://kubernetes.github.io/ingress-nginx 
DEBU[17:43:49] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default dependency build charts/nginx-ingress-controller 
DEBU[17:43:51] $ KUBECONFIG=/Users/vince/.kube/config helm --namespace default repo remove dep-nginx-ingress-controller-0 
ERRO[17:43:51] ❌ Operation failed: failed to deploy nginx-ingress-controller: failed to deploy Helm release: failed to download dependencies: error: clean up resources failed: can not remove remopository: Error: open Library/Preferences/helm/repositories.yaml: permission denied. 
Exiting.

Documentation:

Does this PR introduce a user-facing change?:

kubermatic-installer: improve error handling when building helm chart dependencies

In BuildChartDependencies function, we add the repositories,build the dependencies and thanks to a defer function, we remove the repository.
The problem waq that if the add failed, the error was swallowed by the defer function. (doing  helm repo remove with no repository added.)

Signed-off-by: Vincent Gramer <vincent@kubermatic.com>
@kubermatic-bot kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 18, 2022
Copy link
Contributor

@xrstf xrstf left a comment

Choose a reason for hiding this comment

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

I love this PR and its description!

/approve
/retest

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2022
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e677ccda3ed3bd8d2db467b32d3de2adb1386fee

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vgramer, xrstf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2022
@kubermatic-bot kubermatic-bot merged commit c69eb0f into kubermatic:master May 18, 2022
@vgramer
Copy link
Contributor Author

vgramer commented May 19, 2022

/cherry-pick release/v2.20

@kubermatic-bot
Copy link
Contributor

@vgramer: new pull request created: #9859

In response to this:

/cherry-pick release/v2.20

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.

@vgramer vgramer deleted the dont-swallow-error branch May 20, 2022 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants