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

[WIP] Add alternative debian based base image #7593

Closed
wants to merge 2 commits into from

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Sep 4, 2021

Fixes partially: #7518
This is also an attempt to optimize / solve coredumps problems in #6896

On an attempt to solve random coredump problems, and have some standard image building this PR:

  • Changes the base image from Alpine to Debian
  • Splits the base image in two: nginx and mod_security

The old all in one alpine image got 184Mb, while the new images got 164mb (base NGINX) and 77.9Mb (modsecurity).

Although the sum of the images grows (mostly due to glibc and not optimizing Debian Image the same way it's done in https://github.com/kubernetes/release/tree/master/images/build/debian-base/bullseye, still the reduction for the majority of users will be 20Mb (as modsecurity is going to be optional now)

Before merging this PR, we should wait for kubernetes/test-infra#23478 to get merged

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 4, 2021
@k8s-ci-robot
Copy link
Contributor

@rikatz: This issue is currently awaiting triage.

If Ingress contributors 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 4, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz

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 Sep 4, 2021
@rikatz rikatz mentioned this pull request Sep 4, 2021
2 tasks
@rikatz rikatz requested review from strongjz and tao12345666333 and removed request for bowei September 4, 2021 04:49
Copy link
Member

@tao12345666333 tao12345666333 left a comment

Choose a reason for hiding this comment

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

leave some comments.

images/modsecurity/Makefile Outdated Show resolved Hide resolved
@@ -0,0 +1,24 @@
timeout: 10800s
Copy link
Member

Choose a reason for hiding this comment

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

Should we add license header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cloud build files don't have license header (no idea why)

https://github.com/kubernetes/ingress-nginx/blob/main/cloudbuild.yaml

Comment on lines +23 to +24
_GIT_TAG: "12345"
_PULL_BASE_REF: "master"
Copy link
Member

Choose a reason for hiding this comment

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

These lines should be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this is somehow the way cloudbuild understands this

export BUILD_PATH=/tmp/build

# TODO: Verify and add the same libraries (but not dev) in main container
apt-get install -y \
Copy link
Member

Choose a reason for hiding this comment

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

we should runapt-get update before this.

Or execute before this script, like the Dockerfile 👆

Copy link
Member

Choose a reason for hiding this comment

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

apt-get has a --no-install-recommends flag, we can use it to reduce install packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though I was using --no-install-recommends (actually I am, in debian image...)

Will review all again :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum I remember now why I didn't added --no-install-recommends here: because modsecurity building is kind of annoying, and we are just using this image to copy the end result (libmodsecurity) to the other image (which is properly a busybox).

Do you think still we should do a --no-install-recommends? About the update I agree, should always update the builder image

Copy link
Member

Choose a reason for hiding this comment

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

Do you think still we should do a --no-install-recommends?

Based on the reasons you mentioned above, I think it is not necessary to add --no-install-recommends flag

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can move forward

@clementnuss
Copy link

Is the image already available for download ? I'm interested in testing it, due to the infamous musl-libc/lua nginx crashes.
Tried to search it but only managed to download the alpine based nginx(-controller) images

Thanks for your work 👏🏻

@rikatz
Copy link
Contributor Author

rikatz commented Sep 7, 2021

Is the image already available for download ? I'm interested in testing it, due to the infamous musl-libc/lua nginx crashes.
Tried to search it but only managed to download the alpine based nginx(-controller) images

Thanks for your work 👏🏻

It's a WIP yet and we are waiting the test-infra promoter part to work.

I will fix some things here and can generate an image locally, if you want to test :)

@rikatz rikatz closed this Sep 7, 2021
@rikatz rikatz reopened this Sep 7, 2021
@rikatz
Copy link
Contributor Author

rikatz commented Sep 7, 2021

Damn clicked wrong on button, sorry

@@ -0,0 +1,27 @@
# Copyright 2015 The Kubernetes Authors. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the year here

@@ -0,0 +1,57 @@
# Copyright 2017 The Kubernetes Authors. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here

@@ -0,0 +1,68 @@
# Copyright 2015 The Kubernetes Authors. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here

@@ -0,0 +1,606 @@
#!/bin/bash

# Copyright 2015 The Kubernetes Authors.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also here...

@rikatz
Copy link
Contributor Author

rikatz commented Sep 7, 2021

@tao12345666333 the staging push job is merged.

As soon as you think this is good and gets merged, we can start using the image in staging bucket to write some e2e tests and maybe release a non official debian image so folks can test :)

@dmathieu dmathieu mentioned this pull request Dec 6, 2021
9 tasks
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 6, 2021
@tao12345666333
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 7, 2021
@rikatz rikatz closed this Dec 30, 2021
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. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants