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 bug in which APIServerTracing did not work with some egress selectors #112979

Merged
merged 1 commit into from Oct 18, 2022
Merged

Fix bug in which APIServerTracing did not work with some egress selectors #112979

merged 1 commit into from Oct 18, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Oct 11, 2022 •
edited

What type of PR is this?

/kind bug
/sig instrumentation
/priority important-soon

What this PR does / why we need it:

Prevent the APIServer from panicking when an egress selector without a dialer is used.

Which issue(s) this PR fixes:

Fixes #112976

Testing

I verified that test case added causes the API Server to panic as described in #112976, and that it no longer panics when the fix is applied.

Does this PR introduce a user-facing change?

Fixed an issue where the APIServer would panic on startup if an egress selector without a controlplane configuration is specified when using APIServerTracing

/assign @logicalhan

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. area/apiserver area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 11, 2022
@dashpole
Copy link
Contributor Author

cc @austinlparker

@fedebongio
Copy link
Contributor

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 11, 2022
logicalhan
logicalhan reviewed Oct 11, 2022
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

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

Should we backport this change?

mukul-kr
mukul-kr reviewed Oct 13, 2022
View reviewed changes
staging/src/k8s.io/apiserver/pkg/server/options/tracing.go

otelDialer := func(ctx context.Context, addr string) (net.Conn, error) {
return egressDialer(ctx, "tcp", addr)
if egressDialer != nil {
Copy link

Choose a reason for hiding this comment

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

what is going to happen if egressDialer is nil?

Copy link
Contributor Author

@dashpole dashpole Oct 18, 2022 •
edited

Choose a reason for hiding this comment

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

It won't use a custom dialer in that case (it will just use the default dial function).

Edit: it is equivalent to not having an egress selector at all.

Copy link
Member

Choose a reason for hiding this comment

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

huh... probably worth noting loudly in the Lookup documentation that this can return nil, nil (and maybe sweeping for other callers that assume a nil error means a non-nil dialer)

@dashpole
Copy link
Contributor Author

Should we backport this change?

Yes, I think we should.

@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 18, 2022
logicalhan
logicalhan approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@logicalhan logicalhan left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2022
@dashpole
Copy link
Contributor Author

/assign @liggitt

@liggitt
Copy link
Member

liggitt commented Oct 18, 2022

/lgtm
/approve

thanks for the added test case

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, liggitt, logicalhan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2022
@fedebongio
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 18, 2022
@k8s-ci-robot k8s-ci-robot merged commit 859ada1 into kubernetes:master Oct 18, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 18, 2022
@austinlparker
Copy link

Thanks all!

@dashpole dashpole deleted the fix_tracing_crash branch October 18, 2022 22:52
@dashpole dashpole mentioned this pull request Nov 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@mukul-kr mukul-kr mukul-kr left review comments

@logicalhan logicalhan logicalhan approved these changes

@s-urbaniak s-urbaniak Awaiting requested review from s-urbaniak

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@logicalhan logicalhan

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

APIServer Tracing crashes with nil pointer exception when egress selector is configured
7 participants
@dashpole @fedebongio @liggitt @k8s-ci-robot @austinlparker @logicalhan @mukul-kr