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

Add denylist ip config for datasource endpoint #573

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

heemin32
Copy link
Collaborator

@heemin32 heemin32 commented Oct 27, 2023

Description

Add a denylist ip config for datasource endpoint so that admin can block certain ip addresses from being used in datasource endpoint
This PR mimicked opensearch-project/sql#2042

When IP address is in the deny list, following message will be returned.

{
	"error": {
		"root_cause": [
			{
				"type": "illegal_argument_exception",
				"reason": "Given endpoint is blocked by deny list in cluster setting plugins.geospatial.ip2geo.datasource.endpoint.denylist"
			}
		],
		"type": "illegal_argument_exception",
		"reason": "Given endpoint is blocked by deny list in cluster setting plugins.geospatial.ip2geo.datasource.endpoint.denylist"
	},
	"status": 400
}

Issues Resolved

N/A

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@heemin32 heemin32 added backport 2.x Backport PR to 2.x branch backport 2.11 Backport to 2.11 branch labels Oct 27, 2023
@heemin32 heemin32 force-pushed the denylist branch 3 times, most recently from 3ec949f to a2b5c47 Compare October 27, 2023 05:00
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #573 (726837f) into main (059b10d) will increase coverage by 0.44%.
The diff coverage is 83.78%.

@@             Coverage Diff              @@
##               main     #573      +/-   ##
============================================
+ Coverage     88.59%   89.03%   +0.44%     
- Complexity      753      767      +14     
============================================
  Files            92       93       +1     
  Lines          2717     2746      +29     
  Branches        221      223       +2     
============================================
+ Hits           2407     2445      +38     
+ Misses          230      221       -9     
  Partials         80       80              
Files Coverage Δ
...patial/ip2geo/action/RestPutDatasourceHandler.java 94.11% <100.00%> (+0.78%) ⬆️
...ial/ip2geo/action/RestUpdateDatasourceHandler.java 92.30% <100.00%> (+3.41%) ⬆️
...earch/geospatial/ip2geo/common/Ip2GeoSettings.java 94.44% <100.00%> (+1.11%) ⬆️
...opensearch/geospatial/ip2geo/dao/GeoIpDataDao.java 87.50% <100.00%> (+0.09%) ⬆️
...l/ip2geo/jobscheduler/DatasourceUpdateService.java 91.42% <100.00%> (+0.08%) ⬆️
...opensearch/geospatial/plugin/GeospatialPlugin.java 90.90% <100.00%> (+0.21%) ⬆️
...h/geospatial/ip2geo/common/URLDenyListChecker.java 64.70% <64.70%> (ø)

... and 5 files with indirect coverage changes

@heemin32 heemin32 force-pushed the denylist branch 2 times, most recently from 85ecc6e to f5a7664 Compare October 27, 2023 05:45
@navneet1v
Copy link
Collaborator

navneet1v commented Oct 27, 2023

@heemin32 what is the value of the denylist urls? Not able to find anything in this PR?

@heemin32
Copy link
Collaborator Author

@heemin32 what is the value of the denylist urls? Not able to find anything in this PR?

For example, in cloud provider, they can maintain denylist so that customer cannot make request to internal services with escalated privilege because cluster is running in service account but not customer account.

@naveentatikonda
Copy link
Member

@heemin32 Build is failing with the latest changes. Pls fix it

Copy link
Member

@vamshin vamshin 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

@vibrantvarun
Copy link
Member

Just reviewed the PR to understand the use-case.

@heemin32 heemin32 merged commit 35edec1 into opensearch-project:main Oct 27, 2023
13 of 15 checks passed
@heemin32 heemin32 deleted the denylist branch October 27, 2023 17:45
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 27, 2023
Signed-off-by: Heemin Kim <[email protected]>
(cherry picked from commit 35edec1)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 27, 2023
Signed-off-by: Heemin Kim <[email protected]>
(cherry picked from commit 35edec1)
heemin32 added a commit that referenced this pull request Oct 27, 2023
Signed-off-by: Heemin Kim <[email protected]>
(cherry picked from commit 35edec1)

Co-authored-by: Heemin Kim <[email protected]>
heemin32 added a commit that referenced this pull request Oct 27, 2023
Signed-off-by: Heemin Kim <[email protected]>
(cherry picked from commit 35edec1)

Co-authored-by: Heemin Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport PR to 2.x branch backport 2.11 Backport to 2.11 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants