Refactor SSH Configuration workflow #28122

Monkeychip · 2024-08-19T18:45:45Z

Duplicate of PR #28004. Issue with commit history, so copied changes over to new pr.

Description

Refactor configure SSH. This is prep work for the next PR — > move AWS configurations into one form.

Why SSH?
SSH consumes the same components and routes that AWS does. You'll see some conditions excluding AWS from the new workflow. This is 🤮 and won't be long-standing. While I could technically focus on just AWS and exclude it from these upgrades that flow would cause more problems.

Why SSH first?
It's easier for me to focus on SSH first than AWS (full disclosure I did both at the same time and thus pulled SSH out, which was easier). AWS has extra complexities because it deals with two configs instead of one. It also has a lot more test coverage/changes.

Why separate SSH/AWS changes?
Because the original PR was 33 + files. The test changes where 🤯.

Test Changes

Removes the ssh specific tests from models/secret-engine-test. Secret engine is no longer responsible for saving anything ssh ca-config.
Removes a secret-engine adapter test that checked for finding non-ssh engines. not a thing anymore because we don't use findRecord for SSH things on the secret-engine model.
Updates the configure-ssh component test. This test does the best job at telling you how the component changed.

Additional SSH test coverage

Checks for validation messages (new, we used flash messages previously).
Checks proper transitions.

Screenshots

There are no user facing changes. However, for posterity here is the flow with the changes.

ssh.mov

github-actions · 2024-08-19T18:49:22Z

Build Results:
All builds succeeded! ✅

github-actions · 2024-08-19T18:52:21Z

CI Results:
All Go tests succeeded! ✅

hashishaw

Great test coverage and comments, and typescript!! 🤩

I left a couple notes, but none are blocking (although I do think they're worth considering before merge)

I checked it out locally and one thing I saw is that the new GUI seems to have a lot more lines on the config page 🤔

hashishaw · 2024-08-19T20:08:27Z

ui/app/components/secret-engine/configure-ssh.hbs

+  <div class="box is-fullwidth is-shadowless is-marginless">
+    <NamespaceReminder @mode="save" @noun="configuration" />
+    <MessageError @errorMessage={{this.errorMessage}} />
+    {{#unless @model.isNew}}


I'm seeing this message even creating the config for the first time -- I wonder if maybe we should direct to the details view after create instead, which eliminates the need for this conditional because this screen is always shown when the user is trying to configure

Agreed! The current behavior is to navigate to the same screen, but I'm not a big fan of that. Let me do the suggested and see how it turns out.

hashishaw · 2024-08-19T20:10:48Z

ui/app/components/secret-engine/configure-ssh.ts

+
+  transition(isCancel = false) {
+    // onCancel is the only case in which we transition to the parent route.
+    // If we're creating or editing the configuration we stay on the configuration page to view the new public key.


Again, I'm seeing the public key on the new configuration details page so I'm not sure why this is necessary

Yeah, I'm with ya. I was trying to keep the behavior the same. There is still some work todo on this flow because they still need the edit screen to see the option to delete, but that's for another day.

hashishaw · 2024-08-19T20:16:20Z

ui/app/routes/vault/cluster/secrets/backend/configuration/edit.ts

+      // convert the adapterPath with a name that can be passed to the components
+      // ex: adapterPath = ssh/ca-config, convert to: ssh-ca-config so that you can pass to component @model={{this.model.ssh-ca-config}}
+      for (const key in model) {
+        this.standardizeModelName(key, model);


instead of doing this afterward, I would suggest we standardize the name when we set it on the model at line 47, so we minimize methods with side effects

hashishaw · 2024-08-19T20:23:45Z

ui/tests/acceptance/secrets/backend/ssh/configuration-test.js

-    assert
-      .dom(GENERAL.inputByAttr('generate-signing-key-checkbox'))
-      .isNotChecked('Generate signing key is unchecked');
+    // change in behavior after refactor. Because we refresh the model after delete, the generate signing key is checked by default.


Thanks for this comment! IMO it doesn't need to live in the code (could be a github comment) but that's a very small nit 😄

Good point. I'll amend.

* initial copy from other #28004 * pr feedback * grr

* auto-roll billing start enos test * enos: don't expect curl available in docker image (#27984) Signed-off-by: Ryan Cragun <me@ryan.ec> * Update interoperability-matrix.mdx (#27977) Updating the existing Vault/YubiHSM integration with a newer version of Vault as well as now supporting Managed Keys. * Update hana db pkg (#27950) * database/hana: use go-hdb v1.10.1 * docs/hana: quotes around password so dashes don't break it * Clarify audit log failure telemetry docs. (#27969) * Clarify audit log failure telemetry docs. * Add the note about the misleading counts * Auto-rolling billing start docs PR (#27926) * auto-roll docs changes * addressing comments * address comments * Update website/content/api-docs/system/internal-counters.mdx Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com> * addressing some changes * update docs * update docs with common explanation file * updated note info * fix 1.18 upgrade doc * fix content-check error * Update website/content/partials/auto-roll-billing-start-example.mdx Co-authored-by: miagilepner <mia.epner@hashicorp.com> --------- Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com> Co-authored-by: miagilepner <mia.epner@hashicorp.com> * docker: add upgrade notes for curl removal (#27995) Signed-off-by: Ryan Cragun <me@ryan.ec> * Update vault-plugin-auth-jwt to v0.21.1 (#27992) * docs: fix upgrade 1.16.x (#27999) Signed-off-by: Ryan Cragun <me@ryan.ec> * UI: Add unsupportedCriticalCertExtensions to jwt config expected payload (#27996) * Client Count Docs Updates/Cleanup (#27862) * Docs changes * More condensation of docs * Added some clarity on date ranges * Edited wording' * Added estimation client count info * Update website/content/api-docs/system/internal-counters.mdx Co-authored-by: miagilepner <mia.epner@hashicorp.com> --------- Co-authored-by: miagilepner <mia.epner@hashicorp.com> * update(kubernetes.mdx): k8s-tokenreview URL (#27993) Co-authored-by: akshya96 <87045294+akshya96@users.noreply.github.com> * Update programmatic-management.mdx to clarify Terraform prereqs (#27548) * UI: Replace getNewModel with hydrateModel when model exists (#27978) * Replace getNewModel with hydrateModel when model exists * Update getNewModel to only handle nonexistant model types * Update test * clarify test * Fix auth-config models which need hydration not generation * rename file to match service name * cleanup + tests * Add comment about helpUrl method * Changelog for 1.17.3, 1.16.7 enterprise, 1.15.13 enterprise (#28018) * changelog for 1.17.3, 1.16.7 enterprise, 1.15.13 enterprise * Add spacing to match older changelogs * Fix typo in variables.tf (#27693) intialize -> initialize Co-authored-by: akshya96 <87045294+akshya96@users.noreply.github.com> * Update 1_15-auto-upgrade.mdx (#27675) * Update 1_15-auto-upgrade.mdx * Update known issue version numbers for AP issue --------- Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com> * Update 1_16-default-policy-needs-to-be-updated.mdx (#27157) Made a few grammar changes plus updating term from Vault IU to Vault UI * change instances variable to hosts * for each hosts * add cluster addr port * Add ENVs using NewTestDockerCluster (#27457) * Add ENVs using NewTestDockerCluster Currently NewTestDockerCluster had no means for setting any environment variables. This makes it tricky to create test for functionality that require thems, like having to set AWS environment variables. DockerClusterOptions now exposes an option to pass extra enviroment variables to the containers, which are appended to the existing ones. * adding changelog * added test case for setting env variables to containers * fix changelog typo; env name * Update changelog/27457.txt Co-authored-by: akshya96 <87045294+akshya96@users.noreply.github.com> * adding the missing copyright --------- Co-authored-by: akshya96 <87045294+akshya96@users.noreply.github.com> * UI: Build KV v2 overview page (#28106) * move date-from-now helper to addon * make overview cards consistent across engines * make kv-paths-card component * remove overview margin all together * small styling changes for paths card * small selector additions * add overview card test * add overview page and test * add default timestamp format * cleanup paths test * fix dateFromNow import * fix selectors, cleanup pki selectors * and more selector cleanup * make deactivated state single arg * fix template and remove @isDeleted and @isDestroyed * add test and hide badge unless deactivated * address failings from changing selectors * oops, not ready to show overview tab just yet! * add deletionTime to currentSecret metadata getter * Bump actions/download-artifact from 4.1.7 to 4.1.8 (#27704) Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 4.1.7 to 4.1.8. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@65a9edc...fa0a91b) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: akshya96 <87045294+akshya96@users.noreply.github.com> * Bump actions/setup-node from 4.0.2 to 4.0.3 (#27738) Bumps [actions/setup-node](https://github.com/actions/setup-node) from 4.0.2 to 4.0.3. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](actions/setup-node@60edb5d...1e60f62) --- updated-dependencies: - dependency-name: actions/setup-node dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: akshya96 <87045294+akshya96@users.noreply.github.com> * Add valid IP callout (#28112) Co-authored-by: Yoko Hyakuna <yoko@hashicorp.com> * Refactor SSH Configuration workflow (#28122) * initial copy from other #28004 * pr feedback * grr * Bump browser-actions/setup-chrome from 1.7.1 to 1.7.2 (#28101) Bumps [browser-actions/setup-chrome](https://github.com/browser-actions/setup-chrome) from 1.7.1 to 1.7.2. - [Release notes](https://github.com/browser-actions/setup-chrome/releases) - [Changelog](https://github.com/browser-actions/setup-chrome/blob/master/CHANGELOG.md) - [Commits](browser-actions/setup-chrome@db1b524...facf10a) --- updated-dependencies: - dependency-name: browser-actions/setup-chrome dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: divyaac <divya.chandrasekaran@hashicorp.com> * Bump vault-gcp-secrets-plugin (#28089) Co-authored-by: divyaac <divya.chandrasekaran@hashicorp.com> * docs: correct list syntax (#28119) Co-authored-by: divyaac <divya.chandrasekaran@hashicorp.com> * add semgrepconstraint check in skip step --------- Signed-off-by: Ryan Cragun <me@ryan.ec> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Ryan Cragun <me@ryan.ec> Co-authored-by: Adam Rowan <92474478+bear359@users.noreply.github.com> Co-authored-by: Theron Voran <tvoran@users.noreply.github.com> Co-authored-by: Paul Banks <pbanks@hashicorp.com> Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com> Co-authored-by: miagilepner <mia.epner@hashicorp.com> Co-authored-by: Scott Miller <smiller@hashicorp.com> Co-authored-by: Chelsea Shaw <82459713+hashishaw@users.noreply.github.com> Co-authored-by: divyaac <divya.chandrasekaran@hashicorp.com> Co-authored-by: Roman O'Brien <58272664+romanobrien@users.noreply.github.com> Co-authored-by: Adrian Todorov <adrian.todorov@hashicorp.com> Co-authored-by: VAL <val@hashicorp.com> Co-authored-by: Ikko Eltociear Ashimine <eltociear@gmail.com> Co-authored-by: Owen Zhang <86668876+owenzorrin@users.noreply.github.com> Co-authored-by: gkoutsou <gkoutsou@users.noreply.github.com> Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jonathan Frappier <92055993+jonathanfrappier@users.noreply.github.com> Co-authored-by: Yoko Hyakuna <yoko@hashicorp.com> Co-authored-by: Angel Garbarino <Monkeychip@users.noreply.github.com> Co-authored-by: Max Levine <max@maxlevine.co.uk> Co-authored-by: Steffy Fort <steffyfort@gmail.com>

initial copy from other #28004

872c6f6

Monkeychip requested a review from a team as a code owner August 19, 2024 18:45

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

Monkeychip added ui pr/no-changelog labels Aug 19, 2024

Monkeychip added this to the 1.18.0-rc milestone Aug 19, 2024

Monkeychip mentioned this pull request Aug 19, 2024

Refactor SSH Configure workflow #28004

Closed

1 task

hashishaw approved these changes Aug 19, 2024

View reviewed changes

Monkeychip added 2 commits August 19, 2024 15:12

pr feedback

0878532

grr

740479f

Monkeychip merged commit ec95f85 into main Aug 19, 2024
26 checks passed

Monkeychip deleted the ui/ssh-prep-2 branch August 19, 2024 21:58

akshya96 pushed a commit that referenced this pull request Aug 20, 2024

Refactor SSH Configuration workflow (#28122)

ef986a0

* initial copy from other #28004 * pr feedback * grr

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor SSH Configuration workflow #28122

Refactor SSH Configuration workflow #28122

Monkeychip commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Aug 19, 2024

github-actions bot commented Aug 19, 2024 •

edited

Loading

hashishaw left a comment

hashishaw Aug 19, 2024

Monkeychip Aug 19, 2024

hashishaw Aug 19, 2024

Monkeychip Aug 19, 2024

hashishaw Aug 19, 2024

hashishaw Aug 19, 2024

Monkeychip Aug 19, 2024

Refactor SSH Configuration workflow #28122

Refactor SSH Configuration workflow #28122

Conversation

Monkeychip commented Aug 19, 2024 • edited Loading

Description

Test Changes

Additional SSH test coverage

Screenshots

github-actions bot commented Aug 19, 2024

github-actions bot commented Aug 19, 2024 • edited Loading

hashishaw left a comment

Choose a reason for hiding this comment

hashishaw Aug 19, 2024

Choose a reason for hiding this comment

Monkeychip Aug 19, 2024

Choose a reason for hiding this comment

hashishaw Aug 19, 2024

Choose a reason for hiding this comment

Monkeychip Aug 19, 2024

Choose a reason for hiding this comment

hashishaw Aug 19, 2024

Choose a reason for hiding this comment

hashishaw Aug 19, 2024

Choose a reason for hiding this comment

Monkeychip Aug 19, 2024

Choose a reason for hiding this comment

Monkeychip commented Aug 19, 2024 •

edited

Loading

github-actions bot commented Aug 19, 2024 •

edited

Loading