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

Make the notary version detectable #37

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

LaurentGoderre
Copy link
Member

@LaurentGoderre LaurentGoderre commented Jul 11, 2024

Syft is able to use the ldflags from go to properly detect the main version:

https://github.com/anchore/syft/blob/main/syft/pkg/cataloger/golang/parse_go_binary_test.go#L1034-L1039

In order to make this work, we would need to enable that feature in the Scout Scanner

@LaurentGoderre LaurentGoderre marked this pull request as ready for review July 23, 2024 20:29
@LaurentGoderre LaurentGoderre marked this pull request as draft July 23, 2024 20:29
@LaurentGoderre LaurentGoderre marked this pull request as ready for review August 6, 2024 20:48
Copy link
Member

@tianon-sso tianon-sso left a comment

Choose a reason for hiding this comment

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

We'll also probably need to bump the Alpine version 😭

Copy link

@whalelines whalelines left a comment

Choose a reason for hiding this comment

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

Can the rationale for moving from an intermediate image to a two-stage build be given in the PR description?

Comment on lines -13 to -16
- run: |
docker build notary-builder --tag notary:builder
tag="$(docker run --rm notary:builder sh -c 'echo $TAG' | awk '{gsub(/^v/, ""); print}')"
docker tag notary:builder "notary:${tag}-builder"

Choose a reason for hiding this comment

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

Why is the builder variant being removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -1,3 +1,31 @@
FROM golang:1.19-alpine{{ .alpine }} AS build

Choose a reason for hiding this comment

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

Perhaps #36 should just be closed and its changes included in this PR?

WORKDIR /go/src/$NOTARYPKG
RUN set -eux; \
git clone -b "$TAG" --depth 1 "https://$NOTARYPKG.git" .; \
# In case the version in file doens't match the tag (like in 0.7.0)

Choose a reason for hiding this comment

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

Suggested change
# In case the version in file doens't match the tag (like in 0.7.0)
# In case the version in file doesn't match the tag (like in 0.7.0)

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

When version 0.7.0 was release, the main file used in the code to determine the version wasn't bumped.

This is the 0.7.0 commit where the version is wrong.

https://github.com/notaryproject/notary/blob/b0b6bfdd4933081e8d5ae026b24e8337311dd598/NOTARY_VERSION


RUN apk add --no-cache git make

ENV NOTARYPKG github.com/theupdateframework/notary

Choose a reason for hiding this comment

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

Suggested change
ENV NOTARYPKG github.com/theupdateframework/notary
ENV NOTARYPKG github.com/notaryproject/notary

Copy link
Member Author

Choose a reason for hiding this comment

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

This must match what is in the go manifest which in version 0.7.0 is still that one as per:

https://github.com/notaryproject/notary/blob/b0b6bfdd4933081e8d5ae026b24e8337311dd598/go.mod#L1

Choose a reason for hiding this comment

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

They are the same repo, theupdateframework/notary redirects to notaryproject/notary, as your link shows.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but this is used in the Go modules, so it has to match what Go thinks the module name is 🙃

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly! It's confusing I know

Copy link
Member

Choose a reason for hiding this comment

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

Careful; the module wasn't renamed yet in the current release though (still uses github.com/theupdateframework/notary); https://github.com/notaryproject/notary/blob/v0.7.0/go.mod#L1

i.e. github.com/notaryproject/notary did not yet do a release

go mod vendor; \
# TODO remove for the next release of Notary (which should include efc35b02698644af16f6049c7b585697352451b8 & ca095023296d2d710ad9c6dec019397d46bf8576)
# Make the version detectable by scanners
sed -i -r -E 's|(version.NotaryVersion=\$\(NOTARY_VERSION\))|\1 -X $(NOTARY_PKG)/version.Version=$(NOTARY_VERSION)|' Makefile; \

Choose a reason for hiding this comment

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

Suggested change
sed -i -r -E 's|(version.NotaryVersion=\$\(NOTARY_VERSION\))|\1 -X $(NOTARY_PKG)/version.Version=$(NOTARY_VERSION)|' Makefile; \
sed -i -E 's,(version\.NotaryVersion=\$\(NOTARY_VERSION\)),\1 -X $(NOTARY_PKG)/version.Version=$(NOTARY_VERSION),' Makefile; \

-r and -E are redundant, -E is more common.

I'd avoid using a regular expression special character, | in this case, as the substitution delimiter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not the special character? I've seen it done like this many time

Choose a reason for hiding this comment

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

How would you use the special character? How would you escape it?

Copy link
Member

Choose a reason for hiding this comment

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

I usually use -r, but -E is POSIX:

       -E, -r, --regexp-extended

              use  extended regular expressions in the script (for portability
              use POSIX -E).

Perhaps you meant -e, which is otherwise implied?

           [-e script] [--expression=script]
...
           [script-if-no-other-script]
...
       -e script, --expression=script

              add the script to the commands to be executed

Copy link
Member

Choose a reason for hiding this comment

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

I usually use @ or ! as my "non-/ delimiter", but I don't think the exact choice matters that much here, right? If we want to use | in our regex in the future, it's pretty easy to swap then. 🤷

@LaurentGoderre
Copy link
Member Author

We can undo those change when this lands: notaryproject/notary#1704

Copy link
Contributor

@jonnystoten jonnystoten left a comment

Choose a reason for hiding this comment

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

❤️

@jonnystoten jonnystoten merged commit 4fc4c25 into docker:master Aug 23, 2024
1 check passed
@LaurentGoderre LaurentGoderre deleted the version-auto-detect branch August 26, 2024 13:51
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.

5 participants