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

feat: NVSHAS-9501 support SLSA L3 #27

Closed
wants to merge 2 commits into from

Conversation

holyspectral
Copy link
Collaborator

  1. Move the Jenkins build to GHA as dev.yml and dev-arm64.yml
  2. Provide release.yml as SLSA-compliant release build.

@flavio
Copy link

flavio commented Oct 9, 2024

From a quick look:

  • I would use a matrix to merge the arm64 and x86_64 jobs into a single file
  • The commit about the log levels doesn't belong to this PR

To make this feature work, add below secrets and variables to the repo:

1. DOCKER_REPO secret
2. DOCKER_USERNAME secret
3. DOCKER_PASSWORD secret
4. PRIME_REGISTRY secret
5. PRIME_REPO secret
6. PRIME_REGISTRY_USERNAME secret
7. PRIME_REGISTRY_PASSWORD secret
8. DEV_BUILD_REPOSITORY variable
@holyspectral
Copy link
Collaborator Author

holyspectral commented Oct 9, 2024

@flavio Thanks for the comment!

I would use a matrix to merge the arm64 and x86_64 jobs into a single file

Yes I agree that will be much simple in the code level if we merge them into one file. There are two reasons why it's separate right now:

  1. Many of our components don't support ARM64 right now, so the errors on ARM64 build shouldn't break AMD64 build.
  2. ARM64 takes more time to build as it's emulated. Currently AMD64 build takes only 2.5 minutes, while ARM64 takes 18 mins. Having them separate will save my time to do the test.

I will revisit this once we setup GitHub-hosted ARM64 runners in neuvector, which should speed up the ARM64 build.

Also, according to GitHub blog, We expect to begin offering Arm runners for open source and personal accounts by the end of the year.. Hope folks can start using this in forks soon.

The commit about the log levels doesn't belong to this PR

Oops, it's fixed now. Thanks for noticing it.


RUN cd /usr/bin/ && rm -rf basename chcon chgrp chmod chown chroot cksum dd df dircolors dirname du install install-info join locale localedef mkdir mkfifo mknod mktemp paste pathchk readlink realpath sync smidiff smidump smilink smiquery smistrip smixlate tee tiemout tload top truncate unlink watch

LABEL name="registry-adapter" \
Copy link

Choose a reason for hiding this comment

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

Please note that by using BCI images, you may need to overwrite some specific labels, or you will end-up with labels from the base image instead. Here's more context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. It's very good to know.

RUN cd /usr/bin/ && rm -rf basename chcon chgrp chmod chown chroot cksum dd df dircolors dirname du install install-info join locale localedef mkdir mkfifo mknod mktemp paste pathchk readlink realpath sync smidiff smidump smilink smiquery smistrip smixlate tee tiemout tload top truncate unlink watch

LABEL name="registry-adapter" \
vendor="SuSE Security" \
Copy link

Choose a reason for hiding this comment

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

Should this not be all capitals these days?

Suggested change
vendor="SuSE Security" \
vendor="SUSE Security" \

FROM neuvector/adapter_base:${BASE_IMAGE_TAG}
#
# Builder image
FROM registry.suse.com/bci/golang:1.22.7 AS builder
Copy link

Choose a reason for hiding this comment

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

Either lean on a higher level mutable tag, or add renovate/updatecli to ensure this is being kept up to date.

Suggested change
FROM registry.suse.com/bci/golang:1.22.7 AS builder
FROM registry.suse.com/bci/golang:1.22 AS builder

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jayhuang-suse any concern about this? I don't think Golang would bring breaking changes in minor update, so I'm okay with using 1.22.

COPY --from=base /chroot/ /
COPY --from=builder /src/stage /

RUN cd /usr/bin/ && rm -rf basename chcon chgrp chmod chown chroot cksum dd df dircolors dirname du install install-info join locale localedef mkdir mkfifo mknod mktemp paste pathchk readlink realpath sync smidiff smidump smilink smiquery smistrip smixlate tee tiemout tload top truncate unlink watch
Copy link

Choose a reason for hiding this comment

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

This creates a new layer on the final image, so although the binaries won't be shipped, they would already be present on higher layers which translate in little to no gains in final image size. An alternative here, is to do this on an intermediate layer, and just copy that over to a final scratch image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah this is the code that the current pipeline has. I will check to see if we should do it with SLSA scope or later. Thanks for letting us know!

Comment on lines +25 to +26
ifneq ($(DRONE_TAG),)
GIT_TAG = $(DRONE_TAG)
Copy link

Choose a reason for hiding this comment

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

Suggested change
ifneq ($(DRONE_TAG),)
GIT_TAG = $(DRONE_TAG)
ifneq ($(GITHUB_TAG),)
GIT_TAG = $(GITHUB_TAG)

DIRTY = -dirty
endif

# Prioritise DRONE_TAG for backwards compatibility. However, the git tag
Copy link

@pjbgf pjbgf Oct 9, 2024

Choose a reason for hiding this comment

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

Assuming that we are leaning on GitHub level tags.

Suggested change
# Prioritise DRONE_TAG for backwards compatibility. However, the git tag
# Prioritise GITHUB_TAG for backwards compatibility. However, the git tag

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code comes from cis-operator and I think that's for DRONE only. I will remove this part.

Comment on lines +20 to +22
matrix:
include:
- platforms: linux/amd64,linux/arm64
Copy link

Choose a reason for hiding this comment

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

If a single multi-arch image is to be published, this is not needed.

uses: rancher/ecm-distro-tools/actions/publish-image@master
with:
image: registry-adapter
tag: ${{ github.ref_name }}${{ matrix.tag-suffix }}
Copy link

Choose a reason for hiding this comment

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

Same here:

Suggested change
tag: ${{ github.ref_name }}${{ matrix.tag-suffix }}
tag: ${{ github.ref_name }}

@holyspectral
Copy link
Collaborator Author

I will be working on the PR a little more. Closing it for now to prevent spam. :-)

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.

3 participants