Skip to content
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.

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

Filter Secret Engine List view by engineType and/or name #20481

Merged
merged 22 commits into from
May 15, 2023

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented May 3, 2023

This PR adds two filtering options on the Secret Engine LIST view: filter by engineType (e.g. kv) and/or filter by engine name (e.g. secret). See community Issue for this request here.

  • There was a noticeable lag when having 100+ secret engines in rendering this page. This was always an issue, but very noticeable when testing filtering by a name with 100+ secret engines. After brainstorming solutions with the team, Jordan suggested simplifying the template structure. We had an older, single use contextual component called LinkableItem. TLDR: I changed the structure to match the list structure used in the Auth LIST view, backing out the contextual component. While this expanded the row count for the backends.hbs file, it removed 3 components, and greatly increased the page speed. This is a huge win for customers that have a lot of secret engines.
  • I glimmerized the controller and hbs file.
  • I simplified the structure of the template file by adding the property isSupportedBackend to the model and removing the two lists: supported and unsupported that were rendered in the view.
  • I added two other model properties that replaced two {{let}} definitions in the contextual components. This made this a lot easier to simplify and clean up the passing around of params.
Screen.Recording.2023-05-05.at.11.12.52.AM.mov

In another PR:

  • Created a Sparkle ticket to add pagination to this view.
  • Created a ticket to fix SearchSelect active state, which is a bug from the bulma removal.

If you want to run a script to add loads of secret engines, here is a bash script

echo "create lots of secret engine mounts"
for ((n=1;n<101;n++)); do
  vault secrets enable -path=kv-$n kv
done

@Monkeychip Monkeychip added the ui label May 3, 2023
@Monkeychip Monkeychip added this to the 1.14 milestone May 3, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this to the search select. I needed this functionality because it does not make sense for them to filter by engine Type when they are always going to see one item in the list if they filter by name.
image

@Monkeychip Monkeychip marked this pull request as ready for review May 5, 2023 16:19
update changelog text
@tracked secretEngineOptions = [];
@tracked selectedEngineType = null;
@tracked selectedEngineName = null;

Copy link
Contributor Author

@Monkeychip Monkeychip May 5, 2023

Choose a reason for hiding this comment

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

open to suggestions on ways to make this cleaner, but for context here is the flow:

  1. On init, this getter returns the full list of secret engines to display on the template.
  2. When someone filters, either by name or engine type, this list is updated. This update is triggered because of the tracked properties that change inside the getter when someone filters by name or engine type.
  3. This getter is used to create the options param passed into the SearchSelect components.

Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

this is so much cleaner! 🥲 Great work. Added a couple comments, and I'll hold approving until the filtering tests are in here!

ui/app/controllers/vault/cluster/secrets/backends.js Outdated Show resolved Hide resolved
<ul class="menu-list">
<li class="action">
<LinkTo @route="vault.cluster.secrets.backend.configuration" @model={{backend.id}} data-test-engine-config>
<LinkTo @route="vault.cluster.secrets.backend.configuration" @model={{backend.id}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This route might be different for engines like pki and kmip, no?

Copy link
Contributor Author

@Monkeychip Monkeychip May 15, 2023

Choose a reason for hiding this comment

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

Good question. That is an issue when you click on the whole row, but not when you click on the menu to go to configuration. For example, for PKI when you click on the row it navigates you to the overview page. But the configuration page is the same for all. I confirmed the same behavior on this branch is seen on main.

@hashishaw
Copy link
Contributor

🤦‍♀️ and then I approved it. Well, please ping me when the tests are written I'd love to take a look!

@Monkeychip
Copy link
Contributor Author

🤦‍♀️ and then I approved it. Well, please ping me when the tests are written I'd love to take a look!

Will do! I think I'll finish the tests today, but return to the tests to double check and I'll return to your comments when I have a fresh brain. For now, just so I don't accidentally press something, I'll add the don't merge label.

Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

Thanks for the test coverage! One small comment, but otherwise 🚀

@@ -28,28 +28,20 @@ export default class VaultClusterSecretsBackendController extends Controller {
if (this.selectedEngineType) {
// check first if the user has also filtered by name.
if (this.selectedEngineName) {
return this.filterByName(sortedBackends);
return sortedBackends.filter((backend) => this.selectedEngineName === backend.get('id'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to use .get anymore, we could instead do:

Suggested change
return sortedBackends.filter((backend) => this.selectedEngineName === backend.get('id'));
return sortedBackends.filter((backend) => this.selectedEngineName === backend.id);

But if that doesn't work for some reason I'd like to understand why!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants