Skip to content

[CLI-2190] Resolve panic that can occur during auto-login without a role or environment#1601

Merged
Steven Gagniere (sgagniere) merged 6 commits intomainfrom
cli-2190
Dec 14, 2022
Merged

[CLI-2190] Resolve panic that can occur during auto-login without a role or environment#1601
Steven Gagniere (sgagniere) merged 6 commits intomainfrom
cli-2190

Conversation

@sgagniere
Copy link
Member

Checklist

  1. [CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?
    • yes: ok

What

A panic occurs in ccloudAutoLogin if a user account has no environments associated with it, which can happen when that user has no role-bindings. This PR makes two changes:

  • Switch from direct access to using Get methods to prevent the panic
  • Add an error if the environment is still nil after setting the context.

References

https://confluentinc.atlassian.net/browse/CLI-2190

Test & Review

Ran tests
Manual testing:

  • I Invited myself as a new user to my org and checked that running commands which require Authentication does not panic, and instead returns the new error
  • Checked that basic functionality still works: login/logout, help, update, version.
  • Checked that a user with an environment, but without an active environment, can still run commands without the new error.

@sgagniere Steven Gagniere (sgagniere) requested a review from a team as a code owner December 13, 2022 23:26

if command.Context.GetEnvironment() == nil {
noEnvSuggestions := "This issue may occur if this user has no role bindings. Contact an Organization Admin to create a role binding for this user."
return errors.NewErrorWithSuggestions("this command requires an environment: no environments found", noEnvSuggestions)

Choose a reason for hiding this comment

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

Are you assuming that all authenticated commands require an environment? What about confluent environment create?

Choose a reason for hiding this comment

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

Never mind, we're assuming the only way a user can have 0 environments is when they have no role bindings.

Choose a reason for hiding this comment

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

This seems a bit like a general-purpose bandaid, though... have you tried fixing the panics closer to where they occur?

Choose a reason for hiding this comment

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

Also- we should think about writing system tests for this as a follow-up, since this is an issue that's come up twice in the last few months.

Copy link
Member Author

@sgagniere Steven Gagniere (sgagniere) Dec 13, 2022

Choose a reason for hiding this comment

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

Yeah my reasoning was basically as follows: an organization will have at least one environment that can't (normally) be deleted. Every user with a role is able to view the list of environments, so any user account w/ sufficient permissions to create an environment should not be blocked by this.

  • My only concern is what happens if a user somehow manages to succeed in deleting all of their environments even though the backend doesn't allow this. I think you mentioned that this has happened before; how was it recovered?

I did fix the panic itself, this block of code is actually downwind of that. The reason I added this was based on some discussion w/ David Hyde (@DABH) about what we should do if this occurs. I think the reasoning was that we want an environment to really do anything meaningful, so we may as well just error out and tell the user about it.

Edit: I stuck it here because it seemed like a reasonably high level place so I wouldn't have to reflect the change across multiple files.

Actually, how's this: instead of an error, I issue this as a warning instead. It will function exactly the same as it does now (minus the panic), but it will alert the user that they're probably missing a role. Edit: And this should sidestep the edge case where someone who should be able to create environments would be blocked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Warning seems ok. I kind of like having something like this at a high level because having 0 environments is such an edge case that something is very likely very wrong (e.g. user has no rolebindings) if they hit this scenario.

Choose a reason for hiding this comment

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

I think you mentioned that this has happened before; how was it recovered?

I believe CDMUM had to step in and manually create a new default environment

Co-authored-by: Brian Strauch <bstrauch@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants