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

db_import with keyauth_credentials fails on re-run #12288

Closed
1 task done
ahalay opened this issue Jan 3, 2024 · 9 comments
Closed
1 task done

db_import with keyauth_credentials fails on re-run #12288

ahalay opened this issue Jan 3, 2024 · 9 comments
Labels

Comments

@ahalay
Copy link

ahalay commented Jan 3, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Kong version ($ kong version)

Kong 3.4.0 (Kong 3.3.1 is not affected)

Current Behavior

When I try to run db_import a second time with the same config file, it fails with an error
(I use it for initial configuration and run it in k8s init container before further configuration via decK)

errors:

kong container:

Error: Failed importing:
[postgres] UNIQUE violation detected on '{key="very-secret-key"}'

postgresql container:

ERROR:  duplicate key value violates unique constraint "keyauth_credentials_ws_id_key_unique"
DETAIL:  Key (ws_id, key)=(a6a09e1f-9c1f-415d-918f-5384f5cfdd0c, very-secret-key) already exists.
STATEMENT:  INSERT INTO "keyauth_credentials" ("id", "created_at", "consumer_id", "key", "tags", "ws_id", "ttl")
VALUES ('6a309a66-d35e-4c0a-a325-d8911bb32057', TO_TIMESTAMP(1704296692) AT TIME ZONE 'UTC', 'b703da88-a6ae-5c0e-b775-0643b04b74df', 'very-secret-key', NULL, 'a6a09e1f-9c1f-415d-918f-5384f5cfdd0c', NULL)
ON CONFLICT ("id") DO UPDATE
SET "created_at" = EXCLUDED."created_at", "consumer_id" = EXCLUDED."consumer_id", "key" = EXCLUDED."key", "tags" = EXCLUDED."tags", "ws_id" = EXCLUDED."ws_id", "ttl" = EXCLUDED."ttl"
RETURNING "id", EXTRACT(EPOCH FROM "created_at" AT TIME ZONE 'UTC') AS "created_at", "consumer_id", "key", "tags", "ws_id",FLOOR(EXTRACT(EPOCH FROM ("ttl" AT TIME ZONE 'UTC' - CURRENT_TIMESTAMP AT TIME ZONE 'UTC'))) AS "ttl";

Expected Behavior

Kong version 3.3 allows you to run db_import multiple times with updating existing database records

Steps To Reproduce

Fails on a second run of kong config db_import kong.yaml

kong.yaml:

_format_version: "3.0"
_transform: true

services:
- name: admin-api
  url: http://localhost:8001
  plugins:
  - name: acl
    config:
      allow:
      - admin-api-group
      hide_groups_header: true
  routes:
  - name: admin-api
    hosts:
    - admin-api
    paths:
    - /admin-api
    plugins:
    - name: key-auth
      config:
        hide_credentials: true

consumers:
- username: admin-api-user
  acls:
  - group: admin-api-group
  keyauth_credentials:
  - key: very-secret-key

Anything else?

No response

poelzi added a commit to poelzi/kong that referenced this issue Feb 13, 2024
fixes Kong#12288

The upsert DAO function generated uses the primary
key as an "ON CONFLICT" match. This might not be the case
if a field uses a unique flag or if that field is also
workgroupable. Postgresql requires an index to be specified.

I tried autodetecting the correct behavior but it failed
on other models. Changing the primary key also did not work.
I added a way to specify the conflict index.
It defaults to the primary key.
poelzi added a commit to poelzi/kong that referenced this issue Feb 21, 2024
fixes Kong#12288

The upsert DAO function generated uses the primary
key as an "ON CONFLICT" match. This might not be the case
if a field uses a unique flag or if that field is also
workgroupable. Postgresql requires an index to be specified.

I tried autodetecting the correct behavior but it failed
on other models. Changing the primary key also did not work.
I added a way to specify the conflict index.
It defaults to the primary key.
@hanshuebner
Copy link
Contributor

@ahalay Thank you for reporting this issue. As you have seen in PR #12597, the underlying issue is difficult to fix. We think of db_import primarily as an import tool, with DecK being the solution for updates. As you're using DecK already, can you maybe avoid running the db_import step multiple times or reset the database before you run it?

@hanshuebner hanshuebner added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Mar 4, 2024
@ahalay
Copy link
Author

ahalay commented Mar 4, 2024

Thanks for the info @hanshuebner

Unfortunately this is something we'd really like to avoid, as we haven't found a more convenient way than db_import for our needs. We use it to initially configure route and keyauth for Kong's admin-api without exposing admin port, and then configure everything else through the admin-api route with decK. Dropping records from the database in our case can have unexpected consequences if there are multiple replicas (admin-api could left without keyauth or will not be recreated at all), as we run db_import as an initContainer before Kong's own container for each replica.

@hanshuebner
Copy link
Contributor

Given that a complete solution is not in sight, would it be possible to perform a check on the database to see if it needs to receive its initial payload using db_import in your initContainer?

@ahalay
Copy link
Author

ahalay commented Mar 4, 2024

Yes, it is possible, but it deprives us of the possibility of updating the value of this keyauth, whose uniqueness the function complains about.

@BastiNBG85
Copy link

BastiNBG85 commented Mar 6, 2024

we have exactly the same problem. We also use db_import after the automated migration steps in order to overwrite the admin-api route and the admin credentials (this will avoid locking out the user on accidently removing the route / user / api-key). I just wanted to upgrade from 3.3.0 to 3.6.1 and had the same issues. Using deck would not be a solution as we would have a "chicken-and-egg-problem" when something is accidently removed. so this is a kind of bypass ensuring that we always have access to our productive system (we just need to reboot the container and have access again. we have no other access to the commandline in order to change something as this is managed by our oc4 provider).

@hanshuebner
Copy link
Contributor

We'll see how we can address this issue. (KAG-4044)

@hanshuebner
Copy link
Contributor

We have discussed this internally once more and also reviewed #12597. It is unfortunately not straightforward to completely fix the issue, given that PostgreSQL only allows one ON CONFLICT clause and furthermore, IDs of rows that are being inserted are created in Lua land. The latter, however, shows a way how the problem can be avoided: When the IDs of the entities that are inserted are specified, the existing ON CONFLICT clauses will fire and correctly convert the INSERT into an upsert if the entity already exists.

Would you be able to specify the IDs of your entities in your YAML files?

consumers:
- username: admin-api-user
  acls:
  - group: admin-api-group
  keyauth_credentials:
  - id: 01E83305-9489-4232-8E4E-D7DD5FC6901F
    key: very-secret-key

@ahalay
Copy link
Author

ahalay commented Mar 15, 2024

Yes, I think this option should work, as far as I understand in case an existing record will have a different id it can be safely updated in the database itself before db_import. Thanks for your help.

@chronolaw chronolaw removed the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Mar 15, 2024
@StarlightIbuki
Copy link
Contributor

Closing as this is a non-issue and the reporter seems to have a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants