Skip to content

Commit

Permalink
Fix http client redirect race condition (#964)
Browse files Browse the repository at this point in the history
  • Loading branch information
sverdlov93 authored Jul 7, 2024
1 parent 373141f commit ca94b45
Show file tree
Hide file tree
Showing 11 changed files with 105 additions and 116 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ jobs:

- name: pipelines tests
run: go test -v github.com/jfrog/jfrog-client-go/tests --timeout 0 --test.pipelines --rt.url=${{ secrets.PLATFORM_URL }}/artifactory --pipe.url=${{ secrets.PLATFORM_URL }}/pipelines --rt.user=${{ secrets.PLATFORM_USER }} --rt.password=${{ secrets.PLATFORM_PASSWORD }} --pipe.accessToken=${{ secrets.PLATFORM_ADMIN_TOKEN }} --pipe.vcsToken=${{ secrets.CLI_PIPE_VCS_TOKEN }} --pipe.vcsRepo=${{ secrets.CLI_PIPE_VCS_REPO }} --pipe.vcsBranch=${{ secrets.CLI_PIPE_VCS_BRANCH }} --ci.runId=${{ runner.os }}_pipe

JFrog-Client-Go-Repositories-Tests:
needs: Pretest
name: repositories ubuntu-latest
Expand Down
50 changes: 25 additions & 25 deletions artifactory/services/download.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (ds *DownloadService) getOperationSummary(totalSucceeded, totalFailed int)
return operationSummary
}

func (ds *DownloadService) DownloadFiles(downloadParams ...DownloadParams) (opertaionSummary *utils.OperationSummary, err error) {
func (ds *DownloadService) DownloadFiles(downloadParams ...DownloadParams) (operationSummary *utils.OperationSummary, err error) {
producerConsumer := parallel.NewRunner(ds.GetThreads(), 20000, false)
errorsQueue := clientutils.NewErrorsQueue(1)
expectedChan := make(chan int, 1)
Expand Down Expand Up @@ -117,7 +117,7 @@ func (ds *DownloadService) DownloadFiles(downloadParams ...DownloadParams) (oper
for _, v := range successCounters {
totalSuccess += v
}
opertaionSummary = ds.getOperationSummary(totalSuccess, <-expectedChan-totalSuccess)
operationSummary = ds.getOperationSummary(totalSuccess, <-expectedChan-totalSuccess)
return
}

Expand Down Expand Up @@ -236,7 +236,7 @@ func (ds *DownloadService) produceTasks(reader *content.ContentReader, downloadP
return tasksCount
}
defer func() {
if err := sortedReader.Close(); err != nil {
if err = sortedReader.Close(); err != nil {
log.Warn("Could not close sortedReader. Error: " + err.Error())
}
}()
Expand Down Expand Up @@ -287,7 +287,7 @@ func rbGpgValidate(rbGpgValidationMap map[string]*utils.RbGpgValidator, bundle s
}

// Extract for the aqlResultItem the directory path, store the path the directoriesDataKeys and in the directoriesData map.
// In addition directoriesData holds the correlate DownloadData for each key, later on this DownloadData will be used to create a create dir tasks if needed.
// In addition, directoriesData holds the correlate DownloadData for each key, later on this DownloadData will be used to create a create dir tasks if needed.
// This function append the new data to directoriesDataKeys and to directoriesData and return the new map and the new []string
// We are storing all the keys of directoriesData in additional array(directoriesDataKeys) so we could sort the keys and access the maps in the sorted order.
func collectDirPathsToCreate(aqlResultItem utils.ResultItem, directoriesData map[string]DownloadData, tempData DownloadData, directoriesDataKeys []string) (map[string]DownloadData, []string) {
Expand Down Expand Up @@ -455,7 +455,7 @@ func createLocalSymlink(localPath, localFileName, symlinkArtifact string, symlin
}
// We can't create symlink in case a file with the same name already exist, we must remove the file before creating the symlink
if isFileExists {
if err := os.Remove(localSymlinkPath); err != nil {
if err = os.Remove(localSymlinkPath); err != nil {
return err
}
}
Expand Down Expand Up @@ -495,34 +495,34 @@ func (ds *DownloadService) createFileHandlerFunc(downloadParams DownloadParams,
return func(downloadData DownloadData) parallel.TaskFunc {
return func(threadId int) error {
logMsgPrefix := clientutils.GetLogMsgPrefix(threadId, ds.DryRun)
downloadPath, e := clientutils.BuildUrl(ds.GetArtifactoryDetails().GetUrl(), downloadData.Dependency.GetItemRelativePath(), make(map[string]string))
if e != nil {
return e
downloadPath, err := clientutils.BuildUrl(ds.GetArtifactoryDetails().GetUrl(), downloadData.Dependency.GetItemRelativePath(), make(map[string]string))
if err != nil {
return err
}
log.Info(logMsgPrefix+"Downloading", downloadData.Dependency.GetItemRelativePath())
if ds.DryRun {
successCounters[threadId]++
return nil
}
target, placeholdersUsed, e := clientutils.BuildTargetPath(downloadData.DownloadPath, downloadData.Dependency.GetItemRelativePath(), downloadData.Target, true)
if e != nil {
return e
target, placeholdersUsed, err := clientutils.BuildTargetPath(downloadData.DownloadPath, downloadData.Dependency.GetItemRelativePath(), downloadData.Target, true)
if err != nil {
return err
}
localPath, localFileName := fileutils.GetLocalPathAndFile(downloadData.Dependency.Name, downloadData.Dependency.Path, target, downloadData.Flat, placeholdersUsed)
if downloadData.Dependency.Type == "folder" {
return createDir(localPath, localFileName, logMsgPrefix)
}
if e = removeIfSymlink(filepath.Join(localPath, localFileName)); e != nil {
return e
if err = removeIfSymlink(filepath.Join(localPath, localFileName)); err != nil {
return err
}
if downloadParams.IsSymlink() {
if isSymlink, e := ds.createSymlinkIfNeeded(ds.GetArtifactoryDetails().GetUrl(), localPath, localFileName, logMsgPrefix, downloadData, successCounters, threadId, downloadParams); isSymlink {
return e
}
}
if e = ds.downloadFileIfNeeded(downloadPath, localPath, localFileName, logMsgPrefix, downloadData, downloadParams); e != nil {
log.Error(logMsgPrefix, "Received an error: "+e.Error())
return e
if err = ds.downloadFileIfNeeded(downloadPath, localPath, localFileName, logMsgPrefix, downloadData, downloadParams); err != nil {
log.Error(logMsgPrefix, "Received an error: "+err.Error())
return err
}
successCounters[threadId]++
ds.addToResults(&downloadData.Dependency, ds.GetArtifactoryDetails().GetUrl(), localPath, localFileName)
Expand All @@ -532,28 +532,28 @@ func (ds *DownloadService) createFileHandlerFunc(downloadParams DownloadParams,
}

func (ds *DownloadService) downloadFileIfNeeded(downloadPath, localPath, localFileName, logMsgPrefix string, downloadData DownloadData, downloadParams DownloadParams) error {
isEqual, e := fileutils.IsEqualToLocalFile(filepath.Join(localPath, localFileName), downloadData.Dependency.Actual_Md5, downloadData.Dependency.Actual_Sha1)
if e != nil {
return e
isEqual, err := fileutils.IsEqualToLocalFile(filepath.Join(localPath, localFileName), downloadData.Dependency.Actual_Md5, downloadData.Dependency.Actual_Sha1)
if err != nil {
return err
}
if isEqual {
log.Debug(logMsgPrefix + "File already exists locally.")
if ds.Progress != nil {
ds.Progress.IncrementGeneralProgress()
}
if downloadParams.IsExplode() {
e = clientutils.ExtractArchive(localPath, localFileName, downloadData.Dependency.Name, logMsgPrefix, downloadParams.IsBypassArchiveInspection())
err = clientutils.ExtractArchive(localPath, localFileName, downloadData.Dependency.Name, logMsgPrefix, downloadParams.IsBypassArchiveInspection())
}
return e
return err
}
downloadFileDetails := createDownloadFileDetails(downloadPath, localPath, localFileName, downloadData, downloadParams.IsSkipChecksum())
return ds.downloadFile(downloadFileDetails, logMsgPrefix, downloadParams)
}

func createDir(localPath, localFileName, logMsgPrefix string) error {
folderPath := filepath.Join(localPath, localFileName)
if e := fileutils.CreateDirIfNotExist(folderPath); e != nil {
return e
if err := fileutils.CreateDirIfNotExist(folderPath); err != nil {
return err
}
log.Info(logMsgPrefix + "Creating folder: " + folderPath)
return nil
Expand All @@ -564,8 +564,8 @@ func (ds *DownloadService) createSymlinkIfNeeded(rtUrl, localPath, localFileName
isSymlink := len(symlinkArtifact) > 0
if isSymlink {
symlinkChecksum := getArtifactSymlinkChecksum(downloadData.Dependency.Properties)
if e := createLocalSymlink(localPath, localFileName, symlinkArtifact, downloadParams.ValidateSymlinks(), symlinkChecksum, logMsgPrefix); e != nil {
return isSymlink, e
if err := createLocalSymlink(localPath, localFileName, symlinkArtifact, downloadParams.ValidateSymlinks(), symlinkChecksum, logMsgPrefix); err != nil {
return isSymlink, err
}
successCounters[threadId]++
ds.addToResults(&downloadData.Dependency, rtUrl, localPath, localFileName)
Expand Down
2 changes: 1 addition & 1 deletion artifactory/services/upload.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const (
DefaultMinChecksumDeploy = utils.SizeKib * 10
// The default minimum file size for attempting multi-part upload
defaultUploadMinSplit = utils.SizeMiB * 200
// The default maximum number of parts that can be concurrently uploaded per file during a multi-part upload
// The default maximum number of parts that can be concurrently uploaded per file during a multipart upload
defaultUploadSplitCount = 5
)

Expand Down
28 changes: 0 additions & 28 deletions artifactory/services/utils/specutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,30 +50,6 @@ type CommonParams struct {
Include []string
}

type FileGetter interface {
GetAql() Aql
GetPattern() string
SetPattern(pattern string)
GetExclusions() []string
GetTarget() string
SetTarget(target string)
IsExplode() bool
GetProps() string
GetSortOrder() string
GetSortBy() []string
GetOffset() int
GetLimit() int
GetBuild() string
GetProject() string
GetBundle() string
GetSpecType() (specType SpecType)
IsRecursive() bool
IsIncludeDirs() bool
GetArchiveEntries() string
SetArchiveEntries(archiveEntries string)
GetPatternType() clientutils.PatternType
}

func (params CommonParams) GetArchiveEntries() string {
return params.ArchiveEntries
}
Expand Down Expand Up @@ -110,10 +86,6 @@ func (params *CommonParams) GetExcludeProps() string {
return params.ExcludeProps
}

func (params *CommonParams) IsExplode() bool {
return params.Recursive
}

func (params *CommonParams) IsRecursive() bool {
return params.Recursive
}
Expand Down
24 changes: 12 additions & 12 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ require (
github.com/go-git/go-git/v5 v5.12.0
github.com/golang-jwt/jwt/v4 v4.5.0
github.com/gookit/color v1.5.4
github.com/jfrog/archiver/v3 v3.6.0
github.com/jfrog/archiver/v3 v3.6.1
github.com/jfrog/build-info-go v1.9.29
github.com/jfrog/gofrog v1.7.2
github.com/jfrog/gofrog v1.7.3
github.com/stretchr/testify v1.9.0
github.com/xanzy/ssh-agent v0.3.3
golang.org/x/crypto v0.23.0
golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842
golang.org/x/term v0.20.0
golang.org/x/crypto v0.25.0
golang.org/x/exp v0.0.0-20240613232115-7f521ea00fb8
golang.org/x/term v0.22.0
)

require (
Expand All @@ -35,7 +35,7 @@ require (
github.com/golang/snappy v0.0.4 // indirect
github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect
github.com/kevinburke/ssh_config v1.2.0 // indirect
github.com/klauspost/compress v1.17.4 // indirect
github.com/klauspost/compress v1.17.9 // indirect
github.com/klauspost/cpuid/v2 v2.2.3 // indirect
github.com/klauspost/pgzip v1.2.6 // indirect
github.com/minio/sha256-simd v1.0.1 // indirect
Expand All @@ -46,18 +46,18 @@ require (
github.com/rivo/uniseg v0.4.7 // indirect
github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect
github.com/skeema/knownhosts v1.2.2 // indirect
github.com/ulikunitz/xz v0.5.11 // indirect
github.com/ulikunitz/xz v0.5.12 // indirect
github.com/xi2/xz v0.0.0-20171230120015-48954b6210f8 // indirect
github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778 // indirect
golang.org/x/mod v0.17.0 // indirect
golang.org/x/net v0.25.0 // indirect
golang.org/x/mod v0.18.0 // indirect
golang.org/x/net v0.26.0 // indirect
golang.org/x/sync v0.7.0 // indirect
golang.org/x/sys v0.20.0 // indirect
golang.org/x/tools v0.21.0 // indirect
golang.org/x/sys v0.22.0 // indirect
golang.org/x/tools v0.22.0 // indirect
gopkg.in/warnings.v0 v0.1.2 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)

// replace github.com/jfrog/build-info-go => github.com/eyalbe4/build-info-go v1.8.6-0.20240610015232-844595d5a4f3

// replace github.com/jfrog/gofrog => github.com/jfrog/gofrog dev
// replace github.com/jfrog/gofrog => github.com/jfrog/gofrog dev
Loading

0 comments on commit ca94b45

Please sign in to comment.