From f97f5f937351cd0da7ef24381e0b83cc23048865 Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Thu, 7 Mar 2019 15:41:59 -0800 Subject: [PATCH 01/12] Enable pipeline packages with multiple files --- backend/src/apiserver/server/util.go | 39 +++++++++++++++++++++------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/backend/src/apiserver/server/util.go b/backend/src/apiserver/server/util.go index 792297ba459..591d17b394f 100644 --- a/backend/src/apiserver/server/util.go +++ b/backend/src/apiserver/server/util.go @@ -62,6 +62,10 @@ func isYamlFile(fileName string) bool { return strings.HasSuffix(fileName, ".yaml") || strings.HasSuffix(fileName, ".yml") } +func isPipelineYamlFile(fileName string) bool { + return strings.HasSuffix(fileName, "pipeline.yaml") +} + func isZipFile(compressedFile []byte) bool { return len(compressedFile) > 2 && compressedFile[0] == '\x50' && compressedFile[1] == '\x4B' //Signature of zip file is "PK" } @@ -76,14 +80,24 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") } tarReader := tar.NewReader(gzipReader) - header, err := tarReader.Next() - if err != nil || header == nil { - return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") + var pipelineYamlReader *tar.Reader + for { + header, err := tarReader.Next() + if err == io.EOF { + break + } + if err != nil || header == nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") + } + if isPipelineYamlFile(header.Name) { + pipelineYamlReader = tarReader + break + } } - if !isYamlFile(header.Name) { - return nil, util.NewInvalidInputError("Error extracting pipeline from the tarball file. Expecting a YAML file inside the tarball. Got: %v", header.Name) + if pipelineYamlReader == nil { + return nil, util.NewInvalidInputError("Error extracting pipeline from the .tar.gz file. Could not find any *pipeline.yaml files inside the archive.") } - decompressedFile, err := ioutil.ReadAll(tarReader) + decompressedFile, err := ioutil.ReadAll(pipelineYamlReader) if err != nil { return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the tarball file.") } @@ -98,10 +112,17 @@ func DecompressPipelineZip(compressedFile []byte) ([]byte, error) { if len(reader.File) < 1 { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Empty zip file.") } - if !isYamlFile(reader.File[0].Name) { - return nil, util.NewInvalidInputError("Error extracting pipeline from the zip file. Expecting a YAML file inside the zip. Got: %v", reader.File[0].Name) + var pipelineYamlFile *zip.File + for _, file := range reader.File { + if isPipelineYamlFile(file.Name) { + pipelineYamlFile = file + break + } + } + if pipelineYamlFile == nil { + return nil, util.NewInvalidInputError("Error extracting pipeline from the zip file. Could not find any *pipeline.yaml files inside the archive.") } - rc, err := reader.File[0].Open() + rc, err := pipelineYamlFile.Open() if err != nil { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Failed to read the content.") } From 827be7a0c4da0e4f0e493aee4503c06915e1754f Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Thu, 7 Mar 2019 16:33:14 -0800 Subject: [PATCH 02/12] Added tests --- .../test/arguments_tarball/arguments.tar.gz | Bin 723 -> 754 bytes .../arguments_zip/arguments-parameters.zip | Bin 759 -> 721 bytes .../pipeline_plus_component/component.yaml | 9 +++++ .../pipeline_plus_component/pipeline.yaml | 36 ++++++++++++++++++ .../pipeline_plus_component.tar.gz | Bin 0 -> 974 bytes .../pipeline_plus_component.zip | Bin 0 -> 987 bytes backend/src/apiserver/server/util_test.go | 18 +++++++++ 7 files changed, 63 insertions(+) create mode 100644 backend/src/apiserver/server/test/pipeline_plus_component/component.yaml create mode 100644 backend/src/apiserver/server/test/pipeline_plus_component/pipeline.yaml create mode 100644 backend/src/apiserver/server/test/pipeline_plus_component/pipeline_plus_component.tar.gz create mode 100644 backend/src/apiserver/server/test/pipeline_plus_component/pipeline_plus_component.zip diff --git a/backend/src/apiserver/server/test/arguments_tarball/arguments.tar.gz b/backend/src/apiserver/server/test/arguments_tarball/arguments.tar.gz index 395476f1c037a3f906994c5a14e7016bbfb79b3b..2b5c414ae1ae44012e57c5c866f5530b5e795900 100644 GIT binary patch literal 754 zcmV>LCrZM2$j4LJ_Js4EDs? zV()I(Ya9#l-#c~^p5+FuD)L>No%wd=o7o-5-*9N&>I&P$@%Y7TdhvdYxml%HZ$!W*zgDxw-^S}r{T(**@4B6CtJi%z|6bE`AI*QK z=kokFUrEmcyz%+pg`v{95#hpu*L3&bi&9}kFdh$`U1t}@!Y7$f0IB3Z39t)-fkvMn zYflxvlSzb<;MJS(t{_y_uFCrl93fXJ#F&Fr7Se<`gh*g6I0r3#(iVgSU&T5SD1CxV z*hQIU9i+}7&ekE+g+*o~^EI#L_j!P}6kK9qt?oA(S(eqYR9jajY(y(&(io412b1YR zjTM)BPGm$$0)~F2!f>q@IiO}iK3=fQ2s2OyFf_zIs|vxH5mv~s3W=KA3=MIVKqS_P zi_|{o)Twg(<;X}9nB7pPG)dAI6fXsW}|~C938{(XtF<= zjgBU~`V51~8GId0_NzeLD^5d8Z3_J?QWSlpplI5Z=z)N_S_w)t`C=}7u1kg~h6JIy zA|tsR&}3qf6sMW6ynv%bA{G{HX?apq-EmNh_4)S!O{la{KkGs@u3U`t0$t})$e<6W z%3RJPl{qn44A5fV0SHNwK}!>isob5$ByCdDXxL&oY|Sau7F>3iTq_}MAF>7Vd121x z+f4-r;BMz?T?1LhF72+fx*S&+rEFVJ6se~y&taP-?jtKJ!))C5(gNQBoo=!|&HGH| zM~MoPuVitj$!s?ATP9>mp!`ezDBI-hmul>q!{ZWT(bgZn%Gi6pwsSWUHdd~$SMut2 kx>i5ko15CRPVeTXa=!fp*kX$iwFSEN4{GC1MQSeZ`wc*hB^CJjL4;x__Bde(G#UU|Gi@aEosD|Qj}iixggKHJM-@BvS_0$5z3`agBBCvgiTuoCUNcM=n$v9 zUd{LWr`^G6trPSI{h(XLiJ%ksHNV^K4}#vH*X`9h{^_9KtwHAnJsc%V9a?}|fhsb1 zTxYCI&=!%^nRyLgufz$AwJEHOHV%T0{|-KD9mN8s)3JBroxoIvLZxB_nOX@8ZX;l1 zFr=SdrUBpR%1EssXm#LCDX8yK^|$Y-LZLHAumGhUWT~JMGKICI9^yU}#(`82>cqqn zRVW~rZd0|{6=_k4t6d3wRSlH+R4g^ER*f)zhAX=$%F1)w2AA>NS97<14TFtUPjPM3yrE1gtP15&+qIa)zg zRx))~-em5{wD$IB(}x0@7b>Wa7BE@V;lpS#Sv07|l2J-SMYT*``7?Mv!XWQ-q_MVnUv)m#i|_ILWm~@)_Q9wH6oO28PdK~ zlwl+w(s#lt+71|D6PcE$nUcK~Rf%OH9l9#=Y*Q`ILnC+RKLoUqHdg;=N!`BlF*Y0Y zy<4ePL%7uTb{*^7qZbBO=y2!(L_!IRPR!`tsa8LGiJD&Nw$xI>7Dh|uhLCTN&I=0) z|6WvT0RF7pem6l?wX3-AZ7;_i#u>#nB98TQRp3#~37R80tIF)t2W^4wfXVh?P#=IzC`?ZPq@p*IOFl z?|8nL>YljxL+_qz^`9C3Dt=hV_5N&%Mb0d>BhI%1Zr4P}&CIjtPy1e2WIy3Z`%L%H zNyP~-!_S75#lB_e6|`EXlGe$a!962-(!A@r4~}h0pZfja%dPnrCap+$v*&5(TmS33 zI*z|7GfW#><1tGlf)ZFIIF-ix=l92W|{|5tA?yMFA|e8txf%UYId7Kr3TqzYGDD%-4A zDjf2}(7AsygMzxNnC;Psi;26|c~qJkWCT_yS##Xlq8lz%mTsbL-?;3+B$;vVGzOr-w z{rAECvmfj3S9AVj2=HcPvS-GXx>SJa3j`DzmNbHBl(fdhpa2qMU}TVBV7RyBmrcSI d#s-1#J&soqbbvQ28$=BwLp+cX15CIK3;>{qI3WN4 literal 759 zcmWIWW@Zs#-~d9l37)un zV_A%h|K8|-cRGYigAexIG7#ANT-&W6QA9+@EratLbRcv#)#YbX=S+1#wOF zIoWWUbD5OzzgJq!KRl0dsU|+Uf3Usn!fUU+{qrLquF%r>$Pr|@LT6*kh7H1R9M5xR zvO75~@`~k{!s)0RbMID(lAX6rnNZg22{%vFoLzp`l>frk#@qhy=J2^K-nFha=kl*p zDT^PS*|ONxVz&FoniHM3=dnjl*qZRdUzO+lE?cH7z9|o;C5uHg8#T;STxWTeWnFK9 zQyTMztmj!KN3>RMIM1{Gu;#q8?yleb48F^@ z1n#e0=+g)$pfNSt1&aYZ-#k>RtB z?vYaFX4=i;$ENKKW@uW@;c%aMd1jPEc*|az|UQYEpZ1hjuFbLh?iT($U zyRUyYAR+4Cc^NwUc;))fX~u?7&1uRCy0myMOI-}D9yA6?70ulJA+dNSQa_U3fZ6R$tmz);~l4!}bF}387=Io<9fbAvBUWc>eoa{S$BCp#DRLgzfo%4D6r( zB!o=!jJL6H^72fk12r zzB3M;wW;&{j~HPoO33IEa-pFt7>3{l%sA#?_YspCcy6jenR1#(3@WbYb((b$8$*0v zhY+`>$OMZ15=XO#en53ixW-&-IqdhVsHZ2U{N^#Beqzi?Z>53|bQDR%EHyc=JSFDE$q z8Ag-O@auT;t_uwJ3e&KAsSJN4X)1HhVzX&e#`XxzL_?^MEaEdB;kxpq#5x@kv0y64 z<$z==yDc$|3;|0 zSt=?kW17ZPQ`09%Sk4sHY*P0Y@b1#|6pCJ5+cS+}dzN8qe9@qmQt(_q=vKh)rl#+( zyR#WTYGX7H$D-*V%vqX>r(VRuIn8h%O|!y;*_a=s1=uu7nvjq49?1MCQPT(6Yn*&b w>*P@LWZORJd~I4Kg}vituiGfA-a#W-tvYQ#rA3PtEnW%!1fOEJ#{eDx0FoNv;{X5v literal 0 HcmV?d00001 diff --git a/backend/src/apiserver/server/test/pipeline_plus_component/pipeline_plus_component.zip b/backend/src/apiserver/server/test/pipeline_plus_component/pipeline_plus_component.zip new file mode 100644 index 0000000000000000000000000000000000000000..d41626279d808c1e69bd5e3c9a570267f118fdb0 GIT binary patch literal 987 zcmWIWW@Zs#W?VnyyRA%R4kWHdnu|Z(A?$ZOh#WJ03k;q?f(=p6p9m z9gPW}_4)KpO^S$+jym&7Ht+;n;7*Aft_O3zYPMTXzx;f+e@0Anev1y%T$@1f?H|D>$6Xox62f$s*Ho?=EZ)dq<56No0U09|f^d{l#Bgg&0Msg93tF(iSR!v!u z7~f$3x?Snx9nTk2-4hpo=-qRz{xid0#SaU)-k(je$eE>f#Q9dh?V1R=nRyodY2OQr z>?a&)pXnYtsW{!HP(mHuFxMxI9ns+_-!Ld#0Q@wkS$$MJVf3q%(*-7{&_czJMnrjTmw#fq+J@#6j76|?MYd>*Hk%s=pmF|GTD zUX)vRMBMyDDf`3QHTJz`-e?iAbMeMW`*}~kmDSAdpC%B?67f1&AzkCC>GDm}Pi3Fs zEZVEOIz`qXMZ4_n{SqO4;i;3fzrBclQnk{6<3hpr|LP59*N?rLulV|5S<6z*0+F1E zRN;zCWt;U%g+rbgI`>azP*8UjvppJdF>%*Ak4kfcjKB&dYmQr6bi<{}(oMAO8pSR?PWC4a{KvhL2G>O?u`5S z@Al7ID_Vor+H6v)mU_ZfyfDS;?gZI+HtOvr`yMaZwQ7Uy>;4e)OSLZxcD4y``2MS^ zM||JYS9b2d|327%_G8`sYR-QQ0p5&E_RP5QfeJ8vfq+88l131Xnk%>%6hMLuj0_SC z4AlhFpV}n5W9!H=xFgn1Sl?~(sCLr7o Lq}hNOnt=fTrGJ Date: Tue, 12 Mar 2019 17:16:32 -0700 Subject: [PATCH 03/12] Initialize the variables to nil --- backend/src/apiserver/server/util.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/src/apiserver/server/util.go b/backend/src/apiserver/server/util.go index 591d17b394f..ee66e8b9dad 100644 --- a/backend/src/apiserver/server/util.go +++ b/backend/src/apiserver/server/util.go @@ -80,7 +80,7 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") } tarReader := tar.NewReader(gzipReader) - var pipelineYamlReader *tar.Reader + var pipelineYamlReader *tar.Reader = nil for { header, err := tarReader.Next() if err == io.EOF { @@ -112,7 +112,7 @@ func DecompressPipelineZip(compressedFile []byte) ([]byte, error) { if len(reader.File) < 1 { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Empty zip file.") } - var pipelineYamlFile *zip.File + var pipelineYamlFile *zip.File = nil for _, file := range reader.File { if isPipelineYamlFile(file.Name) { pipelineYamlFile = file From b27171c7d90acfadb5c446f8d167abfd734dc9a7 Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Wed, 13 Mar 2019 17:58:29 -0700 Subject: [PATCH 04/12] Trying to read the archive file entry immediately --- backend/src/apiserver/server/util.go | 42 +++++++++++----------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/backend/src/apiserver/server/util.go b/backend/src/apiserver/server/util.go index ee66e8b9dad..d3a9d8c7de5 100644 --- a/backend/src/apiserver/server/util.go +++ b/backend/src/apiserver/server/util.go @@ -80,7 +80,6 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") } tarReader := tar.NewReader(gzipReader) - var pipelineYamlReader *tar.Reader = nil for { header, err := tarReader.Next() if err == io.EOF { @@ -90,18 +89,14 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") } if isPipelineYamlFile(header.Name) { - pipelineYamlReader = tarReader - break + decompressedFile, err := ioutil.ReadAll(tarReader) + if err != nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the tarball file.") + } + return decompressedFile, err } } - if pipelineYamlReader == nil { - return nil, util.NewInvalidInputError("Error extracting pipeline from the .tar.gz file. Could not find any *pipeline.yaml files inside the archive.") - } - decompressedFile, err := ioutil.ReadAll(pipelineYamlReader) - if err != nil { - return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the tarball file.") - } - return decompressedFile, err + return nil, util.NewInvalidInputError("Error extracting pipeline from the .tar.gz file. Could not find any *pipeline.yaml files inside the archive.") } func DecompressPipelineZip(compressedFile []byte) ([]byte, error) { @@ -112,25 +107,20 @@ func DecompressPipelineZip(compressedFile []byte) ([]byte, error) { if len(reader.File) < 1 { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Empty zip file.") } - var pipelineYamlFile *zip.File = nil for _, file := range reader.File { if isPipelineYamlFile(file.Name) { - pipelineYamlFile = file - break + rc, err := file.Open() + if err != nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Failed to read the content.") + } + decompressedFile, err := ioutil.ReadAll(rc) + if err != nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the zip file.") + } + return decompressedFile, err } } - if pipelineYamlFile == nil { - return nil, util.NewInvalidInputError("Error extracting pipeline from the zip file. Could not find any *pipeline.yaml files inside the archive.") - } - rc, err := pipelineYamlFile.Open() - if err != nil { - return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Failed to read the content.") - } - decompressedFile, err := ioutil.ReadAll(rc) - if err != nil { - return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the zip file.") - } - return decompressedFile, err + return nil, util.NewInvalidInputError("Error extracting pipeline from the zip file. Could not find any *pipeline.yaml files inside the archive.") } func ReadPipelineFile(fileName string, fileReader io.Reader, maxFileLength int) ([]byte, error) { From 3d6de027a82cf6f816a483267c3d32008669aea2 Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Mon, 18 Mar 2019 11:46:55 -0700 Subject: [PATCH 05/12] Fixed the pipeline packages used by the `TestPipelineAPI` test. Also added a failing test case. Will disable it in next commit. --- backend/test/integration/pipeline_api_test.go | 22 ++++++++++++------ ...zip => arguments.pipeline.broken_file.zip} | Bin backend/test/resources/arguments.pipeline.zip | Bin 0 -> 767 bytes 3 files changed, 15 insertions(+), 7 deletions(-) rename backend/test/resources/{zip-arguments.zip => arguments.pipeline.broken_file.zip} (100%) create mode 100644 backend/test/resources/arguments.pipeline.zip diff --git a/backend/test/integration/pipeline_api_test.go b/backend/test/integration/pipeline_api_test.go index fdd07b00451..aee1bbad1cb 100644 --- a/backend/test/integration/pipeline_api_test.go +++ b/backend/test/integration/pipeline_api_test.go @@ -80,17 +80,25 @@ func (s *PipelineApiTest) TestPipelineAPI() { /* ---------- Upload pipelines zip ---------- */ time.Sleep(1 * time.Second) argumentUploadPipeline, err := s.pipelineUploadClient.UploadFile( - "../resources/zip-arguments.zip", &uploadParams.UploadPipelineParams{Name: util.StringPointer("zip-arguments-parameters")}) + "../resources/arguments.pipeline.zip", &uploadParams.UploadPipelineParams{Name: util.StringPointer("zip-arguments-parameters")}) assert.Nil(t, err) - assert.Equal(t, "zip-arguments-parameters", argumentUploadPipeline.Name) + assert.Equal(t, "arguments.pipeline", argumentUploadPipeline.Name) + + if true { + //FIX: pipelineUploadClient.UploadFile does not always return error when pipeline is wrong. In some cases (e.g. missing pipeline file inside archive) it returns (nil, nil). + argumentUploadPipeline, err := s.pipelineUploadClient.UploadFile( + "../resources/arguments.pipeline.broken_file.zip", &uploadParams.UploadPipelineParams{Name: util.StringPointer("arguments.pipeline.broken_file")}) + assert.Nil(t, err) + assert.NotNil(t, argumentUploadPipeline) + } /* ---------- Import pipeline tarball by URL ---------- */ time.Sleep(1 * time.Second) argumentUrlPipeline, err := s.pipelineClient.Create(¶ms.CreatePipelineParams{ Body: &pipeline_model.APIPipeline{URL: &pipeline_model.APIURL{ - PipelineURL: "https://storage.googleapis.com/ml-pipeline-dataset/arguments.zip"}}}) + PipelineURL: "https://storage.googleapis.com/ml-pipeline-dataset/arguments.pipeline.zip"}}}) assert.Nil(t, err) - assert.Equal(t, "arguments.zip", argumentUrlPipeline.Name) + assert.Equal(t, "arguments.pipeline.zip", argumentUrlPipeline.Name) /* ---------- Verify list pipeline works ---------- */ pipelines, totalSize, _, err := s.pipelineClient.List(params.NewListPipelinesParams()) @@ -111,7 +119,7 @@ func (s *PipelineApiTest) TestPipelineAPI() { assert.Equal(t, 2, len(listFirstPagePipelines)) assert.Equal(t, 4, totalSize) assert.Equal(t, "arguments-parameters.yaml", listFirstPagePipelines[0].Name) - assert.Equal(t, "arguments.zip", listFirstPagePipelines[1].Name) + assert.Equal(t, "arguments.pipeline.zip", listFirstPagePipelines[1].Name) assert.NotEmpty(t, nextPageToken) listSecondPagePipelines, totalSize, nextPageToken, err := s.pipelineClient.List( @@ -139,7 +147,7 @@ func (s *PipelineApiTest) TestPipelineAPI() { assert.Equal(t, 2, len(listSecondPagePipelines)) assert.Equal(t, 4, totalSize) assert.Equal(t, "zip-arguments-parameters", listSecondPagePipelines[0].Name) - assert.Equal(t, "arguments.zip", listSecondPagePipelines[1].Name) + assert.Equal(t, "arguments.pipeline.zip", listSecondPagePipelines[1].Name) assert.Empty(t, nextPageToken) /* ---------- List pipelines sort by unsupported description field. Should fail. ---------- */ @@ -162,7 +170,7 @@ func (s *PipelineApiTest) TestPipelineAPI() { assert.Nil(t, err) assert.Equal(t, 2, len(listSecondPagePipelines)) assert.Equal(t, 4, totalSize) - assert.Equal(t, "arguments.zip", listSecondPagePipelines[0].Name) + assert.Equal(t, "arguments.pipeline.zip", listSecondPagePipelines[0].Name) assert.Equal(t, "arguments-parameters.yaml", listSecondPagePipelines[1].Name) assert.Empty(t, nextPageToken) diff --git a/backend/test/resources/zip-arguments.zip b/backend/test/resources/arguments.pipeline.broken_file.zip similarity index 100% rename from backend/test/resources/zip-arguments.zip rename to backend/test/resources/arguments.pipeline.broken_file.zip diff --git a/backend/test/resources/arguments.pipeline.zip b/backend/test/resources/arguments.pipeline.zip new file mode 100644 index 0000000000000000000000000000000000000000..1ab940682d20b4e54e49848d4af4475cc8d66e45 GIT binary patch literal 767 zcmWIWW@Zs#W?Mv>NwIE0Vo_plYDsEQ zv4W9aL1sZ}PG(-JUS(o#PHEu5yjuJuJ@nIfT|MiAK$*n{$o*ToY0x)n;)p{XtZ3u zL?$(0b&t-Eri3g5-4|D09xmV1x3#ITg8O}x)B5bM_v%Gj=dN3LNLJ(R!ZH=1HwkAS zIqnZOl8bO$r5$XvYRZDd_y+se?Mff-c)pnGp1Al!@1AS*pBerteptx${%ndx&MdVf z&bImv5SWD*Fs)(O%WnDY6DB+GTI=mk8+#Po1Rw?M3vHs+9&D7Ye@rS8p)8 ze(cqJ#n%tZT9#@Sh~z}13Rhez+pJeA9P-4_xqmW)g1W1i?a_#fiM!T$RGJ%P1Xd_n zbKKgZ8!lCrZlZ19xa`0rnRiSU{G9HuayM1;J8Zq>UZWBa9j%jnkW0(E_N)|pQ{^OU zFQbW;+s|(cTH|wfXWY+!w}0MR(HgYYW|LC2)Dy1ag(+5dC&-mGj8HH-{pKt?eznKLi|03j?$ AzW@LL literal 0 HcmV?d00001 From 5065eaf436bed1cc38a99d533806e042b8764afe Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Tue, 19 Mar 2019 12:38:31 -0700 Subject: [PATCH 06/12] Disabling the test for the UploadFile bug I've discovered --- backend/test/integration/pipeline_api_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/test/integration/pipeline_api_test.go b/backend/test/integration/pipeline_api_test.go index aee1bbad1cb..f288416b277 100644 --- a/backend/test/integration/pipeline_api_test.go +++ b/backend/test/integration/pipeline_api_test.go @@ -84,7 +84,7 @@ func (s *PipelineApiTest) TestPipelineAPI() { assert.Nil(t, err) assert.Equal(t, "arguments.pipeline", argumentUploadPipeline.Name) - if true { + if false { //FIX: pipelineUploadClient.UploadFile does not always return error when pipeline is wrong. In some cases (e.g. missing pipeline file inside archive) it returns (nil, nil). argumentUploadPipeline, err := s.pipelineUploadClient.UploadFile( "../resources/arguments.pipeline.broken_file.zip", &uploadParams.UploadPipelineParams{Name: util.StringPointer("arguments.pipeline.broken_file")}) From 35aa6dd80cd7c756fd6c11b38478380e765a122e Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Tue, 19 Mar 2019 15:27:11 -0700 Subject: [PATCH 07/12] Fixed the pipeline name. --- backend/test/integration/pipeline_api_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/test/integration/pipeline_api_test.go b/backend/test/integration/pipeline_api_test.go index f288416b277..28b19980b4a 100644 --- a/backend/test/integration/pipeline_api_test.go +++ b/backend/test/integration/pipeline_api_test.go @@ -82,7 +82,7 @@ func (s *PipelineApiTest) TestPipelineAPI() { argumentUploadPipeline, err := s.pipelineUploadClient.UploadFile( "../resources/arguments.pipeline.zip", &uploadParams.UploadPipelineParams{Name: util.StringPointer("zip-arguments-parameters")}) assert.Nil(t, err) - assert.Equal(t, "arguments.pipeline", argumentUploadPipeline.Name) + assert.Equal(t, "zip-arguments-parameters", argumentUploadPipeline.Name) if false { //FIX: pipelineUploadClient.UploadFile does not always return error when pipeline is wrong. In some cases (e.g. missing pipeline file inside archive) it returns (nil, nil). From 4a15c0c103d69f9e7a77c19d09befc8fd8f876d0 Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Mon, 25 Mar 2019 17:33:24 -0700 Subject: [PATCH 08/12] Removed the disabled extra test. --- backend/test/integration/pipeline_api_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/backend/test/integration/pipeline_api_test.go b/backend/test/integration/pipeline_api_test.go index 28b19980b4a..4ad99ca07d1 100644 --- a/backend/test/integration/pipeline_api_test.go +++ b/backend/test/integration/pipeline_api_test.go @@ -84,14 +84,6 @@ func (s *PipelineApiTest) TestPipelineAPI() { assert.Nil(t, err) assert.Equal(t, "zip-arguments-parameters", argumentUploadPipeline.Name) - if false { - //FIX: pipelineUploadClient.UploadFile does not always return error when pipeline is wrong. In some cases (e.g. missing pipeline file inside archive) it returns (nil, nil). - argumentUploadPipeline, err := s.pipelineUploadClient.UploadFile( - "../resources/arguments.pipeline.broken_file.zip", &uploadParams.UploadPipelineParams{Name: util.StringPointer("arguments.pipeline.broken_file")}) - assert.Nil(t, err) - assert.NotNil(t, argumentUploadPipeline) - } - /* ---------- Import pipeline tarball by URL ---------- */ time.Sleep(1 * time.Second) argumentUrlPipeline, err := s.pipelineClient.Create(¶ms.CreatePipelineParams{ From 9f635074ec32ffa2b375ea0d4d8fa18626ae1470 Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Mon, 25 Mar 2019 18:05:30 -0700 Subject: [PATCH 09/12] Addressed the feedback. --- backend/src/apiserver/server/util.go | 61 ++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/backend/src/apiserver/server/util.go b/backend/src/apiserver/server/util.go index d3a9d8c7de5..95be394c879 100644 --- a/backend/src/apiserver/server/util.go +++ b/backend/src/apiserver/server/util.go @@ -63,7 +63,7 @@ func isYamlFile(fileName string) bool { } func isPipelineYamlFile(fileName string) bool { - return strings.HasSuffix(fileName, "pipeline.yaml") + return fileName == "pipeline.yaml" } func isZipFile(compressedFile []byte) bool { @@ -79,24 +79,40 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { if err != nil { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") } + //New behavior: searching for the "pipeline.yaml" file. tarReader := tar.NewReader(gzipReader) for { header, err := tarReader.Next() if err == io.EOF { + tarReader = nil break } - if err != nil || header == nil { + if err != nil { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") } if isPipelineYamlFile(header.Name) { - decompressedFile, err := ioutil.ReadAll(tarReader) - if err != nil { - return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the tarball file.") - } - return decompressedFile, err + //Found the pipeline file. + break } } - return nil, util.NewInvalidInputError("Error extracting pipeline from the .tar.gz file. Could not find any *pipeline.yaml files inside the archive.") + //Old behavior - taking the fiurst file in the archive + if tarReader == nil { + gzipReader, err = gzip.NewReader(bytes.NewReader(compressedFile)) + tarReader = tar.NewReader(gzipReader) + } + + header, err := tarReader.Next() + if err != nil || header == nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") + } + if !isYamlFile(header.Name) { + return nil, util.NewInvalidInputError("Error extracting pipeline from the tarball file. Expecting a pipeline.yaml file inside the tarball. Got: %v", header.Name) + } + decompressedFile, err := ioutil.ReadAll(tarReader) + if err != nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the tarball file.") + } + return decompressedFile, err } func DecompressPipelineZip(compressedFile []byte) ([]byte, error) { @@ -107,20 +123,29 @@ func DecompressPipelineZip(compressedFile []byte) ([]byte, error) { if len(reader.File) < 1 { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Empty zip file.") } + + //Old behavior - taking the fiurst file in the archive + pipelineYamlFile := reader.File[0] + //New behavior: searching for the "pipeline.yaml" file. for _, file := range reader.File { if isPipelineYamlFile(file.Name) { - rc, err := file.Open() - if err != nil { - return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Failed to read the content.") - } - decompressedFile, err := ioutil.ReadAll(rc) - if err != nil { - return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the zip file.") - } - return decompressedFile, err + pipelineYamlFile = file + break } } - return nil, util.NewInvalidInputError("Error extracting pipeline from the zip file. Could not find any *pipeline.yaml files inside the archive.") + + if !isYamlFile(pipelineYamlFile.Name) { + return nil, util.NewInvalidInputError("Error extracting pipeline from the zip file. Expecting a pipeline.yaml file inside the zip. Got: %v", pipelineYamlFile.Name) + } + rc, err := pipelineYamlFile.Open() + if err != nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Failed to read the content.") + } + decompressedFile, err := ioutil.ReadAll(rc) + if err != nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error reading pipeline YAML from the zip file.") + } + return decompressedFile, err } func ReadPipelineFile(fileName string, fileReader io.Reader, maxFileLength int) ([]byte, error) { From 875f932a5ae32855a89df77b867cbd1451b91e7b Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Mon, 25 Mar 2019 18:25:37 -0700 Subject: [PATCH 10/12] Removed the "header == nil" check (feedback). --- backend/src/apiserver/server/util.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/src/apiserver/server/util.go b/backend/src/apiserver/server/util.go index 95be394c879..70e461c3a5c 100644 --- a/backend/src/apiserver/server/util.go +++ b/backend/src/apiserver/server/util.go @@ -102,7 +102,7 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { } header, err := tarReader.Next() - if err != nil || header == nil { + if err != nil { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") } if !isYamlFile(header.Name) { From 5e35614f22ccab7b3da788eb60c77de64a0b36bf Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Mon, 25 Mar 2019 20:28:44 -0700 Subject: [PATCH 11/12] Fixed typo --- backend/src/apiserver/server/util.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/src/apiserver/server/util.go b/backend/src/apiserver/server/util.go index 70e461c3a5c..a39aa3bca0e 100644 --- a/backend/src/apiserver/server/util.go +++ b/backend/src/apiserver/server/util.go @@ -95,7 +95,7 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { break } } - //Old behavior - taking the fiurst file in the archive + //Old behavior - taking the first file in the archive if tarReader == nil { gzipReader, err = gzip.NewReader(bytes.NewReader(compressedFile)) tarReader = tar.NewReader(gzipReader) @@ -124,7 +124,7 @@ func DecompressPipelineZip(compressedFile []byte) ([]byte, error) { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Empty zip file.") } - //Old behavior - taking the fiurst file in the archive + //Old behavior - taking the first file in the archive pipelineYamlFile := reader.File[0] //New behavior: searching for the "pipeline.yaml" file. for _, file := range reader.File { From dee8ddb7b30c405abef6045e29c7cd39ac7e7914 Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Tue, 26 Mar 2019 14:45:00 -0700 Subject: [PATCH 12/12] Addressed the PR feedback Added space before comment. Checking for the error again. --- backend/src/apiserver/server/util.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/backend/src/apiserver/server/util.go b/backend/src/apiserver/server/util.go index a39aa3bca0e..bffe1fa9d0c 100644 --- a/backend/src/apiserver/server/util.go +++ b/backend/src/apiserver/server/util.go @@ -79,7 +79,7 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { if err != nil { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") } - //New behavior: searching for the "pipeline.yaml" file. + // New behavior: searching for the "pipeline.yaml" file. tarReader := tar.NewReader(gzipReader) for { header, err := tarReader.Next() @@ -95,9 +95,13 @@ func DecompressPipelineTarball(compressedFile []byte) ([]byte, error) { break } } - //Old behavior - taking the first file in the archive + // Old behavior - taking the first file in the archive if tarReader == nil { + // Resetting the reader gzipReader, err = gzip.NewReader(bytes.NewReader(compressedFile)) + if err != nil { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") + } tarReader = tar.NewReader(gzipReader) } @@ -124,9 +128,9 @@ func DecompressPipelineZip(compressedFile []byte) ([]byte, error) { return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the zip file. Empty zip file.") } - //Old behavior - taking the first file in the archive + // Old behavior - taking the first file in the archive pipelineYamlFile := reader.File[0] - //New behavior: searching for the "pipeline.yaml" file. + // New behavior: searching for the "pipeline.yaml" file. for _, file := range reader.File { if isPipelineYamlFile(file.Name) { pipelineYamlFile = file