Add identity_token_key to Azure and GCP secret engines #28822

Monkeychip · 2024-11-01T17:18:47Z

enterprise tests pass

Description

This PR is the first step to allowing WIF configuration on the Azure and GCP Secret engines—it adds the field identity_token_key when mounting the engine—API for adding to config here. The API we hit for creating a new key is here.

This follows the exact same pattern as the AWS secret engine so the actual changes are minor and test coverage framework was already in place.

AWS PR that talks about what identity_token_key does/is.

azure-gcp-mount.mov

github-actions · 2024-11-01T17:21:05Z

CI Results:
All Go tests succeeded! ✅

github-actions · 2024-11-01T17:21:40Z

Build Results:
All builds succeeded! ✅

Monkeychip · 2024-11-01T17:25:07Z

ui/app/components/mount-backend-form.ts

@@ -191,6 +191,13 @@ export default class MountBackendForm extends Component<Args> {
    this.checkPathChange(value);
  }

+  @action
+  resetForm(value: string) {
+    // clears the identityTokenKey and path, not the entire form.


Would argue the flow here could be improved upon, but we do currently handle the full "teardown" in willDestroy. I am simply keeping the previous behavior and adding an extra check for removing the identity_token_key if a user presses the back button.

So this is new functionality since right now we don't clear the path value when clicking the back button. Is this intended?

If we want to keep this method I think we should make it more flexible and not pass a value here. A couple options are just calling this.resetForm and then inside this function we could do this.handleIdentityTokenKeyChange = '' Then we could expand it to include additional reset values in the future..

However, I'm not entirely sure we need this functionality...looking at ember data, the param is removed when clicking "back" as it is so it feels superfluous

We do clear the path when they press the back button though it's not super clear by looking at the js file. It was on the hbs file. See here. The empty param is kind of a carry over from how it was done before.

And I had to add this function in because it was failing on a test and then I confirmed locally. If you fill in the IdentityTokenKey and click "Cancel" then choose another Secret engine with this option (say aws and then azure) you'll see it filled in.

That line you pointed to clears the mountType not the mount path.

Maybe that's a UX question then - if I was a user and had filled out a form, then meant to select a different WIF engine, I'd want that data to be saved and not cleared when switching to a different engine type. I think this is an edge case, so probably not worth delving into too much. Just my two cents!

Ah, thank you for the clarification. Let me remove this for now, return to previous functionality and then ask design as a follow up.

hellobontempo

Before approving just want to confirm the anticipated form change by reseting the path value

hellobontempo · 2024-11-04T20:47:24Z

ui/app/helpers/mountable-secret-engines.js

@@ -134,8 +134,8 @@ const MOUNTABLE_SECRET_ENGINES = [
  },
 ];

-// A list of Workload Identity Federation engines. Will eventually include Azure and GCP.


👏 thanks for updating this comment!

hellobontempo · 2024-11-04T20:52:24Z

ui/app/models/secret-engine.js

@@ -232,7 +232,7 @@ export default class SecretEngineModel extends Model {
        // no ttl options for keymgmt
        optionFields = [...CORE_OPTIONS, 'config.allowedManagedKeys', ...STANDARD_CONFIG];
        break;
-      case 'aws':
+      case WIF_ENGINES.find((type) => type === this.engineType):


Nice and clean way to add this case! 😍

hellobontempo · 2024-11-04T21:05:42Z

ui/app/components/mount-backend-form.ts

@@ -191,6 +191,13 @@ export default class MountBackendForm extends Component<Args> {
    this.checkPathChange(value);
  }

+  @action
+  resetForm(value: string) {
+    // clears the identityTokenKey and path, not the entire form.


So this is new functionality since right now we don't clear the path value when clicking the back button. Is this intended?

If we want to keep this method I think we should make it more flexible and not pass a value here. A couple options are just calling this.resetForm and then inside this function we could do this.handleIdentityTokenKeyChange = '' Then we could expand it to include additional reset values in the future..

However, I'm not entirely sure we need this functionality...looking at ember data, the param is removed when clicking "back" as it is so it feels superfluous

ui/tests/acceptance/settings/mount-secret-backend-test.js

hellobontempo · 2024-11-08T00:48:04Z

ui/tests/integration/components/mount-backend-form-test.js

@@ -206,22 +206,21 @@ module('Integration | Component | mount backend form', function (hooks) {
        await render(
          hbs`<MountBackendForm @mountType="secret" @mountModel={{this.model}} @onMountSuccess={{this.onMountSuccess}} />`
        );
-        for (const engine of WIF_ENGINES) {


I actually think this is a good place for this iteration because this is a place where the form logic depends on type since the type affects what fields are rendered

hellobontempo · 2024-11-08T00:48:14Z

ui/tests/integration/components/mount-backend-form-test.js

@@ -233,18 +232,17 @@ module('Integration | Component | mount backend form', function (hooks) {
          undefined,
          `On init identityTokenKey is not set on the model`
        );
-        for (const engine of WIF_ENGINES) {


hellobontempo

Great work! Thanks for the dialog about the testing - just trying to be thoughtful about the tests we write. You definitely are closer to this work than I am and I trust your judgement ⭐

Monkeychip added 2 commits November 1, 2024 10:03

changes then onto tests

5eb0fcd

fix wif test failures

60c910a

Monkeychip added the ui label Nov 1, 2024

Monkeychip added this to the 1.19.0-rc milestone Nov 1, 2024

Monkeychip requested a review from a team as a code owner November 1, 2024 17:18

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Nov 1, 2024

changelog

17f1cc5

Monkeychip commented Nov 1, 2024

View reviewed changes

clean up

0d8c35d

hellobontempo reviewed Nov 4, 2024

View reviewed changes

Monkeychip requested a review from hellobontempo November 7, 2024 16:34

Monkeychip added 2 commits November 7, 2024 11:02

address pr comments

1c45f66

only test one wif engine for relevant tests

6daed5d

hellobontempo reviewed Nov 8, 2024

View reviewed changes

hellobontempo approved these changes Nov 8, 2024

View reviewed changes

add back engine loop for tests that depend on type

d4103d8

Monkeychip enabled auto-merge (squash) November 8, 2024 16:07

Monkeychip merged commit 2c3c585 into main Nov 8, 2024
32 checks passed

Monkeychip deleted the ui/VAULT-32139/gcp-zure-mount-wif branch November 8, 2024 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add identity_token_key to Azure and GCP secret engines #28822

Add identity_token_key to Azure and GCP secret engines #28822

Monkeychip commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

Monkeychip Nov 1, 2024 •

edited

Loading

hellobontempo Nov 4, 2024

Monkeychip Nov 7, 2024 •

edited

Loading

hellobontempo Nov 7, 2024

Monkeychip Nov 7, 2024

hellobontempo left a comment

hellobontempo Nov 4, 2024

hellobontempo Nov 4, 2024

hellobontempo Nov 4, 2024

hellobontempo Nov 8, 2024 •

edited

Loading

hellobontempo Nov 8, 2024

hellobontempo left a comment •

edited

Loading

Add identity_token_key to Azure and GCP secret engines #28822

Add identity_token_key to Azure and GCP secret engines #28822

Conversation

Monkeychip commented Nov 1, 2024 • edited Loading

Description

github-actions bot commented Nov 1, 2024 • edited Loading

github-actions bot commented Nov 1, 2024 • edited Loading

Monkeychip Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

hellobontempo Nov 4, 2024

Choose a reason for hiding this comment

Monkeychip Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

hellobontempo Nov 7, 2024

Choose a reason for hiding this comment

Monkeychip Nov 7, 2024

Choose a reason for hiding this comment

hellobontempo left a comment

Choose a reason for hiding this comment

hellobontempo Nov 4, 2024

Choose a reason for hiding this comment

hellobontempo Nov 4, 2024

Choose a reason for hiding this comment

hellobontempo Nov 4, 2024

Choose a reason for hiding this comment

hellobontempo Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

hellobontempo Nov 8, 2024

Choose a reason for hiding this comment

hellobontempo left a comment • edited Loading

Choose a reason for hiding this comment

Monkeychip commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

Monkeychip Nov 1, 2024 •

edited

Loading

Monkeychip Nov 7, 2024 •

edited

Loading

hellobontempo Nov 8, 2024 •

edited

Loading

hellobontempo left a comment •

edited

Loading