Skip to content

Add allowed ip check#605

Merged
marc-gr merged 8 commits intoelastic:masterfrom
marc-gr:allowed-ip-check
Dec 14, 2021
Merged

Add allowed ip check#605
marc-gr merged 8 commits intoelastic:masterfrom
marc-gr:allowed-ip-check

Conversation

@marc-gr
Copy link
Contributor

@marc-gr marc-gr commented Dec 2, 2021

To identify any IP in the testing value that is not in the supported Maxmind's allowed set (found here https://github.com/elastic/elastic-package/blob/master/docs/howto/ingest_geoip.md) a validation check is added at test time.

Will output something similar to

0] parsing field value failed: the IP "107.170.139.210" is not one of the allowed test IPs
[1] parsing field value failed: the IP "96.241.146.97" is not one of the allowed test IPs

@marc-gr marc-gr requested a review from mtojek December 2, 2021 11:11
@elasticmachine
Copy link
Collaborator

elasticmachine commented Dec 2, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-12-14T12:04:02.293+0000

  • Duration: 56 min 33 sec

  • Commit: 610ebaa

Test stats 🧪

Test Results
Failed 0
Passed 605
Skipped 0
Total 605

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@mtojek mtojek requested a review from a team December 2, 2021 11:52
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

It looks like a good PR, but I'm concerned about system tests. See the test output:

one or more errors found in documents stored in metrics-aws.ec2_metrics-ep data stream: [0] parsing field value failed: the IP "3.84.55.9" is not one of the allowed test IPs

@marc-gr marc-gr requested a review from mtojek December 2, 2021 14:55
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I think the only one left thing is adjusting pipeline tests?

@mtojek
Copy link
Contributor

mtojek commented Dec 3, 2021

I think you have to update pipeline tests for the AWS test package.

@marc-gr
Copy link
Contributor Author

marc-gr commented Dec 13, 2021

/test

@marc-gr marc-gr mentioned this pull request Dec 13, 2021
@mtojek mtojek self-requested a review December 14, 2021 12:17
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

LGTM

@marc-gr marc-gr merged commit 228a493 into elastic:master Dec 14, 2021
@marc-gr marc-gr deleted the allowed-ip-check branch December 14, 2021 13:51
func initializeAllowedIPsList() map[string]struct{} {
m := map[string]struct{}{
"0.0.0.0": {}, "255.255.255.255": {},
"0:0:0:0:0:0:0:0": {}, "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

If this testing takes output from the net package, this should also probably have "::" since an IPv6 address renders as the abbreviation.

}

if v.enabledAllowedIPCheck && !v.isAllowedIPValue(valStr) {
return fmt.Errorf("the IP %q is not one of the allowed test IPs", valStr)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if this emitted a link to the list of acceptable IPs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants