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

report Erlang and JS version in welcome message #3425

Merged
merged 1 commit into from Jul 13, 2021

Conversation

janl
Copy link
Member

@janl janl commented Mar 14, 2021

closes #3298

@iilyak
Copy link
Contributor

iilyak commented Mar 15, 2021

Currently /_node/<whatever>/_system returns dynamic information showing current load on the system. The spidermonkey version is a statically defined during compilation (in current implementation). One option is to add this information into welcome endpoint. However in this case it would be trickier to get the version for a given node. Not sure if it is a requirement for #3298.

I don't have strong opinion on this.

@janl
Copy link
Member Author

janl commented Mar 15, 2021

we could also add it to the / response, I don’t have a strong opinion about where we report it, as long as we can get it. I don’t mind reporting static things in _system, but I can be convinced otherwise.

@janl
Copy link
Member Author

janl commented Mar 15, 2021

come to think of it, might be worth getting the OTP version out that way as well, whichever we choose

@nickva
Copy link
Contributor

nickva commented Mar 15, 2021

Good idea about reporting the JS version, it might give users an idea which JS language features they could target (if they could map the runtime to JS features).

We do already report the OTP version in the welcome point as a header:

 $ http $DB
                                                                                                                                                                                                  
HTTP/1.1 200 OK
...
Server: CouchDB/3.0.0-d5fae35-dirty (Erlang OTP/20)
X-Couch-Request-ID: e766be1148
X-CouchDB-Body-Time: 0

{
    "couchdb": "Welcome",
    "features": [
        "access-ready",
        "fdb"
    ],
    "git_sha": "d5fae35",
    "uuid": "fake_uuid_for_dev",
    "vendor": {
        "name": "The Apache Software Foundation"
    },
    "version": "3.0.0-d5fae35-dirty"
}

Would it be too obscure to append the JS version to the Server along with the OTP version?

@janl
Copy link
Member Author

janl commented Mar 16, 2021

@nickva I remembered that we report that in the header but thought we might want to not add info to each request that never changes per server.

That kick-started a few more thoughts:

  1. some users treat build versions as security sensitive. This is of course folly, and I have to admit that it hasn’t come up in quite a while.
  2. What is the use of reporting these? The initial request is for diagnostic reasons, but there is another use case: client libraries adjusting behaviour based on what capabilities a server has.
  3. For diagnosis, one can easily argue that an admin-required endpoint is a natural choice (hence _system), but for the client-library use-case, we’d ideally want a public endpoint.
  4. I still think we shouldn’t add static info to each request, even if just 2-3 bytes.

Then I though we should just re-use the existing capabilities mechanism in /, but that gets us to the problem that existing capabilities are a list of strings. But now we’d have tagged strings, something like "otp-20.3.8.25", which folks then need to know how to parse. If we made them proper JSON, say {"otp":"20.3.8.25"}, then we might break capabilities parsers that expect strings.

So I think we should just add this to the server info JSON from /:

{
  "couchdb": "Welcome",
  "version": "3.1.1",
  "otp":"20.3.8.25",
  "spidermonkey": "68",
  "git_sha": "ce596c65d",
  "uuid": "d077378b841f6b2c5d2f9c6ae753c769",
  "features": [
    "access-ready",
    "partitioned",
    "pluggable-storage-engines",
    "reshard",
    "scheduler"
  ],
  "vendor": {
    "name": "The Apache Software Foundation"
  }
}

(the SpiderMonkey version should be a string, so we can express "1.8.5", and not have to switch this field between numbers and strings, and it makes us future-proof against version scheme changes)

That way:

  1. client libraries can get the server config once and behave accordingly,
  2. we don’t add any overhead to responses (we could even deprecate the existing OTP section of the Server header, but one thing at a time ;)
  3. diagnostics can get at the data easily as well

@wohali
Copy link
Member

wohali commented Mar 16, 2021

We added the git_sha without breaking anyone, so I see no reason not to also add other keys like spidermonkey.

I might use otp_version or otp_ver but that's really bikeshedding.

@wohali
Copy link
Member

wohali commented Mar 16, 2021

Also +1 to not using the capability strings, that's meant to advertise features this particular CouchDB is compatible with. I can see why that might be used for JS stuff (ECMAScript compatibility level, e.g.) but that's a translation from the JS version, not the actual SM JS version itself...and wouldn't make sense at all for the Erlang/OTP version.

@nickva
Copy link
Contributor

nickva commented Mar 16, 2021

@janl good point on not adding extra static info to all the headers, I hadn't thought of that

For javascript we might want to add an info object in case we switch away from spidermonkey to say v8 or other engine."javascript_engine": {"name": "spidermonkey", "version": "68"}, for V8 would it be {"name":"v8", "version": "1.0.0"} or something. Maybe "javascript": {...} or "js_engine" I am not very good at names :-)

@wohali good idea, if we know the ECMAScript version we could add it to the {..., "ecmascript_version": ...} to the info object too perahps.

@wohali wohali added this to the 3.2.0 milestone Mar 30, 2021
@janl janl force-pushed the feat/3298/report-sm-version-in-system branch 2 times, most recently from bb1754d to 335170e Compare April 4, 2021 13:37
@janl janl changed the title report sm version in system report Erlang and JS version in welcome message Apr 4, 2021
@janl
Copy link
Member Author

janl commented Apr 4, 2021

Added erlang_version and javascript_version to our welcome message:

{
  "couchdb": "Welcome",
  "version": "3.1.1-813ecd4-dirty",
  "git_sha": "813ecd4",
  "erlang_version": "23.2.7",
  "javascript_engine": {
    "name": "spidermonkey",
    "version": 86
  },
  "uuid": "fake_uuid_for_dev",
  "features": [
    "access-ready",
    "partitioned",
    "pluggable-storage-engines",
    "reshard",
    "scheduler"
  ],
  "vendor": {
    "name": "The Apache Software Foundation"
  }
}

@janl janl force-pushed the feat/3298/report-sm-version-in-system branch 2 times, most recently from cb0ca9d to 809fc53 Compare April 6, 2021 08:09
@nickva
Copy link
Contributor

nickva commented Apr 6, 2021

@janl looks good, I like having the extra info in there. But am wondering, since this is changing the top level API, if we should do a mailing list shout-out. I anticipate there being some resistance around "security" aspects or others may have other opinions on the shape of the results.

@rnewson
Copy link
Member

rnewson commented Apr 6, 2021

knowing some folks (cough IBM cough) would likely prefer not to reveal more version numbers, can we consider a config toggle?

@rnewson
Copy link
Member

rnewson commented Apr 6, 2021

and noting that the OTP version comes back in the Server response header already.

@janl
Copy link
Member Author

janl commented Apr 6, 2021

@nickva sure thing, will do

@rnewson happy to add a ibm_security_through_obscurity toggle ;)

Re the OTP version:

  1. we only send the major version, but that’s often not enough
  2. I opted against updating the Server header with more bytes that will always be the same for an installation.
  • I’d rather add an option to disable the header and save the bytes.

@rnewson
Copy link
Member

rnewson commented Apr 6, 2021

Ideally we'd address these things together. there are a few spots where couchdb shows "internal" information that is strictly unrelated to its role as a database (the couchdb version, erlang version, spidermonkey version, erlang node names, pids, etc).

I agree that hiding version numbers is a "security through obscurity" approach but I can't be the only person who has to regularly defend their presence in audits, etc. In my situation, I can and do argue that the couchdb version number signals compatibility and does not necessary mean we're running that verbatim code.

All this to say, perhaps it is time for a new admin-only endpoint into which we can put all the internal details instead. It is clearly useful to be able to interrogate the various version numbers through the http api, but users don't need that.

For the welcome message, I suggest showing something of value to users, namely "ES5" versus "ES6".

@janl
Copy link
Member Author

janl commented Apr 6, 2021

I’m coming at this mostly from a remote-diagnosis point of view, where we either poke a couch, or ask someone to poke their couch about info that we can then derive some info about its behaviour, so I’d want precise version number.

I don’t mind adding an admin-only endpoint for that (say /_node/*/_versions) instead of overloading the welcome message.

I’m less interested in maintaining a mapping between ES-version and software version, but if someone wants to take that on, I’m not going to veto it either :)

@@ -47,6 +47,11 @@ handle_welcome_req(#httpd{method='GET'}=Req, WelcomeMessage) ->
{couchdb, WelcomeMessage},
{version, list_to_binary(couch_server:get_version())},
{git_sha, list_to_binary(couch_server:get_git_sha())},
{erlang_verison, ?l2b(?COUCHDB_ERLANG_VERSION)},
Copy link
Member

Choose a reason for hiding this comment

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

Nit/typo:

Suggested change
{erlang_verison, ?l2b(?COUCHDB_ERLANG_VERSION)},
{erlang_version, ?l2b(?COUCHDB_ERLANG_VERSION)},

@janl janl force-pushed the feat/3298/report-sm-version-in-system branch 2 times, most recently from d953d2d to 61308a4 Compare July 11, 2021 07:51
@janl
Copy link
Member Author

janl commented Jul 11, 2021

Thank you all for the input. I’ve reworked this to show Erlang and SM versions under /_node/_local/_versions admin-only:

> curl -s a:a@127.0.0.1:15984/_node/_local/_versions | jq
{
  "erlang_version": "22.3.4.20",
  "javascript_engine": {
    "name": "spidermonkey",
    "version": "86"
  }
}

> curl -s 127.0.0.1:15984/_node/_local/_versions | jq    
{
  "error": "unauthorized",
  "reason": "You are not a server admin."
}

@nickva
Copy link
Contributor

nickva commented Jul 12, 2021

Looks pretty good @janl.

What do you think about also removing the Erlang version from server_header() in https://github.com/apache/couchdb/blob/3.x/src/couch/src/couch_httpd.erl#L1050

I can't think of a case when a client library might need to act differently if say Erlang version is 20 vs 24

@janl
Copy link
Member Author

janl commented Jul 12, 2021

@nickva I think since this is technically public-API, we’ll have to make it an option to disable and deprecate it, so we can remove it in 4.x, but no objections to that plan (in a separate PR)

@janl janl force-pushed the feat/3298/report-sm-version-in-system branch from 61308a4 to 80ea7b4 Compare July 12, 2021 19:50
@nickva
Copy link
Contributor

nickva commented Jul 13, 2021

@janl ah fair enough

@janl janl merged commit c42d5b9 into 3.x Jul 13, 2021
@janl janl deleted the feat/3298/report-sm-version-in-system branch July 13, 2021 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants