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

Support credential-get hook tool #186

Closed
dshcherb opened this issue Mar 16, 2020 · 7 comments · Fixed by #1152
Closed

Support credential-get hook tool #186

dshcherb opened this issue Mar 16, 2020 · 7 comments · Fixed by #1152
Assignees
Labels
feature New feature or request small item

Comments

@dshcherb
Copy link
Contributor

credential-get hook tool is used by k8s cloud integrator charms (OpenStack, AWS, GCP, Azure etc.) to get access to cloud credentials via a Juju controller.

https://jaas.ai/docs/deploying-advanced-applications#heading--trusting-an-application-with-a-credential
https://github.com/juju/juju/blob/juju-2.7.4/worker/uniter/runner/jujuc/credential-get.go

E.g. OpenStack Integrator:
https://github.com/juju-solutions/charm-openstack-integrator/blob/101913504d047a1b4b9b6c4dcfb901d66fd9399e/lib/charms/layer/openstack.py#L38-L70

This needs to be modeled in the framework so that this class of charms can be written with the operator framework.

@dshcherb dshcherb added the feature New feature or request label Mar 16, 2020
@benhoyt
Copy link
Collaborator

benhoyt commented Oct 5, 2023

This is somewhat related to #532 where "is my charm trusted" isn't modelling in ops.

@benhoyt
Copy link
Collaborator

benhoyt commented Oct 5, 2023

This definitely seems worth modelling in ops to avoid people implementing it themselves. Tony linked to a GitHub search showing a bunch of places that charms use credential-get (including the charm-openstack-integrator charm linked above).

We'd probably want to make the return type a dataclass of some kind, but there's a map/dict of cloud-specific attributes that we'd keep as a dict (don't want to go to the level of having cloud-specific code in ops just to get better typing).

We'll look at doing this in 24.04.

@benhoyt benhoyt added the 24.04 label Oct 5, 2023
@benhoyt benhoyt changed the title Support credential-get hook tool support Support credential-get hook tool Feb 1, 2024
@IronCore864
Copy link
Contributor

IronCore864 commented Mar 14, 2024

I have done some research and found that:

  1. in juju only type and name are mandatory, others can be empty.

  2. I also checked an AWS charm and an Azure charm, it seems they have different keys inside the credential section.

  3. Then I did some testing on EC2 in AWS with LXD localhost cloud, and it returned attrs inside the credential section, unlike attributes in one of the charms listed above. Example:

{
    "type": "lxd",
    "name": "localhost",
    "region": "localhost",
    "endpoint": "https://10.76.251.1:8443",
    "credential": {
        "auth-type": "certificate",
        "attrs": {
            "client-cert": "...",
            "client-key": "...",
            "server-cert": "..."
        }
    },
    "is-controller-cloud": true
}

Due to resource limitation, I didn't test on Azure or any other cloud; only with AWS/LXD.


Based on the differences between different clouds, my temporary conclusion is that the credential_get hook tool may return either one of the two following types:

  1. a Dict[str, Any]
  2. an object like CloudSpec with most attributes set to Optional, example:
class CloudSpec:
    """Cloud information (metadata)."""

    def __init__(self,
                 type: str,
                 name: str,
                 region: Optional[str],
                 endpoint: Optional[str],
                 isControllerCloud: Optional[str],
                 credential: Optional[Dict[str, Any]],
                 identityEndpoint: Optional[str],
                 storageEndpoint: Optional[str],
                 caACertificates: Optional[List[str]],
                 skipTLSVerify: Optional[bool],
                 ):
        pass

I prefer the second one because:

  • CloudSpec is a struct in juju
  • There are other hook tool in the current ops model code that returns an object (e.g., SecretInfo).

After the investigation, I propose the following design:

  1. Add a get_cloud_spec API to ops.model.Model.
  2. The API should return a similar object like the CloudSpec above, with minimum mandatory fields, with the credential section as a type Dict[str, any].
  3. The API runs self._backend.credential_get to execute the hook tool.

Please review, upon approval I'll start working on this issue.

@tonyandrewmeyer
Copy link
Contributor

I have done some research and found that:

Excellent summary!

  • Credential is a struct in juju

This is the most compelling argument for me. I agree that if Juju is modelling it then ops should do that as well.

@benhoyt
Copy link
Collaborator

benhoyt commented Mar 14, 2024

Excellent, thanks. Seems reasonable, and I think copying those structs in Juju is a good idea (as dataclasses). It looks like the "credential" field should itself be a nested object with auth_type: str, attrs: Dict[str, str], and redacted: List[str] fields.

Actually, I wonder if we should call the top-level API method get_cloud_spec? (And the objects would be CloudSpec and CloudCredential, same as Juju calls them.) The reason being is that it actually returns more cloud "spec" than just credentials -- the credentials are nested under it. And the Juju credential-get docs actually say: "credential-get returns the cloud specification used by the unit's model."

Should it be Model.get_cloud_spec, rather than Unit.foo? Maybe a question for the Juju team, but it seems like this should be on the model level.

Let's also make sure we use Pythonic names with underscores, like is_controller_cloud: bool.

@IronCore864
Copy link
Contributor

Excellent, thanks. Seems reasonable, and I think copying those structs in Juju is a good idea (as dataclasses). It looks like the "credential" field should itself be a nested object with auth_type: str, attrs: Dict[str, str], and redacted: List[str] fields.

Actually, I wonder if we should call the top-level API method get_cloud_spec? (And the objects would be CloudSpec and CloudCredential, same as Juju calls them.) The reason being is that it actually returns more cloud "spec" than just credentials -- the credentials are nested under it. And the Juju credential-get docs actually say: "credential-get returns the cloud specification used by the unit's model."

Should it be Model.get_cloud_spec, rather than Unit.foo? Maybe a question for the Juju team, but it seems like this should be on the model level.

Let's also make sure we use Pythonic names with underscores, like is_controller_cloud: bool.

get_cloud_spec makes more sense than get_credential because the name suggests it's cloud-related, and it should be put under Model rather than unit since you probably can't have different cloud for different units. I'll edit my original comment directly.

@IronCore864
Copy link
Contributor

Also, because we want charmers to be able to test model.get_cloud_spec using Harness in the unit tests of their charms, some simple change for Harness is required. A simple design:

  • Add an API set_cloud_spec to Harness.
  • Add an attribute self._cloud_spec to class _TestingModelBackend.
  • In _TestingModelBackend, create a credential_get method, same as ops.model._ModelBackend.credential_get, but returns self._cloud_spec from _TestingModelBackend, and raises an error if self._cloud_spec isn't set.

tonyandrewmeyer pushed a commit to tonyandrewmeyer/operator that referenced this issue Sep 27, 2024
chore: limit Scenario 6 to use ops 2.12-2.16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request small item
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants