Allow Managed clusters to see Secrets Sync Overview and Sidebar nav #26649

Monkeychip · 2024-04-25T18:26:23Z

Note: merging to sidebranch until backend changes are merged. Final screenshots of all views will be added to sidebranch PR.

Description

Allows Managed clusters to view Secrets Sync from the sidebar nav and secrets sync overview.
Updates badge text to include "Plus" when a user is coming from a managed cluster.
Shows only one CTA message regardless of license / managed type.
Updates tests.

Enterprise tests pass locally.

github-actions · 2024-04-25T18:35:18Z

CI Results:
All Go tests succeeded! ✅

ui/app/services/flags.ts

Monkeychip · 2024-04-25T18:51:55Z

ui/app/services/version.js

In another PR we can decide if we want to change this getter from hasSecretsSync to licenseHasSecretsSync to clarify between HVD vs managed.

love that idea!

Monkeychip · 2024-04-25T18:53:53Z

ui/lib/sync/addon/components/sync-header.ts

I made the service's getters model params that I pass down to the component. In my mind it read clearer than calling a service each time these are used. However, I understand that one of the benefits of services are they can be called anywhere from within the app. Again, just a preference on my end, but I could be persuaded to not add them as model params and instead call from their services in the templates.

glad you brought this up! i usually prefer to call service getters directly since the state they contain is (generally) global to the application. this way we get the most up-to-date state straight from its source, and have less opportunity for it to become out of date during the passing-down process. in this particular situation i know we don't need to worry about that happening though since we're only fetching flags once at a very high level and that data is unlikely (or impossible) to change.

the way you have it set up here is easier to test though, since we can easily use @licenseHasSecretsSync=true without needing to stub the services that back that up. that's pretty nice. i also see that we're passing properties from the version service down from the route to child components too, and that consistency is great. so, i think your approach works just fine as long as we don't anticipate flags or version getters changing frequently! 👍

To this point, it's probably a good idea for an acceptance test to cover activating the feature and ensuring the reliant getters update as expected

Monkeychip · 2024-04-25T18:58:00Z

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

removed wrapping conditional that would hide the nav item if it was a managed cluster.

Monkeychip · 2024-04-25T19:00:59Z

ui/tests/helpers/sync/sync-selectors.js

@@ -70,7 +70,7 @@ export const PAGE = {
      action: (name) => `[data-test-overview-table-action="${name}"]`,
    },
  },


renamed this to match the other badgeText used on the sync-header component.

noelledaley

looking great so far! glad to see the implementation of this isn't as complicated as we were worried it might be. 🙌

noelledaley · 2024-04-25T19:13:48Z

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

+    const isManaged = this.flags.isManaged;
+    const onLicense = this.version.hasSecretsSync;
+    const isEnterprise = this.version.isEnterprise;

-  get syncBadge() {
-    if (this.version.isCommunity) return 'Enterprise';
-    if (!this.version.hasSecretsSync) return 'Premium';
+    if (isManaged) return 'Plus';
+    if (isEnterprise && !onLicense) return 'Premium';
+    if (!isEnterprise) 'Enterprise';


this is great for readability. nice!

noelledaley · 2024-04-25T19:24:27Z

ui/app/services/flags.ts

@@ -38,7 +38,10 @@ export default class flagsService extends Service {
    return null;
  }

-  // TODO getter will be used in the upcoming persona service
+  get isManaged() {
+    return this.managedNamespaceRoot !== null;


to futureproof this code i'd recommend swapping the logic here i.e.

get isManaged(): boolean { return this.flags && this.flags.includes(FLAGS.vaultCloudNamespace); } get managedNamespaceRoot(): string | null { return this.isManaged ? 'admin' : null; }

^ this makes isManaged more about checking the thing that actually determines if the cluster is managed or not, while managedNamespaceRoot can stay focused on answering "what is the name of the root namespace for this managed cluster?"

noelledaley · 2024-04-25T19:26:47Z

ui/app/services/version.js

love that idea!

noelledaley · 2024-04-25T19:48:45Z

ui/lib/sync/addon/components/sync-header.ts

glad you brought this up! i usually prefer to call service getters directly since the state they contain is (generally) global to the application. this way we get the most up-to-date state straight from its source, and have less opportunity for it to become out of date during the passing-down process. in this particular situation i know we don't need to worry about that happening though since we're only fetching flags once at a very high level and that data is unlikely (or impossible) to change.

the way you have it set up here is easier to test though, since we can easily use @licenseHasSecretsSync=true without needing to stub the services that back that up. that's pretty nice. i also see that we're passing properties from the version service down from the route to child components too, and that consistency is great. so, i think your approach works just fine as long as we don't anticipate flags or version getters changing frequently! 👍

noelledaley · 2024-04-25T19:51:22Z

ui/lib/sync/addon/components/sync-header.ts

+    const isManaged = this.flags.isManaged;
+    const onLicense = this.version.hasSecretsSync;
+    const isEnterprise = this.version.isEnterprise;
+
+    if (isManaged) return 'Plus feature';
+    if (isEnterprise && !onLicense) return 'Premium feature';
+    if (!isEnterprise) 'Enterprise feature';
+    // no badge for Enterprise clusters with Secrets Sync on their license--the only remaining option.
+    return undefined;


totally optional, but if this logic is basically the same as that in components/sidebar/nav/cluster.js it'd be nice to wrap that little badge in a component OR wrap the badge text in a helper used in both places. again, optional, since copying and pasting this in 2 places isn't so bad.

I was on the fence about this, and decided to just copy paste the logic. If it was a unique function we could use elsewhere I'd for sure opt for a helper, but this is very sync only.

github-actions · 2024-04-26T20:22:32Z

Build Results:
All builds succeeded! ✅

hellobontempo

Great work! These conditionals read clearly and I think this was the right decision on how to handle this logic flow! Just a couple minor comments/suggestions

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

hellobontempo · 2024-04-26T20:30:43Z

ui/app/services/flags.ts

-    }
-    return null;
+  get isManaged(): boolean {
+    return this.flags && this.flags.includes(FLAGS.vaultCloudNamespace);


this.flags will always be truthy because the default value is an empty array, right? So I think we can remove the first part of the conditional here. 🤔 If we're concerned that the value passed to setFeatureFlags is undefined then I wonder if a more long-term solution is instead adding a conditional check above so that we can rely on this.flags always being an array☝️

setFeatureFlags(flags: string[]) { this.flags = flags ? flags : []; }

Good catch. However, I did some additional moving to make the pattern more consistent and clear in the service how we were setting the tracked property flags (which I now call featureFlags). You'll see I moved the one await fetch call from the application route to this service. Curious if this reads better.

hellobontempo · 2024-04-26T20:36:59Z

ui/app/services/flags.ts

+    return this.flags && this.flags.includes(FLAGS.vaultCloudNamespace);
+  }
+
+  get managedNamespaceRoot(): string | null {


out of curiosity - is declaring the return here : string | null necessary? It seems like typescript should be able to infer the return of this getter since it's fairly straight forward

@noelledaley I'm going to punt this question to you. I probably have the least amount of typescript knowledge on the team. Do you know if this is just a best practice to keep here?

good question! whether or not to explicitly include the return type is somewhat of a stylistic choice, and depends how defensive we want our code to be. 🙂

examples of why you might include the return type:

makes the return type more obvious (i.e., no need to hover over the function in VSCode or skim through the code itself)

enforces type safety. if we later refactor this function to return undefined instead of null, for example, TS will cause an error and we'll have to decide whether the function should return string | undefined | null or just string | null. it helps us make more intentional decisions.

removing the need to hover to find the return type is good for developer accessibility. check out this tremendously awesome article by @ashleemboyer for more.

why you might not include it:

you're fine fine hovering/reading the code to figure out the return type

you trust TypeScript's ability to infer the correct type

personally, thus far i haven't included return types in functions this small/simple, but as you can see there are benefits to doing so. so, it's up to you!🌟

Thank you so much Noelle for the context/conversation. I'll leave in this case per personal preference.

ui/app/services/flags.ts

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

hellobontempo · 2024-04-26T20:48:10Z

ui/lib/sync/addon/components/sync-header.ts

+    const isManaged = this.flags.isManaged;
+    const onLicense = this.version.hasSecretsSync;
+    const isEnterprise = this.version.isEnterprise;
+
+    if (isManaged) return 'Plus feature';
+    if (isEnterprise && !onLicense) return 'Premium feature';
+    if (!isEnterprise) return 'Enterprise feature';
+    // no badge for Enterprise clusters with Secrets Sync on their license--the only remaining option.
+    return undefined;


Same comments here as the get badgeText() above

hellobontempo · 2024-04-26T20:50:25Z

ui/lib/sync/addon/components/sync-header.ts

To this point, it's probably a good idea for an acceptance test to cover activating the feature and ensuring the reliant getters update as expected

noelledaley

coupla test changes here, then all good!

ui/tests/integration/components/sidebar/nav/cluster-test.js

noelledaley · 2024-04-26T20:55:58Z

ui/tests/integration/components/sync/secrets/landing-cta-test.js

-
-  test('it should render CTA copy and action when feature exists on enterprise license and is activated', async function (assert) {
-    await render(hbs`<Secrets::LandingCta @isActivated={{true}} @hasSecretsSync={{true}} /> `, {
+  test('it should render CTA copy and action feature is activated', async function (assert) {


Suggested change

test('it should render CTA copy and action feature is activated', async function (assert) {

test('it should render CTA copy and action if feature is activated', async function (assert) {

Thank you for the catch!

noelledaley · 2024-04-26T20:56:46Z

ui/tests/integration/components/sync/secrets/page/overview-test.js

    this.isActivated = true;
+    this.licenseHasSecretsSync = true;
+    this.isManged = false;


Suggested change

this.isManged = false;

this.isManaged = false;

it looks like we also need to add tests for when isManaged is true or licenseHasSecretsSync is false, right? just want to make sure we're accounting for all possible situations that this conditional checks for 👀

Ah, yes. Let me add those tests in.

noelledaley · 2024-04-26T20:57:29Z

ui/tests/integration/components/sync/secrets/page/overview-test.js

-    const store = this.owner.lookup('service:store');
-    this.destinations = await store.query('sync/destination', {});
+    this.destinations = await this.store.query('sync/destination', {});
+    // component test so set values that would normally be calculated on the route model from the services.


i don't think this comment is necessary since stubbing properties like this is standard for component tests 👍

…match patterns

…um feature so cta message appplies to both managed and non-managed clusters

Monkeychip · 2024-04-29T17:52:48Z

ui/app/services/flags.ts

+    return this.isManaged ? 'admin' : null;
+  }
+
+  getFeatureFlags = keepLatestTask(async () => {


Note: I moved the fetch from the beforeModel hook in the application route to this concurrency task. It follows the pattern of getActivatedFeatures. However, there is a small difference in my pattern and I wanted to point that out. Compare the try/catch here to the previous call:

// previous call const result = await fetch('/v1/sys/internal/ui/feature-flags', { method: 'GET', }); if (result.status === 200) { const body = await result.json(); const flags = body.feature_flags || []; this.flagsService.setFeatureFlags(flags); }

i considered suggesting this change earlier so i'm glad you brought this up. i think it keeps the service's scope nicely organized. i like that this ensures we don't have to manually call this.flags.setFeatureFlags outside of the context of the service. 😀

noelledaley · 2024-04-29T18:47:03Z

ui/app/services/flags.ts

-  setFeatureFlags(flags: string[]) {
-    this.flags = flags;
-  }
+  @tracked featureFlags: string[] = [];


another great change for readability, love it 👏

noelledaley

this is looking great! the only blocking item i see left is to fix the test here 🎉

noelledaley · 2024-04-29T18:50:57Z

ui/app/services/flags.ts

+    return this.isManaged ? 'admin' : null;
+  }
+
+  getFeatureFlags = keepLatestTask(async () => {


i considered suggesting this change earlier so i'm glad you brought this up. i think it keeps the service's scope nicely organized. i like that this ensures we don't have to manually call this.flags.setFeatureFlags outside of the context of the service. 😀

noelledaley · 2024-04-29T18:57:04Z

ui/tests/acceptance/sync/secrets/overview-test.js

@@ -194,8 +193,8 @@ module('Acceptance | sync | overview', function (hooks) {
      this.server.post('/sys/activation-flags/secrets-sync/activate', (_, req) => {
        assert.strictEqual(
          req.requestHeaders['X-Vault-Namespace'],
-          'admin',
-          'Request is made to admin namespace'
+          undefined,


i am fairly sure that this request is supposed to be made to the admin namespace for HVD clusters since HVD users only have admin access (and not root level).

to resolve that, you'll need to add the namespace to the request here:

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

Line 77 in d56f564

.ajax('/v1/sys/activation-flags/secrets-sync/activate', 'POST', { namespace: null });

Ah, good catch. Yes this is the POST on activate that we do need to add the namespace. Got myself mixed up.

noelledaley

well done Angel! this looks fantastic.

update badge text and allow hvd on secrets sync views

bef0e01

Monkeychip added ui do-not-merge labels Apr 25, 2024

Monkeychip added this to the 1.17.0-rc milestone Apr 25, 2024

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

Monkeychip changed the title ~~Sidebranch: Allow Managed clusters to use Secrets Sync~~ Allow Managed clusters to see Secrets Sync Overview and Sidebar nav Apr 25, 2024

Monkeychip changed the base branch from main to ui/VAULT-25468/enable-secrets-sync-hvd April 25, 2024 18:32

update logic in Secrets Sync overview and cta for hvd.

31f831e

Monkeychip added pr/no-changelog and removed do-not-merge labels Apr 25, 2024

Monkeychip commented Apr 25, 2024

View reviewed changes

ui/app/services/flags.ts Show resolved Hide resolved

Monkeychip commented Apr 25, 2024

View reviewed changes

spacing

a8ed0e1

Monkeychip commented Apr 25, 2024

View reviewed changes

noelledaley reviewed Apr 25, 2024

View reviewed changes

Monkeychip added 4 commits April 25, 2024 15:17

rearrange based on pr feedback

f1fb24a

fix return on badgeText and cluster nav test

002af2a

fix landing cta tests

bbaa8d0

update test to reflect new changes

261948c

Monkeychip marked this pull request as ready for review April 26, 2024 20:18

Monkeychip requested a review from a team as a code owner April 26, 2024 20:18

hellobontempo reviewed Apr 26, 2024

View reviewed changes

noelledaley requested changes Apr 26, 2024

View reviewed changes

Monkeychip added 3 commits April 29, 2024 09:57

moved call to feature-flags from application route to the service to …

1ba42f8

…match patterns

add managed test coverage on overview component test and remove premi…

a659c17

…um feature so cta message appplies to both managed and non-managed clusters

missed service name and unskip admin test

4bc4b69

Monkeychip commented Apr 29, 2024

View reviewed changes

clean up

fab3451

Monkeychip requested a review from noelledaley April 29, 2024 17:55

noelledaley reviewed Apr 29, 2024

View reviewed changes

noelledaley requested changes Apr 29, 2024

View reviewed changes

fix tests

174a0e6

Monkeychip requested a review from noelledaley April 29, 2024 20:02

flags test fix

fa68caa

noelledaley approved these changes Apr 29, 2024

View reviewed changes

Monkeychip merged commit 73439b9 into ui/VAULT-25468/enable-secrets-sync-hvd Apr 29, 2024
23 checks passed

Monkeychip deleted the ui/VAULT-26004/secerts-sync-visible-hvd branch April 29, 2024 22:05

	test('it should render CTA copy and action feature is activated', async function (assert) {
	test('it should render CTA copy and action if feature is activated', async function (assert) {

Allow Managed clusters to see Secrets Sync Overview and Sidebar nav #26649

Allow Managed clusters to see Secrets Sync Overview and Sidebar nav #26649

Conversation

Monkeychip commented Apr 25, 2024 • edited

Description

github-actions bot commented Apr 25, 2024 • edited

Monkeychip Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monkeychip Apr 25, 2024 • edited

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

noelledaley 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

github-actions bot commented Apr 26, 2024

hellobontempo 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

noelledaley 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

noelledaley left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelledaley Apr 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelledaley left a comment

Choose a reason for hiding this comment

Monkeychip commented Apr 25, 2024 •

edited

github-actions bot commented Apr 25, 2024 •

edited

Monkeychip Apr 25, 2024 •

edited

Monkeychip Apr 25, 2024 •

edited

noelledaley left a comment •

edited

noelledaley Apr 29, 2024 •

edited