Skip to content

add basic routes for secrets recovery #31412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lane-wetmore merged 9 commits into from
Aug 6, 2025
Merged

add basic routes for secrets recovery #31412

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a193a70
add basic routes for secrets recovery
lane-wetmore Aug 1, 2025
06cd58f
Merge branch 'main' into ui/VAULT-38371-secret-recovery-route-setup
lane-wetmore Aug 1, 2025
40e44eb
lint fix no index invocation
lane-wetmore Aug 1, 2025
304fa74
hide routes in production
lane-wetmore Aug 1, 2025
c3afabb
update routes
lane-wetmore Aug 4, 2025
771a6cc
Merge branch 'main' into ui/VAULT-38371-secret-recovery-route-setup
lane-wetmore Aug 4, 2025
3284ff6
add missing route js files
lane-wetmore Aug 4, 2025
080d7bf
add comments and clean up
lane-wetmore Aug 4, 2025
6c26cb7
update tests
lane-wetmore Aug 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ui/app/components/recovery.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{{#if this.isNotProduction}}
{{yield}}
Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's logic wrapping the sidenav link and the route itself, I don't think we also need a check in the component.

Is there a need for a component like this? The titles are mostly the same between views, except for the load form. I have a small re-filing suggestion but I'll open a PR to demonstrate since I think it might be easier to explain that way

Copy link
Copy Markdown
Contributor Author

@lane-wetmore lane-wetmore Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I've gone ahead and removed it

{{/if}}
13 changes: 13 additions & 0 deletions ui/app/components/recovery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* Copyright (c) HashiCorp, Inc.
* SPDX-License-Identifier: BUSL-1.1
*/

import Component from '@glimmer/component';
import config from 'vault/config/environment';

export default class AlphabetEditComponent extends Component {
get isNotProduction() {
return config.environment !== 'production';
}
}
6 changes: 6 additions & 0 deletions ui/app/components/recovery/page/snapshots/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
}}

<h1> test index</h1>
6 changes: 6 additions & 0 deletions ui/app/components/recovery/page/snapshots/load.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
}}

<h1> test load</h1>
6 changes: 6 additions & 0 deletions ui/app/components/recovery/page/snapshots/snapshot/manage.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
}}

<h1> test manage</h1>
7 changes: 7 additions & 0 deletions ui/app/components/sidebar/nav/cluster.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@
data-test-sidebar-nav-link="Secrets Sync"
/>
{{/if}}
{{#if this.showSecretsRecovery}}
Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we rename this to isNotProduction since I think this name could easily be confused for production code since we may eventually have logic here that hides this route for various cluster states (like DR secondary)

<Nav.Link
@route="vault.cluster.recovery.snapshots"
@text="Secrets Recovery"
data-test-sidebar-nav-link="Secrets Recovery"
/>
{{/if}}
{{#if (has-permission "access")}}
<Nav.Link
@route={{get (route-params-for "access") "route"}}
Expand Down
6 changes: 6 additions & 0 deletions ui/app/components/sidebar/nav/cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import Component from '@glimmer/component';
import { service } from '@ember/service';
import config from 'vault/config/environment';
Copy link
Copy Markdown
Contributor

@emoncuso emoncuso Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool, so this acts as a sort of development feature flag. Good idea!

(This can be in a separate PR) but this could be formalized a bit and drawn into a helper so you don't need the getter every time.

Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is definitely a new pattern to use the environment state like this. But with the new release cycle, I foresee us developing more iteratively instead of relying on sidebranches so it would make sense to make an app-wide helper!


export default class SidebarNavClusterComponent extends Component {
@service currentCluster;
Expand Down Expand Up @@ -57,4 +58,9 @@ export default class SidebarNavClusterComponent extends Component {
// otherwise we show the link depending on whether or not the feature exists
return this.version.hasSecretsSync;
}

// TODO remove conditional once further feature work for single item recovery for release 1.21 is completed
get showSecretsRecovery() {
Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding the comment above! Same comment about naming here 😄

return config.environment !== 'production';
}
}
11 changes: 11 additions & 0 deletions ui/app/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,17 @@ Router.map(function () {
this.route('replication-dr-promote', function () {
this.route('details');
});
// TODO remove conditional once further feature work for single item recovery for release 1.21 is completed
if (config.environment !== 'production') {
this.route('recovery', function () {
this.route('snapshots', function () {
this.route('load');
this.route('snapshot', function () {
this.route('manage');
});
Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the RFC we had talked about this segment of the URL being a dynamic param called snapshot_id (As an aside, it's a good practice to have the url param - snapshot_id - match the param we'll use from the API.)

In practice, this means that buttons or actions that navigate to manage a snapshot would look something like:

const snapshot_id = 1234
this.router.transitionTo('recovery.snapshots.snapshot', snapshot_id)

If this is still the plan we'll need to define the path (docs) for the dynamic segment here. I'm also wondering if manage is even necessary 🤔 Part of me thinks we should just get rid of the child route since right now it's just a single view to either read or recover items. We could have the "read" view just live at snapshot. If doing more things with a snapshot becomes a thing we can always extend it in the future. What do you think?

Suggested change
this.route('snapshot', function () {
this.route('manage');
});
this.route('snapshot', { path: '/:snapshot_id } )

Copy link
Copy Markdown
Contributor Author

@lane-wetmore lane-wetmore Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, didn't mean to leave that out! I've added the dynamic param and removed the manage route for the time being. It'll be easy to add back in later if we decide we need it.

});
});
}
Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be a little more intuitive if this was moved to below sync on line 19) so it sort of follows the sidebar nav order and makes it easier to find.

It's also hard to tell is the diff, but it looks like this is nested correctly so cluster is the immediate parent.

if (config.addRootMounts) {
config.addRootMounts.call(this);
}
Expand Down
8 changes: 8 additions & 0 deletions ui/app/routes/vault/cluster/recovery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* Copyright (c) HashiCorp, Inc.
* SPDX-License-Identifier: BUSL-1.1
*/

import Route from '@ember/routing/route';

export default class VaultClusterRecoveryRoute extends Route {}
Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a nit, but we typically drop VaultCluster from the route class name for brevity

8 changes: 8 additions & 0 deletions ui/app/routes/vault/cluster/recovery/snapshots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* Copyright (c) HashiCorp, Inc.
* SPDX-License-Identifier: BUSL-1.1
*/

import Route from '@ember/routing/route';

export default class VaultClusterRecoverySnapshotsRoute extends Route {}
Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export default class VaultClusterRecoverySnapshotsRoute extends Route {}
export default class RecoverySnapshotsRoute extends Route {}

16 changes: 16 additions & 0 deletions ui/app/templates/vault/cluster/recovery.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
}}

<Recovery>
<PageHeader as |p|>
<p.levelLeft>
<h1 class="title is-3">
Secrets Recovery
</h1>
</p.levelLeft>
</PageHeader>

{{outlet}}
</Recovery>
6 changes: 6 additions & 0 deletions ui/app/templates/vault/cluster/recovery/snapshots.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
}}

{{outlet}}
6 changes: 6 additions & 0 deletions ui/app/templates/vault/cluster/recovery/snapshots/index.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
}}

<Recovery::Page::Snapshots />
6 changes: 6 additions & 0 deletions ui/app/templates/vault/cluster/recovery/snapshots/load.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
}}

<Recovery::Page::Snapshots::Load />
6 changes: 6 additions & 0 deletions ui/app/templates/vault/cluster/recovery/snapshots/snapshot/manage.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
}}

<Recovery::Page::Snapshots::Snapshot::Manage />
Copy link
Copy Markdown
Contributor

@hellobontempo hellobontempo Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you agree with my routing comment above, we'll want to remember to rename this file to recovery/snapshots/snapshot.hbs (It could also just be snapshot.hbs but I kind of like when the "base" route is named index

And the page component renamed so it's: <Recovery::Page::Snapshots::Snapshot />

Copy link
Copy Markdown
Contributor Author

@lane-wetmore lane-wetmore Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

Loading