-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix support of NodePort for Advanced Network setup [CN-752] #23766
Conversation
@@ -58,7 +58,7 @@ default Integer extractPort(JsonValue subsetJson) { | |||
} | |||
} | |||
} | |||
if (ports.size() == 1) { | |||
if (ports.size() > 0) { |
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.
Is there a reason why changed the size check here?
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.
The reason is that for the advanced network setup, it will return a null
port. And although it will work when the port
is specified in the config, without it specified it will still be null
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.
But when I check the code I see we only care about the first port. Is this the way we want to go with this?
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.
Ideally, not, but it requires the changes in the core API so that one endpoint can have separate ports for different protocols. Until this change is done, the whole part is handled by the core that is returning the port configured in the advanced-network
section
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 have an additional question, do we depend on some port being the first element in ports list?
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.
No, as with the advanced network enabled, the Hazelcast will override the port based on the socket protocol. For the other cases, it is recommended to set the hazelcast-service-port
port name
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.
@SeriyBg Hi, note on this. This change appears to be breaking our deployment with 5.3.0. The port name hazelcast-service-port
is too long for k8s port names, so the check never matches. In k8s we have:
"ports": [
{
"name": "http",
"port": 8443,
"protocol": "TCP"
},
{
"name": "admin",
"port": 8444,
"protocol": "TCP"
},
{
"name": "hazelcast",
"port": 5701,
"protocol": "TCP"
}
]
It does not find the hazelcast
port.
#23931) Backport of: #23766 Checklist: - [X] Labels (`Team:`, `Type:`, `Source:`, `Module:`) and Milestone set - [X] Label `Add to Release Notes` or `Not Release Notes content` set - [X] Request reviewers if possible - [X] Send backports/forwardports if fix needs to be applied to past/future releases - [X] New public APIs have `@Nonnull/@Nullable` annotations - [X] New public APIs have `@since` tags in Javadoc
#23932) Backport of: #23766 Checklist: - [X] Labels (`Team:`, `Type:`, `Source:`, `Module:`) and Milestone set - [X] Label `Add to Release Notes` or `Not Release Notes content` set - [X] Request reviewers if possible - [X] Send backports/forwardports if fix needs to be applied to past/future releases - [X] New public APIs have `@Nonnull/@Nullable` annotations - [X] New public APIs have `@since` tags in Javadoc
When Advanced networking is enabled, the K8s plugin might discover several endpoints (per each port) for each member's pod. The changes ensure that the discovery plugin will match only the private IP per endpoint, ignoring the port values.
Checklist:
Team:
,Type:
,Source:
,Module:
) and Milestone setAdd to Release Notes
orNot Release Notes content
set@Nonnull/@Nullable
annotations@since
tags in Javadoc