-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat(Model): support credential-get hook tool in both model and harness #1152
feat(Model): support credential-get hook tool in both model and harness #1152
Conversation
Sample usage: class MyVMCharm(ops.CharmBase):
"""Charm the application."""
def __init__(self, framework: ops.Framework):
super().__init__(framework)
framework.observe(self.on.start, self._on_start)
def _on_start(self, event: ops.StartEvent):
try:
cloud_spec = self.model.get_cloud_spec()
print(CloudSpec: {cloud_spec!r}')
except ops.ModelError as e:
logger.warning(
f'credential-get hook tool does not work on containerized environment: {e!r}\n')
self.unit.status = ops.ActiveStatus() Output: unit-microsample-5: 09:32:40 DEBUG unit.microsample/5.juju-log Emitting Juju event start.
unit-microsample-5: 09:32:40 DEBUG unit.microsample/5.start CloudSpec: CloudSpec(type='lxd', name='localhost', region='localhost', endpoint='https://10.76.251.1:8443', is_controller_cloud=None, credential={'auth-type': 'certificate', 'attrs': {'client-cert': '-----BEGIN CERTIFICATE-----\nMIICAzCCAYmgAwIBAgIRALRDwDXMD/AjHCTB0JHurvYwCgYIKoZIzj0EAwMwLzEM\nMAoGA1UEChMDTFhEMR8wHQYDVQQDDBZ1YnVudHVAaXAtMTcyLTMxLTQyLTIyMB4X\nDTI0MDMxMzA1MjIzN1oXDTM0MDMxMTA1MjIzN1owLzEMMAoGA1UEChMDTFhEMR8w\nHQYDVQQDDBZ1YnVudHVAaXAtMTcyLTMxLTQyLTIyMHYwEAYHKoZIzj0CAQYFK4EE\nACIDYgAEpJG0aeaCkFoJ4+CDwiRdBafrA5cYcrRljIfOu9pNh5iKAttV2LtZ5L39\nCun13Lp531GnL1LKdmJ80pCdI4PPdRPTOXsAQTE5pFdcHuRbVSzqVenIniK3gb7g\n7EtaGVP8o2kwZzAOBgNVHQ8BAf8EBAMCBaAwEwYDVR0lBAwwCgYIKwYBBQUHAwIw\nDAYDVR0TAQH/BAIwADAyBgNVHREEKzApgg9pcC0xNzItMzEtNDItMjKHBH8AAAGH\nEAAAAAAAAAAAAAAAAAAAAAEwCgYIKoZIzj0EAwMDaAAwZQIxALPdVw+tc5njeRD7\nL4WxmcTEiw5N67+nBfDVRfo5LheWcBGey1fUufefdLQBLlLDhQIwOy//D84EMRdU\nR3xJb39jZZ9JTX45qugTb8cd82qvOvEeLYwF52bh8pdgaDchRUdV\n-----END CERTIFICATE-----\n', 'client-key': '-----BEGIN EC PRIVATE KEY-----\nMIGkAgEBBDAa9kwOCVZXdCghEexLdCSmSyaXdWWCD939byLJ+IJhzV/dLlEAh1t5\nJ+XK/F8khf6gBwYFK4EEACKhZANiAASkkbRp5oKQWgnj4IPCJF0Fp+sDlxhytGWM\nh8672k2HmIoC21XYu1nkvf0K6fXcunnfUacvUsp2YnzSkJ0jg891E9M5ewBBMTmk\nV1we5FtVLOpV6cieIreBvuDsS1oZU/w=\n-----END EC PRIVATE KEY-----\n', 'server-cert': '-----BEGIN CERTIFICATE-----\nMIIB/zCCAYWgAwIBAgIRAMRwEkdwDlBEk9aakWv7Q5cwCgYIKoZIzj0EAwMwLTEM\nMAoGA1UEChMDTFhEMR0wGwYDVQQDDBRyb290QGlwLTE3Mi0zMS00Mi0yMjAeFw0y\nNDAzMDQwMTMzMTBaFw0zNDAzMDIwMTMzMTBaMC0xDDAKBgNVBAoTA0xYRDEdMBsG\nA1UEAwwUcm9vdEBpcC0xNzItMzEtNDItMjIwdjAQBgcqhkjOPQIBBgUrgQQAIgNi\nAARMwnKekEznWQTKJYYygp3FJItJha/k6pCUejhuH1q7kO+cfhfZHI+S7xrWRv9q\nH4L3e/xQ+1WriclGStmgn1kRg1GEB2TbCc3D2QJtS6a9yIsTkU3kZxWGp8MDnqN9\n0EajaTBnMA4GA1UdDwEB/wQEAwIFoDATBgNVHSUEDDAKBggrBgEFBQcDATAMBgNV\nHRMBAf8EAjAAMDIGA1UdEQQrMCmCD2lwLTE3Mi0zMS00Mi0yMocEfwAAAYcQAAAA\nAAAAAAAAAAAAAAAAATAKBggqhkjOPQQDAwNoADBlAjAU07TtAgIF746wMa53TJP/\nyU8tMTVl2aUi/uhDCo75PeUAP/G9+Twximprx7VNCGwCMQDHJTOYZLqiLpTq/izR\nO5txgVv3U1CcrBvfaODD4lqrQxr9Qt/uJLhveVPdRLKTvag=\n-----END CERTIFICATE-----\n'}}, identity_endpoint=None, storage_endpoint=None, ca_certificates=None, skip_tls_verify=None) Deploy in K8s: unit-microsample-0: 09:37:40 INFO juju.worker.uniter found queued "start" hook
unit-microsample-0: 09:37:41 INFO unit.microsample/0.juju-log Running legacy hooks/start.
unit-microsample-0: 09:37:41 WARNING unit.microsample/0.juju-log credential-get hook tool does not work on containerized environment: ModelError('ERROR cannot access cloud credentials: credential-get on a "caas" model not supported\n') Harness sample usage in docstring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is a good start! I haven't gone over it with a fine-toothed comb yet, but just a few initial comments. @tonyandrewmeyer will review as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! A few little suggestions, particularly around documentation.
All comments resolved, please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress here, thanks. A whole bunch of comments, mostly very minor, but also a bit of fixes to do with the field names and types from the Juju structs.
…rator into credential-get-hook-tool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Just a couple of small doc requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This is very close now! Just a few minor suggestions to improve the dataclass fields and defaults.
Refreshed according to latest review, please take another look :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks! Nothing from me, other than the Optional adjustment that Ben mentioned.
CloudCredential and CloudSpec typing fixed @benhoyt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, approved! Let's please just do two things before you merge: 1) add a line to CHANGES.md
describing the new feature, and 2) describe the new feature briefly in the PR description.
Added
Model.get_cloud_spec
which uses thecredential-get
hook tool to get details of the cloud where the model is deployed.Changes are made to both
ops.Model
and Harness.Closes #186.