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

feat: check database version compatibility #3310

Merged
merged 3 commits into from
Mar 27, 2018
Merged

Conversation

thibaultcha
Copy link
Member

@thibaultcha thibaultcha commented Mar 17, 2018

Add a check to the DAO's init() method to ensure the compatibility of the underlying database (PostgreSQL or Cassandra). This check is ran:

  1. In the kong start command
  2. In init_by_lua

We support "minimum recommended version", and an optional "oldest deprecated version", which as of today and 0.12.3 are:

PostgreSQL

  • current minimum recommended: 9.5
  • deprecated: 9.4

Cassandra

  • current minimum recommended: 2.2
  • deprecated: 2.1

When using a deprecated datastore, the CLI shows:

$ bin/kong start                 
2018/03/16 17:38:18 [warn] Currently using PostgreSQL 9.4 which is considered deprecated, please use 9.5 or greater
Kong started

It is also shown in the nginx logs when starting Kong from nginx (e.g. Docker images).


When using a version that is below the minimum recommended, and below the oldest deprecated one, we error out from the CLI (and from init_by_lua):

$ bin/kong start
Error: Kong requires PostgreSQL 9.5 or greater (currently using 9.4)

  Run with --v (verbose) or --vv (debug) for more details

A new method, 'check_version_compat()', takes 2 arguments: min and
deprecated.

Given that 'init()' has been called and a version number is now part of
the instance's attributes, this function will compare its arguments to
it.

Version numbers are compared with the 'version.lua' module at:

    https://github.com/Kong/version.lua

- When the current version is above 'min', we succeed.
- When the current version is below 'min'
    - If 'deprecated' is given, and the current version is above it,
      a deprecation warning is logged.
    - Otherwise, we error, since the current version is below the
      minimum recommended, and below the oldest deprecated version.
As of 0.12.3:

PostgreSQL
- current minimum recommended: 9.5
- deprecated: 9.4

Cassandra
- current minimum recommended: 2.2
- deprecated: 2.1

Versions below MIN and DEPRECATED will result in an error.
@bungle
Copy link
Member

bungle commented Mar 19, 2018

Should there be some way to start Kong in case the version is below the oldest deprecated one. I mean I would not want to end up with situation with many old Windows software installers that just refuse to install because of a faulty check (even if the software was just fine with newer version of Windows). Also there might be PostgreSQL / Cassandra compatible client interface in some other databases that just report something different as a version (and limiting someone trying those seems to add very little).

@thibaultcha
Copy link
Member Author

thibaultcha commented Mar 20, 2018

Should there be some way to start Kong in case the version is below the oldest deprecated one.

I would be wary of potentially corrupting the database, but well, if the option is obvious enough that it implies danger, why not, we could.

Also there might be PostgreSQL / Cassandra compatible client interface in some other databases that just report something different as a version

Good point, especially with implementations like ScyllaDB.

Maybe a --ignore-db-compat flag? Or --no-db-check?

@hishamhm
Copy link
Contributor

I mean I would not want to end up with situation with many old Windows software installers that just refuse to install because of a faulty check (even if the software was just fine with newer version of Windows).

@bungle we would run into that if we were checking for a maximum version, but for minimum that is unlikely to happen.

Also there might be PostgreSQL / Cassandra compatible client interface in some other databases that just report something different as a version

Good point, especially with implementations like ScyllaDB.

It is a valid point, indeed, but personally I'd rather avoid the additional "moving part" of such a dangerous flag. For Kong CE, the adventurous user can always edit kong/constants.lua and force the version (and if we get a report that our version check is failing inappropriately for something like ScyllaDB, we could always adjust it accordingly if we decide to support it). But I'd hate to see a scenario where we're helping Support debug some mysterious issue only to discover after a few hours that the customer has been using --ignore-db-compat all along. 😱

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

love this PR!

end

local db_v = version.version(db_infos.version)
local min_v = version.version(min)
Copy link
Member

Choose a reason for hiding this comment

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

you can update the 'version.lua' library to 1.0 and change those into version(min) without the double 'version' name

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I saw; but as usual when possible, we'll make that a separate contribution

@thibaultcha thibaultcha added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/please review labels Mar 27, 2018
@thibaultcha thibaultcha merged commit eecdf22 into master Mar 27, 2018
@thibaultcha thibaultcha deleted the feat/check-db-compat branch March 27, 2018 02:08
thibaultcha added a commit that referenced this pull request Apr 24, 2018
This fixes a regression introduced in #3310.

If the database is unreachabel when Kong start, it is our policy that
the error message be prefixed with `[postgres error]` (in the case of
PostgreSQL) to help the user troubleshoot the issue.

Prior to this patch, this behavior was changed in #3310, since it
introduced a call to `init()` in the CLI. Users would see:
```
$ kong start
Error: connection refused
```

Instead of:
```
$ kong start
Error: [postgres error] could not retrieve server_version: connection refused
```
thibaultcha added a commit that referenced this pull request Apr 24, 2018
This fixes a regression introduced in #3310.

If the database is unreachabel when Kong start, it is our policy that
the error message be prefixed with `[postgres error]` (in the case of
PostgreSQL) to help the user troubleshoot the issue.

Prior to this patch, this behavior was changed in #3310, since it
introduced a call to `init()` in the CLI. Users would see:
```
$ kong start
Error: connection refused
```

Instead of:
```
$ kong start
Error: [postgres error] could not retrieve server_version: connection refused
```

From #3411
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants