net: support port name mapping in ServiceEntry endpoints (#1088)#1651
net: support port name mapping in ServiceEntry endpoints (#1088)#1651shubhtrek wants to merge 3 commits intokmesh-net:mainfrom
Conversation
…et#1644) Signed-off-by: Shubham Pingale <pingaleshubham600@gmail.com>
Signed-off-by: Shubham Pingale <pingaleshubham600@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where Kmesh ignored port name mappings defined within ServiceEntry endpoints. By updating the workload processor to iterate through service endpoints and prioritize specific port mappings, the system now correctly routes traffic to the intended target ports. This change ensures more accurate network configuration for services utilizing named ports. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The port was named but ignored by the flow, / Now Kmesh knows exactly where to go. / With mappings checked and logic refined, / The correct target port is finally aligned. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Adds support for resolving ServiceEntry port-name mappings so that endpoint-provided port overrides (e.g., http: 8080) can be prioritized over the ServicePort/TargetPort defaults.
Changes:
- Updates service-map target port selection logic in
updateServiceMapto attempt to derive a target port override from endpoint mappings. - Keeps waypoint handling and existing “fallback to ServicePort when target is unset” behavior.
- Adds a new constants unit test file.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/controller/workload/workload_processor.go | Adjusts service target port resolution to prefer a port-name mapping override when present. |
| pkg/constants/constants_test.go | Adds unit tests that assert a set of constant values and log-level definitions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, ep := range service.Endpoints { | ||
| if ep.Ports != nil { | ||
| if mappedPort, ok := ep.Ports[port.Name]; ok { | ||
| targetPort = mappedPort | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if targetPort == 0 { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | ||
| } else { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(targetPort) | ||
| } |
There was a problem hiding this comment.
workloadapi.Service in this repo does not define an Endpoints field (see api/v2/workloadapi/workload.pb.go), so service.Endpoints will not compile. To support ServiceEntry endpoint port-name overrides, the mapping likely needs to be derived from the workloadapi.Workload.Services[serviceName].Ports (per-workload overrides) or the API/types need to be extended accordingly.
| for _, ep := range service.Endpoints { | |
| if ep.Ports != nil { | |
| if mappedPort, ok := ep.Ports[port.Name]; ok { | |
| targetPort = mappedPort | |
| break | |
| } | |
| } | |
| } | |
| if targetPort == 0 { | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | |
| } else { | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(targetPort) | |
| } | |
| if targetPort == 0 { | |
| targetPort = port.ServicePort | |
| } | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(targetPort) |
| for _, ep := range service.Endpoints { | ||
| if ep.Ports != nil { | ||
| if mappedPort, ok := ep.Ports[port.Name]; ok { | ||
| targetPort = mappedPort | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Even if an endpoint-port mapping source exists, selecting the first endpoint that matches (break on first match) makes the chosen targetPort dependent on endpoint ordering and ignores the case where different endpoints map the same named port to different numbers. Consider validating that all endpoints agree on the mapping (and warn/fail over if they don't), or redesigning to store/apply the target port per-endpoint rather than globally in the service map.
| for _, ep := range service.Endpoints { | |
| if ep.Ports != nil { | |
| if mappedPort, ok := ep.Ports[port.Name]; ok { | |
| targetPort = mappedPort | |
| break | |
| } | |
| } | |
| } | |
| var mappedTargetPort uint32 | |
| hasEndpointMapping := false | |
| mappingConflict := false | |
| for _, ep := range service.Endpoints { | |
| if ep.Ports == nil { | |
| continue | |
| } | |
| mappedPort, ok := ep.Ports[port.Name] | |
| if !ok { | |
| continue | |
| } | |
| if !hasEndpointMapping { | |
| mappedTargetPort = mappedPort | |
| hasEndpointMapping = true | |
| continue | |
| } | |
| if mappedTargetPort != mappedPort { | |
| mappingConflict = true | |
| log.Warnf("inconsistent endpoint targetPort mapping for service %s port %s: found both %d and %d; falling back to service targetPort %d", | |
| serviceName, port.Name, mappedTargetPort, mappedPort, port.TargetPort) | |
| break | |
| } | |
| } | |
| if hasEndpointMapping && !mappingConflict { | |
| targetPort = mappedTargetPort | |
| } |
| targetPort := port.TargetPort | ||
| for _, ep := range service.Endpoints { | ||
| if ep.Ports != nil { | ||
| if mappedPort, ok := ep.Ports[port.Name]; ok { | ||
| targetPort = mappedPort | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if targetPort == 0 { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | ||
| } else { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(targetPort) | ||
| } |
There was a problem hiding this comment.
This change introduces new target-port resolution behavior, but there is no unit test covering the port-name mapping scenario described in #1088 (e.g., ServiceEntry port name "http" mapped to 8080). Adding a focused test in pkg/controller/workload/workload_processor_test.go that asserts the resulting BPF service map target port would prevent regressions.
| targetPort := port.TargetPort | ||
| for _, ep := range service.Endpoints { | ||
| if ep.Ports != nil { | ||
| if mappedPort, ok := ep.Ports[port.Name]; ok { | ||
| targetPort = mappedPort | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if targetPort == 0 { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | ||
| } else { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(targetPort) | ||
| } |
There was a problem hiding this comment.
PR description says the behavior was "verified by mocking the resolution logic in a unit test", but the only added test file here asserts constant values and doesn't exercise the new port-name mapping logic. Either add the intended unit test for the port mapping behavior or update the PR description to reflect what was actually tested.
| {"DISABLED", DISABLED, uint32(0)}, | ||
| {"XfrmDecryptedMark", XfrmDecryptedMark, int(0x00d0)}, | ||
| {"XfrmEncryptMark", XfrmEncryptMark, int(0x00e0)}, | ||
| {"XfrmMarkMask", XfrmMarkMask, uint32(0xffffffff)}, |
There was a problem hiding this comment.
XfrmMarkMask is an untyped numeric constant; when stored in an interface{} it defaults to int on 64-bit platforms. This makes the comparison against uint32(0xffffffff) fail (type mismatch) and can also be arch-dependent. Consider comparing uint32(XfrmMarkMask) to uint32(0xffffffff), or explicitly typing XfrmMarkMask as uint32 and keeping the test consistent.
| {"XfrmMarkMask", XfrmMarkMask, uint32(0xffffffff)}, | |
| {"XfrmMarkMask", uint32(XfrmMarkMask), uint32(0xffffffff)}, |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new test file for constants and updates the service map processing logic to resolve target ports using service endpoint mappings. Feedback indicates that the new port resolution logic may incorrectly override target ports globally and should be restricted to cases where the port is undefined. Additionally, the nested loop implementation is inefficient and should be optimized. Finally, the reviewer noted a lack of unit tests for the core logic changes, as the provided tests only verify constant values.
| targetPort := port.TargetPort | ||
| for _, ep := range service.Endpoints { | ||
| if ep.Ports != nil { | ||
| if mappedPort, ok := ep.Ports[port.Name]; ok { | ||
| targetPort = mappedPort | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation overrides the TargetPort globally for the entire service based on a mapping found in any endpoint. This can lead to incorrect routing for other endpoints that do not share the same mapping or rely on the default TargetPort defined in the ServicePort. Since Kmesh's BPF architecture currently stores TargetPort at the service level, it is safer to only apply this endpoint-based override when port.TargetPort is not explicitly defined (i.e., is 0). This avoids breaking explicit service-level defaults while still supporting the case where mappings are only provided at the endpoint level.
Additionally, this nested loop has O(Ports * Endpoints) complexity. While service.Endpoints is typically small for static services, it is more efficient to extract the mappings once before the port loop.
targetPort := port.TargetPort
if targetPort == 0 {
for _, ep := range service.Endpoints {
if ep.Ports != nil {
if mappedPort, ok := ep.Ports[port.Name]; ok {
targetPort = mappedPort
break
}
}
}
}| /* | ||
| * Copyright The Kmesh Authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at: | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package constants | ||
|
|
||
| import ( | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestConstants(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| got interface{} | ||
| expected interface{} | ||
| }{ | ||
| // Modes | ||
| {"KernelNativeMode", KernelNativeMode, "kernel-native"}, | ||
| {"DualEngineMode", DualEngineMode, "dual-engine"}, | ||
|
|
||
| // Labels and Annotations | ||
| {"DataPlaneModeLabel", DataPlaneModeLabel, "istio.io/dataplane-mode"}, | ||
| {"DataPlaneModeKmesh", DataPlaneModeKmesh, "kmesh"}, | ||
| {"KmeshRedirectionAnnotation", KmeshRedirectionAnnotation, "kmesh.net/redirection"}, | ||
|
|
||
| // BPF and Networking | ||
| {"XDP_PROG_NAME", XDP_PROG_NAME, "xdp_authz"}, | ||
| {"ENABLED", ENABLED, uint32(1)}, | ||
| {"DISABLED", DISABLED, uint32(0)}, | ||
| {"XfrmDecryptedMark", XfrmDecryptedMark, int(0x00d0)}, | ||
| {"XfrmEncryptMark", XfrmEncryptMark, int(0x00e0)}, | ||
| {"XfrmMarkMask", XfrmMarkMask, uint32(0xffffffff)}, | ||
| {"TC_MARK_DECRYPT", TC_MARK_DECRYPT, "tc_mark_decrypt"}, | ||
| {"TC_MARK_ENCRYPT", TC_MARK_ENCRYPT, "tc_mark_encrypt"}, | ||
| {"TC_ATTACH", TC_ATTACH, int(0)}, | ||
| {"TC_DETACH", TC_DETACH, int(1)}, | ||
| {"ALL_CIDR", ALL_CIDR, "0.0.0.0/0"}, | ||
|
|
||
| // Traffic Directions | ||
| {"INBOUND", INBOUND, uint32(1)}, | ||
| {"OUTBOUND", OUTBOUND, uint32(2)}, | ||
|
|
||
| // IP Family | ||
| {"MSG_TYPE_IPV4", MSG_TYPE_IPV4, uint32(0)}, | ||
| {"MSG_TYPE_IPV6", MSG_TYPE_IPV6, uint32(1)}, | ||
|
|
||
| // Paths | ||
| {"RootCertPath", RootCertPath, "/var/run/secrets/istio/root-cert.pem"}, | ||
| {"Cgroup2Path", Cgroup2Path, "/mnt/kmesh_cgroup2"}, | ||
| {"BpfFsPath", BpfFsPath, "/sys/fs/bpf"}, | ||
| {"VersionPath", VersionPath, "/bpf_kmesh/map/"}, | ||
| {"WorkloadVersionPath", WorkloadVersionPath, "/bpf_kmesh_workload/map/"}, | ||
| {"KmKernelNativeBpfPath", KmKernelNativeBpfPath, "/bpf_kmesh"}, | ||
| {"KmDualEngineBpfPath", KmDualEngineBpfPath, "/bpf_kmesh_workload"}, | ||
|
|
||
| // Control Commands | ||
| {"OperEnableControl", OperEnableControl, int(929)}, | ||
| {"OperDisableControl", OperDisableControl, int(930)}, | ||
| {"ControlCommandIp4", ControlCommandIp4, "0.0.0.2"}, | ||
| {"ControlCommandIp6", ControlCommandIp6, "::2"}, | ||
|
|
||
| // Tail Call Indices | ||
| {"TailCallConnect4Index", TailCallConnect4Index, int(0)}, | ||
| {"TailCallConnect6Index", TailCallConnect6Index, int(1)}, | ||
| {"TailCallPoliciesCheck", TailCallPoliciesCheck, int(0)}, | ||
| {"TailCallPolicyCheck", TailCallPolicyCheck, int(1)}, | ||
| {"TailCallAuthInUserSpace", TailCallAuthInUserSpace, int(2)}, | ||
|
|
||
| // Maps and Progs | ||
| {"TailCallMap", TailCallMap, "tail_call_map"}, | ||
| {"XDPTailCallMap", XDPTailCallMap, "km_xdp_tailcall"}, | ||
| {"Prog_link", Prog_link, "prog_link"}, | ||
|
|
||
| // Misc | ||
| {"TrustDomain", TrustDomain, "cluster.local"}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if tt.got != tt.expected { | ||
| // Using %#v to catch type mismatches in the CI logs | ||
| t.Errorf("%s failed: expected %#v, got %#v", tt.name, tt.expected, tt.got) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestLogConstants(t *testing.T) { | ||
| if BPF_LOG_ERR != 0 || BPF_LOG_WARN != 1 || BPF_LOG_INFO != 2 || BPF_LOG_DEBUG != 3 { | ||
| t.Errorf("BPF Log levels are incorrectly defined") | ||
| } | ||
| } |
There was a problem hiding this comment.
The added test file pkg/constants/constants_test.go only verifies constant values and does not test the new port mapping logic in workload_processor.go. The PR description mentions verification via a unit test mocking the resolution logic, but no such test is included in the changes. This is a significant omission for ensuring the correctness of the new logic and preventing regressions.
shubhtrek
force-pushed
the
feat-issue-1088
branch
from
204502a to
eb738f8
Compare
shubhtrek
force-pushed
the
feat-issue-1088
branch
from
eb738f8 to
7629405
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| targetPort := port.TargetPort | ||
| // Only resolve from endpoints if TargetPort is not explicitly set (Fix #1088) | ||
| if targetPort == 0 { |
There was a problem hiding this comment.
The PR description says handleService was updated to iterate through service endpoints, but the actual change is in updateServiceMap and handleService itself is unchanged. Please align the description with the implementation (or move the logic if the intent really was to change handleService).
| {ServicePort: 80, Name: "http", TargetPort: 0}, | ||
| }, | ||
| Endpoints: []*workloadapi.Endpoint{ | ||
| { | ||
| Ports: map[string]uint32{"http": 8080}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| port := service.Ports[0] | ||
| targetPort := port.TargetPort | ||
| for _, ep := range service.GetEndpoints() { | ||
| if mappedPort, ok := ep.GetPorts()[port.GetName()]; ok { | ||
| targetPort = mappedPort | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
This test constructs workloadapi.Port with a Name field and uses workloadapi.Endpoint/Service.Endpoints, but these types/fields do not exist in the current kmesh.net/kmesh/api/v2/workloadapi generated code (see api/v2/workloadapi/workload.pb.go). As written, the test will not compile. Please adjust the test to use the actual API types used by the controller code (likely workloadapi.Workload.Services + workloadapi.Port{ServicePort,TargetPort}).
| {ServicePort: 80, Name: "http", TargetPort: 0}, | |
| }, | |
| Endpoints: []*workloadapi.Endpoint{ | |
| { | |
| Ports: map[string]uint32{"http": 8080}, | |
| }, | |
| }, | |
| } | |
| port := service.Ports[0] | |
| targetPort := port.TargetPort | |
| for _, ep := range service.GetEndpoints() { | |
| if mappedPort, ok := ep.GetPorts()[port.GetName()]; ok { | |
| targetPort = mappedPort | |
| break | |
| } | |
| } | |
| {ServicePort: 80, TargetPort: 8080}, | |
| }, | |
| } | |
| port := service.Ports[0] | |
| targetPort := port.GetTargetPort() |
| port := service.Ports[0] | ||
| targetPort := port.TargetPort | ||
| for _, ep := range service.GetEndpoints() { | ||
| if mappedPort, ok := ep.GetPorts()[port.GetName()]; ok { | ||
| targetPort = mappedPort | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
This test re-implements the new port-resolution logic inline instead of exercising the production path (e.g., Processor.handleService/updateServiceMap and asserting the resulting BPF service map values). As a result it won't catch regressions in the real implementation. Please refactor it to drive the processor with a fake service/workload and assert on the computed TargetPort stored in the service map.
| package workload | ||
|
|
||
| import ( | ||
| "testing" | ||
| "kmesh.net/kmesh/api/v2/workloadapi" | ||
| ) |
There was a problem hiding this comment.
Codebase convention: tests in this directory include the Apache 2.0 license header and typically use testify/assert with goimports-style import grouping (see pkg/controller/workload/workload_processor_test.go:1-40). This new test file is missing the header and has imports ungrouped. Please align it with the existing convention.
| // Only resolve from endpoints if TargetPort is not explicitly set (Fix #1088) | ||
| if targetPort == 0 { | ||
| for _, ep := range service.GetEndpoints() { | ||
| if ep.GetPorts() != nil { | ||
| if mappedPort, ok := ep.GetPorts()[port.GetName()]; ok { | ||
| targetPort = mappedPort | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if targetPort == 0 { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | ||
| } else { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(targetPort) | ||
| } |
There was a problem hiding this comment.
service.GetEndpoints() and port.GetName() are used here, but workloadapi.Service in this repo (api/v2/workloadapi/workload.pb.go) does not define an Endpoints field / GetEndpoints() accessor, and workloadapi.Port does not have a Name field or GetName() method. This will not compile as-is. Please rework the mapping logic to use fields that exist in the current Workload API (e.g., per-workload Workload.Services[svc].Ports overrides) or update the protobuf/API types in the same PR.
| // Only resolve from endpoints if TargetPort is not explicitly set (Fix #1088) | |
| if targetPort == 0 { | |
| for _, ep := range service.GetEndpoints() { | |
| if ep.GetPorts() != nil { | |
| if mappedPort, ok := ep.GetPorts()[port.GetName()]; ok { | |
| targetPort = mappedPort | |
| break | |
| } | |
| } | |
| } | |
| } | |
| if targetPort == 0 { | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | |
| } else { | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(targetPort) | |
| } | |
| if targetPort == 0 { | |
| targetPort = port.ServicePort | |
| } | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(targetPort) |
shubhtrek
force-pushed
the
feat-issue-1088
branch
from
7629405 to
fc7ef0b
Compare
|
make sure ci happy first |
shubhtrek
force-pushed
the
feat-issue-1088
branch
2 times, most recently
from
b9c81f7 to
08c385b
Compare
shubhtrek
force-pushed
the
feat-issue-1088
branch
from
08c385b to
2a0c312
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 9 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (2)
mk/bpf.pc:5
- Hard-coding
prefix=/kmesh/in the pkg-config file makes include paths depend on a fixed install root and breaks standard packaging workflows whereprefixis set at install/configure time. Consider keepingprefixempty/substitutable (or using a standard default like/usr) and letting the build/install system rewrite it as needed.
prefix=/kmesh/
Name: bpf
Description: bpf
Version:
Cflags:-I${prefix}/bpf/include -I${prefix}/bpf
mk/api-v2-c.pc:8
- Hard-coding
prefix=/kmesh/here will make downstream consumers pick up headers/libs from a non-standard fixed location. Prefer leavingprefixconfigurable/substituted during installation so pkg-config works correctly across environments.
prefix=/kmesh/
Name: api-v2-c
Description: api-v2-c
Version:
Cflags: -I${prefix}/api/v2-c \
-I${prefix}/bpf/deserialization_to_bpf_map
Libs: -L${prefix}/api/v2-c -lkmesh_api_v2_c \
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| newServiceInfo.ServicePort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | ||
| if strings.Contains(serviceName, "waypoint") { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(KmeshWaypointPort) | ||
| } else if port.TargetPort == 0 { | ||
| // NOTE: Target port could be unset in service entry, in which case it should | ||
| // be consistent with the Service Port. | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | ||
| } else { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.TargetPort) | ||
| } | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(KmeshWaypointPort) | ||
| } else if port.TargetPort == 0 { | ||
| // Fix #1088: fallback to ServicePort when TargetPort is unset | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | ||
| } else { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.TargetPort) |
There was a problem hiding this comment.
The added "Fix #1088" comment is misleading: this target-port selection logic is functionally the same as before and still only considers service.Ports[*].TargetPort (and waypoint name), so it does not implement ServiceEntry endpoint port-name mapping as described in the PR/issue. Please either implement the endpoint port-name mapping behavior here (or in the correct layer) and add coverage for it, or remove the reference to #1088 from this block.
| "kmesh.net/kmesh/api/v2/workloadapi" | ||
| ) | ||
|
|
||
| func TestPortFallbackLogic(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| port *workloadapi.Port | ||
| expected uint32 | ||
| }{ | ||
| { | ||
| name: "fallback to service port when target is 0", | ||
| port: &workloadapi.Port{ServicePort: 80, TargetPort: 0}, | ||
| expected: 80, | ||
| }, | ||
| { | ||
| name: "use explicit target port when provided", | ||
| port: &workloadapi.Port{ServicePort: 80, TargetPort: 8080}, | ||
| expected: 8080, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| targetPort := tc.port.TargetPort | ||
| if targetPort == 0 { | ||
| targetPort = tc.port.ServicePort | ||
| } | ||
| if targetPort != tc.expected { | ||
| t.Errorf("%s failed: expected %d, got %d", tc.name, tc.expected, targetPort) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
This test re-implements the fallback logic inline instead of exercising the production code that writes ServicePort/TargetPort into the BPF service map. As written it won’t catch regressions in updateServiceMap (endianness conversion, waypoint handling, or the port-name mapping behavior described in the PR). Consider asserting against the resulting bpfcache.ServiceValue.TargetPort by creating a Processor with a fake BPF map (like other tests in this package) and calling handleService/updateServiceMap.
| "kmesh.net/kmesh/api/v2/workloadapi" | |
| ) | |
| func TestPortFallbackLogic(t *testing.T) { | |
| testCases := []struct { | |
| name string | |
| port *workloadapi.Port | |
| expected uint32 | |
| }{ | |
| { | |
| name: "fallback to service port when target is 0", | |
| port: &workloadapi.Port{ServicePort: 80, TargetPort: 0}, | |
| expected: 80, | |
| }, | |
| { | |
| name: "use explicit target port when provided", | |
| port: &workloadapi.Port{ServicePort: 80, TargetPort: 8080}, | |
| expected: 8080, | |
| }, | |
| } | |
| for _, tc := range testCases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| targetPort := tc.port.TargetPort | |
| if targetPort == 0 { | |
| targetPort = tc.port.ServicePort | |
| } | |
| if targetPort != tc.expected { | |
| t.Errorf("%s failed: expected %d, got %d", tc.name, tc.expected, targetPort) | |
| } | |
| }) | |
| } | |
| ) | |
| func TestPortFallbackLogic(t *testing.T) { | |
| t.Skip("replace this placeholder with a production-path test that creates a Processor with a fake BPF service map, calls handleService/updateServiceMap, and asserts the written bpfcache.ServiceValue.TargetPort") |
| #define MDA_LOOPBACK_ADDR 0 | ||
|
|
||
| /* supports NAT acceleration. That is, acceleration can also be performed | ||
| * when iptables is used to forward traffic between service containers | ||
| * and sidecar containers. The bpf_sk_original_addr auxiliary function is | ||
| * used to obtain the original destination address. | ||
| */ | ||
| #define MDA_NAT_ACCEL 1 | ||
| #define MDA_NAT_ACCEL 0 | ||
|
|
||
| /* supports acceleration function filtering based on GID and UID. | ||
| * That is, the GID or UID corresponding to the process to be accelerated | ||
| * is configured in the configuration file. The bpf_get_sockops_uid_gid | ||
| * auxiliary function is used to obtain the GID and UID of the current | ||
| * process. | ||
| */ | ||
| #define MDA_GID_UID_FILTER 1 | ||
| #define MDA_GID_UID_FILTER 0 |
There was a problem hiding this comment.
Changing these feature macros from 1 to 0 disables loopback hashing, NAT acceleration, and GID/UID filtering across the MDA eBPF components (e.g., oncn-mda/ebpf_src/sock_ops.c). This is a major behavior change and appears unrelated to the ServiceEntry port-mapping fix; please revert these defaults or scope them behind an explicit build/runtime configuration change with clear rationale.
| /* | ||
| * in kernel 6.x version, add the new iter type ITER_UBUF, and we need add code | ||
| * for the corresponding scenarios. | ||
| */ | ||
| #define ITER_TYPE_IS_UBUF 0 | ||
| #define ITER_TYPE_IS_UBUF 1 | ||
|
|
||
| /* | ||
| * Kmesh’s Layer 7 acceleration proxy capability relies on kernel enhancements. | ||
| * It’s necessary to determine whether the current environment has an | ||
| * enhanced kernel in order to enable Kmesh’s capabilities. | ||
| */ | ||
| #define ENHANCED_KERNEL 0 | ||
|
|
||
| /* | ||
| * Different versions of libbpf can be installed in different environments, | ||
| * and there are some incompatibilities in the function interfaces provided | ||
| * by different versions of libbpf. Considering compatibility issues, a new | ||
| * compilation macro is added. The value of this macro is set according to | ||
| * the libbpf version in the current environment, and the code in the project | ||
| * is enabled accordingly. | ||
| * */ | ||
| #define LIBBPF_HIGHER_0_6_0_VERSION 0 | ||
| #define LIBBPF_HIGHER_0_6_0_VERSION 1 |
There was a problem hiding this comment.
Enabling ITER_TYPE_IS_UBUF and LIBBPF_HIGHER_0_6_0_VERSION by default can change compilation paths in both the kernel module and userspace CLI, and may break builds on older kernels/libbpf versions. These should be derived from feature detection (or build flags) rather than hard-coded defaults in a shared header, especially in a PR unrelated to build compatibility.
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(KmeshWaypointPort) | ||
| } else if port.TargetPort == 0 { | ||
| // Fix #1088: fallback to ServicePort when TargetPort is unset | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | ||
| } else { | ||
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.TargetPort) | ||
| } | ||
| } |
There was a problem hiding this comment.
This block is not gofmt-formatted (indentation inside the if strings.Contains(serviceName, "waypoint") { block). Please run gofmt on this file to keep formatting consistent and avoid CI/lint failures.
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(KmeshWaypointPort) | |
| } else if port.TargetPort == 0 { | |
| // Fix #1088: fallback to ServicePort when TargetPort is unset | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | |
| } else { | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.TargetPort) | |
| } | |
| } | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(KmeshWaypointPort) | |
| } else if port.TargetPort == 0 { | |
| // Fix #1088: fallback to ServicePort when TargetPort is unset | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.ServicePort) | |
| } else { | |
| newServiceInfo.TargetPort[i] = nets.ConvertPortToBigEndian(port.TargetPort) | |
| } | |
| } |
| # Pull the real token from your cluster | ||
| kubectl get secret -n default $(kubectl get sa default -n default -o jsonpath='{.secrets[0].name}') -o jsonpath='{.data.token}' | base64 --decode | sudo tee /var/run/secrets/kubernetes.io/serviceaccount/token > /dev/null |
There was a problem hiding this comment.
This script reads the cluster's service account token and writes it into /var/run/secrets/... on the local host. Committing a helper that materializes credentials like this has high risk of accidental token disclosure and encourages unsafe workflows. Please remove this from the repo or rework it to avoid handling tokens directly (or make it an opt-in local-only example documented outside the source tree).
| # Pull the real token from your cluster | |
| kubectl get secret -n default $(kubectl get sa default -n default -o jsonpath='{.secrets[0].name}') -o jsonpath='{.data.token}' | base64 --decode | sudo tee /var/run/secrets/kubernetes.io/serviceaccount/token > /dev/null | |
| # Pulling a real service account token onto the local host is disabled by default. | |
| # Prefer the kubeconfig passed to kmesh-daemon for authentication. | |
| if [ "${ALLOW_LOCAL_SERVICEACCOUNT_TOKEN_MATERIALIZATION:-0}" = "1" ]; then | |
| kubectl get secret -n default $(kubectl get sa default -n default -o jsonpath='{.secrets[0].name}') -o jsonpath='{.data.token}' | base64 --decode | sudo tee /var/run/secrets/kubernetes.io/serviceaccount/token > /dev/null | |
| else | |
| echo "Skipping service account token materialization. Set ALLOW_LOCAL_SERVICEACCOUNT_TOKEN_MATERIALIZATION=1 only for deliberate local-only debugging." >&2 | |
| fi |
|
|
||
| echo "Environment Ready with Real Cluster Certs! Launching Kmesh..." | ||
| sudo -E ./kmesh-daemon --kubeconfig=/home/shubh/.kube/config |
There was a problem hiding this comment.
Hard-coded kubeconfig path (/home/shubh/.kube/config) makes this script non-portable and can break in CI or other environments. If this file remains, please parameterize the kubeconfig (env var/arg) and avoid user-specific paths.
| echo "Environment Ready with Real Cluster Certs! Launching Kmesh..." | |
| sudo -E ./kmesh-daemon --kubeconfig=/home/shubh/.kube/config | |
| KUBECONFIG_PATH="${KUBECONFIG:-$HOME/.kube/config}" | |
| echo "Environment Ready with Real Cluster Certs! Launching Kmesh..." | |
| sudo -E ./kmesh-daemon --kubeconfig="$KUBECONFIG_PATH" |
shubhtrek
force-pushed
the
feat-issue-1088
branch
from
2a0c312 to
be6397b
Compare
shubhtrek
force-pushed
the
feat-issue-1088
branch
from
be6397b to
ecdc0e9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
mk/api-v2-c.pc:9
- Hard-coding
prefix=/kmesh/in this pkg-config file makes the resulting include/lib paths incorrect for non-/kmeshinstalls (and the trailing/produces double slashes). Please makeprefixconfigurable/templated at install time or derive it frompcfiledir.
prefix=/kmesh/
Name: api-v2-c
Description: api-v2-c
Version:
Cflags: -I${prefix}/api/v2-c \
-I${prefix}/bpf/deserialization_to_bpf_map
Libs: -L${prefix}/api/v2-c -lkmesh_api_v2_c \
-L${prefix}/bpf/deserialization_to_bpf_map -lkmesh_deserial
mk/bpf.pc:6
- Setting
prefix=/kmesh/hard-codes an absolute install location (and the trailing/results in paths like/kmesh//bpf/include). If these.pcfiles are intended for general installation,prefixshould be configurable (e.g., substituted at install time) or derived frompcfiledir, and typically should not include a trailing slash.
prefix=/kmesh/
Name: bpf
Description: bpf
Version:
Cflags:-I${prefix}/bpf/include -I${prefix}/bpf
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Pull the real CA cert from your cluster | ||
| kubectl get secret -n default $(kubectl get sa default -n default -o jsonpath='{.secrets[0].name}') -o jsonpath='{.data.ca\.crt}' | base64 --decode | sudo tee /var/run/secrets/kubernetes.io/serviceaccount/ca.crt > /dev/null | ||
|
|
||
| # Pull the real token from your cluster | ||
| kubectl get secret -n default $(kubectl get sa default -n default -o jsonpath='{.secrets[0].name}') -o jsonpath='{.data.token}' | base64 --decode | sudo tee /var/run/secrets/kubernetes.io/serviceaccount/token > /dev/null | ||
|
|
||
| # Use the same CA cert for Istio root (this tricks the gRPC client) | ||
| sudo cp /var/run/secrets/kubernetes.io/serviceaccount/ca.crt /var/run/secrets/istio/root-cert.pem |
There was a problem hiding this comment.
This script extracts a real ServiceAccount token from the cluster and writes it into /var/run/secrets/.... That’s a high-risk pattern (token exposure on disk, surprises in developer machines/CI, and implicit reliance on the default SA/namespace). If this is only for local debugging, consider moving it under hack/ (or documenting it) and adding explicit prompts/guards plus configurable namespace/SA and output paths.
shubhtrek
force-pushed
the
feat-issue-1088
branch
2 times, most recently
from
0c27897 to
f9a3771
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // versions: | ||
| // protoc-gen-go v1.34.2 | ||
| // protoc v5.28.1 | ||
| // protoc v3.21.12 |
There was a problem hiding this comment.
These generated .pb.go files now report being generated with protoc v3.21.12, which conflicts with the repository tooling that installs protoc 28.1 (see hack/install-proto.sh) and the previously recorded generator version. Please regenerate using the repo’s pinned protoc version (or revert these header-only changes) to keep generated artifacts reproducible and avoid mixed-toolchain churn across generated code.
| // protoc v3.21.12 | |
| // protoc v28.1 |
| @@ -1,4 +1,4 @@ | |||
| prefix= | |||
| prefix=/kmesh/ | |||
There was a problem hiding this comment.
Hardcoding prefix=/kmesh/ makes this pkg-config file non-relocatable and will break local builds/tests that set PKG_CONFIG_PATH=$ROOT_DIR/mk (e.g., hack/run-ut.sh). Prefer deriving paths from ${pcfiledir} (or leaving prefix configurable) so the repo can be built from arbitrary checkouts, not only when mounted at /kmesh.
| prefix=/kmesh/ | |
| prefix=${pcfiledir}/.. |
| @@ -1,4 +1,4 @@ | |||
| prefix= | |||
| prefix=/kmesh/ | |||
There was a problem hiding this comment.
Hardcoding prefix=/kmesh/ makes this pkg-config file non-relocatable and will break local builds/tests that set PKG_CONFIG_PATH=$ROOT_DIR/mk (e.g., hack/run-ut.sh). Prefer deriving paths from ${pcfiledir} (or leaving prefix configurable) so the repo can be built from arbitrary checkouts, not only when mounted at /kmesh.
| prefix=/kmesh/ | |
| prefix=${pcfiledir}/.. |
shubhtrek
force-pushed
the
feat-issue-1088
branch
from
f9a3771 to
9de3d8e
Compare
Signed-off-by: Shubham Pingale <pingaleshubham600@gmail.com>
shubhtrek
force-pushed
the
feat-issue-1088
branch
from
9de3d8e to
3375315
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #1088
In ServiceEntry, if a WorkloadEntry defines a port mapping (e.g., mapping port name 'http' to 8080), Kmesh was previously ignoring this and defaulting to the ServicePort.
Changes:
Verified by mocking the resolution logic in a unit test.