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

Password resolution in connector #1002

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

cderici
Copy link
Contributor

@cderici cderici commented Jan 8, 2024

Description

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, so we pass an empty password to the connection.

This fixes #1001 by checking the credentials before trying to establish connection, and asks for whichever cred is required:

QA Steps

This will require a 2.9 controller, so bootstrap one.

 $ juju_29 bootstrap localhost issue1001

Then check the accounts.yaml file to see a line like the following for the admin, make sure the password is in there:

issue1001:
    user: admin
    password: asdasdsadasdadasdasdasdasd
    last-known-access: superuser

Now go ahead and try to connect via python-libjuju repl:

$ python -m asyncio
>>> from juju import controller
>>> c=controller.Controller()
>>> await c.connect()
>>>
exiting asyncio REPL...
# Connection works without any issues.

Change the admin user password with juju change-user-password:

$ juju change-user-password
new password: <your-password>
type new password again: <your-password>
Your password has been changed.
 $ juju users
Controller: lxd292

Name    Display name  Access     Date created   Last connection
admin*  admin         superuser  2 minutes ago  just now

Go take another look at the accounts.yaml, the password field should've disappeared.

issue1001:
    user: admin
    last-known-access: superuser

Without this change, you'd see the ssl error from #1001, because pylibjuju was trying to establish a connection with passing None as password. But with this change, first you need to see a nice error message when it fails:

$ python -m asyncio
>>> from juju import controller
>>> c=controller.Model()
>>> await c.connect()
>>>
...
...
ValueError: Some authentication parameters are required : password

Now provide your password in connect and it should connect just fine:

>>> await c.connect(password=<your-password-as-string>) # string

Notes & Discussion

JUJU-5267

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.
@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
Copy link
Member

@jack-w-shaw jack-w-shaw left a comment

Choose a reason for hiding this comment

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

LGTM, QA passes

proxy=proxy,
)
kwargs.update(endpoint=endpoints,
uuid=None,
Copy link
Member

Choose a reason for hiding this comment

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

Does None need to be provided in uuid, or can it be left out?

jujubot added a commit that referenced this pull request Jan 11, 2024
…r-connection

#1003

#### 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):

```sh
 $ juju add-user caner
 $ juju register
```

Now let's make a password for this user.

```sh
 $ 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:

```python
# 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
@jujubot jujubot merged commit 2990139 into juju:2.9 Jan 11, 2024
11 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