-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
getEndpoints uses service target port directly if it's a number and mismatch with port name in endpoint #7393
Conversation
@fatedier: This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Welcome @fatedier! |
Hi @fatedier. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @ElvinEfendi |
/kind feature |
@@ -113,6 +118,8 @@ func getEndpoints(s *corev1.Service, port *corev1.ServicePort, proto corev1.Prot | |||
upsServers = append(upsServers, ups) | |||
processedUpstreamServers[ep] = struct{}{} | |||
} | |||
// only one port could be matched, no need to iterate remaining ports | |||
break |
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.
Thanks for your contributions. The first thing to confirm is whether there will be side effects 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.
I'm not sure if there could be a endpoint subsets with different port number and same port name. I think it will not appear because k8s service validation will reject duplicate port name values.
Howerver, endpoint validation doesn't validate this rule.
We can remove break
here. It will not affect result but cost more compute resources.
What do you think?
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.
// EndpointPort is a tuple that describes a single port.
// +structType=atomic
type EndpointPort struct {
// The name of this port. This must match the 'name' field in the
// corresponding ServicePort.
// Must be a DNS_LABEL.
// Optional only if one port is defined.
// +optional
Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
// The port number of the endpoint.
Port int32 `json:"port" protobuf:"varint,2,opt,name=port"`
...
}
// ServicePort contains information on service's port.
type ServicePort struct {
// The name of this port within the service. This must be a DNS_LABEL.
// All ports within a ServiceSpec must have unique names. When considering
// the endpoints for a Service, this must match the 'name' field in the
// EndpointPort.
// Optional if only one ServicePort is defined on this service.
// +optional
Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
// The IP protocol for this port. Supports "TCP", "UDP", and "SCTP".
// Default is TCP.
// +default="TCP"
// +optional
Protocol Protocol `json:"protocol,omitempty" protobuf:"bytes,2,opt,name=protocol,casttype=Protocol"`
...
}
From spec, EndpointPort name should match the 'name' of ServicePort, and ServicePort's name should be unique.
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 pushed a new commit to remove break here to avoid unknown problem.
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.
@tao12345666333 Do you have time to review this PR?
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.
sorry for delay. I will review it today.
/assign
…ismatch with port name in endpoint
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.
/ok-to-test
@tao12345666333 Any progress? |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fatedier, rikatz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ismatch with port name in endpoint (kubernetes#7393)
What this PR does / why we need it:
This PR avoid requests fail for a little while after changing service port name if service target port is a number.
Types of changes
Which issue/s this PR fixes
fixes #7390
How Has This Been Tested?
Follow the steps described in linked issue.
Checklist: