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

Improve dynamic configurations by adding cache and simplifying client fetch #6364

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

tianleh
Copy link
Member

@tianleh tianleh commented Apr 8, 2024

Description

Address the comments from this issue #6300

This PR updates the parameters to the getConfigurationClient function and adds a cache.

Issues Resolved

closes #6300
closes #6341

Screenshot

Testing the changes

call the get API

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules'

{"value":"frame-ancestors https://example-site.org"}%

Then call the get API again.

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules'

{"value":"frame-ancestors https://example-site.org"}%

We find two entries in the logs where the first one doesn't show cache entry but the second one shows cache entry.

server    log   [02:13:58.836] [info][server][OpenSearchDashboards][http] http server running at http://localhost:5601
server    log   [02:14:04.440] [info][applicationConfig][plugins] Received a request to get entity config for csp.rules.
server    log   [02:14:04.441] [info][applicationConfig][plugins] Key csp.rules is not found from cache.
server    log   [02:14:07.936] [info][applicationConfig][plugins] Received a request to get entity config for csp.rules.

Verified that the CSP rules are set properly.
Screenshot 2024-04-09 at 19 00 10

call update API

curl 'http://localhost:5601/api/appconfig/csp.rules' -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"newValue":"script-src 'unsafe-eval' 'self'; worker-src blob: 'self'; style-src 'unsafe-inline' 'self'; frame-ancestors 'self' file://* filesystem:"}'

{"newValue":"script-src unsafe-eval self; worker-src blob: self; style-src unsafe-inline self; frame-ancestors self file://* 

call the get API

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules'

{"value":"script-src unsafe-eval self; worker-src blob: self; style-src unsafe-inline self; frame-ancestors self file://* filesystem:"}%

From the logs, we can see that cache value is used.

server    log   [02:21:34.446] [info][applicationConfig][plugins] Received a request to update entity csp.rules with new value script-src unsafe-eval self; worker-src blob: self; style-src unsafe-inline self; frame-ancestors self file://* filesystem:.
server    log   [02:21:39.821] [info][applicationConfig][plugins] Received a request to get entity config for csp.rules.

call the delete API

curl 'http://localhost:5601/api/appconfig/csp.rules' -X DELETE -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty'

{"deletedEntity":"csp.rules"}%


call the get API

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules'

{"statusCode":404,"error":"Not Found","message":"Response Error"}%

From the log, we can see that cache entry has been removed.

server    log   [02:22:10.642] [info][applicationConfig][plugins] Received a request to delete entity csp.rules.
server    log   [02:22:16.856] [info][applicationConfig][plugins] Received a request to get entity config for csp.rules.
server    log   [02:22:16.856] [info][applicationConfig][plugins] Key csp.rules is not found from cache.

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@tianleh
Copy link
Member Author

tianleh commented Apr 8, 2024

Send the functional part for early feedback. Will add more tests and polish code later.

@SuZhou-Joe

@tianleh
Copy link
Member Author

tianleh commented Apr 8, 2024

cc @seraphjiang for visibility

@tianleh tianleh added the v2.14.0 label Apr 8, 2024
@SuZhou-Joe
Copy link
Member

@tianleh Thanks for the PR and I think this PR address all the concerns in #6300 . Thanks for the quick update!

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.20%. Comparing base (85df662) to head (2a1fd11).
Report is 5 commits behind head on main.

❗ Current head 2a1fd11 differs from pull request most recent head 14e34ae. Consider uploading reports for the commit 14e34ae to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6364       +/-   ##
===========================================
- Coverage   55.58%   44.20%   -11.38%     
===========================================
  Files        1199     2762     +1563     
  Lines       24259    54606    +30347     
  Branches     4087     8595     +4508     
===========================================
+ Hits        13485    24141    +10656     
- Misses      10133    29022    +18889     
- Partials      641     1443      +802     
Flag Coverage Δ
Linux_1 32.65% <ø> (?)
Linux_4 34.96% <ø> (?)
Windows_2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@tianleh tianleh changed the title Improve dynamic config Improve dynamic configurations by adding cache and simplifying client fetch Apr 10, 2024
@tianleh tianleh marked this pull request as ready for review April 10, 2024 02:35
@tianleh
Copy link
Member Author

tianleh commented Apr 10, 2024

@tianleh Thanks for the PR and I think this PR address all the concerns in #6300 . Thanks for the quick update!

Thanks for the early feedback! Now the PR is polished to be ready for review. Could you please take a look? @SuZhou-Joe

@yubonluo
Copy link
Contributor

@tianleh, I have a question about getEntityConfig(entity: string): Promise<string>, why is the return value written as Promise<string>? In my a current requirement, I want to get an array such as ['a','b']. for example:

curl --insecure {osd endpoint}/api/appconfig/opensearchDashboards.dashboardAdmin.users -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"newValue":"['a','b']"}' -u 'admin:admin'
curl --insecure {osd endpoint}/api/appconfig/opensearchDashboards.dashboardAdmin.groups -u 'admin:admin' -X GET

In the current situation, I can only adapt manually.

@tianleh
Copy link
Member Author

tianleh commented Apr 10, 2024

@tianleh, I have a question about getEntityConfig(entity: string): Promise<string>, why is the return value written as Promise<string>? In my a current requirement, I want to get an array such as ['a','b']. for example:

curl --insecure {osd endpoint}/api/appconfig/opensearchDashboards.dashboardAdmin.users -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"newValue":"['a','b']"}' -u 'admin:admin'
curl --insecure {osd endpoint}/api/appconfig/opensearchDashboards.dashboardAdmin.groups -u 'admin:admin' -X GET

In the current situation, I can only adapt manually.

Good call out! Each use case which uses app config may have different input type. For simplicity, we just use a string as input and output. Then each use case can parse and stringify accordingly.

@tianleh
Copy link
Member Author

tianleh commented Apr 10, 2024

The failed one https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/8625666516/job/23642618463?pr=6364 is not related to my change and seems flaky.

Could anyone help re-run it?

@tianleh
Copy link
Member Author

tianleh commented Apr 10, 2024

@SuZhou-Joe All checks passed.

@tianleh
Copy link
Member Author

tianleh commented Apr 10, 2024

@kavilla

Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: Tianle Huang <[email protected]>
@tianleh
Copy link
Member Author

tianleh commented Apr 11, 2024

The commits seem out of sync. Fixing it now.

Signed-off-by: Tianle Huang <[email protected]>
Copy link
Member

@SuZhou-Joe SuZhou-Joe left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the refactor.

@tianleh
Copy link
Member Author

tianleh commented Apr 11, 2024

Tests

call the get API twice

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules'
{"value":"frame-ancestors https://example-site.org"}%

 curl -X GET 'http://localhost:5601/api/appconfig/csp.rules'
{"value":"frame-ancestors https://example-site.org"}%

We find two entries in the logs where the first one doesn't show cache entry but the second one shows cache entry.


server    log   [03:41:49.429] [info][applicationConfig][plugins] Received a request to get entity config for csp.rules.
server    log   [03:41:49.430] [info][applicationConfig][plugins] Key csp.rules is not found from cache.
server    log   [03:41:53.745] [info][applicationConfig][plugins] Received a request to get entity config for csp.rules.

Verified that the CSP rules are set properly.

Screenshot 2024-04-10 at 20 43 02

call update API


curl 'http://localhost:5601/api/appconfig/csp.rules' -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"newValue":"script-src 'unsafe-eval' 'self'; worker-src blob: 'self'; style-src 'unsafe-inline' 'self'; frame-ancestors 'self' file://* filesystem:"}'

{"newValue":"script-src unsafe-eval self; worker-src blob: self; style-src unsafe-inline self; frame-ancestors self file://* filesystem:"}

call the get API

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules'

{"value":"script-src unsafe-eval self; worker-src blob: self; style-src unsafe-inline self; frame-ancestors self file://* filesystem:"}%

From the logs, we can see that cache value is used.

server    log   [03:43:58.168] [info][applicationConfig][plugins] Received a request to update entity csp.rules with new value script-src unsafe-eval self; worker-src blob: self; style-src unsafe-inline self; frame-ancestors self file://* filesystem:.
server    log   [03:44:10.680] [info][applicationConfig][plugins] Received a request to get entity config for csp.rules.

call the delete API

curl 'http://localhost:5601/api/appconfig/csp.rules' -X DELETE -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty'
{"deletedEntity":"csp.rules"}%

call the get API

curl -X GET 'http://localhost:5601/api/appconfig/csp.rules'
{"statusCode":404,"error":"Not Found","message":"Response Error"}%

From the log, we can see that cache entry has been removed.

server    log   [03:46:38.886] [info][applicationConfig][plugins] Received a request to delete entity csp.rules.
server    log   [03:46:46.924] [info][applicationConfig][plugins] Received a request to get entity config for csp.rules.
server    log   [03:46:46.924] [info][applicationConfig][plugins] Key csp.rules is not found from cache.

The CSP default values are being used on UI.

Screenshot 2024-04-10 at 20 48 11

@ZilongX ZilongX merged commit cd857cb into opensearch-project:main Apr 11, 2024
66 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 11, 2024
… fetch (#6364)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
(cherry picked from commit cd857cb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 11, 2024
… fetch (#6364)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
(cherry picked from commit cd857cb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
yubonluo pushed a commit to yubonluo/OpenSearch-Dashboards that referenced this pull request Apr 11, 2024
… fetch (opensearch-project#6364)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: yubonluo <[email protected]>
# Conflicts:
#	CHANGELOG.md
#	src/plugins/application_config/server/types.ts
#	src/plugins/csp_handler/server/csp_handlers.test.ts
#	src/plugins/csp_handler/server/csp_handlers.ts
SuZhou-Joe pushed a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 11, 2024
… fetch (opensearch-project#6364) (#324)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
Signed-off-by: yubonluo <[email protected]>
# Conflicts:
#	CHANGELOG.md
#	src/plugins/application_config/server/types.ts
#	src/plugins/csp_handler/server/csp_handlers.test.ts
#	src/plugins/csp_handler/server/csp_handlers.ts

Co-authored-by: Tianle Huang <[email protected]>
zhongnansu pushed a commit that referenced this pull request Apr 11, 2024
… fetch (#6364) (#6404)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
(cherry picked from commit cd857cb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
zhongnansu pushed a commit that referenced this pull request Apr 11, 2024
… fetch (#6364)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
(cherry picked from commit cd857cb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
zhongnansu pushed a commit that referenced this pull request Apr 11, 2024
… fetch (#6364)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
(cherry picked from commit cd857cb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
zhongnansu pushed a commit that referenced this pull request Apr 12, 2024
… fetch (#6364)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
(cherry picked from commit cd857cb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
zhongnansu pushed a commit that referenced this pull request Apr 15, 2024
… fetch (#6364)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
(cherry picked from commit cd857cb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
zhongnansu pushed a commit that referenced this pull request Apr 15, 2024
… fetch (#6364) (#6403)

* Improve dynamic config

Signed-off-by: Tianle Huang <[email protected]>

* reset yml

Signed-off-by: Tianle Huang <[email protected]>

* bring back previous changes

Signed-off-by: Tianle Huang <[email protected]>

---------

Signed-off-by: Tianle Huang <[email protected]>
(cherry picked from commit cd857cb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[RFC] Proposal for Adding Cache in App Config [Application config] Usage and performance optimization
4 participants