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 CLI 'st2 apikey load' idempotence and API '/api/v1/apikeys' not honoring 'ID' for the new record creation #4542

Merged
merged 6 commits into from
Feb 12, 2019

Conversation

arm4b
Copy link
Member

@arm4b arm4b commented Feb 8, 2019

We've had this problem before and it seems a regression:

Reproduce

$ st2 apikey list --detail --show-secrets --yaml > apikeys.yaml
$ st2 apikey delete ...
$ st2 apikey load apikeys.yaml
$ st2 apikey load apikeys.yaml

Description

At a high level PR fixes st2api key load that wasn't idempotent and failing to import same record twice with the following error:

HTTPError: 409 Client Error: Conflict
MESSAGE: Tried to save duplicate unique keys (E11000 duplicate key error collection: st2.api_key_d_b index: key_hash_1 dup key: { : "903b7f0761094265969761443f4f25fc87c780b6248c38ce8dbaadfa52a2b53fd72381ac141072cb53ae78ff02e16fdfc0e40001472b9f25585a7b2864c501db" }) for url: http://127.0.0.1:9101/v1/apikeys

Investigation

Looking at st2api key load behavior, we do several requests to guarantee command idempotence:

  1. Try to check if ST2 API key ID exists
  2. If inexistent, - create a new record with needed ID

However result of request 2. goes wrong:

# -------- begin 140602007925392 request ----------
curl -X POST -H  'Connection: keep-alive' -H  'Accept-Encoding: gzip, deflate' -H  'Accept: */*' -H  'User-Agent: python-requests/2.14.2' -H  'content-type: application/json' -H  'X-Auth-Token: 80fdee5a251c40ac9599afa889dbdce7' -H  'Content-Length: 231' --data-binary '{"key_hash": "903b7f0761094265969761443f4f25fc87c780b6248c38ce8dbaadfa52a2b53fd72381ac141072cb53ae78ff02e16fdfc0e40001472b9f25585a7b2864c501db", "metadata": {}, "enabled": true, "user": "st2admin", "id": "58e3f3330c0517062a3fda43"}' http://127.0.0.1:9101/v1/apikeys
# -------- begin 140602007925392 response ----------
{
    "uid": "api_key:903b7f0761094265969761443f4f25fc87c780b6248c38ce8dbaadfa52a2b53fd72381ac141072cb53ae78ff02e16fdfc0e40001472b9f25585a7b2864c501db",
    "created_at": "2019-02-08T19:50:15.500100Z",
    "enabled": true,
    "user": "st2admin",
    "key": null,
    "id": "5c5ddd776cb8de530e0a1391",
    "metadata": {}
}
# -------- end 140602007925392 response ------------

Instead of creating a record with the requested ID, it creates a new one with random ID and so next import st2 apikey load for the same data fails.

This leads to a root cause bug fixed in this PR: API POST /api/v1/apikeys wasn't creating new record with the provided ID: https://api.stackstorm.com/api/v1/apikeys/#/api_key_controller.post


Was spot during the StackStorm/stackstorm-k8s#45 work.

…ovided ID

At a high level fixes `st2api key load` that wasn't idempotent and failing to import same file twice with:
```
HTTPError: 409 Client Error: Conflict
MESSAGE: Tried to save duplicate unique keys (E11000 duplicate key error collection: st2.api_key_d_b index: key_hash_1 dup key: { : "903b7f0761094265969761443f4f25fc87c780b6248c38ce8dbaadfa52a2b53fd72381ac141072cb53ae78ff02e16fdfc0e40001472b9f25585a7b2864c501db" }) for url: http://127.0.0.1:9101/v1/apikeys
```
because it couldn't import records with the requested ID.
@arm4b arm4b changed the title Fix st2api key load failing to import the same record Fix CLI 'st2 apikey load' idempotence and API '/api/v1/apikeys' not honoring 'ID' for the new record creation Feb 8, 2019
@Kami Kami added this to the 2.10.2 milestone Feb 9, 2019
@Kami
Copy link
Member

Kami commented Feb 9, 2019

Good catch 👍

Can we please also add a test case for it to avoid regression again in the future? :)

@arm4b
Copy link
Member Author

arm4b commented Feb 10, 2019

Right, that's the plan per status:missing tests tag.

@arm4b arm4b requested a review from Kami February 12, 2019 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants