Skip to content

CLI: set json output as default for cilium get endpoint #36537

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 18, 2025
Merged

CLI: set json output as default for cilium get endpoint #36537

merged 1 commit into from
Jan 18, 2025

Conversation

saiaunghlyanhtet
Copy link
Member

@saiaunghlyanhtet saiaunghlyanhtet commented Dec 12, 2024 •
edited by joestringer
Loading

Set json output as default for cilium get endpoint

Fixes: #29247

Set json output as default for `cilium-dbg endpoint get`

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 12, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Dec 12, 2024
@saiaunghlyanhtet saiaunghlyanhtet mentioned this pull request Dec 12, 2024
Closed
2 tasks
joestringer
joestringer reviewed Dec 12, 2024
View reviewed changes
@saiaunghlyanhtet saiaunghlyanhtet marked this pull request as ready for review December 13, 2024 15:21
@saiaunghlyanhtet saiaunghlyanhtet requested a review from a team as a code owner December 13, 2024 15:21
@joestringer joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Dec 13, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 13, 2024
@joestringer
Copy link
Member

/test

@saiaunghlyanhtet
Copy link
Member Author

@joestringer, do I need to rebase for some failing CI checks?

@joestringer
Copy link
Member

@saiaunghlyanhtet At a glance, the failures do not look related to this change. I've retriggered the failed jobs.

@tommyp1ckles
Copy link
Contributor

ignore my previous comment, misunderstood the commit. Change looks good 😄

@joestringer
Copy link
Member

@tommyp1ckles FWIW if we decide to create an actual text format for this, that would be an alternative. But since the default output currently is just trying to create a single flattened JSON object (but failing on the way), I figured it's easier to just reduce the scope to always generate JSON for now.

@julianwiedmann
Copy link
Member

I'm seeing CI failures that have been resolved in latest main. Rebasing.

@julianwiedmann
Copy link
Member

/test

1 similar comment
@julianwiedmann
Copy link
Member

/test

@julianwiedmann
Copy link
Member

@saiaunghlyanhtet this CI failure is robust, and looks genuine. Maybe something in the ginkgo tests is parsing the output, and observes changed behavior?

@saiaunghlyanhtet
Copy link
Member Author

saiaunghlyanhtet commented Jan 13, 2025 •
edited
Loading

@saiaunghlyanhtet this CI failure is robust, and looks genuine. Maybe something in the ginkgo tests is parsing the output, and observes changed behavior?

I found the problem. I think that the ginko tests are not parsing any output. The problem is like the following:

First, we call CiliumEndpointIPv6 here (

cilium/test/k8s/datapath_configuration.go

Line 430 in f2c7fe0

endpointIPs = kubectl.CiliumEndpointIPv6(ciliumPodK8s2, "-l k8s:zgroup=testDS")
), which use "cilium-dbg endpoint get" command with -o jsonpath= (

cilium/test/helpers/kubectl.go

Line 2827 in f2c7fe0

"cilium-dbg endpoint get %s -o jsonpath='%s'", endpoint, filter)).KVOutput()
). But, the current fix to setting JSON output as default for cilium-dbg endpoint does not work with -o jsonpath. So, it returns a long json instead of endpoint IPv6s. So, in that way, the test failed.

So, I think that I need to revise on setting json output as default.

@joestringer I also want opinions from you.

joestringer
joestringer reviewed Jan 13, 2025
View reviewed changes
auto-merge was automatically disabled January 14, 2025 00:50

Head branch was pushed to by a user without write access

@saiaunghlyanhtet
CLI: set json output as default for cilium get endpoint
2bc60c3 
Signed-off-by: saiaunghlyanhtet <saiaunghlyanhtet2003@gmail.com>
@joestringer joestringer reopened this Jan 15, 2025
@joestringer
Copy link
Member

I closed and re-opened the PR because some image builds failed when you uploaded, most likely due to infrastructure issues. I'll retrigger the test that was failing to confirm whether you have fixed the issue.

@joestringer
Copy link
Member

joestringer commented Jan 15, 2025 •
edited
Loading

/ci-ginkgo

EDIT: Looks like it passed except for one test that passed in some jobs and failed in a single job - most likely unrelated to the changes here.

@joestringer
Copy link
Member

/test

@saiaunghlyanhtet
Copy link
Member Author

3 still failing but as far as I checked, they are not related to the changes in this PR.

@joestringer joestringer enabled auto-merge January 18, 2025 00:24
@joestringer joestringer added this pull request to the merge queue Jan 18, 2025
Merged via the queue into cilium:main with commit 018ed47 Jan 18, 2025
95 of 109 checks passed
@joestringer joestringer added the needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch label Feb 12, 2025
@joestringer
Copy link
Member

Nominating for v1.17 backport to address problems where users can't get cilium endpoints via local CLI.

@joestringer joestringer added affects/v1.15 This issue affects v1.15 branch affects/v1.16 This issue affects v1.16 branch affects/v1.17 This issue affects v1.17 branch labels Feb 12, 2025
@nbusseneau nbusseneau mentioned this pull request Feb 14, 2025
Merged
22 tasks
@jschwinger233 jschwinger233 mentioned this pull request Feb 19, 2025
Merged
12 tasks
@jschwinger233 jschwinger233 added backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. and removed needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels Feb 19, 2025
@github-actions github-actions bot added backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. and removed backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. labels Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
affects/v1.15 This issue affects v1.15 branch affects/v1.16 This issue affects v1.16 branch affects/v1.17 This issue affects v1.17 branch backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. kind/community-contribution This was a contribution made by a community member. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cilium get endpoint fails with "Failed to decode nested JSON"
5 participants
@saiaunghlyanhtet @joestringer @tommyp1ckles @julianwiedmann @jschwinger233