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.

Sign up for GitHub

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

Jump to bottom

uninstall: fix no confirmation when istiod cannot be connected #47617

Merged
merged 2 commits into from
Oct 31, 2023
Merged

uninstall: fix no confirmation when istiod cannot be connected #47617

merged 2 commits into from
Oct 31, 2023

Conversation

hanxiaop
Copy link
Member

@hanxiaop hanxiaop commented Oct 27, 2023 •
edited by istio-policy-bot
Loading

Please provide a description of this PR:
Currently, when istiod cannot be connected, the components will be uninstalled without any confirmation. Not sure if the behavior - skipping confirmation when no proxy info was obtained - was intentional, however I think it doesn't mean there's no proxy so it's better to have a confirmation like other situations do.

@hanxiaop hanxiaop requested a review from a team as a code owner October 27, 2023 10:20
@istio-policy-bot
Copy link

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 27, 2023
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 30, 2023
@zirain zirain added the cherrypick/release-1.20 Set this label on a PR to auto-merge it to the release-1.20 branch label Oct 30, 2023
@zirain
Copy link
Member

zirain commented Oct 30, 2023

can you add a test for this?

@hanxiaop
Copy link
Member Author

can you add a test for this?

@zirain There was no test for this previously. Since we are using MockClient to test CLIClient, which doesn't return errors when retrieving proxy info from Istiod, If we want to test this simple check, we have to add hacky logics.

However I have the sample outputs:

// with no istiod:
istioctl uninstall --revision=default       
Unable to find any proxies pointing to the default control plane. This may be because the control plane cannot be connected or there is no default control plane.
Proceed? (y/N) 

// with istiod connection failure - most often
 istioctl uninstall --revision=default       
2023-10-30T03:24:09.035019Z     error   klog    an error occurred forwarding 59500 -> 15014: error forwarding port 15014 to pod 075cfe3e938bb2fde61ad1bb6552adc9413ac92fe1db47016976a1acd235fbfc, uid : failed to execute portforward in network namespace "/var/run/netns/cni-4ec1d5be-d3d9-5466-8c2c-27ec39f2b018": failed to connect to localhost:15014 inside namespace "075cfe3e938bb2fde61ad1bb6552adc9413ac92fe1db47016976a1acd235fbfc", IPv4: dial tcp4 127.0.0.1:15014: connect: connection refused IPv6 dial tcp6 [::1]:15014: connect: connection refused 
2023-10-30T03:24:09.035471Z     error   port forward failed: lost connection to pod
Unable to find any proxies pointing to the default control plane. This may be because the control plane cannot be connected or there is no default control plane.
Proceed? (y/N)

If we don't have the logics in this PR, the two situation mentioned above will be skipped without confirmation.

@hanxiaop hanxiaop requested a review from zirain October 31, 2023 01:17
@istio-testing istio-testing merged commit 7aa04ee into istio:master Oct 31, 2023
1 check passed
@istio-testing
Copy link
Collaborator

In response to a cherrypick label: new pull request created: #47652

@istio-testing istio-testing mentioned this pull request Oct 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zirain zirain zirain approved these changes

Assignees
No one assigned
Labels
area/environments cherrypick/release-1.20 Set this label on a PR to auto-merge it to the release-1.20 branch size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hanxiaop @istio-policy-bot @zirain @istio-testing