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.

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

Fix kubectl describe pod probe host info #129321

Closed
wants to merge 1 commit into from
Closed

Fix kubectl describe pod probe host info #129321

wants to merge 1 commit into from

Conversation

astraw99
Copy link
Member

@astraw99 astraw99 commented Dec 20, 2024 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

Using kubectl describe po xxx to show pod probe info, when the probe host is empty, there is no default value showed in the output, like below, so this PR add the default value.

    State:          Running
      Started:      Thu, 19 Dec 2024 15:25:42 +0800
    Ready:          False
    Restart Count:  0
    ...
    Liveness:       http-get http://:8080/healthz delay=0s timeout=5s period=10s #success=1 #failure=3
    ======== to below
    Liveness:       http-get http://localhost:8080/healthz delay=0s timeout=5s period=10s #success=1 #failure=3

Which issue(s) this PR fixes:

Fixes #129320

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 20, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 20, 2024
@k8s-ci-robot k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 20, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: astraw99
Once this PR has been reviewed and has the lgtm label, please assign soltysh for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@astraw99 astraw99 mentioned this pull request Dec 20, 2024
Open
googs1025
googs1025 reviewed Dec 20, 2024
View reviewed changes
staging/src/k8s.io/kubectl/pkg/describe/describe.go
@@ -2055,6 +2055,9 @@ func DescribeProbe(probe *corev1.Probe) string {
} else {
url.Host = probe.HTTPGet.Host
}
if url.Host == "" {
url.Host = "localhost"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use localhost or 127.0.0.1 ? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to change it, based on documentation we default it to pod ip:

kubernetes/staging/src/k8s.io/api/core/v1/types.go

Lines 2497 to 2500 in fab7593

// Host name to connect to, defaults to the pod IP. You probably want to set
// "Host" in httpHeaders instead.
// +optional
Host string `json:"host,omitempty" protobuf:"bytes,3,opt,name=host"`
so printing here default or localhost is both confusing and wrong. Leaving it empty, similarly to what we do with .tcpSocket.host below is probably the safest change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about add "$podIP" instead of "localhost" in the kubectl describe pod output?

soltysh
soltysh requested changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to close this since it's not needed.
/close

staging/src/k8s.io/kubectl/pkg/describe/describe.go
@@ -2055,6 +2055,9 @@ func DescribeProbe(probe *corev1.Probe) string {
} else {
url.Host = probe.HTTPGet.Host
}
if url.Host == "" {
url.Host = "localhost"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to change it, based on documentation we default it to pod ip:

kubernetes/staging/src/k8s.io/api/core/v1/types.go

Lines 2497 to 2500 in fab7593

// Host name to connect to, defaults to the pod IP. You probably want to set
// "Host" in httpHeaders instead.
// +optional
Host string `json:"host,omitempty" protobuf:"bytes,3,opt,name=host"`
so printing here default or localhost is both confusing and wrong. Leaving it empty, similarly to what we do with .tcpSocket.host below is probably the safest change.

@k8s-ci-robot
Copy link
Contributor

@soltysh: Closed this PR.

In response to this:

I'm going to close this since it's not needed.
/close

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-sigs/prow repository.

@soltysh soltysh mentioned this pull request Dec 20, 2024
Closed
@ardaguclu ardaguclu mentioned this pull request Dec 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@googs1025 googs1025 googs1025 left review comments

@soltysh soltysh soltysh requested changes

@ardaguclu ardaguclu Awaiting requested review from ardaguclu

@brianpursley brianpursley Awaiting requested review from brianpursley

Assignees
No one assigned
Labels
area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
SIG CLI
Status: Closed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubectl describe pod probe host is empty
4 participants
@astraw99 @k8s-ci-robot @soltysh @googs1025