sylabs / scs-build-client Goto Github PK

Once this project is open-sourced (#4), use the Codecov ORB to report code coverage, and add a code coverage badge to the README.

SubmitBuild() currently has a libraryRef and libraryURL parameter. This is confusing for a couple of reasons:

1. It should be possible to submit a build without specifying a libraryRef or libraryURL. This is arguably the common case in Singularity today. The builder will use its configured Library URL, and auto-generate a path.
2. I think we should combine these into a single libraryURI parameter, since it should be possible to specify both with a single URI (library://host:port/path:tag).

We can satisfy both of these cases with the same function, but just make it clear which params are optional, and what happens in the case they are not supplied (the Build Service will dictate)?

Dependabot has been acquired by GitHub, and as a result has made some changes that will require a bit of work to migrate. Details available here.

https://github.com/sylabs/scs-key-client has been migrated, and it went smoothly other than a minor issue with the badge (sylabs/scs-key-client#37). I'd suggest we hold off migrating this repo until that niggle is sorted. Progress on the badge niggle will likely be reported in dependabot/dependabot-core#1912.

The publish-release job failed to run against version v0.7.0 (ref).

The failure was during the Attach attestations to image step:

Generating ephemeral keys...
Retrieving signed certificate...
error opening browser: exec: "xdg-open": executable file not found in $PATH
Go to the following link in a browser:

	 https://oauth2.sigstore.dev/auth/auth?access_type=online&client_id=sigstore&code_challenge=6zCdBdUBBPIFAqQr-H5jwTAzO8Zqnm6Mo3t_5RCYNbU&code_challenge_method=S256&nonce=29G8jQl786Ox6hrXn9zGXtoyYfX&redirect_uri=urn%3Aietf%3Awg%3Aoauth%3A2.0%3Aoob&response_type=code&scope=openid+email&state=29G8jWQA9kJOiz6JqyfD7Ze6nPX
Enter verification code: 2022/05/16 19:09:56 error during command execution: getting key from Fulcio: retrieving cert: oauth2: cannot fetch token: 400 Bad Request
Response: {"error":"invalid_request","error_description":"Required param: code."}

Exited with code exit status 1

Seeing an occasional data race reported during testing. This appears to be related to the mockService, so it's possibly an issue with the unit tests themselves.

One recent example can be found here. Dump from that run:

==================
WARNING: DATA RACE
Write at 0x00c0001244c8 by goroutine 30:
  github.com/sylabs/scs-build-client/client_test.TestOutput.func1()
      /root/project/client/output_test.go:93 +0x1e4
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/go/src/testing/testing.go:1306 +0x47

Previous read at 0x00c0001244c8 by goroutine 41:
  github.com/sylabs/scs-build-client/client_test.(*mockService).ServeWebsocket()
      /root/project/client/mock_test.go:142 +0x2f6
  github.com/sylabs/scs-build-client/client_test.(*mockService).ServeWebsocket-fm()
      /root/project/client/mock_test.go:128 +0x57
  net/http.HandlerFunc.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2047 +0x4d
  net/http.(*ServeMux).ServeHTTP()
      /usr/local/go/src/net/http/server.go:2425 +0xc5
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2879 +0x89a
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:1930 +0x12e4
  net/http.(*Server).Serve·dwrap·87()
      /usr/local/go/src/net/http/server.go:3034 +0x58

Goroutine 30 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1306 +0x726
  github.com/sylabs/scs-build-client/client_test.TestOutput()
      /root/project/client/output_test.go:82 +0x7f2
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1259 +0x22f
  testing.(*T).Run·dwrap·21()
      /usr/local/go/src/testing/testing.go:1306 +0x47

Goroutine 41 (finished) created at:
  net/http.(*Server).Serve()
      /usr/local/go/src/net/http/server.go:3034 +0x847
  net/http/httptest.(*Server).goServe.func1()
      /usr/local/go/src/net/http/httptest/server.go:308 +0xc4
==================
some_outputsome_output--- FAIL: TestOutput (0.01s)
    --- FAIL: TestOutput/OutputReaderFailure (0.00s)
        testing.go:1152: race detected during execution of test
    testing.go:1152: race detected during execution of test
FAIL
coverage: 82.8% of statements
FAIL	github.com/sylabs/scs-build-client/client	0.044s
FAIL

Exited with code exit status 1

Incorrectly resolved web socket URL when the build service URL contains path element(s) (ie. https://host.com/build)

In this case, the exit code for scs-build should be non-zero to report an error so the CI/CD environment can detect it properly.

...so that singularity CLI could capture Ctrl+C and call this function to cancel a build.

Update CI to verify this module is (and continues to be) compatible with Go v1.13. The cache_go_mod step should be unnecessary, with the default module proxy serving a similar purpose.

The github.com/sylabs/scs-build-client/client package exposes a log.Logger. In all but one case, it is logging things that the package user already has visibility into. For that reason, I think it might be best to simplify the package by removing the logging.

Rather than using glog, could we provide a Logger interface and use that instead (as Dave Cheney suggests)?

As a result, the response from the submit build request is attempted to be deserialized, which leads to a potentially misleading error being passed back to the caller (ie. singularity)

#110 worked around an issue installing cosign (sigstore/cosign#1844), but has the side-effect of pinning cosign to version 1.8.0. We should circle back in the future to see if there is an easy way to install the latest version in our CI.

[tfrisch@wedge ~]$ scs-build build –-sign docker://alpine:3 library://user/default/alpine:3
Application init error: unsupported library ref scheme docker
Error: application init error: unsupported library ref scheme docker

The actual error is with the "library://" library ref, not with the docker URI

We should generate an scs-build container and publish it when a release is cut.

We should take some time to examine the API for opportunities to reduce the exported API surface, and/or make future extensions possible without breaking API compatibility, such as using functional options and accessors, where appropriate.

We should generate a reproducible scs-build binary and publish it when a release is cut.

As mentioned by @dtrudg in #101 (review), the output of build on error is quite messy, outputting repetitive error messages intermixed with command usage. We should fix that.

GetOutput currently accepts an type OutputReader interface, which the caller must implement:

The messageType int here is problematic, because it requires the caller to understand implementation details. In particular, the caller needs to understand the possible messageType values, and how to handle them. In practice, these are defined as "text" and "binary" in RFC 6455. All implementations that I could find handle text messages, and discard binary ones.

With all that in mind, I propose we change the signature from this:

func (c *Client) GetOutput(ctx context.Context, buildID string, or OutputReader) error

To this:

func (c *Client) GetOutput(ctx context.Context, buildID string, w io.Writer) error

This way, the caller doesn't need to worry about any of the web socket implementation details under the hood, and can pass any io.Writer such as os.Stdout, simplifying usage.

https://godoc.org/ will eventually be replaced by https://pkg.go.dev/. We should update our documentation badge to the new location when appropriate.

Only minor blocker I can see at the moment is golang/go#36982. Suggest progressing once that is sorted.

We should ship a software bill of materials (SBOM) with each release of siftool. Goreleaser supports SBOM generation using Anchore Syft under the hood (ref). Since we're already using that to generate releases, adding SBOM generation should be relatively trivial.

When attempting to perform a build using an expired auth token, the reported error message isn't clear to the end user:

Error: error uploading build context: error getting build context files: def file parse error: build server error (HTTP status 401)

This should be a more user friendly message indicating the token is invalid.

Output() which uses a websocket should use same TLS client configuration as non-websocket APIs.

NewClient() does not currently validate the URL scheme. If a Config.BaseURL value is passed that does not contain http or https, commands that rely on Client.BaseURL will fail. To make this a little more user friendly, I propose we validate the URL scheme in NewClient().

I'm a little concerned about our use of Definition rather than taking a raw definition file. This structure closely mimics an internal data structure in Singularity, and that data structure is evolving over time. As a practical example of how this might get messy, multi-stage builds (sylabs/singularity#2518) has significantly changed this structure.

We should generate scs-build containers for multiple architectures, and publish them when a release is cut.

Need to review all files for correct licenses.

scs-build does not currently have a unique user agent.

When a Client is created with a path component (ex. https://example.com/path), the path component ends up being ignored when building up request URLs.

The problem originates in (*Client).newRequest, which calls (*url.URL).ResolveReference to construct a request URI to BaseURL/path. When the path parameter begins with a /, it is treated as an absolute path and thus any path component of the BaseURL is ignored.

When building and pushing directly to Library refs with certain formats, the build succeeds but the image is not pushed, and an error is displayed:

$ docker run -e SYLABS_AUTH_TOKEN="${SYLABS_AUTH_TOKEN}" \
    sylabsio/scs-build build docker://alpine library://cloud.sylabs.io/sylabs-adam/a/c
Building for amd64...
...
FATAL:   Unable to push image to library: malformed image path: /sylabs-adam/a/c

It looks to me as if this is failing for any ref format other than library:path:tags. Will need to trace through to see if this is a problem in this module, or in the ref parsing code.

Error from CircleCI:

Command "packages" is deprecated, use `syft scan` instead
Using payload from: sbom.cdx.json
Error: signing sylabsio/scs-build:0.9.5-amd64: signing: getting fetching default hash function: getting public key: operation error KMS: GetPublicKey, failed to sign request: failed to retrieve credentials: failed to refresh cached credentials, no EC2 IMDS role found, operation error ec2imds: GetMetadata, http response error StatusCode: 404, request to EC2 IMDS failed
main.go:62: error during command execution: signing sylabsio/scs-build:0.9.5-amd64: signing: getting fetching default hash function: getting public key: operation error KMS: GetPublicKey, failed to sign request: failed to retrieve credentials: failed to refresh cached credentials, no EC2 IMDS role found, operation error ec2imds: GetMetadata, http response error StatusCode: 404, request to EC2 IMDS failed

Exited with code exit status 1

A couple of suggestions:

1. We should remove wsURL string as a parameter and always use c.BaseURL as the URL. In practice, these must always be the same URL, and keeping this level of flexibility will likely cause more confusion that it's worth.
2. At the moment, StreamOutput() prints messages of type TextMessage to os.Stdout, and for each message of type websocket.BinaryMessage it outputs "Ignoring binary message" to os.Stdout. This behaviour could be problematic for clients, who might wish the text output to go somewhere other than os.Stdout, and/or handle websocket.BinaryMessage messages.

What about if we define an OutputReader interface with a Read(messageType int, p []byte) (n int, error) method? We could then change the function prototype to be:

func (c *Client) StreamOutput(ctx context.Context, or OutputReader) error

Which results in error as follows when attempting to build def file containing %files section:

Building for amd64...
Error: failed to build amd64: open /tmp/scs-build-context-502634966: no such file or directory