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

Prep work for creating one WIF configuration component #29345

Merged
merged 6 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
13 changes: 11 additions & 2 deletions ui/app/models/aws/lease-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,17 @@ export default class AwsLeaseConfig extends Model {
})
lease;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no functional changes, just restructuring to match azure and gcp models—see here.

configurableParams = ['lease', 'leaseMax'];

get displayAttrs() {
const keys = ['lease', 'leaseMax'];
return expandAttributeMeta(this, keys);
// while identical to formFields, keeping the same pattern as other configurable secret engines for consistency
// and to easily filter out displayAttributes in the future if needed
return this.formFields;
}

get formFields() {
return expandAttributeMeta(this, this.configurableParams);
}

displayTitle = 'Lease Configuration';
}
34 changes: 22 additions & 12 deletions ui/app/models/aws/root-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,33 @@ export default class AwsRootConfig extends Model {
iamEndpoint;
@attr('string', { label: 'STS endpoint' }) stsEndpoint;
@attr('number', {
label: 'Maximum retries',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The api name for this is "Max retries". I'm going to opt out of overriding this because it makes it complicated for testing.

subText: 'Number of max retries the client should use for recoverable errors. Default is -1.',
})
maxRetries;

Copy link
Contributor Author

@Monkeychip Monkeychip Jan 10, 2025

Choose a reason for hiding this comment

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

no functional changes, just restructuring to match azure and gcp models—see here.

configurableParams = [
'roleArn',
'identityTokenAudience',
'identityTokenTtl',
'accessKey',
'secretKey',
'region',
'iamEndpoint',
'stsEndpoint',
'maxRetries',
];

get isWifPluginConfigured() {
return !!this.identityTokenAudience || !!this.identityTokenTtl || !!this.roleArn;
}

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

get displayAttrs() {
const keys = [
'roleArn',
'identityTokenAudience',
'identityTokenTtl',
'accessKey',
'region',
'iamEndpoint',
'stsEndpoint',
'maxRetries',
];
return expandAttributeMeta(this, keys);
const formFields = expandAttributeMeta(this, this.configurableParams);
return formFields.filter((attr) => attr.name !== 'secretKey');
}

// "filedGroupsWif" and "fieldGroupsIam" are passed to the FormFieldGroups component to determine which group to show in the form (ex: @groupName="fieldGroupsWif")
Expand Down
1 change: 1 addition & 0 deletions ui/app/models/gcp/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export default class GcpConfig extends Model {
})
maxTtl;

/* GCP credential config field */
@attr('string', {
label: 'JSON credentials',
subText:
Expand Down
11 changes: 5 additions & 6 deletions ui/app/models/ssh/ca-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

@Monkeychip Monkeychip Jan 10, 2025

Choose a reason for hiding this comment

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

no functional changes, just restructuring to match azure and gcp models—see here.

@attr('boolean', { defaultValue: true })
generateSigningKey;
@attr('boolean', { defaultValue: true }) generateSigningKey;

configurableParams = ['privateKey', 'publicKey', 'generateSigningKey'];

// do not return private key for configuration.index view
get displayAttrs() {
return this.formFields.filter((attr) => attr.name !== 'privateKey');
}
// return private key for edit/create view

get formFields() {
const keys = ['privateKey', 'publicKey', 'generateSigningKey'];
return expandAttributeMeta(this, keys);
return expandAttributeMeta(this, this.configurableParams);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ module('Acceptance | aws | configuration', function (hooks) {
assert
.dom(GENERAL.infoRowValue('Identity token TTL'))
.doesNotExist('Identity token TTL does not show.');
assert.dom(GENERAL.infoRowValue('Maximum retries')).doesNotExist('Maximum retries does not show.');
assert.dom(GENERAL.infoRowValue('Max retries')).doesNotExist('Max retries does not show.');
// cleanup
await runCmd(`delete sys/mounts/${path}`);
});
Expand Down Expand Up @@ -219,7 +219,6 @@ module('Acceptance | aws | configuration', function (hooks) {
});

test('it shows AWS mount configuration details', async function (assert) {
assert.expect(12);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

assert.expect unnecessary because we'll get hit if one of the keys fails.

const path = `aws-${this.uid}`;
const type = 'aws';
this.server.get(`${path}/config/root`, (schema, req) => {
Expand All @@ -231,6 +230,7 @@ module('Acceptance | aws | configuration', function (hooks) {
createConfig(this.store, path, type); // create the aws root config in the store
await click(SES.configTab);
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.

You'll see a couple of these exceptions added to these loops. This is because of a clean up work done on the secret-engine-helper.

In that helper, I changed the method expectedConfigKeys to create generic arrays of expected values. Before I had two arrays for each engine, one for details and one for create—I opted to remove the two arrays and just make one for each engine to clarify within the tests, as I do here, what key we should not see in this iteration because the value is not returned from the API.

if (key === 'Secret key') return; // secret-key is not returned by the API
Copy link
Contributor

Choose a reason for hiding this comment

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

I think return will stop overall execution of the function.

Maybe continue?

Suggested change
if (key === 'Secret key') return; // secret-key is not returned by the API
if (key === 'Secret key') continue; // secret-key is not returned by the API

assert.dom(GENERAL.infoRowLabel(key)).exists(`${key} on the ${type} config details exists.`);
const responseKeyAndValue = expectedValueOfConfigKeys(type, key);
assert
Expand Down Expand Up @@ -368,7 +368,7 @@ module('Acceptance | aws | configuration', function (hooks) {
.doesNotExist('Access type section does not render for a community user');
// check all the form fields are present
await click(GENERAL.toggleGroup('Root config options'));
for (const key of expectedConfigKeys('aws-root-create')) {
Copy link
Contributor Author

@Monkeychip Monkeychip Jan 10, 2025

Choose a reason for hiding this comment

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

the value passed into expectedConfigKeys is changed here because of the above mentioned changed: I cleaned up this method to create generic arrays of keys that could come in camelized or not camelized.

true as a second param indicates "camelized"

for (const key of expectedConfigKeys('aws', true)) {
assert.dom(GENERAL.inputByAttr(key)).exists(`${key} shows for root section.`);
}
for (const key of expectedConfigKeys('aws-lease')) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ module('Acceptance | Azure | configuration', function (hooks) {
});
await enablePage.enable(this.type, path);
for (const key of expectedConfigKeys('azure')) {
if (key === 'Client secret') return; // client-secret is not returned by the API
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (key === 'Client secret') return; // client-secret is not returned by the API
if (key === 'Client secret') continue; // client-secret is not returned by the API

assert.dom(GENERAL.infoRowLabel(key)).exists(`${key} on the ${this.type} config details exists.`);
const responseKeyAndValue = expectedValueOfConfigKeys(this.type, key);
assert
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ module('Acceptance | GCP | configuration', function (hooks) {
service_account_email: 'service-email',
identity_token_audience: 'audience',
identity_token_ttl: 720000,
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 did not originally tests these values on edit, but I am now.

max_ttl: 14400,
ttl: 3600,
};
this.server.get(`${path}/config`, () => {
assert.ok(true, 'request made to config when navigating to the configuration page.');
Expand Down Expand Up @@ -102,6 +104,7 @@ module('Acceptance | GCP | configuration', function (hooks) {
});
await enablePage.enable(this.type, path);
for (const key of expectedConfigKeys(this.type)) {
if (key === 'Credentials') return; // not returned by the API
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (key === 'Credentials') return; // not returned by the API
if (key === 'Credentials') continue; // not returned by the API

const responseKeyAndValue = expectedValueOfConfigKeys(this.type, key);
assert
.dom(GENERAL.infoRowValue(key))
Expand Down
Loading
Loading