Allow Configuration of Azure Secret Engine, including WIF for enterprise users #29047
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: |
|
Build Results: |
| backend, | ||
| type, | ||
| }); | ||
| // some of the models return a 200 if they are not configured (ex: azure) | ||
| // so instead of checking a catch or httpStatus, we check if the model is configured based on the getter `isConfigured` on the engine's model | ||
| // if the engine is not configured we update the record to get the default values |
There was a problem hiding this comment.
Azure, at the moment, is the only engine that returns a 200 when the config has not been modified. However, future engines might also keep this pattern / or it could just be an Azure thing. For now, I have this line to handle it and isConfigured getter added to the model. Is it my favorite? No, but I feel like it's pretty smooth for the situation. 🤷♀️
| } | ||
|
|
||
| get modelAttrChanged() { | ||
| // "backend" dirties model state so explicity ignore it here |
There was a problem hiding this comment.
FWIW I don't think this would happen if you didn't create a new record, and reused the record from queryRecord. But I'm not 💯 sure
| // if the issuer has changed show modal with warning that the config will change | ||
| // if the modal is shown, the user has to click confirm to continue saving | ||
| this.saveIssuerWarning = `You are updating the global issuer config. This will overwrite Vault's current issuer ${ | ||
| issuerConfig.queryIssuerError ? 'if it exists ' : '' |
There was a problem hiding this comment.
Personally I don't think we need to hold onto queryIssuerError here and could just always say "if it exists" but that's just me in favor of keeping things simple 😅
There was a problem hiding this comment.
Agree. This was Chelsea and design working through dynamic language to show a more detailed message if you had read access to this endpoint vs if you didn't. I'll add this comment to the nice-to-have ticket for refactoring the issuer-modal-model save flow.
| // transition if the model was saved successfully | ||
| // or transition if no attributes on the model have changed and the issuer was saved successfully |
There was a problem hiding this comment.
I wonder if this would be clearer as "We only prevent a transition if the model is edited and fails saving." Since the comment explains when we do transition which we can see from the conditional 💬
ui/app/models/azure/config.js
Outdated
| // clientSecret is not checked here because it's never return by the API | ||
| // however it is an Azure account field |
There was a problem hiding this comment.
thanks for this comment!
| @@ -65,4 +65,41 @@ export default class AzureConfig extends Model { | |||
| get formFields() { | |||
| return expandAttributeMeta(this, this.configurableParams); | |||
| } | |||
|
|
|||
| // "filedGroupsWif" and "fieldGroupsAzure" are passed to the FormFieldGroups component to determine which group to show in the form (ex: @groupName="fieldGroupsWif") | |||
| get fieldGroupsWif() { | |||
There was a problem hiding this comment.
This seems like quite a bit of extra code (these two getters plus the formFieldGroups function below) - I think we already have helpers that do this 🤔 Can we reuse any of the model decorators or existing patterns to get the same functionality?
There was a problem hiding this comment.
Did a little updating, let me know if that helps.
| <fieldset class="field form-fieldset" id="protection" data-test-access-type-section> | ||
| <legend class="is-label">Access Type</legend> | ||
| <p class="sub-text" data-test-access-type-subtext> | ||
| {{#if this.disableAccessType}} | ||
| You cannot edit Access Type if you have already saved access credentials. | ||
| {{else}} | ||
| Choose the way to configure access to Azure. Access can be configured either using an Azure account or with the | ||
| Plugin Workload Identity Federation (WIF). | ||
| {{/if}} | ||
| </p> | ||
| <div> | ||
| <RadioButton | ||
| id="access-type-azure" | ||
| name="azure" | ||
| class="radio" | ||
| data-test-access-type="azure" | ||
| @value="azure" | ||
| @groupValue={{this.accessType}} | ||
| @onChange={{fn this.onChangeAccessType "azure"}} | ||
| @disabled={{this.disableAccessType}} | ||
| /> | ||
| <label for="access-type-azure">Azure account credentials</label> | ||
| <RadioButton | ||
| id="access-type-wif" | ||
| name="wif" | ||
| class="radio has-left-margin-m" | ||
| data-test-access-type="wif" | ||
| @value="wif" | ||
| @groupValue={{this.accessType}} | ||
| @onChange={{fn this.onChangeAccessType "wif"}} | ||
| @disabled={{this.disableAccessType}} | ||
| /> | ||
| <label for="access-type-wif">Workload Identity Federation</label> | ||
| </div> | ||
| </fieldset> |
There was a problem hiding this comment.
Since this is reused in AWS curious why we didn't make a shared component?
There was a problem hiding this comment.
GCP, AWS and Azure will all have some small overlap because we're adding this "Access Type" option to enterprise views. However, they are some differences, specifically the group values that relate back to these engine's models.
My preference was to keep all configuration code separate in each engines own configuration file. The win for sharing this chunk of code didn't feel significant enough to introduce a shared component that would have to work around each engine's specific "group" or labeling.
ui/app/templates/vault/cluster/secrets/backend/configuration/index.hbs
Outdated
Show resolved
Hide resolved
| assert.notOk( | ||
| true, | ||
| 'post request was made to issuer endpoint when on community and data not changed. test should fail.' | ||
| ); |
There was a problem hiding this comment.
Noelle had a great idea that instead of using assert here (which can be kind of confusing and unclear) we can throw an error, like this:
https://github.com/hashicorp/vault/blob/main/ui/tests/integration/components/dashboard/overview-test.js#L105-L108
So instead of the assertion count being the only thing that lets us know this errors, the test actually fails when the bad thing happens
There was a problem hiding this comment.
I did this a couple of places, but this test will fail if the server.post gets hit because notOk first param should be false, but here it's true (thus, the assert.expect isn't the only catch).
To me, I prefer the assertion failing like this, but I'm clearly not beholden to one way or the other having used both approaches.
ui/tests/acceptance/secrets/backend/azure/azure-configuration-test.js
Outdated
Show resolved
Hide resolved
| assert.expect(2); | ||
| await click(SES.configure); | ||
| const url = currentURL(); | ||
| const path = url.split('/')[3]; // get path from url because we can't pass the path from beforeEach hook to individual test. |
There was a problem hiding this comment.
could we save the path as this.path in the beforeEach hook instead?
There was a problem hiding this comment.
Oish, I tried. It doesn't work for a couple reasons that I remember. If I set the path to something dynamic like path-${this.uuid} it changes before each test and then when i call it within the test it changes again. And setting it to something hard coded likepath-azure, makes it a little harder to debug what path is being used in what tests and prone to flakiness even with cleanup after each run.
…ise users (#29047) * transfer over all changes from original pr * changelog * add serialize catch for no empty string environment * move ttl format logic to parent route * Update 29047.txt * clean up some comments * Update changelog/29047.txt Co-authored-by: claire bontempo <[email protected]> * Update changelog/29047.txt Co-authored-by: claire bontempo <[email protected]> * Update ui/app/components/secret-engine/configure-azure.hbs Co-authored-by: claire bontempo <[email protected]> * first round of addressing pr comments, holding off on the issue save flow for error messaging to keep separate * Update CODEOWNERS merge issue * small clean up tasks * updates * test coverage * small cleanup * small clean up * clean up * clean up getters on model --------- Co-authored-by: claire bontempo <[email protected]>
Description
This PR adds to the Secret engine Azure:
Screenshare walk through:
azurewalkthrough.mov
How it looks in community, no option to change Access Type and therefor select WIF enterprise only params.
