GET and DELETE method for managing api_key

[siddardh-ra]

This PR deals with some updation in the POST method with /api/v1/key?label=label where that label will be associated with the key generated

Added GET /api/v1/key to retrieve the user's keys and DELETE /api/v1/key/{key_id} to delete a key.

Modified the APIKey table with id field as primary_key.

[dbutenhof]

pbench.client.IncorrectParameterCount: API template API.KEY requires 1 parameters ({})

Ha. I ran into this myself recently, where a defined URI template parameter is optional. The PbenchServerClient._uri doesn't have any defined way to determine that, and is failing if there are no parameters provided. The easiest way to get around this is to change the client create_api_key method to

    response = self.post(api=API.KEY, {"key": ""}

That'll turn the /key/{key} template into /key/, which ought to work.

It's fairly easy to run the functional tests on your branch before submitting. I always do

jenkins/run tox server python
jenkins/runlocal --cleanup

(Although because I'm on RHEL 8 instead of RHEL 9 or Fedora, I need to prefix the runlocal with BASE_IMAGE=quay.io/centos/centos:stream9 ... but you probably don't!)

[dbutenhof]

One final comment. I didn't make an issue of this during the PR review; but I think I should have, and I'd really like everyone on the team to start thinking of this along with unit testing.

Please consider writing a functional test for this new code. I already created a functional test case to prove that I can create an API key and use it to authenticate a GET /datasets?mine, and it'd be easy to extend that test case (or about as easy to write a second test case) to show GET /key[{key}] as well as DELETE /key/{key}.

You might consider creating a test_api_key.py rather than adding more to test_put.py (which should probably be renamed to test_datasets.py or some such)... aside from the test case I already have, you could write test cases for API Key management that don't require any datasets and therefore don't need to be dependent on any of the existing tests.

But, aside from making the suggestion, I won't require changes for that at this point. (Although if you don't, a follow-up PR with functional tests would be great...)

[webbnh]

There are also a few lingering items from my earlier review.

• [x] Consider renaming the APIKey column, name, to description (and, Dave's contemplation that we should rename the api_key column to just key 🙂).
• [x] The unit test code is including the key "name" in the API key value when the corresponding code in the server does not.
• [x] The key deletion unit test would be better if there were more than one key in the database when we try to delete the target, because it could then show that the other key is still intact.
• [x] My questions (1, 2) about audit record values.

[riya-17]

  [2023-05-09 11:28:50] [build-stdout] [INFO] [2] Read 21 nodes and 1 edges from TSG output
  [2023-05-09 11:28:50] [build-stdout] [WARN] [2] Failed to analyse imports of /home/runner/work/pbench/pbench/agent/bench-scripts/test-bin/fio-histo-log-pctiles.py : Syntax Error (line 3)
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/modules.py", line 107, in py_ast
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/modules.py", line 101, in old_py_ast
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/parser/__init__.py", line 100, in parse
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmleFile "<string>", line 3
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/imports.py", line 89, in get_import_nodes
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/modules.py", line 119, in py_ast
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/modules.py", line 116, in py_ast
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmle/python/parser/tsg_parser.py", line 220, in parse
  [2023-05-09 11:28:50] [build-stdout] [TRACEBACK] [2] "semmleFile "<string>", line 3
  [2023-05-09 11:28:51] [build-stdout] [INFO] [2] Read 21 nodes and 1 edges from TSG output
  [2023-05-09 11:28:51] [build-stdout] [INFO] [2] Extracted file /home/runner/work/pbench/pbench/agent/bench-scripts/test-bin/fio-histo-log-pctiles.py in 1164ms```

Hey @siddardh-ra this must be the issue for Analyze (python) failure

[dbutenhof]

I'm assuming these errors are due to changes in the scanners. They both seem to be legitimate, but not very interesting to us... and definitely not related to your changes.

The "Javascript" code problem seems to be related to the Tool Meister Prometheus support:

Error: Extraction failed in /home/runner/work/pbench/pbench/agent/templates/prometheus.yml with error found character '%' that cannot start any token. (Do not use % for indentation)

I haven't investigated, but I'm assuming this means we're jinja2 processing to expand the list of tools we're scraping, and the code analyzer doesn't understand that it's intentionally not pure YML. Operationally I suspect that means this should be prometheus.yml.jinja2 or something.

That is to say, this has nothing to do with your change, and may be configuration changes in the code scanner. We should fix this with another PR; and we can either force your change in regardless of these annoying messages, figure out how to prevent the scanner from complaining about this, or wait for that "fix" to go in.

For the Python scanner, this seems to be

Warning: Extraction failed in /home/runner/work/pbench/pbench/agent/bench-scripts/test-bin/fio-histo-log-pctiles.py with error Syntax Error

This one appears to be because despite the .py file it's really a bash script. I presume was because something in the Agent tests refers to the file with the Python file type and the legacy tests are mocking some simplistic behavior. (It could as easily have been a Python that just did print statements.)

Again, we need to figure out how to either reconfigure the scanners to not care, or fix the root issue, perhaps, as I mentioned, by rewriting this mock as Python.

[npalaska]

Looks like it's only failing due to #3417.

It's merged now, Siddardh you can rebase on main now.

[siddardh-ra]

Looks like it's only failing due to #3417.

It's merged now, Siddardh you can rebase on main now.

Done. Rebased :+1: