Enabling Secrets Sync for HVD #26841

Monkeychip · 2024-05-06T16:15:41Z

enterprise test pass

The main focus of this PR is to allow HVD managed clusters to access Secrets Sync. This requires that the UI allow HVD users to activate Secrets Sync and see Secrets Sync from within Client Counts and Sidebar nav. In order to activate from an hvd cluster, the backend now accepts "admin" as a namespace header when POST'ing to the secrets-sync/activate endpoint. The backend still expects namespace: null for non-hvd clusters.

The UI cannot tell if the HVD cluster actually has Secrets Sync on their license so we display an error depending on whatever the backend returns.

Additionally, this PR:

renames the getter on the flag service from managedNamespaceRoot to hvdManagedNamespaceRoot
separates out the client counts nav bar into it's own component for easier reading/testing.
uses the flag service to check if a user has permissions to post to the activate endpoint and hides activation actions depending on their permissions.
Adds another badge text option of "plus" for hvd clusters.
reduces the description on the upsell to one message instead of custom messages for premium vs enterprise vs plus.
calls the feature-flags endpoint for a concurrency task on the flag service instead of inside the application route.

Walk through video of user with activate permissions (root user) on an hvd cluster

secrets_sync_hvd.mov

Walk through video of user who does not have activate permissions (default policy) on an hvd cluster

permissions.mov

…26649) * update badge text and allow hvd on secrets sync views * update logic in Secrets Sync overview and cta for hvd. * spacing * rearrange based on pr feedback * fix return on badgeText and cluster nav test * fix landing cta tests * update test to reflect new changes * moved call to feature-flags from application route to the service to match patterns * add managed test coverage on overview component test and remove premium feature so cta message appplies to both managed and non-managed clusters * missed service name and unskip admin test * clean up * fix tests * flags test fix

* renames * lowercase HVD to match * missed some * test failure

…26713) * feat: split client counts navbar into separate component * acceptance/clients/counts/overview-test: remove tests now covered by int tests * clients counts route: rename isSecretsSyncActivated to showSecretsSync * sync clients page: show unactivated state unless sync client history or feature is activated * client counts navbar: show sync tab only if client history or is /able to be/ activated * clients overview page: only show sync charts if activated * fix: rename isManaged to isHvd * acceptance/counts/overview-test: add HVD tests * acceptance/counts/overview-test: clean up unused cruft * aceptance/clients/counts/overview-test: ensure we dont get false negatives * chore: move Clients::Error to Clients::Counts::Error * chore: calculate showSecretSync in page component instead of route * chore: add copyright headers * acceptance/clients/counts/overview-test: stub activated flags to fix test

* acceptance/clients/counts/overview-test: use imported test selectors * general-selectors: add missing emptyStateSubtitle property * acceptance/clients/counts/sync: nest tests in top level module for easier test runs

* add permissions check to flags service and consume in overview template * add back missing refresh * fix test failures * add test coverage * clean up * address flaky test * grr

github-actions · 2024-05-07T15:02:44Z

CI Results:
All Go tests succeeded! ✅

github-actions · 2024-05-08T15:59:31Z

Build Results:
All builds succeeded! ✅

hashishaw

So much great cleanup work! Some things to consider but nothing blocking

hashishaw · 2024-05-09T20:33:07Z

ui/app/components/sidebar/nav/cluster.js

-    if (this.version.isCommunity) return 'Enterprise';
-    if (!this.version.hasSecretsSync) return 'Premium';
-    return undefined;
+    if (isHvdManaged) return 'Plus';


To make sure I understand correctly -- HVD vault clusters will show the "Plus" badge next to this nav item, whether or not their cluster is plus tier?

Correct. The UI will cannot tell if the hvd cluster is plus or not, so we're basically upselling it to plus.

hashishaw · 2024-05-09T20:33:28Z

ui/app/routes/application.js

-      this.flagsService.setFeatureFlags(flags);
-    }
+  beforeModel() {
+    return this.flagsService.fetchFeatureFlags();


much cleaner 👏

hashishaw · 2024-05-09T20:34:34Z

ui/app/services/flags.ts

+  });
+
+  fetchFeatureFlags() {
+    return this.getFeatureFlags.perform();


Do we want to add any kind of caching, so that we don't refetch the flags if they've already been retrieved?

Another conservative approach on my end. The previous behavior did not have a caching check and because we use this response for things outside Secrets Sync I didn't want to modify the behavior. 😬

hashishaw · 2024-05-09T20:36:01Z

ui/app/services/flags.ts

+        method: 'GET',
+      });
+
+      if (result.status === 200) {


we're swallowing any errors that might happen from the API, since those will return with a non-200 status. Is that intentional?

I carried over the previous method exactly as it was in fear of changing the behavior from how it was before.

hashishaw · 2024-05-09T20:37:45Z

ui/lib/sync/addon/components/secrets/page/overview.hbs

-{{/if}}
+{{#unless @isActivated}}
+  {{#if (or @licenseHasSecretsSync @isHvdManaged)}}
+    {{#unless this.hideOptIn}}


These if statements 😵 I wonder if we can move this logic to a backing class?

Let me add this as a todo and revisit when Noelle returns. There's been a lot of back and forth on how to work through this logic (a persona service, etc). Additionally, there's a solid chance that an api change will clean some of this logic up.

hashishaw · 2024-05-09T20:38:46Z

ui/lib/sync/addon/components/secrets/page/overview.ts

@@ -71,10 +75,12 @@ export default class SyncSecretsDestinationsPageComponent extends Component<Args
  @task
  @waitFor
  *onFeatureConfirm() {
+    // must return null instead of root for non managed cluster.
+    const namespace = this.args.isHvdManaged ? 'admin' : null;


The behavior is still that it can only be enabled in root or admin, right? Not other child namespaces? Might be good to add that to the comment

Yep, you're correct. And I'll amend the comment to include, good idea.

hashishaw · 2024-05-09T20:39:29Z

ui/tests/acceptance/clients/counts/overview-test.js

-    await click('[data-test-show-calendar]');
-    await click('[data-test-previous-year]');
-    await click(`[data-test-calendar-month=${ARRAY_OF_MONTHS[LICENSE_START.getMonth()]}]`);
+    await click(CLIENT_COUNT.calendarWidget.customEndMonth);


Monkeychip and others added 5 commits April 29, 2024 16:05

Rename isManaged and managedNamespaceRoot (#26697)

5c1d774

* renames * lowercase HVD to match * missed some * test failure

[Secrets sync] update sync test selectors (#26824)

9d3bc7e

* acceptance/clients/counts/overview-test: use imported test selectors * general-selectors: add missing emptyStateSubtitle property * acceptance/clients/counts/sync: nest tests in top level module for easier test runs

Merge branch 'main' into ui/VAULT-25468/enable-secrets-sync-hvd

840fcfe

Monkeychip added ui do-not-merge labels May 6, 2024

Monkeychip added this to the 1.17.0-rc milestone May 6, 2024

Monkeychip added 2 commits May 6, 2024 16:19

Add permissions check to show/hide activate button (#26840)

7b28bed

* add permissions check to flags service and consume in overview template * add back missing refresh * fix test failures * add test coverage * clean up * address flaky test * grr

Merge branch 'main' into ui/VAULT-25468/enable-secrets-sync-hvd

e126086

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

vercel bot deployed to Preview May 7, 2024 14:58 View deployment

Merge branch 'main' into ui/VAULT-25468/enable-secrets-sync-hvd

9624475

vercel bot deployed to Preview May 7, 2024 16:39 View deployment

Monkeychip and others added 6 commits May 7, 2024 10:53

address test failures

dfacd0d

add changelog

b282729

try to fix test failure only on gh

ad1aac7

Merge branch 'main' into ui/VAULT-25468/enable-secrets-sync-hvd

0fd4de2

fix fetch to match previous implementation of feature-flags

8f51b9c

Merge branch 'main' into ui/VAULT-25468/enable-secrets-sync-hvd

5ccb1be

vercel bot deployed to Preview May 8, 2024 14:31 View deployment

fix failing test

b78cbc4

Monkeychip changed the title ~~WIP: Sidebranch for enabling Secrets Sync HVD~~ Enabling Secrets Sync for HVD May 8, 2024

Monkeychip marked this pull request as ready for review May 8, 2024 15:54

Monkeychip requested review from a team as code owners May 8, 2024 15:54

Monkeychip removed the do-not-merge label May 8, 2024

Merge branch 'main' into ui/VAULT-25468/enable-secrets-sync-hvd

1e6699e

vercel bot deployed to Preview May 9, 2024 14:23 View deployment

hashishaw approved these changes May 9, 2024

View reviewed changes

update comment

863c6c0

Monkeychip merged commit 077c70f into main May 9, 2024
31 checks passed

Monkeychip deleted the ui/VAULT-25468/enable-secrets-sync-hvd branch May 9, 2024 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling Secrets Sync for HVD #26841

Enabling Secrets Sync for HVD #26841

Monkeychip commented May 6, 2024 •

edited

Loading

github-actions bot commented May 7, 2024 •

edited

Loading

github-actions bot commented May 8, 2024

hashishaw left a comment

hashishaw May 9, 2024

Monkeychip May 9, 2024

hashishaw May 9, 2024

hashishaw May 9, 2024

Monkeychip May 9, 2024

hashishaw May 9, 2024

Monkeychip May 9, 2024

hashishaw May 9, 2024

Monkeychip May 9, 2024

hashishaw May 9, 2024

Monkeychip May 9, 2024

hashishaw May 9, 2024

Enabling Secrets Sync for HVD #26841

Enabling Secrets Sync for HVD #26841

Conversation

Monkeychip commented May 6, 2024 • edited Loading

github-actions bot commented May 7, 2024 • edited Loading

github-actions bot commented May 8, 2024

hashishaw left a 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

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

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

Monkeychip commented May 6, 2024 •

edited

Loading

github-actions bot commented May 7, 2024 •

edited

Loading