-
Notifications
You must be signed in to change notification settings - Fork 316
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
cli: uninstall command #725
Conversation
92a202b
to
ac3a4fb
Compare
cli/README.md
Outdated
-skip-wipe-data | ||
Skip deleting all PVCs, Secrets, and Service Accounts associated with | ||
Consul Helm installation without prompting for approval to delete. The | ||
default is false. | ||
|
||
-wipe-data | ||
Delete all PVCs, Secrets, and Service Accounts associated with Consul | ||
Helm installation without prompting for approval to delete. Only use | ||
this when persisted data from previous installations is no longer | ||
necessary. The default is false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If skip-wipe-data is defaulted false
(meaning wipe the data) and wipe-data is defaulted false
(meaning dont wipe the data) what is the actual default behaviour? I'm assuming that we meant that skip-wipe-data is defaulted to true
?
c.once.Do(c.init) | ||
|
||
defer func() { | ||
if err := c.Close(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity what does c.Close() here do? would c.UI.Output() still be usable if c.Close() failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 that's a good question. I'm curious about this as well. It looks like c.Close
attempts to close the UI if the UI implements io.Closer
interface, so I'm curious if that means that we won't be able to output to it anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched this to using the logger instead. In our case, the UI doesn't implement the closer interface, so Close() would be a no-op. But we could potentially choose to implement it in the future if we say "upgrade" our UI.
} | ||
} | ||
if len(secretNames) > 0 { | ||
c.UI.Output("Consul secrets deleted.", terminal.WithSuccessStyle()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a super nit comment, but if there are 3 secrets and 2 got deleted and the third exits the for loop because you cant delete it for some reason then neither of these Outputs get processed properly. I suppose it is okay since it "should work" in happy path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the correct thing to do is output the Error via c.UI.Output()
instead of return the error, and then continue to process the other secrets?
I guess this logic applies to the other similar functions, if you choose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah +1 to Kyle's question. I left a similar question in the deletePVCs
function too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its ok because in an error case, it will exit the function at the point of the error without printing either of these outputs here, and just print the first error it got to. Then when a user re-runs uninstall after fixing their issue, it should succeed and delete the secrets properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really great, I left some mostly minor comments but assuming you get time to look at them I think it's good to go!
I did not get a chance to test run the CLI unfortunately.. will get to that soon :)
4857b15
to
749c942
Compare
ac3a4fb
to
4f73fd4
Compare
8b22214
to
5f3b109
Compare
4f73fd4
to
dd282f4
Compare
dd282f4
to
d00c301
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is impressive work!!! I'm so excited about this ❤️ This is a lot of work, and I really appreciate how easy this was to read.
I've only reviewed the command (and not tests) and had some comments that I wanted to clarify before proceeding with the rest of the review. I've also left some minor edits.
I've also ran some manual tests and discussed the issues I ran into with Nitya already but wanted to document them here for posterity:
- I think we need to change the helm list functionality to list any pending/uninstalled releases so that if your installation has failed, the uninstall will still find the release.
- We need make sure that the kubernetes client is initialized with the correct namespace. With the 0.33.0 release currently, uninstall fails because tls-init-cleanup job doesn't have
Namespace
set in its template. We've fixed it now in the latest helm release, but we should still make sure that the client is initialized correctly. - It would be nice to default release name and namespace to
consul
for a case when I forgot to wipe-data in a previous uninstall. That way rerunning uninstall would still just work.
There is also another issue that we haven't discussed that I think would be nice to fix but doesn't have to be part of this PR. In my testing I kept running into errors saying that tls-init-cleanup job or service account etc already exist. It'd be nice if the uninstall command cleaned those up before calling helm delete.
c.once.Do(c.init) | ||
|
||
defer func() { | ||
if err := c.Close(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 that's a good question. I'm curious about this as well. It looks like c.Close
attempts to close the UI if the UI implements io.Closer
interface, so I'm curious if that means that we won't be able to output to it anymore.
} | ||
} | ||
if len(secretNames) > 0 { | ||
c.UI.Output("Consul secrets deleted.", terminal.WithSuccessStyle()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah +1 to Kyle's question. I left a similar question in the deletePVCs
function too.
} | ||
|
||
// deleteServiceAccounts deletes service accounts that have foundReleaseName in their name. | ||
func (c *Command) deleteServiceAccounts(foundReleaseName, foundReleaseNamespace string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why do we delete only service accounts and not roles, rolebindings, clusterroles, and clusterrolebindings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this function in favor of: https://app.asana.com/0/1200500259727746/1201031492148291/f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually in testing I am noticing the consul-tls-init role, rolebinding, and service account are sticking around if they are not manually deleted when installing with -preset=secure. I went ahead and added that logic to delete roles/rolebindings/service accounts. I plan to add logic in the future for jobs, clusterroles, and clusterrolebindings if we see cases those aren't being deleted!
a5a0c22
to
eb31fec
Compare
- delete roles, rolebindings, service accounts by label - use hack for setting namespace for helm kube client - fallback to release name/namespace consul
560b896
to
280cd71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good ❤️ Thank you so much for considering the feedback so thoughtfully 🙏
I've added some minor comments, but they are not blocking.
rolebindings, err := c.kubernetes.RbacV1().RoleBindings("default").List(context.TODO(), metav1.ListOptions{}) | ||
require.NoError(t, err) | ||
require.Len(t, rolebindings.Items, 0) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can also add tests for the findExistingInstallation
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is kind of tricky for similar reasons to
consul-k8s/cli/cmd/install/install.go
Line 338 in e77da79
// Note that this function is tricky to test because mocking out the action.Configuration struct requires a |
I think I can open a separate PR this week to refactor the helm logic out of uninstall.findExistingInstallation
and install.checkForPreviousInstallations
, and unit test the part that's unrelated to helm logic. The reason to pull it out into a separate PR is since I'll be focusing on testing within the acceptance tests PR we can add this additional testing there after a quick refactor.
The port-forward sometimes fails randomly
Based off of a demo branch @sadjamz worked on here: https://github.com/hashicorp/consul-k8s-cli/tree/uninstall
Changes proposed in this PR:
consul-k8s uninstall
uninstalls the Consul Helm installation with options to wipe all dataIf you don't provide the release name/namespace, it will find the release whose metadata.name matches "consul", and then prompt to uninstall that release and any pvcs/secrets based on that.
You can provide the release name/namespace in a case where you have already
helm uninstall
'ed the consul helm installation, but still have PVCs, Secrets, etc to clean up.Note that this PR is difficult to unit test-- acceptance tests are coming as the next task after install/uninstall are complete. The Helm steps are difficult to mock out for reasons mentioned in the install PR:
cli: support install command #713 (comment)
consul-k8s/cli/cmd/install/install.go
Line 338 in e77da79
How I've tested this PR:
Manual testing of the cases mentioned below, a few unit tests.
How I expect reviewers to test this PR:
Code review
Test it out:
cd cli
go build -o ./bin/consul-k8s
./bin/consul-k8s uninstall
If you can test out the command with the following flags and let me know if you have behaviour suggestions.
a) no flags
b) -name and -namespace set
c) -auto-approve
d) -wipe-all-data
Checklist: