Skip to content

update: add the ability to specify a http-timeout #233

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.

Sign up for GitHub

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

Open
crozzy wants to merge 1 commit into
base: main
Choose a base branch
from
+28 −3
Open

update: add the ability to specify a http-timeout #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ inputs:
description: 'Vulnerability database file created for mode `update` or DB file used for `report` mode'
required: false
default: ''
updater-timeout:
description: 'The http timeout for the requests made by the database update process'
required: false
default: '120s'

runs:
using: "docker"
Expand All @@ -52,3 +56,4 @@ runs:
- '-u ${{ inputs.docker-config-dir }}'
- '-w ${{ inputs.mode }}'
- '-b ${{ inputs.db-file }}'
- '-t ${{ inputs.updater-timeout }}'
19 changes: 18 additions & 1 deletion cmd/clair-action/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import (
"net/http"
"time"

"github.com/quay/claircore"
"github.com/quay/claircore/libvuln"
"github.com/quay/claircore/libvuln/driver"
"github.com/quay/claircore/rhel/vex"
_ "github.com/quay/claircore/updater/defaults"
"github.com/urfave/cli/v2"

Expand All @@ -24,19 +27,32 @@ var updateCmd = &cli.Command{
Usage: "where to look for the matcher DB",
EnvVars: []string{"DB_PATH"},
},
&cli.DurationFlag{
Name: "http-timeout",
Value: 2 * time.Minute,
Usage: "the timeout for HTTP requests",
EnvVars: []string{"HTTP_TIMEOUT"},
},
},
}

func update(c *cli.Context) error {
ctx := c.Context
dbPath := c.String("db-path")
httpTimeout := c.Duration("http-timeout")
matcherStore, err := datastore.NewSQLiteMatcherStore(dbPath, true)
if err != nil {
return fmt.Errorf("error creating sqlite backend: %v", err)
}

cl := &http.Client{
Timeout: 2 * time.Minute,
Timeout: httpTimeout,
}
factoryConfigs := make(map[string]driver.ConfigUnmarshaler)
factoryConfigs["rhel-vex"] = func(v interface{}) error {
cfg := v.(*vex.FactoryConfig)
cfg.CompressedFileTimeout = claircore.Duration(httpTimeout)
Copy link
Collaborator Author

@crozzy crozzy Sep 4, 2025

Choose a reason for hiding this comment

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

Could also just fix claircore to use the passed client's timeout instead of a default

Copy link
Member

@hdonnay hdonnay Sep 10, 2025

Choose a reason for hiding this comment

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

Speak on this; what do you mean

Copy link
Collaborator Author

@crozzy crozzy Sep 10, 2025

Choose a reason for hiding this comment

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

So, right now the VEX fetcher has the ability to configure the timeout manually falling back to a default of 2 mins. But that makes situations like this annoying when the client's timeout is potentially set above 2 mins (because we're defaulting to less), it's like maybe the default timeout should just be the passed client's timeout and the CompressedFileTimeout should override that if needed.

Copy link
Member

@hdonnay hdonnay Sep 10, 2025

Choose a reason for hiding this comment

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

"The client's timeout" meaning like, a timeout on the passed context.Context?

Copy link
Collaborator Author

@crozzy crozzy Sep 10, 2025

Choose a reason for hiding this comment

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

Well, whatever someone has set the Timeout to be in the http.Client they pass to Claircore

Copy link
Member

@hdonnay hdonnay Sep 10, 2025

Choose a reason for hiding this comment

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

The Timeout setting on the http.Client is a pretty big hammer, as it covers the whole time of making the request to calling http.Response.Body.Close(); I wouldn't expect that it'd be set for the passed Client.

Copy link
Collaborator Author

@crozzy crozzy Sep 10, 2025

Choose a reason for hiding this comment

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

Hmmm fair enough. Now I'm really confused how to proceed; if I remove the configurable Client.Timeout then the arg http-timeout is kind of misleading, I could change the arg to be specific to VEX but it's future usefulness becomes kind of lessened.

Another option is to not set the Client.Timeout unless it's explicitly passed as an argument so it's infinite by default but I still think we'd need some way to specify the CompressedFileTimeout.

Copy link
Member

@hdonnay hdonnay Sep 10, 2025

Choose a reason for hiding this comment

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

I guess this might be fine as a big kludge? We should probably think this through in a new interface.

Copy link
Collaborator Author

@crozzy crozzy Sep 10, 2025

Choose a reason for hiding this comment

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

Do you think it ever makes sense to have a default Deadline for all updater http calls and then individual levers for each updater as needed? Or should we take the "infinite by default" approach and allow users to set Deadlines as they wish?

return nil
}

matcherOpts := &libvuln.Options{
Expand All @@ -45,6 +61,7 @@ func update(c *cli.Context) error {
Locker: NewLocalLockSource(),
DisableBackgroundUpdates: true,
UpdateWorkers: 1,
UpdaterConfigs: factoryConfigs,
}

lv, err := libvuln.New(ctx, matcherOpts)
Expand Down
7 changes: 5 additions & 2 deletions entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/bin/bash
set -e

while getopts "r:p:f:o:c:d:u:w:b:" o; do
while getopts "r:p:f:o:c:d:u:w:b:t:" o; do
case "${o}" in
r)
export imageRef="$(sed -e 's/^[ \t]*//'<<<"${OPTARG}")"
Expand Down Expand Up @@ -30,12 +30,15 @@ while getopts "r:p:f:o:c:d:u:w:b:" o; do
b)
export dbPath="$(sed -e 's/^[ \t]*//'<<<"${OPTARG}")"
;;
t)
export httpTimeout="$(sed -e 's/^[ \t]*//'<<<"${OPTARG}")"
;;
esac
done

if [[ ${mode} = "update" ]]
then
clair-action update --db-path=${dbPath}
clair-action update --db-path=${dbPath} ${httpTimeout:+--http-timeout=${httpTimeout}}
else
clair-action report \
--image-path=${GITHUB_WORKSPACE}/${imagePath} \
Expand Down
Loading