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

Raft Snapshot Download Bug #17769

Merged
merged 3 commits into from Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/17769.txt
@@ -0,0 +1,3 @@
```release-note:bug
ui: Fixes issue with not being able to download raft snapshot via service worker
```
24 changes: 23 additions & 1 deletion ui/app/components/raft-storage-overview.js
Expand Up @@ -5,6 +5,8 @@ import { inject as service } from '@ember/service';

export default Component.extend({
flashMessages: service(),
auth: service(),

useServiceWorker: null,

async init() {
Expand All @@ -16,12 +18,32 @@ export default Component.extend({
if ('serviceWorker' in navigator) {
// this checks to see if there's an active service worker - if it failed to register
// for any reason, then this would be null
let worker = await navigator.serviceWorker.getRegistration(config.serviceWorkerScope);
const worker = await navigator.serviceWorker.getRegistration(config.serviceWorkerScope);
if (worker) {
navigator.serviceWorker.addEventListener('message', this.serviceWorkerGetToken.bind(this));

this.set('useServiceWorker', true);
}
}
},
willDestroy() {
if (this.useServiceWorker) {
navigator.serviceWorker.removeEventListener('message', this.serviceWorkerGetToken);
}
this._super(...arguments);
},

serviceWorkerGetToken(event) {
const { action } = event.data;
const [port] = event.ports;

if (action === 'getToken') {
port.postMessage({ token: this.auth.currentToken });
Copy link
Contributor

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?

Copy link
Contributor Author

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.

} else {
console.error('Unknown event', event); // eslint-disable-line
port.postMessage({ error: 'Unknown request' });
}
},

actions: {
async removePeer(model) {
Expand Down
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

So I think the only issue here is that the handler in init won't be called on the initial installation of the worker. This handler gets called when the worker is registered: https://github.com/DockYard/ember-service-worker/blob/4564e495666865b80fd29daf15784e2801b59d1b/service-worker-registration/index.js#L13 - so the check in the init function will only be truthy on subsequent tries.

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 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 🤔.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 😅 !

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
Expand Down
45 changes: 42 additions & 3 deletions ui/tests/integration/components/raft-storage-overview-test.js
Expand Up @@ -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) {
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 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
});
});