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

WIF engines configurations to one component #29323

Closed
wants to merge 12 commits into from

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Jan 8, 2025

Description

GCP is the next WIF engine to allow create/edit configuration. While creating it, I noticed it was a near identical copy/paste of the configure-azure component. Thus, I decided to create a shared create/edit configure component called configure-create-edit for all WIF configurable engines. Short-term this includes: AWS, AZURE and soon GCP. Long-term this may include more 🤷‍♀️.

All of these WIF configurations include shared workflows, hence me combining them in a single component:

  1. Access Type options
  2. Issuer modal and network requests
  3. Community vs. Enterprise views
  4. Error flow if either the modal.save() fails and/or issuer.save() fails.

This PR:

  • Deletes aws-configure and azure-configure components and their component tests.
  • Does not touch SSH. SSH is also one of the 4 configurable engines (AWS, Azure, GCP and SSH). However, it's configuration workflow is significantly different because it's not WIF configurable and it does some odd generate key things.
  • Combines the component test but not the acceptance configuration tests for these engines.
  • Has no user facing changes. GCP will be added in a subsequent PR.
  • Creates a new helper to help with camelCasing strings or array of strings. This is relevant (and frustrating) in this workflow because checking for a value in a details view— a table row— passes in the attr name with spacing, etc (e.g. data-test-row-label="Identity token audience") vs checking if that form field exists on create-edit view—an input— is done by searching the camelCase (e.g. data-test-input="identityTokenAudience"`). To fix this inconsistency is beyond the scope of this PR and would likely break a lot of previously setup tests.

Note: I renamed and modified the azure-configure component, which is the new combined component—might help to see what changes were required, but also might make it harder to see all the changes together. Apologies.

  • write test for new string to camelCase helper (maybe pull into separate PR)

@Monkeychip Monkeychip added the ui label Jan 8, 2025
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jan 8, 2025
Copy link

github-actions bot commented Jan 8, 2025

CI Results:
All Go tests succeeded! ✅

<div class="box is-fullwidth is-sideless">
{{! accessType can be "azure" or "wif" - since WIF is an enterprise only feature we default to "azure" for community users and only display those related form fields. }}
{{#if this.version.isEnterprise}}
Copy link
Contributor Author

@Monkeychip Monkeychip Jan 9, 2025

Choose a reason for hiding this comment

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

The layout logic changed some (got easier). Because only WIF engines use this component, we don't have to do any checks for WIF engines. Instead the logic we're concerned with is:

  • Is this an enterprise version? If "yes" allow users to change the access type
  • What accessType is currently selected? If it's "account" then show non-wif fields, otherwise show wif fields.
  • Is there a second config model? If "yes" then show that. Second-config models are not included in accessType or enterprise logic, they should just show if they exist.

import errorMessage from 'vault/utils/error-message';

import type ConfigModel from 'vault/models/azure/config';
import type ConfigModel from 'vault/models/secret-engine/config';
Copy link
Contributor Author

@Monkeychip Monkeychip Jan 9, 2025

Choose a reason for hiding this comment

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

I moved all WIF engine config types to the same types model as there was a lot of overlap.

import type IdentityOidcConfigModel from 'vault/models/identity/oidc/config';
import type Router from '@ember/routing/router';
import type StoreService from 'vault/services/store';
import type VersionService from 'vault/services/version';
import type FlashMessageService from 'vault/services/flash-messages';

/**
* @module SecretEngineConfigureAzure component is used to configure the Azure secret engine
* For enterprise users, they will see an additional option to config WIF attributes in place of Azure account attributes.
* @module SecretEngineConfigureCreateEdit component is used to configure WIF_ENGINES (aws, azure)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to naming changes 🤷‍♀️

get isAccountPluginConfigured() {
return !!this.accessKey;
}

get displayAttrs() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this looks like I added secretKey to something I shouldn't have. It's a little hard to parse, but the views are still the same. For displayAttrs I filter out secret-key.

@@ -38,16 +38,15 @@ export default class SshCaConfig extends Model {
@attr('string') backend; // dynamic path of secret -- set on response from value passed to queryRecord
@attr('string', { sensitive: true }) privateKey; // obfuscated, never returned by API
@attr('string') publicKey;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know I said I didn't touch SSH, but I did clean up the model to match the pattern of the others.

@@ -91,6 +106,8 @@ export default class SecretsBackendConfigurationEdit extends Route {
model['identity-oidc-config'] = { queryIssuerError: true };
}
}
// add displayName to the model
model['displayName'] = displayName;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This saves us a little code as we use displayName at least twice in the edit.hbs file. Instead of calling the method to return displayName, now I can just access it on the model. It's a very small savings, and if this annoys folks, I'm happy to just call the method in its place.

@@ -45,40 +45,50 @@ export default class AwsRootConfig extends Model {
iamEndpoint;
@attr('string', { label: 'STS endpoint' }) stsEndpoint;
@attr('number', {
label: 'Maximum retries',
Copy link
Contributor Author

@Monkeychip Monkeychip Jan 9, 2025

Choose a reason for hiding this comment

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

So here's the deal: calling this Maximum when the api attr is Max makes things messy when running through tests. I'm doing and asking for permission later here.

* Copyright (c) HashiCorp, Inc.
* SPDX-License-Identifier: BUSL-1.1
*/

Copy link
Contributor Author

@Monkeychip Monkeychip Jan 9, 2025

Choose a reason for hiding this comment

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

This is the "new" component test. A couple of things:

-I did my best to run for loops for the tests that made sense to run on each wif engine.
-I did my best to test once if that tests wasn't dependent on engine type.
-And I did my best to cover all engine specific things.

All that to say, I did my best, but my brain is fried after running through this so many times. If you catch an efficiency, please point it out.

@@ -48,16 +48,29 @@ module('Integration | Component | SecretEngine/ConfigurationDetails', function (
);

for (const key of expectedConfigKeys(type)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test changed because I made my helper expectedConfigKeys a little easier to read. Instead of creating config arrays the excluded the keys not returned from the api, I just exclude them in the loop and check they're not in the view.

@@ -6,6 +6,7 @@
import { click, fillIn } from '@ember/test-helpers';
import { GENERAL } from 'vault/tests/helpers/general-selectors';
import { SECRET_ENGINE_SELECTORS as SES } from 'vault/tests/helpers/secret-engine/secret-engine-selectors';
import { stringToCamelCase } from 'vault/helpers/string-to-camel-case';
import { v4 as uuidv4 } from 'uuid';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some reorganizing that makes this seem like a lot of changes.

@Monkeychip Monkeychip closed this Jan 17, 2025
@Monkeychip Monkeychip deleted the ui/VAULT-33327/one-config-component branch January 17, 2025 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant