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

pylxd/models/certificate: re-add password arg for backward compat #603

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

simondeziel
Copy link
Member

@simondeziel simondeziel commented Sep 9, 2024

Fixes canonical/charm-lxd#168 where charm-lxd is calling certificates.create():

  config: Dict[str, Union[str, bytes, List[str], bool]] = {
      "name": name,
      "password": "",
      "cert_data": cert.encode(),
  }

  client.certificates.create(**config)

causing:

  File "./src/charm.py", line 1139, in _on_https_relation_changed
    if self.lxd_trust_add(cert=cert, name=cert_name, projects=projects):
  File "./src/charm.py", line 2294, in lxd_trust_add
    client.certificates.create(**config)
TypeError: create() got an unexpected keyword argument 'password'

This should fix an unexpected regression introduced in commit ec1b3ee.

@simondeziel simondeziel marked this pull request as ready for review September 9, 2024 17:53
@hamistao
Copy link
Contributor

hamistao commented Sep 9, 2024

Wouldn't it make more sense to update charm-lxd instead?

Maybe by updating the function call based on the pyLXD version.

Maybe there is some limitation there that I am not aware of.

@simondeziel
Copy link
Member Author

simondeziel commented Sep 9, 2024

Fixing charm-lxd would be easy but there are probably other pyLXD users out there we don't want to break, at least not without a compelling reason.

@hamistao
Copy link
Contributor

hamistao commented Sep 9, 2024

@simondeziel Oh I see, and that reminds me that the extension does not imply in the password being deprecated, so this would still break users using positional arguments for certificates.create with a LXD 5.21 that includes the extension but still supports password auth.

So I propose using token instead of secret and using it an optional parameter to indicate opting for token over password authentication. Maybe we could return an error if using the token arg without the extension, and also return a more informative error if not using the token with LXD 6.1, that deprecated the password option (although version checking may be undesirable).

And maybe next release we make token the default option since it would break users that using password as positional parameters with 5.21.

An alternative would be using a boolean flag instead of an optional token parameter.

That way we allow for using password auth even with the extension and don't break any users. What do you think?

If that is too dev-y for you I can quickly add these changes here later today if you agree they make sense.

@simondeziel
Copy link
Member Author

The fact that this method always used password and recently got a secret argument means we have to keep those otherwise we risk breaking users.

If I missed your point, please feel free to suggest a different approach inline in the code review.

@simondeziel simondeziel force-pushed the certificate-secret-password branch 2 times, most recently from bd6b150 to 1f00d60 Compare September 9, 2024 20:39
Fixes canonical/charm-lxd#168 where charm-lxd is calling certificates.create():

```python
  config: Dict[str, Union[str, bytes, List[str], bool]] = {
      "name": name,
      "password": "",
      "cert_data": cert.encode(),
  }

  client.certificates.create(**config)
```

causing:

```
  File "./src/charm.py", line 1139, in _on_https_relation_changed
    if self.lxd_trust_add(cert=cert, name=cert_name, projects=projects):
  File "./src/charm.py", line 2294, in lxd_trust_add
    client.certificates.create(**config)
TypeError: create() got an unexpected keyword argument 'password'
```

Signed-off-by: Simon Deziel <[email protected]>
@simondeziel simondeziel force-pushed the certificate-secret-password branch 4 times, most recently from 3f8ff39 to 6493f49 Compare September 9, 2024 21:40
@simondeziel
Copy link
Member Author

@hamistao ready for another look when you have time, thanks!

pylxd/client.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hamistao hamistao left a comment

Choose a reason for hiding this comment

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

I am assuming this is ready for review.

If so it looks fine to me, thanks for this!

@simondeziel simondeziel merged commit 0f102cb into canonical:main Sep 10, 2024
14 checks passed
@simondeziel simondeziel deleted the certificate-secret-password branch September 10, 2024 17:03
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.

lxd https-relation changed hook failure (using password)
2 participants