-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
UI: Fix enabling replication capabilities bug #28371
Changes from all commits
2ef2121
6efc65a
4c37de4
e71e4f9
1f43b8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:bug | ||
ui: Fix UI improperly checking capabilities for enabling performance and dr replication | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,16 +19,15 @@ import { waitFor } from '@ember/test-waiters'; | |
* but otherwise it handles the rest of the form inputs. On success it will clear the form and call the onSuccess callback. | ||
* | ||
* @example | ||
* ```js | ||
* <EnableReplicationForm @replicationMode="dr" @canEnablePrimary={{true}} @canEnableSecondary={{false}} @performanceReplicationDisabled={{false}} @onSuccess={{this.reloadCluster}} /> | ||
* @param {string} replicationMode - should be one of "dr" or "performance" | ||
* @param {boolean} canEnablePrimary - if the capabilities allow the user to enable a primary cluster | ||
* @param {boolean} canEnableSecondary - if the capabilities allow the user to enable a secondary cluster | ||
* @param {boolean} performanceMode - should be "primary", "secondary", or "disabled". If enabled, form will show a warning when attempting to enable DR secondary | ||
* @param {Promise} onSuccess - (optional) callback called after successful replication enablement. Must be a promise. | ||
* @param {boolean} doTransition - (optional) if provided, passed to onSuccess callback to determine if a transition should be done | ||
* /> | ||
* ``` | ||
* | ||
* @param {string} replicationMode - should be one of "dr" or "performance" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔤 alphabetized 🤓 |
||
* @param {boolean} canEnablePrimary - if the capabilities allow the user to enable a primary cluster, parent getter returns capabilities based on type (i.e. "dr" or "performance") | ||
* @param {boolean} canEnableSecondary - if the capabilities allow the user to enable a secondary cluster, parent getter returns capabilities based on type (i.e. "dr" or "performance") | ||
* @param {boolean} performanceMode - should be "primary", "secondary", or "disabled". If enabled, form will show a warning when attempting to enable DR secondary | ||
* @param {Promise} onSuccess - (optional) callback called after successful replication enablement. Must be a promise. | ||
* @param {boolean} doTransition - (optional) if provided, passed to onSuccess callback to determine if a transition should be done | ||
* | ||
*/ | ||
export default class EnableReplicationFormComponent extends Component { | ||
@service version; | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,40 @@ | ||||||||||
/** | ||||||||||
* Copyright (c) HashiCorp, Inc. | ||||||||||
* SPDX-License-Identifier: BUSL-1.1 | ||||||||||
*/ | ||||||||||
|
||||||||||
import Component from '@glimmer/component'; | ||||||||||
|
||||||||||
/** | ||||||||||
* @module PageModeIndex | ||||||||||
* | ||||||||||
* @example | ||||||||||
* <Page::ModeIndex | ||||||||||
* @cluster={{this.model}} | ||||||||||
* @onEnableSuccess={{this.onEnableSuccess}} | ||||||||||
* @replicationDisabled={{this.replicationForMode.replicationDisabled} | ||||||||||
* @replicationMode={{this.replicationMode}} | ||||||||||
* /> | ||||||||||
* | ||||||||||
* @param {model} cluster - cluster route model | ||||||||||
* @param {function} onEnableSuccess - callback after enabling is successful, handles transition if enabled from the top-level index route | ||||||||||
* @param {boolean} replicationDisabled - whether or not replication is enabled | ||||||||||
* @param {string} replicationMode - should be "dr" or "performance" | ||||||||||
*/ | ||||||||||
export default class PageModeIndex extends Component { | ||||||||||
canEnable = (type) => { | ||||||||||
const { cluster, replicationMode } = this.args; | ||||||||||
let perm; | ||||||||||
if (replicationMode === 'dr') { | ||||||||||
// returns canEnablePrimaryDr or canEnableSecondaryDr | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added these comments so future devs see they are used here if performing a global search for the capabilities defined in the route vault/ui/lib/replication/addon/routes/application.js Lines 57 to 60 in 6efc65a
|
||||||||||
perm = `canEnable${type}Dr`; | ||||||||||
} | ||||||||||
if (replicationMode === 'performance') { | ||||||||||
// returns canEnablePrimaryPerformance or canEnableSecondaryPerformance | ||||||||||
perm = `canEnable${type}Performance`; | ||||||||||
} | ||||||||||
// if there's a problem checking capabilities, default to true | ||||||||||
// since the backend can gate as a fallback | ||||||||||
return cluster[perm] ?? true; | ||||||||||
}; | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,4 +8,24 @@ import { tracked } from '@glimmer/tracking'; | |
|
||
export default class ReplicationIndexController extends ReplicationModeBaseController { | ||
@tracked modeSelection = 'dr'; | ||
|
||
getPerm(type) { | ||
if (this.modeSelection === 'dr') { | ||
// returns canEnablePrimaryDr or canEnableSecondaryDr | ||
return `canEnable${type}Dr`; | ||
} | ||
if (this.modeSelection === 'performance') { | ||
Comment on lines
+13
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to explicitly check for each mode instead of do a ternary statement since that'd mean if there wasn't a mode we'd fallback on a whichever was the falsy capability which didn't feel like the most stable thing to do. |
||
// returns canEnablePrimaryPerformance or canEnableSecondaryPerformance | ||
return `canEnable${type}Performance`; | ||
} | ||
} | ||
|
||
// if there's a problem checking capabilities, default to true | ||
// since the backend will gate as a fallback | ||
get canEnablePrimary() { | ||
return this.model[this.getPerm('Primary')] ?? true; | ||
} | ||
get canEnableSecondary() { | ||
return this.model[this.getPerm('Secondary')] ?? true; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ | |
|
||
import { service } from '@ember/service'; | ||
import { setProperties } from '@ember/object'; | ||
import { hash } from 'rsvp'; | ||
import Route from '@ember/routing/route'; | ||
import ClusterRoute from 'vault/mixins/cluster-route'; | ||
|
||
|
@@ -14,6 +13,23 @@ export default Route.extend(ClusterRoute, { | |
store: service(), | ||
auth: service(), | ||
router: service(), | ||
capabilities: service(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about not using the new service here so that it more cleanly backported to older versions (this only exists in 1.18 so far), BUT the service is nice because we only make one request for all of these capabilities instead of a request for each path which I think is a win! 🙌 |
||
|
||
async fetchCapabilities() { | ||
const enablePath = (type, cluster) => `sys/replication/${type}/${cluster}/enable`; | ||
const perms = await this.capabilities.fetchMultiplePaths([ | ||
enablePath('dr', 'primary'), | ||
enablePath('dr', 'primary'), | ||
enablePath('performance', 'secondary'), | ||
enablePath('performance', 'secondary'), | ||
]); | ||
return { | ||
canEnablePrimaryDr: perms[enablePath('dr', 'primary')].canUpdate, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. originally I wrote all these variables as |
||
canEnableSecondaryDr: perms[enablePath('dr', 'primary')].canUpdate, | ||
canEnablePrimaryPerformance: perms[enablePath('performance', 'secondary')].canUpdate, | ||
canEnableSecondaryPerformance: perms[enablePath('performance', 'secondary')].canUpdate, | ||
}; | ||
}, | ||
|
||
beforeModel() { | ||
if (this.auth.activeCluster.replicationRedacted) { | ||
|
@@ -29,21 +45,21 @@ export default Route.extend(ClusterRoute, { | |
return this.auth.activeCluster; | ||
}, | ||
|
||
afterModel(model) { | ||
return hash({ | ||
canEnablePrimary: this.store | ||
.findRecord('capabilities', 'sys/replication/primary/enable') | ||
.then((c) => c.canUpdate), | ||
canEnableSecondary: this.store | ||
.findRecord('capabilities', 'sys/replication/secondary/enable') | ||
.then((c) => c.canUpdate), | ||
}).then(({ canEnablePrimary, canEnableSecondary }) => { | ||
setProperties(model, { | ||
canEnablePrimary, | ||
canEnableSecondary, | ||
}); | ||
return model; | ||
async afterModel(model) { | ||
const { | ||
canEnablePrimaryDr, | ||
canEnableSecondaryDr, | ||
canEnablePrimaryPerformance, | ||
canEnableSecondaryPerformance, | ||
} = await this.fetchCapabilities(); | ||
|
||
setProperties(model, { | ||
canEnablePrimaryDr, | ||
canEnableSecondaryDr, | ||
canEnablePrimaryPerformance, | ||
canEnableSecondaryPerformance, | ||
}); | ||
return model; | ||
}, | ||
actions: { | ||
refresh() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wording feels strange? Open to alternatives!