SECURITY-59: Add GHA workflows by Xiangs18 · Pull Request #63 · kbase/search_api2

Xiangs18 · 2025-04-28T23:33:03Z

No description provided.

+    if: github.base_ref == 'develop' && github.event.pull_request.merged == false
+    uses: kbase/.github/.github/workflows/reusable_build.yml@main
+    secrets: inherit
+  build-develop-merge:


+    if: github.base_ref == 'develop' && github.event.pull_request.merged == true
+    uses: kbase/.github/.github/workflows/reusable_build-push.yml@main
+    with:
+      name: '${{ github.event.repository.name }}-develop'
+      tags: pr-${{ github.event.number }},latest
+    secrets: inherit
+  build-main-open:


+    if: (github.base_ref == 'main' || github.base_ref == 'master') && github.event.pull_request.merged == false
+    uses: kbase/.github/.github/workflows/reusable_build-push.yml@main
+    with:
+      name: '${{ github.event.repository.name }}'
+      tags: pr-${{ github.event.number }}
+    secrets: inherit
+  build-main-merge:


+    if: (github.base_ref == 'main' || github.base_ref == 'master') && github.event.pull_request.merged == true
+    uses: kbase/.github/.github/workflows/reusable_build-push.yml@main
+    with:
+      name: '${{ github.event.repository.name }}'
+      tags: pr-${{ github.event.number }},latest-rc
+    secrets: inherit
+  trivy-scans:


+    if: (github.base_ref == 'develop' || github.base_ref == 'main' || github.base_ref == 'master' ) && github.event.pull_request.merged == false
+    uses: kbase/.github/.github/workflows/reusable_trivy-scans.yml@main
+    secrets: inherit


+    uses: kbase/.github/.github/workflows/reusable_validate-branch.yml@main
+    with:
+      build_branch: '${{ github.event.release.target_commitish }}'
+  validate-release-tag:


+    needs: check-source-branch
+    uses: kbase/.github/.github/workflows/reusable_validate-release-tag.yml@main
+    with:
+      release_tag: '${{ github.event.release.tag_name }}'
+  build-push:


+    needs: validate-release-tag
+    uses: kbase/.github/.github/workflows/reusable_build-push.yml@main
+    with:
+      name: '${{ github.event.repository.name }}'
+      tags: '${{ github.event.release.tag_name }},latest'
+    secrets: inherit


github-advanced-security · 2025-04-30T19:47:18Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

codecov · 2025-05-06T20:01:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e17f604) to head (efa12c8).
Report is 37 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop       #63   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        21    +1     
  Lines          658       660    +2     
=========================================
+ Hits           658       660    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Xiangs18 · 2025-05-06T20:29:07Z

self note:

- ensure integration tests pass locally
- add some readmes in jsonrpc directories explaining what's going on, why they're vendored, and where the code came from

Xiangs18 · 2025-05-13T22:55:00Z

MrCreosote · 2025-05-13T23:06:33Z

Standard question about boilerplate GHA yamls

standard boilerplate GHA yamls

MrCreosote · 2025-05-13T23:12:05Z

not reviewing the vendored code here, assuming it's an exact C&P. If that's not true LMK

MrCreosote · 2025-05-13T23:13:24Z

not reviewing the vendored code here, assuming it's an exact C&P. If that's not true LMK

I did add one line to fix the failing unit test; otherwise, it would raise a KeyError.

search_api2/jsonrpcbase/main.py

Line 59 in 4fd352a

-32003: 'Elasticsearch response error',

That suggests we're using the wrong version of this library, if there's a missing handler for an internal exception

yeah, so the last commit for jsonrpcbase is alpha5: https://github.com/kbaseincubator/jsonrpcbase/commits/master/

But this codebase is using alpha6

... never mind the fact that the codebase is using something labeled with an alpha at all

#63 (comment)

MrCreosote · 2025-05-13T23:15:23Z

-        container_process.send_signal(signal.SIGTERM)
+
+        # Stop and remove containers
+        cwd = 'tests/integration/docker'
+        subprocess.run("docker-compose --ansi never down", shell=True, check=True, cwd=cwd)


Why this change?

This was because the old code could not successfully shut down the container, which caused errors in the integration tests. My current code fixes this problem.

What changed such that the container no longer shuts down correctly?

I'm not sure what you're asking. I didn't change anything that would cause the container to stop shutting down. It simply doesn't shut down when I start running the integration tests.

Presumably the integration tests worked at some point in the past. That means that something changed so that the prior method of shutting down the container stopped working. I'm wondering what that change was.

Yes, it needs to be logger.warning; otherwise, the unit test would not pass either.

We need to switch to docker-compose down approach. It was designed for this.

The reason I'm continuing to ask questions is that the service should respond to a sigterm. What we're seeing is that it's not, and (I assume) the reason docker compose down is working is that it's sending a sigterm, waiting 10 seconds, and then sending a sigkill.

All that being said, this is pretty good evidence that I was wrong, and the devs did allow the tests to pass without the service responding to a sigterm: 728b712

As such, can you make an issue noting that the service / container needs to be updated to respond to sigterm correctly, and then go ahead with the docker compose down fix for now

Without the or change, does it time out and print the warning?

If I change logger.warning to Exception in the unit tests, it will raise a timeout error. Now it kind of justifies why logger.warning is used in the unit tests.

Oh great, there's a commit to verify my comment!
#63 (comment)

fixed by docker compose to shut down the container/service
issue created: #64

MrCreosote · 2025-05-13T23:15:59Z

-        if not common.wait_for_line("container.err",
-                                    lambda line: 'Stopping' in line and 'done' in line,
+        if not common.wait_for_line("container.out",
+                                    lambda line: "exited with code 0" in line,


Same question - why the change?

Running docker-compose down shuts the containers down gracefully, as confirmed by the presence of the "exited with code 0" message in the container.out file.

Sending a sigterm also shuts the containers down gracefully. If a sigterm isn't enough, then docker is sending a sigkill which isn't a graceful shutdown

When using container_process.send_signal(signal.SIGTERM), Docker will not automatically send a SIGKILL if the container does not stop. The behavior you described applies to docker stop and docker-compose.

Right, what I'm saying is that the container shouldn't require a sigkill to stop - it should respond to just a sigterm

#63 (comment)

MrCreosote · 2025-05-13T23:18:13Z

            assert res['error']
            assert res['error']['code'] == -32003
-            assert res['error']['message'] == 'Server error'
+            assert res['error']['message'] == 'Elasticsearch response error'


Why did this change? This seems like a bug that should be fixed in the search api rather than adjusting the tests. I'm guessing an exception changed and is no longer being caught correctly?

Based on the README documentation, -32003 represents an "Elasticsearch response error." This is also asserted on line 52. Additionally, I traced the code, and if it's correct, it should be raising an Elasticsearch response error.

search_api2/src/search2_rpc/service.py

Line 34 in e17f604

raise ElasticsearchError(resp.text)

I think this is probably related to #63 (comment)

The code in this PR is a revision behind that in the requirements toml file. I'd guess this is fixed in in the alpha 6 release

Where is the Alpha6 release? Alpha5 is the vendored repository that we are currently using. Am i missing something?

Search requires alpha 6, not alpha 5: https://github.com/kbase/search_api2/blob/develop/pyproject.toml#L14

As far as where that code lives, none of the branches in the jsonrpcbase repo look promising since they're all behind master. I'd extract it from pypi and update the readme to explain where the code came from

Extracted jsonrpc alpha6 from PyPI, and the unit test passed.

Did you have to make any changes or is it just strict C&P now?

This is a direct copy with no changes. The only difference between alpha5 and alpha6 is the line 'message': RPC_ERRORS.get(code, 'Server error') in the main.py file.

MrCreosote · 2025-05-13T23:19:14Z

 def test_search_objects_private_and_public_counts(service):
-    assert_counts(service, 1, 1, 12)
+    assert_counts(service, 1, 1, 21)


 def test_search_objects_private_counts(service):
-    assert_counts(service, 1, 0, 5)
+    assert_counts(service, 1, 0, 12)


 def test_search_objects_public_counts(service):
-    assert_counts(service, 0, 1, 9)
+    assert_counts(service, 0, 1, 11)


Just to be sure and leave a record: The data returned from the search api is a super set of the objects and workspaces in the response files in this repo, right?

I verified that the data returned from the search API is a superset of the objects and workspaces in the response files (case-03-response.json) in this repository.

… integration tests

…ning

MrCreosote

Note to self: all comments above this review are resolved

MrCreosote · 2025-05-16T23:25:44Z

+Simple JSON-RPC service without transport layer.
+
+This repository includes a vendored copy of [jsonrpcbase](https://github.com/kbaseincubator/jsonrpcbase) to resolve dependency conflicts.
+
+Specifically, this is version kbase-jsonrpcbase 0.3.0a6, extracted from [pypi](https://pypi.org/project/kbase-jsonrpcbase/)
+
+The original repository was only used by this project, so the code has been brought in directly to simplify maintenance and eliminate external dependency issues.


Suggested change

Simple JSON-RPC service without transport layer.

This repository includes a vendored copy of [jsonrpcbase](https://github.com/kbaseincubator/jsonrpcbase) to resolve dependency conflicts.

Specifically, this is version kbase-jsonrpcbase 0.3.0a6, extracted from [pypi](https://pypi.org/project/kbase-jsonrpcbase/)

The original repository was only used by this project, so the code has been brought in directly to simplify maintenance and eliminate external dependency issues.

Simple JSON-RPC service without transport layer.

This repository includes a vendored copy of [jsonrpcbase](https://pypi.org/project/kbase-jsonrpcbase/) version `0.3.0a6` to resolve dependency conflicts.

Note that there exists a [github repo](https://github.com/kbaseincubator/jsonrpcbase) which is presumably the source of the PyPi module, but it appears to only contain the alpha5 release of the code vs. PyPi's alpha6. It is not clear where the alpha6 code resides other than PyPi,

The original repository was only used by this project, so the code has been brought in directly to simplify maintenance and eliminate external dependency issues.

MrCreosote · 2025-05-16T23:30:05Z

        logger.info('Waiting until service has stopped...')

-        if not common.wait_for_line("container.err",
-                                    lambda line: 'Stopping' in line and 'done' in line,
+        if not common.wait_for_line("container.out",
+                                    lambda line: "exited with code 0" in line,
                                    timeout=stop_timeout,
                                    line_count=1):
            raise Exception(f'Container did not stop in the alloted time of {stop_timeout} seconds')


Since subprocess run will block until docker compose down is complete, I don't think all this checking code does anything anymore. It won't run until the down is finished

I believe this check is useful and necessary because it verifies that the containers are actually shutting down.

They must be down at this point. Run blocks until the command exits, and check ensures that the exit code is 0. If not, an exception will be thrown. This code can only be reached if the containers are down

Interestingly the tests are no longer passing. I don't see how the changes you made could've caused that...

removed .common

MrCreosote · 2025-05-16T23:31:16Z

        logger.info('Waiting until service has stopped...')
-        if not common.wait_for_line("container.err",
-                                    lambda line: 'Stopping' in line and 'done' in line,
+        if not common.wait_for_line("container.out",
+                                    lambda line: "exited with code 0" in line,
                                    timeout=stop_timeout,
                                    line_count=2):
+
+            # Use logger.warning here to allow tests to pass in CI.
+            # Note: Containers shut down properly when run locally, but may not behave the same in CI environments.
            logger.warning(f'Container did not stop in the alotted time of {stop_timeout} seconds')


Same comment as above

MrCreosote

LGTM. @bio-boris do you want to review?

Xiangs18 added 2 commits April 28, 2025 16:27

add codeql and dependabot

20318c4

add standard workflows

d757a40

github-advanced-security AI found potential problems Apr 29, 2025

View reviewed changes

Xiangs18 added 5 commits April 29, 2025 12:36

update test.yml file

c77e348

update python version in pyproject.toml and update lock file

586e111

add --no-root flag to fix package issue

cb38726

update dockerfile

4c7540e

fix docker build error

860cede

Xiangs18 added 14 commits April 30, 2025 13:06

switch back to jsonrpcbase

60c19df

copy jsonrpc11base over

268545f

revert previous jsonrpc changes

36047d6

comment mypy

c2d35ba

copy over jsonrpcbase

48a2198

remove jsonrpcbase dep

3233636

update pytest, pytest-cov dep

392362a

update test.yml

5be7663

update requests dep

87f6454

add debug for failed test

2ad4f69

make pytest fail early

fc9a2b3

use docker compose

b127c62

get all tests running fast

d513b80

fix unit tests

e0bcc93

remove coverage dep

f1159c9

Xiangs18 changed the title ~~[WIP] SECURITY-59: Add GHA workflows~~ SECURITY-59: Add GHA workflows May 6, 2025

Xiangs18 added 3 commits May 13, 2025 11:36

fix container cannot stop issue

572cdbc

update docker-compose file

a899501

fix failed tests

40d548e

Xiangs18 added 2 commits May 13, 2025 12:07

update integration-testing.md

2054361

add readme file for vendored repo

4fd352a

Xiangs18 requested review from MrCreosote and bio-boris May 13, 2025 22:57

MrCreosote reviewed May 13, 2025

View reviewed changes

Xiangs18 added 5 commits May 15, 2025 10:38

fix and test containers are correctly shutting downs in both unit and…

82bd335

… integration tests

extend stop_timeout

dcd0381

add timeout flag

8561fe2

remove version from docker-compose file and revert back to logger.war…

4044d9e

…ning

use kbase-jsonrpcbase 0.3.0a6 and update readme.md file

cccb89b

Xiangs18 mentioned this pull request May 16, 2025

The service / container needs to be updated to respond to sigterm correctly #64

Open

update changelog

542b9dc

Xiangs18 requested a review from MrCreosote May 16, 2025 21:37

MrCreosote reviewed May 16, 2025

View reviewed changes

Xiangs18 added 3 commits May 16, 2025 16:52

update jsonrpc readme.md file

623dad4

clean up stop_serivce function

8cf55e5

remove unused dep

efa12c8

MrCreosote reviewed May 17, 2025

View reviewed changes

bio-boris approved these changes May 19, 2025

View reviewed changes

Xiangs18 merged commit 23ec7c1 into develop May 19, 2025
12 checks passed

Xiangs18 deleted the dev-add_workflows branch May 19, 2025 17:53

Conversation

Xiangs18 commented Apr 28, 2025

Uh oh!

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

github-advanced-security AI commented Apr 30, 2025

Uh oh!

codecov Bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Xiangs18 commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xiangs18 commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrCreosote May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xiangs18 May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrCreosote May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 6, 2025 •

edited

Loading

Xiangs18 commented May 6, 2025 •

edited

Loading

MrCreosote May 14, 2025 •

edited

Loading

Xiangs18 May 13, 2025 •

edited

Loading

MrCreosote May 14, 2025 •

edited

Loading