NETWORKING: http.publish_host Should Contain CNAME by original-brownbear · Pull Request #32806 · elastic/elasticsearch

original-brownbear · 2018-08-13T12:45:28Z

Restores old 2.x behaviour of prepending the hostname to http.publish_host if available
closes Should http.publish_host resolve the CNAME configured. #22029

* Closes #22029

elasticmachine · 2018-08-13T12:45:30Z

Pinging @elastic/es-core-infra

jasontedor

After discussion with @elastic/es-clients we need to take a more careful approach here as this change will break some of the official language clients. Instead, we should:

in 6.x we will retain the current behavior of not including the hostname in the publish address
however, we will deprecation warn if the publish address would include the hostname with the behavior that we are proposing in #32806
in 6.x we will introduce a system property that will enable users that want the hostname in the publish address to do so by using this system property; this system property can only be set to true and not to false as the system property is unneeded if the user wants to retain the current behavior
in 7.0.0 we will change the behavior to include the hostname in the publish address (if available)
in 7.x we can deprecation warn that the system property no longer has any impact

original-brownbear · 2018-08-30T11:33:38Z

@jasontedor sounds good :) => on it :)

original-brownbear · 2018-08-30T12:24:52Z

@jasontedor done I think, implemented the 6.x behavior here now. I guess the 7.0 change goes into a separate PR right?

Transferring review ownership to @tbrooks8

Tim-Brooks

I'm not super familiar with the behavior we are looking for here. But this looks alright to me in regards to the API response / deprecation.

However, it is trivial to make it have some weird behavior in regards to IPv6.

public void testCorrectDisplayPublishedIpv6() throws Exception {
        InetAddress localhost = InetAddress.getByName(NetworkAddress.format(InetAddress.getByName("0:0:0:0:0:0:0:1")));
        int port = 9200;
        assertPublishAddress(
            new HttpInfo(
                new BoundTransportAddress(
                    new TransportAddress[]{new TransportAddress(localhost, port)},
                    new TransportAddress(localhost, port)
                ), 0L, true
            ), NetworkAddress.format(localhost) + ':' + port
        );
    }

org.junit.ComparisonFailure: expected:<[::1]:9200> but was:<[0:0:0:0:0:0:0:1/[::1]]:9200>

Are we okay that IPv6 address will be in the host string section if the comparison fails due to address compression?

@jasontedor

I don't see an easy fix for that. We could always add like a new field for v7.0. But that introduces some backwards compatibility work.

original-brownbear · 2018-09-11T08:13:16Z

@tbrooks8 fixed the test case you pointed out now :) We already had a utility method (probably copied from Guava) that can identify whether or not a String is a valid IP (4 and 6) => just used that => works fine :)

Tim-Brooks

LGTM

original-brownbear · 2018-09-12T04:15:26Z

@tbrooks8 thanks!

* master: (43 commits) [HLRC][ML] Add ML put datafeed API to HLRC (elastic#33603) Update AWS SDK to 1.11.406 in repository-s3 (elastic#30723) Expose CCR stats to monitoring (elastic#33617) [Docs] Update match-query.asciidoc (elastic#33610) TEST: Adjust rollback condition when shard is empty [CCR] Improve shard follow task's retryable error handling (elastic#33371) Forbid negative `weight` in Function Score Query (elastic#33390) Clarify context suggestions filtering and boosting (elastic#33601) Disable CCR REST endpoints if CCR disabled (elastic#33619) Lower version on full cluster restart settings test Upgrade remote cluster settings (elastic#33537) NETWORKING: http.publish_host Should Contain CNAME (elastic#32806) Add test coverage for global checkpoint listeners Reset replica engine to global checkpoint on promotion (elastic#33473) HLRC: ML Delete Forecast API (elastic#33526) Remove debug logging in full cluster restart tests (elastic#33612) Expose CCR to the transport client (elastic#33608) Mute testIndexDeletionWhenNodeRejoins SQL: Make Literal a NamedExpression (elastic#33583) [DOCS] Adds missing built-in user information (elastic#33585) ...

* master: (128 commits) [HLRC][ML] Add ML put datafeed API to HLRC (elastic#33603) Update AWS SDK to 1.11.406 in repository-s3 (elastic#30723) Expose CCR stats to monitoring (elastic#33617) [Docs] Update match-query.asciidoc (elastic#33610) TEST: Adjust rollback condition when shard is empty [CCR] Improve shard follow task's retryable error handling (elastic#33371) Forbid negative `weight` in Function Score Query (elastic#33390) Clarify context suggestions filtering and boosting (elastic#33601) Disable CCR REST endpoints if CCR disabled (elastic#33619) Lower version on full cluster restart settings test Upgrade remote cluster settings (elastic#33537) NETWORKING: http.publish_host Should Contain CNAME (elastic#32806) Add test coverage for global checkpoint listeners Reset replica engine to global checkpoint on promotion (elastic#33473) HLRC: ML Delete Forecast API (elastic#33526) Remove debug logging in full cluster restart tests (elastic#33612) Expose CCR to the transport client (elastic#33608) Mute testIndexDeletionWhenNodeRejoins SQL: Make Literal a NamedExpression (elastic#33583) [DOCS] Adds missing built-in user information (elastic#33585) ...

* Follow up to #32806 setting the setting to true for 7.x

* NETWORKING: http.publish_host Should Contain CNAME * Closes #22029

Relates #elastic#32806 Closes elastic#39970

This commit adds CNAME reporting for transport.publish_address same way it's done for http.publish_address. Relates #32806 Relates #39970

This commit adds CNAME reporting for transport.publish_address same way it's done for http.publish_address. Relates #32806 Relates #39970 (cherry picked from commit e0a2558)

Follow up on #32806. The system property es.http.cname_in_publish_address is deprecated starting from 7.0.0 and deprecation warning should be added if the property is specified. This commit goes to 7.x and master. Follow-up PR to remove es.http.cname_in_publish_address property completely will go to the master.

Follow up on #32806. The system property es.http.cname_in_publish_address is deprecated starting from 7.0.0 and deprecation warning should be added if the property is specified. This PR will go to 7.x and master. Follow-up PR to remove es.http.cname_in_publish_address property completely will go to the master. (cherry picked from commit a5ceca7)

…5616) Follow up on elastic#32806. The system property es.http.cname_in_publish_address is deprecated starting from 7.0.0 and deprecation warning should be added if the property is specified. This PR will go to 7.x and master. Follow-up PR to remove es.http.cname_in_publish_address property completely will go to the master.

original-brownbear added 2 commits August 13, 2018 13:10

NETWORKING: http.publish_host Should Contain CNAME

2fe6c67

* Closes #22029

add tests

ace1f45

original-brownbear added >bug WIP :Distributed/Network Http and internode communication implementations v7.0.0 v6.5.0 labels Aug 13, 2018

Merge remote-tracking branch 'elastic/master' into 22029

2b888b5

original-brownbear removed the WIP label Aug 13, 2018

original-brownbear requested a review from jasontedor August 13, 2018 21:30

jbaiera mentioned this pull request Aug 17, 2018

Modify address parsing code for node discovery elastic/elasticsearch-hadoop#1186

Closed

jasontedor previously requested changes Aug 30, 2018

View reviewed changes

Merge remote-tracking branch 'elastic/master' into 22029

c16aa42

original-brownbear added 2 commits August 30, 2018 14:23

CR: Add deprecation behaviour

33e3c41

CR: Add deprecation behaviour

2a1f101

original-brownbear added 4 commits August 30, 2018 20:19

Merge remote-tracking branch 'elastic/master' into 22029

7cfd129

Merge remote-tracking branch 'elastic/master' into 22029

e282f23

Merge remote-tracking branch 'elastic/master' into 22029

f73f144

Merge remote-tracking branch 'elastic/master' into 22029

c97d33c

original-brownbear requested a review from jasontedor September 3, 2018 15:49

jasontedor requested review from Tim-Brooks and removed request for jasontedor September 8, 2018 02:43

original-brownbear added the review label Sep 8, 2018

Tim-Brooks reviewed Sep 10, 2018

View reviewed changes

original-brownbear added 2 commits September 11, 2018 09:29

Merge remote-tracking branch 'elastic/master' into 22029

becebff

CR: correctly handle IPV6

fe3e161

Tim-Brooks approved these changes Sep 11, 2018

View reviewed changes

original-brownbear merged commit 94cdf0c into elastic:master Sep 12, 2018

original-brownbear deleted the 22029 branch September 12, 2018 04:15

original-brownbear mentioned this pull request Sep 12, 2018

NETWORKING:Def CName in Http Publish Addr to True #33631

Merged

original-brownbear added a commit that referenced this pull request Sep 18, 2018

NETWORKING:Def CName in Http Publish Addr to True (#33631)

87cedef

* Follow up to #32806 setting the setting to true for 7.x

original-brownbear added a commit that referenced this pull request Nov 22, 2018

NETWORKING: http.publish_host Should Contain CNAME (#32806) (#35838)

f17891b

* NETWORKING: http.publish_host Should Contain CNAME * Closes #22029

original-brownbear added v6.6.0 and removed v6.5.0 labels Nov 23, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

DaveCTurner mentioned this pull request Mar 12, 2019

Transport's publish address should report its hostname #39970

Closed

andrershov mentioned this pull request Aug 15, 2019

Deprecate es.http.cname_in_publish_address setting #45616

Merged

andrershov pushed a commit to andrershov/elasticsearch that referenced this pull request Aug 15, 2019

transport.publish_address should contain CNAME

d16774c

Relates #elastic#32806 Closes elastic#39970

andrershov pushed a commit to andrershov/elasticsearch that referenced this pull request Aug 15, 2019

transport.publish_address should contain CNAME

2d1939d

Relates #elastic#32806 Closes elastic#39970

andrershov mentioned this pull request Aug 15, 2019

transport.publish_address should contain CNAME #45626

Merged

andrershov pushed a commit that referenced this pull request Aug 16, 2019

transport.publish_address should contain CNAME (#45626)

e0a2558

This commit adds CNAME reporting for transport.publish_address same way it's done for http.publish_address. Relates #32806 Relates #39970

hub-cap mentioned this pull request Nov 12, 2019

Java-REST-Client NodeSniffer Fails parsing publish_address properly #48950

Closed

i-skrypnyk mentioned this pull request Sep 14, 2020

td-agent v3.7.1 ssl hostname does not match the server certificate uken/fluent-plugin-elasticsearch#763

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETWORKING: http.publish_host Should Contain CNAME#32806

NETWORKING: http.publish_host Should Contain CNAME#32806
original-brownbear merged 12 commits intoelastic:masterfrom
original-brownbear:22029

original-brownbear commented Aug 13, 2018 •

edited

Loading

Uh oh!

elasticmachine commented Aug 13, 2018

Uh oh!

jasontedor left a comment

Uh oh!

original-brownbear commented Aug 30, 2018

Uh oh!

original-brownbear commented Aug 30, 2018

Uh oh!

Tim-Brooks left a comment

Uh oh!

original-brownbear commented Sep 11, 2018

Uh oh!

Tim-Brooks left a comment

Uh oh!

original-brownbear commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

original-brownbear commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 13, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Aug 30, 2018

Uh oh!

original-brownbear commented Aug 30, 2018

Uh oh!

Tim-Brooks left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 11, 2018

Uh oh!

Tim-Brooks left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

original-brownbear commented Aug 13, 2018 •

edited

Loading