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

disable network for package builds, use only ADD to get network things, fix any packages that need changes to build #2861

Closed
wants to merge 1 commit into from

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Oct 16, 2022

This PR:

  1. Removes network access for RUN statements except for very selected packages (pkg/alpine, pkg/fscrypt)
  2. changes any pkg/ directories to fix anything that depended on RUN network access

This forces the arbitrary RUN curl or RUN git clone or RUN wget (etc. etc.) to be converted to using ADD https://.... This is not any more efficient or better, except in one way: it makes it trivial to scan the Dockerfile to find all downloaded sources.

Without this, the number of permutations is very large and scanning a Dockerfile is nearly impossible.

Most software scanners to build a software bill of materials (SBoM) do a pretty good job of scanning the final image for OS packages (apk), and a pretty good job of scanning source directories for compilation source (go or rust). What they do not do is capture things downloaded as part of the Dockerfile (RUN curl, RUN wget, RUN git clone, etc. etc.).

This does not help them find it, but it does make all additional downloads caused by precisely one command (ADD) and one format (url after the ADD command). This, in turn, makes it possible to request updates to the SBoM scanners, or augment them with something that does it. But at least it is sane and doable.

Note that normal ADD does not yet support git clones, so we use the upstream frontend support, targeted at 1.5-lab.

It also moves fscrypt download out of pillar into its own dedicated package, for network access.

It also reorders some of pkg/grub/Dockerfile to resolve dependency issues on riscv64.

Based on discussions with @eriknordmark

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

There are some git clone/checkout which specify both a branch to clone from and a commit to checkout. Can we handle that with zipball as well? (Example in pkg/uefi/Dockerfile)

I guess if we use ADD across the board if means we might end up picking up some conditional ones and later deciding not to use them? (pkg/uefi/Dockerfile and pkg/fw/Dockerfile are examples of this.)

@deitch
Copy link
Contributor Author

deitch commented Oct 17, 2022

There are some git clone/checkout which specify both a branch to clone from and a commit to checkout. Can we handle that with zipball as well? (Example in pkg/uefi/Dockerfile)

Not all git repos support it. GitHub does, indeed. I ran an experiment using one of them in pkg/uefi/Dockerfile:

ENV SBI_COMMIT cbaa9b0333517b3c25bea8d1c71ac8005ff1f727
RUN if [ "$(uname -m)" = riscv64 ]; then \
        git clone https://github.com/riscv/opensbi.git /opensbi && \
        git -C /opensbi checkout ${SBI_COMMIT}; \
    fi
ubuntu@ip-172-31-15-153:/tmp$ curl -L -o sbi.zip https://github.com/riscv/opensbi/zipball/cbaa9b0333517b3c25bea8d1c71ac8005ff1f727
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  498k    0  498k    0     0  1283k      0 --:--:-- --:--:-- --:--:-- 1283k
ubuntu@ip-172-31-15-153:/tmp$ ls -la sbi.zip
-rw-rw-r-- 1 ubuntu ubuntu 510098 Oct 17 17:36 sbi.zip
ubuntu@ip-172-31-15-153:/tmp$ file sbi.zip
sbi.zip: Zip archive data, at least v1.0 to extract
ubuntu@ip-172-31-15-153:/tmp$ mkdir sbi && cd sbi
ubuntu@ip-172-31-15-153:/tmp/sbi$ unzip ../sbi.zip
Archive:  ../sbi.zip
cbaa9b0333517b3c25bea8d1c71ac8005ff1f727
   creating: riscv-software-src-opensbi-cbaa9b0/
...

It downloads the source for a specific commit. Kind of convenient actually.

My assumption is that within a few months the ADD git@... will work as well. For now, this holds fine.

@eriknordmark
Copy link
Contributor

There are some git clone/checkout which specify both a branch to clone from and a commit to checkout. Can we handle that with zipball as well? (Example in pkg/uefi/Dockerfile)

FWIW The more complex case is
RUN git clone -b edk2-stable202005 https://github.com/tianocore/edk2.git /ws && git -C /ws checkout ${EDK_COMMIT}

If one can avoid specifying the branch and only specify the commit we should be fine.

@deitch
Copy link
Contributor Author

deitch commented Oct 18, 2022

The more complex case is
RUN git clone -b edk2-stable202005 https://github.com/tianocore/edk2.git /ws && git -C /ws checkout ${EDK_COMMIT}
If one can avoid specifying the branch and only specify the commit we should be fine

It should, based on how git works. I just ran an experiment with the various arch-based commits, and it works correctly. Really kind of convenient.

@deitch
Copy link
Contributor Author

deitch commented Oct 18, 2022

I will update this PR to do it for all of our packages.

@deitch
Copy link
Contributor Author

deitch commented Oct 18, 2022

Got most of them. Still working on the following:

  • acrn
  • acrn-kernel
  • grub
  • kernel
  • new-kernel
  • u-boot
  • uefi

I have solutions for almost all of them, but getting this iteratively.

@deitch deitch force-pushed the pkg-build-no-network branch 5 times, most recently from 89e68d9 to 2d4b865 Compare October 18, 2022 12:02
@deitch
Copy link
Contributor Author

deitch commented Oct 18, 2022

So far so good. I am sure there will be errors, I will fix them.

The one issue I had was with grub. Unlike the kernel or github, git.savannah.gnu.org does not have a URL to download an entire repo as a tarball or zipfile, let alone a specific commit, at least not that I can find. I am not sure what to do about that one.

Let's let all of the others have their CI run, I can fix whatever breaks, and then we can figure out how to tackle that last one.

@giggsoff
Copy link
Contributor

The one issue I had was with grub. Unlike the kernel or github, git.savannah.gnu.org does not have a URL to download an entire repo as a tarball or zipfile, let alone a specific commit, at least not that I can find. I am not sure what to do about that one.

Is it what you want to find: https://git.savannah.gnu.org/cgit/grub.git/snapshot/grub-71f9e4ac44142af52c3fc1860436cf9e432bf764.tar.gz ?

@deitch
Copy link
Contributor Author

deitch commented Oct 18, 2022

And managed to switch almost all of them to tgz from zip.

@deitch
Copy link
Contributor Author

deitch commented Oct 18, 2022

Is it what you want to find

One of these lifetimes, I am going to meet @giggsoff in person, and have to decide whether I buy him a beer for all of the times he has helped solve issues for me, or hit him over the head with it for all of the times he has made my problems look simple. 😆

pkg/xen-tools/Dockerfile Show resolved Hide resolved
pkg/kernel/Dockerfile Show resolved Hide resolved
pkg/xen/Dockerfile Outdated Show resolved Hide resolved
@deitch deitch force-pushed the pkg-build-no-network branch 2 times, most recently from c237f43 to c9cc28f Compare October 18, 2022 12:57
pkg/dom0-ztools/Dockerfile Outdated Show resolved Hide resolved
pkg/kernel/Dockerfile Outdated Show resolved Hide resolved
pkg/kernel/Dockerfile Outdated Show resolved Hide resolved
@deitch deitch force-pushed the pkg-build-no-network branch 7 times, most recently from 587b85e to 0ac9606 Compare October 18, 2022 15:19
@deitch
Copy link
Contributor Author

deitch commented Nov 3, 2022

As an alternative solution to not have separate docker image per program and to move to the single repository: #2901

Let's keep that one in our pocket. The reason I am hesitant is that rebuilding alpine is an expensive process with lots of downstream impacts. So if we decide, e.g. to change fscrypt build or version, we would have to update eve-alpine and, by extension, possibly everything else. This limits the impact to just one image and a single downstream (pillar).

If that sounds like from experience, yeah, I have had to rebuild linuxkit alpine more than a few times when it had non-apk dependencies inside. I then spent time ripping them into intermediate containers for just this reason. 😢

@giggsoff
Copy link
Contributor

giggsoff commented Nov 3, 2022

For now, I did it with a separate pkg/fscrypt

If we will decide to use separate pkg, please do not forget to request the docker image https://github.com/lf-edge/eve/blob/master/docs/BUILD.md#building-packages.

@deitch
Copy link
Contributor Author

deitch commented Nov 3, 2022

Yeah, good point. I will take care of that so it works. We always can delete it if we do not want it.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

I think the PR description needs an update since this touches more than 2 Dockerfiles now.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Also, can you add the lines to shut up the hadolint complaints which have a number (or fix the code in the case of prevent globbing?).

That way we hopefully drop below 10 complaints so we can see all of them.

@deitch deitch changed the title disable network and use only ADD to get network things disable network for package builds, use only ADD to get network things, fix any packages that need changes to build Nov 3, 2022
@deitch
Copy link
Contributor Author

deitch commented Nov 3, 2022

I think the PR description needs an update since this touches more than 2 Dockerfiles now.

Done

@deitch
Copy link
Contributor Author

deitch commented Nov 3, 2022

Also, can you add the lines to shut up the hadolint complaints which have a number (or fix the code in the case of prevent globbing?).

I couldn't find any way to tell hadolint to ignore complaints that do not have a code. I raised an issue with them.

I will add the correct ignore statements for those with a code.

…syntax for cases that *really* need ADD git

Signed-off-by: Avi Deitcher <avi@deitcher.net>
@deitch
Copy link
Contributor Author

deitch commented Nov 3, 2022

Added ignore statements

@deitch
Copy link
Contributor Author

deitch commented Nov 3, 2022

Failed for cache miss

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Re-run eden

@eriknordmark
Copy link
Contributor

@deitch GH actions can't build this; 5 tries at https://github.com/lf-edge/eve/actions/runs/3384723653

So I think need to do a few packages at a time to reduce the build time. Perhaps split out the kernel/new-kernel/acrn-kernel changes into a separate PR and see if that one can build and test? Then see if the rest is sane or if we need more splits.

@deitch
Copy link
Contributor Author

deitch commented Nov 8, 2022

The build to which you link has this error:

2022-11-08T14:47:40.7173334Z #9 https://ftpmirror.gnu.org/autoconf-archive/autoconf-archive-2019.01.06.tar.xz.sig
2022-11-08T14:47:40.7176156Z #9 ERROR: Get "https://gnu.askapache.com/autoconf-archive/autoconf-archive-2019.01.06.tar.xz.sig": net/http: TLS handshake timeout
2022-11-08T14:47:40.8294944Z 

I am able to download it, so why is it failing there?

I see what you mean about the kernel. Each one takes about 1:30-2:00 hrs, so changing those is pretty major. Do you think this is just timing out and getting cut, and that is why the curl is failing?

I don't think anything stops us from splitting this into multiple PRs:

  1. Update Makefile.
  2. Update kernel
  3. Update new-kernel
  4. Update acrn-kernel
  5. Update all of the rest

If that is the issue, I am happy to do that.

The other thing we could think about is sharding pkg builds so that they are 1 shard each for kernel, new-kernel, acrn-kernel, everything else. I wouldn't do that for this, but in general it might be a good idea.

Let me know about this issue.

@eriknordmark
Copy link
Contributor

I am able to download it, so why is it failing there?

I don't know. DId the 4 previous runs fail the same way? I was assuming it is running for too long hence getting killed resulting on various different errors. But that's just a guess without much data,

@deitch
Copy link
Contributor Author

deitch commented Nov 8, 2022

Either way, it cannot hurt. I will split it right away.

@deitch
Copy link
Contributor Author

deitch commented Nov 8, 2022

#2911 and then more coming once that one is in.

@deitch
Copy link
Contributor Author

deitch commented Nov 9, 2022

#2913 is the second part: all packages without network except for the 3 kernel packages; those will come next.

@deitch
Copy link
Contributor Author

deitch commented Nov 13, 2022

With #2913 in, now adding pkg/kernel #2920 and pkg/new-kernel #2921

@deitch
Copy link
Contributor Author

deitch commented Nov 13, 2022

And, hopefully the last, pkg/acrn-kernel in #2922 , after which we will be able to close this one. 🎉

@deitch
Copy link
Contributor Author

deitch commented Nov 15, 2022

This is great. Just pkg/kernel left to merge in #2920 and we will be done with this @eriknordmark

@deitch
Copy link
Contributor Author

deitch commented Nov 16, 2022

With #2920 in, this PR no longer is necessary. We have success!

@deitch deitch closed this Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants