Add GCP secret engine configuration Create/Edit views #29423

Monkeychip · 2025-01-27T16:03:30Z

Description

Adds the ability to configure GCP secret engine.
For enterprise users, allows them to configure WIF

enterprise tests pass

Walk through of the changes
https://github.com/user-attachments/assets/cca57d1c-0860-4bf3-95b3-bdd109be123b

Screenshots:

By marking this a wif engine, we automatically get the IdentityTokenKey param/workflow added to the mount view.

Identity Token on the configuration details view after a fresh mount

Configure for GCP account access type.

Configure for WIF access type

If I set any wif fields, automatically defaults to wif access type

Details view

github-actions · 2025-01-27T16:26:00Z

CI Results:
All Go tests succeeded! ✅

Monkeychip · 2025-01-27T21:42:14Z

ui/tests/acceptance/secrets/backend/gcp/gcp-configuration-test.js

@@ -3,10 +3,11 @@
 * SPDX-License-Identifier: BUSL-1.1
 */

-import { click, visit, currentURL } from '@ember/test-helpers';


Not as clean of a diff as I would have like, but rearranged some tests to better align with Azure and AWS acceptance tests

github-actions · 2025-01-27T22:10:38Z

Build Results:
All builds succeeded! ✅

hellobontempo · 2025-01-30T00:46:21Z

ui/app/models/gcp/config.js

@@ -65,8 +69,45 @@ export default class GcpConfig extends Model {
    'identityTokenTtl',
  ];

+  get isWifPluginConfigured() {
+    return !!this.identityTokenAudience || !!this.identityTokenTtl || !!this.serviceAccountEmail;


Only one of these needs to be set for WIF to be configured?

The user can set serviceAccountEmail as a stand alone and that means it's wif accessType, but if they set identityTokenAudience we required serviceAccountEmail (may be a miss on the backend validation). Thus, defaulting to any of these fields being set means it's wif.

hellobontempo · 2025-01-30T00:52:55Z

ui/app/models/gcp/config.js

+  get isAccountPluginConfigured() {
+    // credentials is not checked here because it's never return by the API
+    // additionally credentials can be set via GOOGLE_APPLICATION_CREDENTIALS env var so we cannot call it a required field in the ui.
+    // thus we can never say for sure if the account accessType has been configured so we always return false
+    return false;
+  }


If this is always false then this doesn't need to be a getter and can just be a property on the model. Also it might be helpful to clarify that we're referring to the "credentials" parameter here, since credentials can refer to a set of params in some instances

Suggested change

get isAccountPluginConfigured() {

// credentials is not checked here because it's never return by the API

// additionally credentials can be set via GOOGLE_APPLICATION_CREDENTIALS env var so we cannot call it a required field in the ui.

// thus we can never say for sure if the account accessType has been configured so we always return false

return false;

}

isAccountPluginConfigured = false

// credentials is not checked here because it's never return by the API

// additionally credentials can be set via GOOGLE_APPLICATION_CREDENTIALS env var so we cannot call it a required field in the ui.

// thus we can never say for sure if the account accessType has been configured so we always return false

I like it. Thanks.

ui/tests/acceptance/secrets/backend/gcp/gcp-configuration-test.js

hellobontempo · 2025-01-30T00:56:50Z

ui/tests/acceptance/secrets/backend/gcp/gcp-configuration-test.js

+    await runCmd(`delete sys/mounts/${this.path}`);
+  });
+
+  module('isCommunity', function (hooks) {


nit: I think we tend to make module names human readable and not written like variables. So I'd expect this to just be "Community"

hellobontempo · 2025-01-30T00:58:19Z

ui/tests/acceptance/secrets/backend/gcp/gcp-configuration-test.js

-      assert.dom(GENERAL.emptyStateActions).doesNotContainText('Configure GCP');
-      // cleanup
-      await runCmd(`delete sys/mounts/${path}`);
+    module('details', function () {


do we need to wrap each of these tests in an additional module (details, create, edit? It seems like slightly unnecessary nesting since each only contains one or two tests

Breaking it up this way helped me make sure I was covering all the logic related to if the model had been already configured or not (edit vs create). For the details module, this tested workflow on a different route. While there's are only several tests in each, I think it helps break up the relevant logic. Again, this was more helpful in test writing, so if you think it's confusing for test reading, I'll default to making the later easier.

No need to change for this, I just think the modules should be used sparingly and for setting up shared test context. A lot of nesting seems like it's just setting us up for future flakiness.

In the future I think a CRUD module is sufficient (if it's even necessary)

hellobontempo · 2025-01-30T01:05:34Z

ui/tests/helpers/secret-engine/secret-engine-helpers.js

+export const fillInGcpConfig = async (situation = 'gcp') => {
+  if (situation === 'gcp') {
+    await click(GENERAL.toggleGroup('More options'));
+    await click(GENERAL.ttl.toggle('Config TTL'));
+    await fillIn(GENERAL.ttl.input('Config TTL'), '7200');
+    await click(GENERAL.ttl.toggle('Max TTL'));
+    await fillIn(GENERAL.ttl.input('Max TTL'), '8200');
+    await click(GENERAL.textToggle);
+    await fillIn(GENERAL.textToggleTextarea, '{"some-key":"some-value"}');
+  }
+  if (situation === 'withWif') {
+    await click(SES.wif.accessType('wif')); // toggle to wif
+    await fillIn(GENERAL.inputByAttr('identityTokenAudience'), 'azure-audience');
+    await click(GENERAL.ttl.toggle('Identity token TTL'));
+    await fillIn(GENERAL.ttl.input('Identity token TTL'), '7200');
+    await fillIn(GENERAL.inputByAttr('serviceAccountEmail'), '[email protected]');
+  }
+};


since the method name is fillInGcpConfig I think it's safe to assume either situation is gcp related, another slightly more concise way to write this could be:

export const fillInGcpConfig = async (withWif = false) => { if (withWif) { await click(SES.wif.accessType('wif')); // toggle to wif await fillIn(GENERAL.inputByAttr('identityTokenAudience'), 'azure-audience'); await click(GENERAL.ttl.toggle('Identity token TTL')); await fillIn(GENERAL.ttl.input('Identity token TTL'), '7200'); await fillIn(GENERAL.inputByAttr('serviceAccountEmail'), '[email protected]'); else { await click(GENERAL.toggleGroup('More options')); await click(GENERAL.ttl.toggle('Config TTL')); await fillIn(GENERAL.ttl.input('Config TTL'), '7200'); await click(GENERAL.ttl.toggle('Max TTL')); await fillIn(GENERAL.ttl.input('Max TTL'), '8200'); await click(GENERAL.textToggle); await fillIn(GENERAL.textToggleTextarea, '{"some-key":"some-value"}'); } };

Unless you wanted to combine all of the different engine types then perhaps a switch statement would make sense.

Good catch; there was some modification to this and I ended up removing another situation but missed the clean up.

hellobontempo

Looks good! Just some small cleanup suggestions, the main one I think to address is the model getter (still small, though!). Thanks for including the video walkthrough - I'm unable to see it without downloading it. Is it possible to add a screenshot or two?

hellobontempo · 2025-01-30T19:10:54Z

ui/app/models/gcp/config.js

+  isAccountPluginConfigured = false;
+  // the "credentials" param is not checked for "isAccountPluginConfigured" because it's never return by the API
+  // additionally credentials can be set via GOOGLE_APPLICATION_CREDENTIALS env var so we cannot call it a required field in the ui.
+  // thus we can never say for sure if the account accessType has been configured so we always return false


In the future, I think it's helpful for comments to be before the thing they're describing. No need to change as that will make the approval stale, but just as a future suggestion

hellobontempo

Nice work getting this over the finish line! It's hard to follow all of the details of the WIF workflow, so I'm trusting that your test coverage is sufficient! Thanks for adding coverage ⭐

gcp initial changes

afb78e7

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jan 27, 2025

Monkeychip added 3 commits January 27, 2025 12:08

acceptance test coverage for gcp

af75cee

update config-wif component test so tests are passing

0b12b44

specific gcp test coverage

a0ea545

Monkeychip added this to the 1.19.0-rc milestone Jan 27, 2025

Monkeychip added the ui label Jan 27, 2025

Monkeychip added 2 commits January 27, 2025 14:22

changelog

10dea61

comment clean up

35fc2e8

Monkeychip commented Jan 27, 2025

View reviewed changes

Monkeychip added 2 commits January 27, 2025 15:01

one more test

a1c03b0

comment things

2a7bd21

Monkeychip marked this pull request as ready for review January 27, 2025 22:07

Monkeychip requested review from a team as code owners January 27, 2025 22:07

Monkeychip requested a review from mladlow January 27, 2025 22:07

hellobontempo reviewed Jan 30, 2025

View reviewed changes

ui/tests/acceptance/secrets/backend/gcp/gcp-configuration-test.js Show resolved Hide resolved

hellobontempo reviewed Jan 30, 2025

View reviewed changes

address pr comments

b3d8c0f

hellobontempo reviewed Jan 30, 2025

View reviewed changes

hellobontempo approved these changes Jan 30, 2025

View reviewed changes

Monkeychip merged commit 14082d0 into main Jan 30, 2025
32 of 33 checks passed

Monkeychip deleted the ui/VAULT-33155/gcp branch January 30, 2025 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GCP secret engine configuration Create/Edit views #29423

Add GCP secret engine configuration Create/Edit views #29423

Monkeychip commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading

Monkeychip Jan 27, 2025

github-actions bot commented Jan 27, 2025

hellobontempo Jan 30, 2025

Monkeychip Jan 30, 2025

hellobontempo Jan 30, 2025

Monkeychip Jan 30, 2025

hellobontempo Jan 30, 2025

hellobontempo Jan 30, 2025 •

edited

Loading

Monkeychip Jan 30, 2025 •

edited

Loading

hellobontempo Jan 30, 2025 •

edited

Loading

hellobontempo Jan 30, 2025

Monkeychip Jan 30, 2025 •

edited

Loading

hellobontempo left a comment •

edited

Loading

hellobontempo Jan 30, 2025

hellobontempo left a comment

Add GCP secret engine configuration Create/Edit views #29423

Add GCP secret engine configuration Create/Edit views #29423

Conversation

Monkeychip commented Jan 27, 2025 • edited Loading

Description

github-actions bot commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Monkeychip Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

hellobontempo Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeychip Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

hellobontempo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo left a comment

Choose a reason for hiding this comment

Monkeychip commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading

hellobontempo Jan 30, 2025 •

edited

Loading

Monkeychip Jan 30, 2025 •

edited

Loading

hellobontempo Jan 30, 2025 •

edited

Loading

Monkeychip Jan 30, 2025 •

edited

Loading

hellobontempo left a comment •

edited

Loading