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.

Sign up for GitHub

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

Jump to bottom

Support zone ID in IPv6 addresses. Issue #37107 #37547

Closed
wants to merge 15 commits into from
Closed

Support zone ID in IPv6 addresses. Issue #37107 #37547

wants to merge 15 commits into from

Conversation

cshtdd
Copy link

@cshtdd cshtdd commented Jan 16, 2019

Issue #37107 describes problems with IPv6 addresses that contain a zone ID.

The scope of this PR is to ignore the zone ID if any.
This on itself will address the reported bug.

Keep in mind this is the first PR of a series. Subsequent PR will properly read the zone ID and construct IPv6 addresses correctly.

Here's a summary of the code changes

  • change ipStringToBytes to ignore anything after a percent
  • make sure percent does not appear before dot
  • make sure percent does not appear before colon
  • can only contain one percent symbol
  • make sure mixed IPv6IPv4 addresses support zoneId
  • test an address with dots and colons and percent
  • test an address with dots and percents
  • test an address with colons and percents
  • make sure percent does not appear right after dot
  • make sure percent does not appear right after colon

cshtdd added 10 commits January 16, 2019 07:24
@cshtdd
add some characterization tests to prove the address conversion outpu…
6da0f58 
…t can be converted back to its input
@cshtdd
create a test to reproduce the error from issue elastic#37107
4e71f6f 
and elastic/beats#9836
@cshtdd
better to create a separate test for the failing scenarion
2a8913f
@cshtdd
extract an overload of ipStringToBytes that builds the bytes
46104df
@cshtdd
change ipStringToBytes to ignore the zoneId
9ec96f3 
further commits will treat it accordingly
@cshtdd
correct typo that completely changes the meaning of the comment
d1516dd
@cshtdd
make sure IPv6 addresses with IPv4 components can contain a zoneId
b6ae23c
@cshtdd
add some validations to the zoneId delimiter parsing
1345cc4
@cshtdd
Merge branch 'master' into issue_37107
d06ba4d 
* master:
  Deprecate _type from LeafDocLookup (elastic#37491)
  Allow system privilege to execute proxied actions (elastic#37508)
  Update Put Watch to allow unknown fields (elastic#37494)
  AwaitsFix testAddNewReplicas
  SQL: Add protocol tests and remove jdbc_type from drivers response (elastic#37516)
  SQL: [Docs] Add an ES-SQL column for data types (elastic#37529)
  IndexMetaData#mappingOrDefault doesn't need to take a type argument. (elastic#37480)
  Simplify + Cleanup Dead Code in Settings (elastic#37341)
  Reject all requests that have an unconsumed body (elastic#37504)
  [Ml] Prevent config snapshot failure blocking migration (elastic#37493)
  Fix line length for aliases and remove suppression (elastic#37455)
  Add SSL Configuration Library (elastic#37287)
  SQL: Remove slightly used meta commands (elastic#37506)
  Simplify Snapshot Create Request Handling (elastic#37464)
  Remove the use of AbstracLifecycleComponent constructor elastic#37488 (elastic#37488)
  [ML] log minimum diskspace setting if forecast fails due to insufficient d… (elastic#37486)
@cshtdd
Merge branch 'master' into issue_37107
7347a72 
* master:
  SQL: Lowercase the datatypes in validation error msgs (elastic#37524)
  DedicatedClusterSnapshotRestoreIT to Zen2 (elastic#37489)
  SQL: Lowercase es data type (mapping) returned from SQL Commands (elastic#37531)
  Remove SYS CATALOGS leftover
  SQL: Describe aliases as views (elastic#37496)
@cshtdd cshtdd changed the title Ignore zone ID in IPv6 addresses. First Pull Request for Issue 37107 Ignore zone ID in IPv6 addresses. First Pull Request for Issue #37107 Jan 16, 2019
@cshtdd cshtdd changed the title Ignore zone ID in IPv6 addresses. First Pull Request for Issue #37107 Ignore zone ID in IPv6 addresses. First Pull Request for Issue https://github.com/elastic/elasticsearch/issues/37107 Jan 16, 2019
@cshtdd cshtdd changed the title Ignore zone ID in IPv6 addresses. First Pull Request for Issue https://github.com/elastic/elasticsearch/issues/37107 Ignore zone ID in IPv6 addresses. First Pull Request for Issue #37107 Jan 16, 2019
@cshtdd cshtdd mentioned this pull request Jan 16, 2019
Closed
@polyfractal polyfractal added the :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP label Jan 16, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@jakelandis
Copy link
Contributor

@camilin87 - thanks for the PR. However, until we address the comment here: #37107 (comment) we should hold off on this PR. I am hesitant to proceed with this change in the common library since it touches so much of the code base. In particular, I am concerned about data manipulation (loosing information) when indexed.

I will re-open a discussion about our support (or lack there-of) of zoneId.

cshtdd added 3 commits January 16, 2019 21:40
@cshtdd
correct failing tests in NetworkAddressTests that don't expect the zo…
a744c39 
…ne ID to be returned
@cshtdd
Merge branch 'master' into issue_37107
b075c7c 
* master:
  Change file descriptor limit to 65535 (elastic#37537)
  Increase timeout for testAddNewReplicas
@cshtdd
Merge branch 'issue_37107' into issue_37107_part2
a24a650 
* issue_37107:
  Change file descriptor limit to 65535 (elastic#37537)
  Increase timeout for testAddNewReplicas
@cshtdd
Copy link
Author

cshtdd commented Jan 17, 2019

I know this issue may need some discussion.
I updated the PR either way in case it adds some clarity.

That being said. I understand if after your discussions this issue is no longer relevant

The updated PR makes sure to read the zone ID

Here's a summary of the changes

  • test non zero zoneIds
    • test it gets read
    • extract a constant for zero
    • make sure it supports hex numbers
  • test the string conversion of non zero zone Id
    • make sure it supports hex numbers
  • fix other failing tests

@cshtdd cshtdd changed the title Ignore zone ID in IPv6 addresses. First Pull Request for Issue #37107 Support zone ID in IPv6 addresses. Issue #37107 Jan 17, 2019
@dakrone dakrone requested a review from jakelandis January 23, 2019 15:16
@jakelandis
Copy link
Contributor

jakelandis commented Jan 31, 2019 •
edited
Loading

@camilin87 - We appreciate your contribution here, and have discussed this internally. We landed on implementing an ingest node processor to help handle IPv6 zone_ids. I have logged #38064. I am going to close this PR as we heading a different direction. Feel free to pick up that issue if you like and ping me directly with question or reviews.

@jakelandis jakelandis closed this Jan 31, 2019
@iquirino
Copy link

Getting this issue on latest version (installation this weekend)

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

@jakelandis jakelandis Awaiting requested review from jakelandis

Assignees
No one assigned
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cshtdd @elasticmachine @jakelandis @iquirino @polyfractal