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

Fix moment.js license expiration parsing #19014

Closed
wants to merge 1 commit into from
Closed

Fix moment.js license expiration parsing #19014

wants to merge 1 commit into from

Conversation

rhoboat
Copy link

@rhoboat rhoboat commented May 11, 2018

Attempting to fix #18388

I think this is actually in nanoseconds (see comments).

@rhoboat rhoboat requested a review from epixa May 11, 2018 20:25
@rhoboat rhoboat self-assigned this May 11, 2018
@rhoboat rhoboat added bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels May 11, 2018
@@ -116,11 +116,13 @@ export class XPackInfo {
path: '/_xpack'
});

const millis = parseInt(get(response, 'license.expiry_date_in_millis') / 1000000);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an ES bug if the value for expiry_date_in_millis is in microseconds.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rhoboat rhoboat added the review label May 18, 2018
@rhoboat
Copy link
Author

rhoboat commented May 18, 2018

@epixa ready for review. I checked with es team and got the sense that it's meant to be "never". but they didn't get back to me on whether it's enough to just say "never" if the license type is 'basic'. Is there a scenario where it could be basic but we want to flag a problem with an invalid date? Either way, this is written so that we show "never" when both the given expiry_date_in_millis to be unparseable by moment and the license is basic.

pinging @tvernum here in case he can get back to me now with more context.

_getExpiryDate(xpackInfo) {
let expiryDate = moment(get(xpackInfo, 'license.expiry_date_in_millis')).format('YYYY MMM DD HH:mm');

if (expiryDate === 'Invalid date' && get(xpackInfo, 'license.mode') === 'basic') {
Copy link
Author

Choose a reason for hiding this comment

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

this.license.getType() doesn't work here. is this the right way to check for basic license??

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tvernum
Copy link
Contributor

tvernum commented May 18, 2018

You'll need @tbrooks8 to confirm, I was just the messenger.

@epixa
Copy link
Contributor

epixa commented May 24, 2018

It's still not clear to me what is actually being returned by Elasticsearch. We need to confirm that it is indeed nanoseconds. If it's anything but milliseconds, then there's a bug in Elasticsearch. If this is a bug in Elasticsearch and we went forward with this PR as-is, then sometime down the line when that bug is fixed in Elasticsearch, the logic we're relying on in Kibana would disappear, and we'd start showing a ridiculously far into the future date. Even if this was acceptable, it's not what we intended, and that's not good.

Ideally Elasticsearch wouldn't be returning any timestamp at all. It may have been convenient from an engineering perspective to just throw in some super far into the future date, but we're telling everyone that basic licenses never expire, which doesn't seem to be accurate.

@Tim-Brooks
Copy link

We need to confirm that it is indeed nanoseconds.

It is milliseconds.

Ideally Elasticsearch wouldn't be returning any timestamp at all. It may have been convenient from an engineering perspective to just throw in some super far into the future date, but we're telling everyone that basic licenses never expire, which doesn't seem to be accurate.

The binary signature of licenses have a expiration date in milliseconds. There is not really a way around that decision that was made a long time ago. Generated basic licenses have a hardcoded expiration milliseconds of 9223372005318775807 (1 year before 2^63). The current logic is remove that from the rest response if that is the expiration milliseconds to prevent a confusing rest response.

If elasticsearch is returning 9223372005318776000 for something that seems to be a bug. We could use the steps from kibana that can reproduce this issue.

@Tim-Brooks
Copy link

I have identified an issue on the elasticsearch side and opened a PR (elastic/elasticsearch#30848). The /_xpack route uses a different serialization path then the get license rest route. So that route had to be modified too. That does not explain why the milliseconds is slightly different for you (9223372005318775807 vs 9223372005318776000). Is that the number that you consistently get? When I call the rest route prior to my fix I get the hard coded basic license expiration (9223372005318775807) which is what I expect.

@epixa
Copy link
Contributor

epixa commented May 24, 2018

@tbrooks8 Thanks a lot for clarifying. That all makes sense.

You can reproduce with a GET request to _xpack:

curl -XGET "http://localhost:9200/_xpack?pretty"

@epixa
Copy link
Contributor

epixa commented May 24, 2018

Ah yes, thanks. That is indeed the issue we had, so thanks a lot for identifying it and fixing it. Can you backport it to 6.3?

For what it's worth, I get 9223372005318775807 as you would expect.

@Tim-Brooks
Copy link

Yes. It will backported to 6.4 and 6.3

@epixa
Copy link
Contributor

epixa commented May 30, 2018

Closing this in favor of #19565

@epixa epixa closed this May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
6 participants