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

actions: debootstrap: Add parent-suite property to indicate which suite a downstream is based on #424

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,12 @@ jobs:
test:
- { name: "recipes", case: "recipes" }
- { name: "templating", case: "templating", variables: " -t escaped:\\$ba\\'d\\$gers\\ snakes" }
- { name: "debian (amd64)", case: "debian", variables: "-t architecture:amd64" }
- { name: "debian (arm64)", case: "debian", variables: "-t architecture:arm64" }
- { name: "debian (armhf)", case: "debian", variables: "-t architecture:armhf" }
- { name: "debian (bookworm amd64)", case: "debian", variables: "-t architecture:amd64 -t suite:bookworm" }
- { name: "debian (bookworm arm64)", case: "debian", variables: "-t architecture:arm64 -t suite:bookworm" }
- { name: "debian (bookworm armhf)", case: "debian", variables: "-t architecture:armhf -t suite:bookworm" }
- { name: "debian (trixie amd64)", case: "debian", variables: "-t architecture:amd64 -t suite:trixie" }
- { name: "debian (trixie arm64)", case: "debian", variables: "-t architecture:arm64 -t suite:trixie" }
- { name: "debian (trixie armhf)", case: "debian", variables: "-t architecture:armhf -t suite:trixie" }
Copy link
Member

Choose a reason for hiding this comment

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

this massively extends the number of test jobs; Does testing trixie on arm64/armhf give us extra data? Also i'm a tad wary about it breaking CI non-deterministically

Copy link
Member

Choose a reason for hiding this comment

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

as was done to tune our runs; for trixie for now please just build amd64 and only on kvm

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah probably best to remove trixie

exclude:
- backend: nofakemachine
test: { name: "partitioning", case: "partitioning" }
Expand All @@ -136,6 +139,12 @@ jobs:
test: { name: "arch", case: "arch" }
- backend: kvm
test: { name: "apertis", case: "apertis" }
- backend: kvm
test: { name: "kali", case: "kali" }
Copy link
Member

Choose a reason for hiding this comment

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

Does kali not build without this? Fwiw i'm wary about adding rolling releases as they might break CI too easily

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello Sjoerd,

Does kali not build without this

Nope, it doesn't, since it's based on Debian testing. I tried with latest debos from the Docker Hub, just to be sure:

$ cat recipe-kali.yaml 
architecture: amd64
actions:
  - action: debootstrap
    mirror: http://kali.download/kali/
    suite: kali-rolling
    components: [ main ]
    keyring-file: kali-archive-keyring.gpg
    keyring-package: kali-archive-keyring
  - action: apt
    packages: [ procps ]

$ podman run --rm -it --device /dev/kvm --workdir /recipes \
  --mount "type=bind,source=$(pwd),destination=/recipes" \
  --security-opt label=disable \
  godebos/debos:main-20240113 recipe-kali.yaml

Running /debos --artifactdir /recipes /recipes/recipe-kali.yaml using kvm backend
2024/01/15 04:49:58 ==== debootstrap ====
2024/01/15 04:49:58 excluding usr-is-merged as package is not in suite
2024/01/15 04:49:58 Debootstrap | I: Retrieving InRelease 
[...]
2024/01/15 04:51:41 Debootstrap | I: Base system installed successfully.
2024/01/15 04:51:42 ==== apt ====
2024/01/15 04:51:42 apt | Hit:1 http://kali.download/kali kali-rolling InRelease
2024/01/15 04:51:43 apt | Reading package lists...
2024/01/15 04:51:43 apt | Reading package lists...
2024/01/15 04:51:43 apt | Building dependency tree...
2024/01/15 04:51:43 apt | procps is already the newest version (2:4.0.4-2+b1).
2024/01/15 04:51:43 apt | You might want to run 'apt --fix-broken install' to correct these.
2024/01/15 04:51:43 apt | The following packages have unmet dependencies:
2024/01/15 04:51:43 apt |  init-system-helpers : Depends: usrmerge but it is not going to be installed or
2024/01/15 04:51:43 apt |                                 usr-is-merged
2024/01/15 04:51:43 apt | E: Unmet dependencies. Try 'apt --fix-broken install' with no packages (or specify a solution).
2024/01/15 04:51:43 Action `apt` failed at stage Run, error: exit status 100

I suppose all derivatives based on Debian testing/sid are affected.

Note that we can (and we do) workaround by adding this just after the debootstrap step:

  - description: "Install usr-is-merged (cf. debos #361 and #362)"
    action: apt
    packages: [ usr-is-merged ]

So it's not a dealbreaker, we can live without the MR. But it's nicer when we don't have to use cryptic workarounds.

i'm wary about adding rolling releases as they might break CI too easily

Debootstrapping Kali rolling is just like debootstrapping Debian testing, only two packages in the set are forked, the rest is just Debian testing. Usually kali-dev (more or less equivalent to Debian sid) breaks every now and then, but we have solid QA that keeps kali-rolling working.

You might want to use the mirror kali.download instead of http.kali.org, so you'll hit Cloudflare CDN rather than our redirector, for maximum reliability.

Thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to keep the Kali test in - with the mirror change @elboulangero mentions above.

@sjoerdsimons are you happy with that ?

- backend: kvm
test: { name: "apertis v2022", case: "apertis", variables: "-t suite:v2022" }
obbardc marked this conversation as resolved.
Show resolved Hide resolved
- backend: kvm
test: { name: "apertis v2024dev3", case: "apertis", variables: "-t suite:v2024dev3 -t parent_suite:bookworm" }
obbardc marked this conversation as resolved.
Show resolved Hide resolved
name: ${{matrix.test.name}} on ${{matrix.backend}}
runs-on: ${{ matrix.backend == 'kvm' && 'kvm' || 'ubuntu-latest' }}
steps:
Expand Down
58 changes: 51 additions & 7 deletions actions/debootstrap_action.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,19 @@ Example:
- certificate -- client certificate stored in file to be used for downloading packages from the server.

- private-key -- provide the client's private key in a file separate from the certificate.

- parent-suite -- release code name which this suite is based on. Useful for downstreams which do
not use debian codenames for their suite names (e.g. "stable").
Comment on lines +50 to +51
Copy link
Member

Choose a reason for hiding this comment

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

Fwiw this could be used for non-debian parents as well.. E.g. parent-suite could be jammy for ubuntu derivatives (or kali for kali derivatives). We should require the parent-suite to be known by debootstrap (as in a script exists) if given. (potentially this requirement should only exist if there is no script for suite, but in that case this property is useless).


- script -- the full path of the script to use to build the target rootfs. (e.g. `/usr/share/debootstrap/scripts/kali`)
If unspecified, the property will be automatically determined in the following order,
with the path "/usr/share/debootstrap/scripts/" prepended:
`suite` property, `parent-suite` property then `unstable`.
*/
package actions

import (
"errors"
"fmt"
"io"
"log"
Expand All @@ -64,6 +73,7 @@ import (

type DebootstrapAction struct {
debos.BaseAction `yaml:",inline"`
ParentSuite string `yaml:"parent-suite"`
Suite string
Mirror string
Variant string
Expand All @@ -74,6 +84,7 @@ type DebootstrapAction struct {
Components []string
MergedUsr bool `yaml:"merged-usr"`
CheckGpg bool `yaml:"check-gpg"`
Script string
}

func NewDebootstrapAction() *DebootstrapAction {
Expand Down Expand Up @@ -115,6 +126,10 @@ func (d *DebootstrapAction) Verify(context *debos.DebosContext) error {
return fmt.Errorf("suite property not specified")
}

if len(d.ParentSuite) == 0 {
d.ParentSuite = d.Suite
}

files := d.listOptionFiles(context)

// Check if all needed files exists
Expand Down Expand Up @@ -163,9 +178,9 @@ func (d *DebootstrapAction) RunSecondStage(context debos.DebosContext) error {
return err
}

// Guess if suite is something before usr-is-merged was introduced
func (d *DebootstrapAction) isLikelyOldSuite() bool {
switch strings.ToLower(d.Suite) {
// Check if suite is something before usr-is-merged was introduced
func shouldExcludeUsrIsMerged(suite string) bool {
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 flip this function around so the default is false as it won't be needed for newer suites (whose set will increase) but will be for older debian ones (whose set won't change).. It also should default to false for newer debian derivatives (using >= bookworm) .

fwiw debootstrap has the following list: etch*|lenny|squeeze|wheezy|jessie*|stretch|buster|bullseye

no need to deal with jessie* though given it is completely unsupported since 2020

switch strings.ToLower(suite) {
case "sid", "unstable":
return false
case "testing":
Expand All @@ -181,6 +196,10 @@ func (d *DebootstrapAction) isLikelyOldSuite() bool {
}
}

func getDebootstrapScriptPath(script string) string {
return path.Join("/usr/share/debootstrap/scripts/", script)
}

func (d *DebootstrapAction) Run(context *debos.DebosContext) error {
cmdline := []string{"debootstrap"}

Expand Down Expand Up @@ -226,16 +245,41 @@ func (d *DebootstrapAction) Run(context *debos.DebosContext) error {
cmdline = append(cmdline, fmt.Sprintf("--variant=%s", d.Variant))
}

// workaround for https://github.com/go-debos/debos/issues/361
if d.isLikelyOldSuite() {
log.Println("excluding usr-is-merged as package is not in suite")
if shouldExcludeUsrIsMerged(d.ParentSuite) {
log.Printf("excluding usr-is-merged as package is not in parent suite %s\n", d.ParentSuite)
cmdline = append(cmdline, "--exclude=usr-is-merged")
}

cmdline = append(cmdline, d.Suite)
cmdline = append(cmdline, context.Rootdir)
cmdline = append(cmdline, d.Mirror)
cmdline = append(cmdline, "/usr/share/debootstrap/scripts/unstable")

if len(d.Script) > 0 {
if _, err := os.Stat(d.Script); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

this should be done in verify not at run time

return fmt.Errorf("cannot find debootstrap script %s", d.Script)
}
} else {
/* Auto determine debootstrap script to use from d.Suite, falling back to
d.ParentSuite if it doesn't exist. Finally, fallback to unstable if a
script for the parent suite does not exist. */
for _, s := range []string{d.Suite, d.ParentSuite, "unstable"} {
d.Script = getDebootstrapScriptPath(s)
if _, err := os.Stat(d.Script); err == nil {
break
} else {
log.Printf("cannot find debootstrap script %s\n", d.Script)

/* Unstable should always be available so error out if not */
if s == "unstable" {
return errors.New("cannot find debootstrap script for unstable")
}
}
}

log.Printf("using debootstrap script %s\n", d.Script)
}

cmdline = append(cmdline, d.Script)

/* Make sure /etc/apt/apt.conf.d exists inside the fakemachine otherwise
debootstrap prints a warning about the path not existing. */
Expand Down
11 changes: 8 additions & 3 deletions tests/apertis/test.yaml
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
---
# Test building a non-debian distribution such as apertis to ensure
# bootstrapping suites that debootstrap won't internally know about works
{{- $architecture := or .architecture "amd64"}}
architecture: {{$architecture}}

{{- $architecture := or .architecture "amd64" }}
{{- $suite := or .suite "v2022" }}
{{- $parent_suite := or .parent_suite "" }}

architecture: {{ $architecture }}

actions:
- action: debootstrap
suite: v2022
suite: {{ $suite }}
parent-suite: {{ $parent_suite }}
components:
- target
mirror: https://repositories.apertis.org/apertis/
Expand Down
8 changes: 5 additions & 3 deletions tests/debian/test.yaml
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
---
{{- $architecture := or .architecture "amd64"}}
architecture: {{$architecture}}
{{- $architecture := or .architecture "amd64" }}
{{- $suite := or .suite "bookworm" }}

architecture: {{ $architecture }}

actions:
- action: debootstrap
suite: bullseye
suite: {{ $suite }}
variant: minbase
merged-usr: true

Expand Down
Binary file added tests/kali/kali-archive-keyring.gpg
Binary file not shown.
23 changes: 23 additions & 0 deletions tests/kali/test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
---
# Test building a non-debian distribution based on unstable such as kali-rolling
# to ensure bootstrapping suites that debootstrap won't internally know about
# works
{{- $architecture := or .architecture "amd64" }}

architecture: {{ $architecture }}

actions:
- action: debootstrap
suite: kali-rolling
parent-suite: testing
components:
- main
mirror: https://http.kali.org/kali/
variant: minbase
keyring-package: kali-archive-keyring
keyring-file: kali-archive-keyring.gpg

- action: apt
description: Install some base packages
packages:
- procps