Skip to content

UI: Migrate <ConfigureAwsSecret /> to HDS & TS #27367

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
andaley merged 4 commits into from
Jun 6, 2024
Merged

UI: Migrate <ConfigureAwsSecret /> to HDS & TS #27367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b903912
feat: migrate configure-aws-secret to HDS::Tabs
Jun 5, 2024
e97081a
chore: co-locate configure-aws-secret template
Jun 5, 2024
f21b6b0
chore: convert configure-aws-secret to TS
Jun 5, 2024
52af959
tests: fix aws acceptance tests
Jun 5, 2024
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
141 changes: 141 additions & 0 deletions ui/app/components/configure-aws-secret.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
{{!
Copyright (c) HashiCorp, Inc.
SPDX-License-Identifier: BUSL-1.1
~}}

<Hds::Tabs class="has-top-margin-l" as |T|>
<T.Tab data-test-tab="access-to-aws">
Dynamic IAM root credentials
</T.Tab>
<T.Tab data-test-tab="lease">
Comment on lines +7 to +10
Copy link
Copy Markdown
Contributor Author

@andaley andaley Jun 5, 2024

Choose a reason for hiding this comment

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

one thing to note -- this page was previously using query params to keep track of which tabs were selected. if this is helpful for ensuring users can copy/paste a URL with a tab pre-selected i can totally add that back in, just wanted to make sure it was necessary first. 🙂

Copy link
Copy Markdown
Contributor

@Monkeychip Monkeychip Jun 5, 2024

Choose a reason for hiding this comment

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

I think the new designs Yankun is working on will remove tabs all together. That aside, I think removing queryParams here is okay because given the fact this page never properly transitioned after save I suspect it's not being used as much as we'd hope.

Copy link
Copy Markdown
Contributor Author

@andaley andaley Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ah, good to know! should i look into fixing the transition?

Copy link
Copy Markdown
Contributor

@Monkeychip Monkeychip Jun 5, 2024

Choose a reason for hiding this comment

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

Two thoughts on the transition:

  1. we make a pr that just fixes the transition and can be backported (so it couldn't depend on any changes here).
  2. we address the transition here or a future pr and don't backport it. Nobody has complained.

I think ideally, we'd do number 1, but only if it was easy to both fix and backport—otherwise, we can proceed with a fix in this aws 1.18.x work.

Copy link
Copy Markdown
Contributor Author

@andaley andaley Jun 6, 2024

Choose a reason for hiding this comment

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

good point, let's do #1. thanks!

Leases
</T.Tab>
<T.Panel>
<form
onsubmit={{action
"saveRootCreds"
Comment on lines +13 to +16
Copy link
Copy Markdown
Contributor Author

@andaley andaley Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this component was responsible for too much IMO, so i'll be splitting the 2 forms out into their own component in #27368 and #27370 and adding more specific integration tests

Copy link
Copy Markdown
Contributor

@Monkeychip Monkeychip Jun 5, 2024

Choose a reason for hiding this comment

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

I'd say wait for the new designs first. We just spoke in standup about how KVv2 create also makes two network requests on save. I think it's in the same component but rendered conditionally. Also, something to keep in mind, do we need to make new components if they're not consumed elsewhere or is one configuration create component sufficient and separates the concerns clearly?

Copy link
Copy Markdown
Contributor Author

@andaley andaley Jun 5, 2024

Choose a reason for hiding this comment

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

I'd say wait for the new designs first.

ah mk, if designs are going to change i can hold off, although i do already have PRs ready for them (see links in my first comment) 😅

We just spoke in standup about how KVv2 create also makes two network requests on save. I think it's in the same component but rendered conditionally.

whatcha mean by this? this component is only used when configuring an AWS secret engine. is there something else behind the scenes i'm missing?

do we need to make new components if they're not consumed elsewhere or is one configuration create component sufficient and separates the concerns clearly?

very fair question! there are a couple reasons why i often opt for splitting components up, even if they're not reused:

  1. enforcing separation of concerns: if a component starts to handle multiple responsibilities (e.g., complex logic, numerous conditionals, and various arguments), it's an indicator that it might benefit from being split into smaller, more focused components. this makes each component simpler and easier to understand.

  2. supporting readability and maintainability: when each component has a clear purpose, other developers (or my future self) can more easily understand what it does. this readability makes maintaining and updating the component simpler and less error-prone.

  3. testing (this is usually my biggest motivator): components with too many responsibilities can become hard to test. writing comprehensive tests for a single, monolithic component requires covering a wide range of scenarios, which can be complex and time-consuming. by splitting components, you can create more targeted tests for each component's specific functionality, leading to more reliable and maintainable tests.

Copy link
Copy Markdown
Contributor

@Monkeychip Monkeychip Jun 5, 2024

Choose a reason for hiding this comment

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

whatcha mean by this? this component is only used when configuring an AWS secret engine. is there something else behind the scenes i'm missing?

If we move away from these tabs and have only one view, then this will become a similar scenario to KVv2 create. Chelsea brought this up as example—it's just another create page that makes two network requests and both endpoints have different permission checks because they're different paths—similar to aws configure with /root vs /leases. It was just an example we could turn to if we needed to see how we handled a similar situation.

Yankun is working on the designs today so they should be done soon but I suspect we'll move to one create view with now tabs.

And for splitting up in components, thank you for the thoughts. Testing is a great point, and with that in mind splitting into two components makes sense. I'll check out your current WIP prs and any thoughts and feelings I'll put there, but please feel free to continue on that work.

(hash access_key=@accessKey iam_endpoint=@iamEndpoint sts_endpoint=@stsEndpoint secret_key=@secretKey region=@region)
}}
data-test-aws-root-creds-form="true"
aria-label="save root creds form"
>
<div class="box is-fullwidth is-shadowless is-marginless">
<NamespaceReminder @mode="save" @noun="configuration" />
<p class="has-text-grey-dark">
Note: the client uses the official AWS SDK and will use the specified credentials, environment credentials, shared
file credentials, or IAM role/ECS task credentials in that order.
</p>
</div>

<div class="field">
<label for="access" class="is-label">
Access key
</label>
<div class="control">
<Input
@type="text"
id="access"
name="access"
class="input"
autocomplete="off"
spellcheck="false"
@value={{@accessKey}}
data-test-aws-input="accessKey"
/>
</div>
</div>

<div class="field">
<label for="secret" class="is-label">
Secret key
</label>
<div class="control">
<Input
@type="password"
id="secret"
name="secret"
class="input"
@value={{@secretKey}}
data-test-aws-input="secretKey"
/>
</div>
</div>

<ToggleButton @isOpen={{this.showOptions}} @onClick={{fn (mut this.showOptions)}} />
{{#if this.showOptions}}
<div class="box is-marginless">
<div class="field">
<label for="region" class="is-label">
Region
</label>
<div class="control is-expanded">
<div class="select is-fullwidth">
<select
name="region"
id="region"
onchange={{action (mut @region) value="target.value"}}
data-test-input="region"
>
<option value=""></option>
{{#each (aws-regions) as |val|}}
<option>{{val}}</option>
{{/each}}
</select>
</div>
</div>
</div>
<div class="field">
<label for="iam" class="is-label">
IAM endpoint
</label>
<div class="control">
<Input @type="text" id="iam" name="iam" class="input" @value={{@iamEndpoint}} />
</div>
</div>
<div class="field">
<label for="sts" class="is-label">
STS endpoint
</label>
<div class="control">
<Input @type="text" id="sts" name="sts" class="input" @value={{@stsEndpoint}} />
</div>
</div>
</div>
{{/if}}

<div class="box is-bottomless is-fullwidth">
<Hds::Button @text="Save" data-test-aws-input="root-save" type="submit" />
</div>
</form>
</T.Panel>
<T.Panel>
<form
onsubmit={{action "saveLease" (hash lease=@model.lease lease_max=@model.leaseMax)}}
aria-label="save lease form"
data-test-aws-leases-form="true"
>
<div class="box is-fullwidth is-shadowless is-marginless">
<NamespaceReminder @mode="saved" @noun="configuration" />
<MessageError @model={{@model}} />
<p class="has-text-grey-dark">
If you do not supply lease settings, we will use the default values in AWS.
</p>
</div>
<TtlPicker
@label="Lease"
@initialValue={{@model.lease}}
@initialEnabled={{@model.lease}}
@onChange={{fn this.handleTtlChange "lease"}}
/>
<TtlPicker
@label="Maximum Lease"
@initialValue={{@model.leaseMax}}
@initialEnabled={{@model.leaseMax}}
@onChange={{fn this.handleTtlChange "leaseMax"}}
/>
<div class="box is-bottomless is-fullwidth">
<Hds::Button @text="Save" data-test-aws-input="lease-save" type="submit" />
</div>
</form>
</T.Panel>
</Hds::Tabs>
34 changes: 30 additions & 4 deletions ui/app/components/configure-aws-secret.js → ui/app/components/configure-aws-secret.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
import Component from '@glimmer/component';
import { action } from '@ember/object';

import type SecretEngineModel from 'vault/models/secret-engine';
import type { TtlEvent } from 'vault/app-types';

/**
* @module ConfigureAwsSecretComponent
*
Expand Down Expand Up @@ -34,21 +37,44 @@ import { action } from '@ember/object';
* @param {Function} saveAWSLease - parent action which updates AWS lease information
*
*/
export default class ConfigureAwsSecretComponent extends Component {

type AWSRootCredsFields = {
access_key: string | null;
iam_endpoint: string | null;
sts_endpoint: string | null;
secret_key: string | null;
region: string | null;
};

type LeaseFields = { lease: string; lease_max: string };

interface Args {
model: SecretEngineModel;
tab?: string;
accessKey: string;
secretKey: string;
region: string;
iamEndpoint: string;
stsEndpoint: string;
saveAWSRoot: (data: AWSRootCredsFields) => void;
saveAWSLease: (data: LeaseFields) => void;
}

export default class ConfigureAwsSecretComponent extends Component<Args> {
@action
saveRootCreds(data, event) {
saveRootCreds(data: AWSRootCredsFields, event: Event) {
event.preventDefault();
this.args.saveAWSRoot(data);
}

@action
saveLease(data, event) {
saveLease(data: LeaseFields, event: Event) {
event.preventDefault();
this.args.saveAWSLease(data);
}

@action
handleTtlChange(name, ttlObj) {
handleTtlChange(name: string, ttlObj: TtlEvent) {
// lease values cannot be undefined, set to 0 to use default
const valueToSet = ttlObj.enabled ? ttlObj.goSafeTimeString : 0;
this.args.model.set(name, valueToSet);
Expand Down
159 changes: 0 additions & 159 deletions ui/app/templates/components/configure-aws-secret.hbs
View file
Open in desktop

This file was deleted.

Loading