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

No controller model access needed for connection with a non-admin user #1003

Merged
merged 5 commits into from
Jan 11, 2024

Conversation

cderici
Copy link
Contributor

@cderici cderici commented Jan 8, 2024

Description

This is a follow up PR, includes changes from #1002. If you're reviewing this and #1002 hasn't landed yet, you might see some additional changes that'll land with #1002. We need those changes to be able to test this change.

ControllerAPIInfoForModels call in update_endpoints after a controller connection requires the uuid of the controller model, and if the user doesn't have at least read access, they won't be able to have that, so the update_endpoint call at the end of a
regular connection fails.

Fixes #998

QA Steps

We'll need a 2.9 controller, so:

 $ juju_29 bootstrap localhost issue998

Now we have the admin user on this by default, but we don't wanna use that. Let's create a new user (that doesn't have superuser so it can't read the controller model):

 $ juju add-user caner
 $ juju register

Now let's make a password for this user.

 $ juju change-user-password # to be able to access the admin user later
 $ juju logout
 $ juju login -u caner -c issue998
 $ juju change-user-password
<your-password>
 This should work just fine, we're just making sure the user is ready to go.

Now let's spawn a pylibjuju repl to test this change with this user (that can't access the controller model):

Wihtout this change, you'd see the error in #998. With this change the connection should work just fine:

# cd pylibjuju directory
$ python -m asyncio
>>> from juju import controller; c=controller.Controller()
>>> await c.connect(username='caner', password=<your-password>)
>>>
exiting asyncio REPL...
# Connection works without any issues.

Notes & Discussion

JUJU-5268

For both model and controller connections, accounts.yaml is required
to have both username and password for connection to be
established. This is not always true, for example, juju
change-user-password actually removes the password from the
accounts.yaml file.
Fixes juju#998

ControllerAPIInfoForModels call requires the uuid of the controller
model, and if the user doesn't have at least read access, they won't
be able to have that, and the update_endpoint call at the end of a
regular connection fails.
@cderici cderici added bug fix hint/2.9 going on 2.9 branch area/forward-port to be forward ported - remove label after port labels Jan 8, 2024
@cderici cderici requested review from anvial and jack-w-shaw January 9, 2024 20:28
@anvial
Copy link
Member

anvial commented Jan 10, 2024

Thx, @cderici

I have the following problem on the last QA step:

>>> await c.connect(username='caner', password='q')
Traceback (most recent call last):
  File "/home/anvial/Projects/python-libjuju/juju/controller.py", line 143, in update_endpoints
    info = await self.info()
           ^^^^^^^^^^^^^^^^^
  File "/home/anvial/Projects/python-libjuju/juju/controller.py", line 289, in info
    raise errors.JujuPermissionError('Requires access to controller model.')
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'juju.errors' has no attribute 'JujuPermissionError'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.11/asyncio/__main__.py", line 58, in runcode
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/concurrent/futures/_base.py", line 456, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "<console>", line 1, in <module>
  File "/home/anvial/Projects/python-libjuju/juju/controller.py", line 139, in connect
    await self.update_endpoints()
  File "/home/anvial/Projects/python-libjuju/juju/controller.py", line 148, in update_endpoints
    except errors.JujuPermissionError:
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'juju.errors' has no attribute 'JujuPermissionError'

PS: question - why do we need the juju register step in the QA?

@anvial
Copy link
Member

anvial commented Jan 10, 2024

updated comment

@cderici
Copy link
Contributor Author

cderici commented Jan 10, 2024

AttributeError: module 'juju.errors' has no attribute 'JujuPermissionError'

I have no idea how that attribute is lost in between my branches, thanks for actually running it so it's caught. Updating now 👍

PS: question - why do we need the juju register step in the QA?

Yeah we don't need it, I tried a bunch of different things in my local and copy/pasted these steps into the QA from my terminal, so that must've lingered in there by mistake.

@anvial
Copy link
Member

anvial commented Jan 11, 2024

@cderici,

Still have the problems on the last QA step:

>>> await c.connect(username='caner', password='w')
Traceback (most recent call last):
  File "/home/anvial/Projects/python-libjuju/juju/controller.py", line 143, in update_endpoints
    info = await self.info()
           ^^^^^^^^^^^^^^^^^
  File "/home/anvial/Projects/python-libjuju/juju/controller.py", line 289, in info
    raise errors.JujuPermissionError('Requires access to controller model.')
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'juju.errors' has no attribute 'JujuPermissionError'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.11/asyncio/__main__.py", line 58, in runcode
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/concurrent/futures/_base.py", line 456, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "<console>", line 1, in <module>
  File "/home/anvial/Projects/python-libjuju/juju/controller.py", line 139, in connect
    await self.update_endpoints()
  File "/home/anvial/Projects/python-libjuju/juju/controller.py", line 148, in update_endpoints
    except errors.JujuPermissionError:
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'juju.errors' has no attribute 'JujuPermissionError'

@anvial
Copy link
Member

anvial commented Jan 11, 2024

ups, I forgot to checkout...

Looks like everything now works fine:

>>> await c.connect(username='caner', password='w')
This user doesn't have at least read access to the controller model, so endpoints are not updated after connection.

@cderici
Copy link
Contributor Author

cderici commented Jan 11, 2024

/merge

@jujubot jujubot merged commit beed55f into juju:2.9 Jan 11, 2024
11 of 12 checks passed
@cderici cderici removed the area/forward-port to be forward ported - remove label after port label Feb 7, 2024
jujubot added a commit that referenced this pull request Feb 8, 2024
#1022

#### Description

This brings onto the 3.x track some of the latest fixes from to the 2.9 track. Here're the details:

* Fix for #989 from #990
* Fix for #1001 from #1002
* Fix for #998 from #1003


#### QA Steps

No QA needed for #990. For 1002 and 1003 please refer to their QA steps. Though they are very related so I'd expect the QA for both of them can be done in one fell swoop.

* #1002 
* #1003 

All CI tests need to pass (there's still some known intermittent ones in there).
@cderici cderici mentioned this pull request Feb 8, 2024
jujubot added a commit that referenced this pull request Feb 13, 2024
#1023

## What's Changed
* Drop use of walrus operator by @freyes in #993
* No controller model access needed for connection with a non-admin user by @cderici in #1003
* Password resolution in connector by @cderici in #1002
* Remove dependency to juju-cli for controller_name by @cderici in #1009

#### Notes & Discussion

JUJU-5413
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hint/2.9 going on 2.9 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants