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

Revert "Build non-static binaries with PIE buildmode" #105352

Merged
merged 1 commit into from Oct 1, 2021
Merged

Revert "Build non-static binaries with PIE buildmode" #105352

merged 1 commit into from Oct 1, 2021

Conversation

ehashman
Copy link
Member

@ehashman ehashman commented Sep 29, 2021 •
edited

Reverts #102323

In #105294 we identified that this change has caused a ~30% memory utilization increase for the Kubelet in 1.22.

As far as I'm aware, there were no comms from SIG Release on making this change on k-dev or in the release note regarding what the possible impact would be; we found the memory increase while investigating regressions and had to perform a full git tree bisect to pinpoint it.

I see there is discussion in #102323 around the individual binary sizes, but not on ASLR's impact on memory utilization. If this impacted Kubelet, it likely impacted other components as well.

I'd like to have a wider discussion on the impact of turning this feature on. Have we seen any evidence of attacks against Golang binaries that exploit this? +30% is a significant memory regression, especially in resource-constrained environments, and I want to make sure that we understand the tradeoffs.

/sig node release security
/kind regression
/priority important-soon

Revert building binaries with PIE mode.

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/release Categorizes an issue or PR as relevant to SIG Release. 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/security Categorizes an issue or PR as relevant to SIG Security. kind/regression Categorizes issue or PR as related to a regression from a prior release. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed 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 Sep 29, 2021
@dims
Copy link
Member

dims commented Sep 29, 2021

@saschagrunert @kubernetes/sig-release fyi

@dims
Copy link
Member

dims commented Sep 29, 2021

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 29, 2021
@ehashman
Copy link
Member Author

/cc @saschagrunert @BenTheElder

spiffxp
spiffxp approved these changes Sep 29, 2021
View reviewed changes
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 29, 2021
@ehashman
Copy link
Member Author

/hold

want to ensure tagged folks have a chance to review

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021
@pacoxu pacoxu added this to Waiting on Author in SIG Node PR Triage Sep 30, 2021
@ehashman
Copy link
Member Author

Ah, @saschagrunert is OOO for another week, looks like we got an ack from @justaugustus so I will remove the hold.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2021
@spiffxp
Copy link
Member

spiffxp commented Sep 30, 2021

/test pull-kubernetes-node-e2e-containerd
ref: #105381

@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

1 similar comment
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@BenTheElder
Copy link
Member

I wasn't particularly keen on that change 🙃 #102323 (comment)

Regarding wider discussion: I don't think we have a good pattern for this more generally yet, (see previously: bazel, hyperkube, ...), it's hard to bring in everyone on anything and not clear what the right set is. I at least blocked this on SIG security input and we have build + SIG release OWNERS (incl myself).

Clearly this change needs wider discussion though.

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, ehashman, spiffxp

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

@BenTheElder
Copy link
Member

/retest

@BenTheElder
Copy link
Member

err, and this part seems like a major oversight 😞

As far as I'm aware, there were no comms from SIG Release on making this change on k-dev or in the release note regarding what the possible impact would be; we found the memory increase while investigating regressions and had to perform a full git tree bisect to pinpoint it.

@rphillips
Copy link
Member

This PR should be backported to 1.22 if someone can get to it.

@k8s-ci-robot k8s-ci-robot merged commit e1b94fd into kubernetes:master Oct 1, 2021
SIG Node PR Triage automation moved this from Waiting on Author to Done Oct 1, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Oct 1, 2021
@liggitt liggitt mentioned this pull request Oct 4, 2021
Merged
@liggitt
Copy link
Member

liggitt commented Oct 4, 2021

pick open at #105452

This was referenced Oct 4, 2021
Merged
Merged
Closed
Closed
Closed
Closed
Merged
k8s-ci-robot added a commit that referenced this pull request Oct 7, 2021
@k8s-ci-robot
Merge pull request #105452 from liggitt/automated-cherry-pick-of-#105…
cb64861 
…352-upstream-release-1.22

Automated cherry pick of #105352: Revert "Build non-static binaries with PIE buildmode"
@openshift-ci-robot openshift-ci-robot mentioned this pull request Oct 29, 2021
Closed
This was referenced Nov 10, 2021
Merged
Merged
@philnichol philnichol mentioned this pull request Jan 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spiffxp spiffxp spiffxp approved these changes

@BenTheElder BenTheElder Awaiting requested review from BenTheElder

@saschagrunert saschagrunert Awaiting requested review from saschagrunert

@lavalamp lavalamp Awaiting requested review from lavalamp

@xichengliudui xichengliudui Awaiting requested review from xichengliudui

Assignees

@spiffxp spiffxp

@BenTheElder BenTheElder

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. kind/regression Categorizes issue or PR as related to a regression from a prior release. 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/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/security Categorizes an issue or PR as relevant to SIG Security. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node PR Triage
Done
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ehashman @dims @spiffxp @k8s-triage-robot @BenTheElder @k8s-ci-robot @rphillips @liggitt