Conversation
1496454 to
3a35ad4
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new GitHub Action (gh-actions/infra/jaas-auth) to authenticate to a JAAS Juju controller using service-account credentials, with an always-run post step to clean up Juju client state. Includes a workaround to strip the embedded controller CA cert from controllers.yaml after juju login --trust.
Changes:
- Introduces the JAAS auth action (main + post entrypoints) and its inputs/outputs.
- Adds
clearCaCert()helper (with unit tests) to remove embeddedca-certfrom Jujucontrollers.yaml. - Adds a JavaScript QA workflow to syntax-check JS/MJS files and run Node unit tests.
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
gh-actions/infra/jaas-auth/action.yml |
Defines the JAAS auth action interface (inputs/outputs) and node entrypoints. |
gh-actions/infra/jaas-auth/dist/main/index.mjs |
Main action logic: install Juju if needed, login, optionally clear embedded CA cert, export outputs/env for cleanup. |
gh-actions/infra/jaas-auth/dist/post/index.mjs |
Post action cleanup: best-effort juju logout, remove Juju state, clear env vars. |
gh-actions/infra/jaas-auth/dist/clear_ca_cert.mjs |
Implements controllers.yaml CA cert removal via atomic rewrite. |
gh-actions/infra/jaas-auth/tests/clear_ca_cert.test.mjs |
Unit tests covering multiline CA cert clearing, idempotency, and missing-key behavior. |
.github/workflows/js-qa.yaml |
Adds JS syntax checking and Node unit test execution in CI. |
.gitignore |
Ignores node_modules/ for JS actions/deps. |
Comments suppressed due to low confidence (1)
.github/workflows/js-qa.yaml:37
- This workflow pins
actions/checkout@v7, but the rest of the repo’s workflows pinactions/checkout@v6(e.g..github/workflows/test-build-deb.yaml). Ifv7is not available yet, this will break CI; please align to the repo’s pinned version unless there’s a deliberate upgrade plan.
- name: Checkout code
uses: actions/checkout@v7
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CarlosNihelton
left a comment
There was a problem hiding this comment.
Good job. I only have minor questions, but I'm quite out of my comfort zone, so perhaps not well informed enough to approve this PR right away.
CarlosNihelton
left a comment
There was a problem hiding this comment.
I'll let you decide on the snap install thing. Otherwise, LGTM!
didrocks
left a comment
There was a problem hiding this comment.
Quite similar to Carlos, I’m a little bit too far to get a full view on all the changes, but I didn’t spot anything that was not already mentioned and taken into account.
Good work!
There was a problem hiding this comment.
I've been browsing some of the other teams setups:
Q1:
Though not strictly necessary, it's safer to do this when considering the possibility of non-ephemeral runners
Is this a concern you see in the future? From what I can see in the other repo's, everyone else using runners all seem to be on ephemeral runners across the org, including when running on self-hosted runners. It just adds allot of JS complexity we don't seem to need, no?
Q2:
Note that because of a bug with Juju, the login action requires an additional controller normalization step, where we clear the embedded controller CA certificate from the Juju controllers file.
Do we need this? From what I can tell, on PS7, jaas is a trusted cert, so you don't need --trust. jaas.ps7.canonical.com is served with a public Let's Encrypt cert, so the runner's system CA store validates it automatically. You end up with so little code, I think we can forgo a dedicated action altogether?
Is there a reason we can't just condense this down to something similar to what PE is doing here? I don't think we want to make a generalized backup solution, that is more of an IS concern, we just want some interim measure while we wait for support for managed MySQL db's on our infra?
Likely no, but it's still a safety mechanism considering that it is direct infrastructure access into our servers. Plus, we do in fact have non-ephemeral runners, and I wouldn't be too surprised if one day someone uses them more directly, though its true it's not a concern for the foreseeable future.
During my testing, both locally and with the runner, yes, at least for the very first login. Once the fingerprint is trusted, we don't run into it again. It may have changed since then though. |
Here, we add a JAAS auth action, taking in a client ID and client secret to authenticate with JAAS. By taking advantage of JS action features, we also implement a cleanup hook that clears out the Juju data and logs the user out of JAAS. Though not strictly necessary, it's safer to do this when considering the possibility of non-ephemeral runners.
Note that because of a bug with Juju, the login action requires an additional controller normalization step, where we clear the embedded controller CA certificate from the Juju controllers file. This workaround needs to be enabled until juju/juju#22931 makes it to a stable release.
UDENG-11019