-
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
Raft Snapshot Download Bug #17769
Raft Snapshot Download Bug #17769
Changes from all commits
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: Fixes issue with not being able to download raft snapshot via service worker | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,6 @@ | ||
import { addSuccessHandler } from 'ember-service-worker/service-worker-registration'; | ||
import Namespace from '@ember/application/namespace'; | ||
|
||
function getToken() { | ||
// fix this later by allowing registration somewhere in the app lifecycle were we can have access to | ||
// services, etc. | ||
return Namespace.NAMESPACES_BY_ID['vault'].__container__.lookup('service:auth').currentToken; | ||
} | ||
|
||
addSuccessHandler(function (registration) { | ||
// attach the handler for the message event so we can send over the auth token | ||
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. So I think the only issue here is that the handler in 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 was wondering about that too. I was thinking since the event listener for the message is added when the component initializes then it would be listening before the user has a chance to click the download action 🤔. 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. OK I think you're right - I looked at the file I linked again (lol), and it runs when the JS package loads, so it should be installed by the time you get to the download page (or by the time it renders). Sorry it's been a little bit since I looked at this 😅 ! 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. Great thanks for talking through it with me! This is the first time I have looked at that part of the code and wanted to make sure I am understanding things. |
||
navigator.serviceWorker.addEventListener('message', (event) => { | ||
let { action } = event.data; | ||
let port = event.ports[0]; | ||
|
||
if (action === 'getToken') { | ||
let token = getToken(); | ||
if (!token) { | ||
console.error('Unable to retrieve Vault tokent'); // eslint-disable-line | ||
} | ||
port.postMessage({ token: token }); | ||
} else { | ||
console.error('Unknown event', event); // eslint-disable-line | ||
port.postMessage({ | ||
error: 'Unknown request', | ||
}); | ||
} | ||
}); | ||
|
||
// attempt to unregister the service worker on unload because we're not doing any sort of caching | ||
window.addEventListener('unload', function () { | ||
registration.unregister(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,17 +2,56 @@ import { module, test } from 'qunit'; | |
import { setupRenderingTest } from 'ember-qunit'; | ||
import { render } from '@ember/test-helpers'; | ||
import hbs from 'htmlbars-inline-precompile'; | ||
import sinon from 'sinon'; | ||
|
||
module('Integration | Component | raft-storage-overview', function (hooks) { | ||
setupRenderingTest(hooks); | ||
|
||
test('it renders', async function (assert) { | ||
let model = [ | ||
hooks.beforeEach(function () { | ||
this.model = [ | ||
{ address: '127.0.0.1:8200', voter: true }, | ||
{ address: '127.0.0.1:8200', voter: true, leader: true }, | ||
]; | ||
this.set('model', model); | ||
}); | ||
|
||
test('it renders', async function (assert) { | ||
await render(hbs`<RaftStorageOverview @model={{this.model}} />`); | ||
assert.dom('[data-raft-row]').exists({ count: 2 }); | ||
}); | ||
|
||
test('it should download snapshot via service worker', async function (assert) { | ||
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 couldn't come up with a nice way to test this end to end with the service worker in an appreciable amount of time so I added a test for the component to ensure that it's doing its part. |
||
assert.expect(3); | ||
|
||
const token = this.owner.lookup('service:auth').currentToken; | ||
const generateMockEvent = (action) => ({ | ||
data: { action }, | ||
ports: [ | ||
{ | ||
postMessage(message) { | ||
const getToken = action === 'getToken'; | ||
const expected = getToken ? { token } : { error: 'Unknown request' }; | ||
assert.deepEqual( | ||
message, | ||
expected, | ||
`${ | ||
getToken ? 'Token' : 'Error' | ||
} is returned to service worker in message event listener callback` | ||
); | ||
}, | ||
}, | ||
], | ||
}); | ||
|
||
sinon.stub(navigator.serviceWorker, 'getRegistration').resolves(true); | ||
sinon.stub(navigator.serviceWorker, 'addEventListener').callsFake((name, cb) => { | ||
assert.strictEqual(name, 'message', 'Event listener added for service worker message'); | ||
cb(generateMockEvent('getToken')); | ||
cb(generateMockEvent('unknown')); | ||
}); | ||
|
||
await render(hbs`<RaftStorageOverview @model={{this.model}} />`); | ||
// avoid clicking the download button or the url will change | ||
// the service worker invokes the event listener callback when it intercepts the request to /v1/sys/storage/raft/snapshot | ||
// for the test we manually fire the callback as soon as it is passed to the addEventListener stub | ||
}); | ||
}); |
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.
Do we still want to account for when/if the token isn't retrieved?
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.
It doesn't look like the original implementation was doing anything other than logging an error to the console. If there is no token the request would result in a 403. I'm not sure in practice if this would occur now since the user would be redirected to the auth route before the component is instantiated.