-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
NETWORKING: http.publish_host Should Contain CNAME #32806
Conversation
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 NETWORKING: http.publish_host Should Contain CNAME #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 tofalse
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
@jasontedor sounds good :) => on it :) |
@jasontedor done I think, implemented the |
Transferring review ownership to @tbrooks8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
@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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@tbrooks8 thanks! |
* Follow up to elastic#32806 setting the setting to true for 7.x
* 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 elastic#22029
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.
http.publish_host
if available