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

Add KUBE_CGO_OVERRIDES env var to force enabling CGO #64219

Merged

Conversation

ixdy
Copy link
Member

@ixdy ixdy commented May 23, 2018

What this PR does / why we need it: as detailed in kubernetes/release#469 (and elsewhere), there is a desire to have kubectl built with CGO enabled on mac OS.

There currently isn't a great way to do this in our official cross builds, but we should allow mac users to build their own kubectl with CGO enabled if they desire, e.g. through homebrew.

This change enables that; you can now do KUBE_CGO_OVERRIDES=kubectl make WHAT=cmd/kubectl and get a cgo-enabled kubectl.

The default build outputs remain unchanged.

Release note:

kubectl built for darwin from darwin now enables cgo to use the system-native C libraries for DNS resolution. Cross-compiled kubectl (e.g. from an official kubernetes release) still uses the go-native netgo DNS implementation. 

/assign @BenTheElder @cblecker
cc @bks7 @bitglue

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 23, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 23, 2018
@ixdy
Copy link
Member Author

ixdy commented May 23, 2018

W0523 19:09:32.913] /workspace/k8s.io/kubernetes/hack/lib/golang.sh: line 219: KUBE_CGO_OVERRIDES[@]: unbound variable

why didn't that fail locally?
/shrug

@k8s-ci-robot k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label May 23, 2018
@ixdy
Copy link
Member Author

ixdy commented May 23, 2018

Added one small commit to explicitly enable cgo when building kubectl for darwin from darwin, which would eliminate most/all of the homebrew patch.

It'd still mean that the "official" kubectl binary for darwin would be statically linked without cgo, but it'd be easy for homebrew users to get a kubectl that works better.

I don't have a mac handy to test this right now; can someone else verify that it actually works?

e.g. run make WHAT=cmd/kubectl && file _output/local/go/bin/kubectl

@ixdy ixdy force-pushed the hack-lib-golang-cgo-overrides branch from 07bad51 to 25a87f0 Compare May 23, 2018 19:31
@ixdy ixdy force-pushed the hack-lib-golang-cgo-overrides branch from 25a87f0 to e4ded2b Compare May 23, 2018 19:32
@BenTheElder
Copy link
Member

@ixdy on a macbook:

$ make WHAT=cmd/kubectl && file _output/local/go/bin/kubectl
+++ [0523 12:50:55] Building go targets for darwin/amd64:
    cmd/kubectl
_output/local/go/bin/kubectl: Mach-O 64-bit executable x86_64

@ixdy
Copy link
Member Author

ixdy commented May 23, 2018

@BenTheElder hm, I have no idea if that's actually statically linked or not. Can you also run ldd _output/local/go/bin/kubectl?

/retest

@ixdy
Copy link
Member Author

ixdy commented May 23, 2018

/cc @pwittrock
@kubernetes/sig-cli-maintainers

@cblecker
Copy link
Member

I am going to test this locally as well.

@BenTheElder
Copy link
Member

@ixdy

$ otool -L  _output/local/go/bin/kubectl
_output/local/go/bin/kubectl:
	/usr/lib/libSystem.B.dylib (compatibility version 0.0.0, current version 0.0.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 0.0.0, current version 0.0.0)
	/System/Library/Frameworks/Security.framework/Versions/A/Security (compatibility version 0.0.0, current version 0.0.0)

(otool -L is more or less ldd equivalent for macOS)

@bitglue
Copy link

bitglue commented May 23, 2018

Since otool shows it linked to stuff I bet it will work, but the direct test is to run the binary with GODEBUG=netdns=9 set in the environment.

If you try this with the current, static builds, you will get this output:

go package net: built with netgo build tag; using Go's DNS resolver

If cgo is available, you'll get this output:

go package net: using cgo DNS resolver

@BenTheElder
Copy link
Member

@bitglue good point, I checked the docs and it seems now you should use something like export GODEBUG=netdns=cgo+9 so I did that and got:

$  _output/local/go/bin/kubectl create -f http://prow.k8s.io
go package net: using cgo DNS resolver
go package net: hostLookupOrder(prow.k8s.io) = cgo
go package net: hostLookupOrder(prow.k8s.io) = cgo
error: error validating "http://prow.k8s.io": error validating data: invalid object to validate; if you choose to ignore these errors, turn validation off with --validate=false

(note: you have to trigger an actual DNS lookup, with kubectl pointed at GKE the control plane has a bare IP address which does not give any output for say kubectl cluster-info since there's no DNS lookups going on, hence the create pointed at a domain)

@ixdy
Copy link
Member Author

ixdy commented May 23, 2018

so assuming I'm reading that right, this means this actually works?

@ixdy
Copy link
Member Author

ixdy commented May 23, 2018

/retest

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 23, 2018
@BenTheElder
Copy link
Member

BenTheElder commented May 23, 2018 via email

@bitglue
Copy link

bitglue commented May 24, 2018

@BenTheElder You shouldn't need to include the cgo option since it's used by default on OS X. From those same docs:

When cgo is available, the cgo-based resolver is used instead under a variety of conditions: on systems that do not let programs make direct DNS requests (OS X)

In any case, the debug output does show that cgo is available, so I'm 👍

Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, ixdy

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

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@cblecker
Copy link
Member

/milestone v1.11
/status approved-for-milestone

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@BenTheElder @cblecker @ixdy

Action required: This pull request requires label changes. If the required changes are not made within 3 days, the pull request will be moved out of the v1.11 milestone.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.
sig owner: Must specify at least one label prefixed with sig/.

Help

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 64338, 64219, 64486, 64495, 64347). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 835268e into kubernetes:master May 31, 2018
@k8s-ci-robot
Copy link
Contributor

@ixdy: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-bazel-test e4ded2b link /test pull-kubernetes-bazel-test
pull-kubernetes-e2e-gce-device-plugin-gpu e4ded2b link /test pull-kubernetes-e2e-gce-device-plugin-gpu
pull-kubernetes-kubemark-e2e-gce-big e4ded2b link /test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

@warmchang
Copy link
Contributor

what is the label "¯_(ツ)_/¯" mean? 😄

image

@dims
Copy link
Member

dims commented Jun 6, 2018

@warmchang see meaning here - https://emojipedia.org/shrug/

@rohitagarwal003
Copy link
Member

The original motivation for adding these environment variables (DNS resolution on macOS) has been fixed in Go 1.20 (golang/go#12524 (comment))

So, assuming that these environment variables aren't being used for something else, we can clean this up now.

@dims
Copy link
Member

dims commented Jun 1, 2023

what is the label "¯_(ツ)_/¯" mean? 😄

🤷 (shrug)

@BenTheElder
Copy link
Member

The original motivation for adding these environment variables (DNS resolution on macOS) has been fixed in Go 1.20 (golang/go#12524 (comment))

Awesome!

So, assuming that these environment variables aren't being used for something else, we can clean this up now.

This is a big assumption 5 years later, we don't know who else is using this and for what reasons.

I imagine some distros will still want to build in cgo mode because of quirks between the resolvers though, even if the gap has closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/incomplete-labels release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants