Skip to content
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

Antrea Traceflow should validate that destination port isn't empty when destination is a K8s Service #6594

Closed
Atish-iaf opened this issue Aug 7, 2024 · 1 comment · Fixed by #6601
Assignees
Labels
area/ops/traceflow Issues or PRs related to the Traceflow feature kind/bug Categorizes issue or PR as related to a bug.

Comments

@Atish-iaf
Copy link
Contributor

Describe the bug
Antrea Traceflow should validate that destination port isn't empty when provided destination is a K8s service.
If destination port is provided, it works fine but if it is missed it just uses destinaton Service IP(clusterIP) and runs Traceflow as Pod-to-external IP ICMP case which is incorrect.

To Reproduce
Run Antrea Traceflow with source as a Pod and destination as a K8s Service
antctl tf -S ns1/deployment1-cb9cdd4bd-62dbp -D ns1/svc2

Expected
It should return error that destination port is required when destination is a service.

Actual behavior

name: ns1-deployment1-cb9cdd4bd-62dbp-to-ns1-svc2-jtkgwnz4
phase: Succeeded
source: ns1/deployment1-cb9cdd4bd-62dbp
destination: ns1/svc2
results:
- node: antrea-2
  timestamp: 1722949239
  observations:
  - component: SpoofGuard
    action: Forwarded
    srcPodIP: 192.168.2.5
  - component: Forwarding
    componentInfo: Output
    action: ForwardedOutOfOverlay

Additional context
Also, antctl tf should validate that flow arg shouldn't be empty when destination is a K8s service.

@Atish-iaf Atish-iaf added kind/bug Categorizes issue or PR as related to a bug. area/ops/traceflow Issues or PRs related to the Traceflow feature labels Aug 7, 2024
@antoninbas
Copy link
Contributor

Should we use a reasonable default instead? For example, we could use the first protocol / port in the Service's ports list. Default would be populated in the Traceflow controller in the Antrea Agent.

On the antctl side, we could issue a warning but still proceed with the Traceflow request.

Atish-iaf added a commit to Atish-iaf/antrea that referenced this issue Aug 8, 2024
If destination protocol/port isn't provided when destination is service
in antrea traceflow, it just uses destinaton service's IP(clusterIP) and
runs traceflow as Pod-to-IP(icmp) case which is incorrect.

To fix this, we use the first protocol/port from the service's 'ports'
list as default value.

Fixes antrea-io#6594

Signed-off-by: Kumar Atish <[email protected]>
@Atish-iaf Atish-iaf self-assigned this Aug 8, 2024
@rajnkamr rajnkamr added this to the Antrea v2.2 release milestone Aug 8, 2024
Atish-iaf added a commit to Atish-iaf/antrea that referenced this issue Aug 11, 2024
If destination protocol/port isn't provided when destination is service
in antrea traceflow, it just uses destinaton service's IP(clusterIP) and
runs traceflow as Pod-to-IP(icmp) case which is incorrect.

To fix this, we use the first protocol and port from the service's 'ports'
list as default value.

Fixes antrea-io#6594

Signed-off-by: Kumar Atish <[email protected]>
Atish-iaf added a commit to Atish-iaf/antrea that referenced this issue Aug 21, 2024
If destination protocol/port isn't provided when destination is service
in antrea traceflow, it just uses destinaton service's IP(clusterIP) and
runs traceflow as Pod-to-IP(icmp) case which is incorrect.

To fix this, we use the first protocol and port from the service's 'ports'
list as default value.

Fixes antrea-io#6594

Signed-off-by: Kumar Atish <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ops/traceflow Issues or PRs related to the Traceflow feature kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants