From 2c2445df83fa879387a200747cc20f72a7ee9727 Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Thu, 28 Mar 2019 15:09:58 -0700 Subject: [PATCH] Enable pipeline packages with multiple files (#939) * Enable pipeline packages with multiple files * Added tests * Initialize the variables to nil * Trying to read the archive file entry immediately * Fixed the pipeline packages used by the `TestPipelineAPI` test. Also added a failing test case. Will disable it in next commit. * Disabling the test for the UploadFile bug I've discovered * Fixed the pipeline name. * Removed the disabled extra test. * Addressed the feedback. * Removed the "header == nil" check (feedback). * Fixed typo * Addressed the PR feedback Added space before comment. Checking for the error again. --- .../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.go | 50 ++++++++++++++++-- backend/src/apiserver/server/util_test.go | 18 +++++++ backend/test/integration/pipeline_api_test.go | 12 ++--- ...zip => arguments.pipeline.broken_file.zip} | Bin backend/test/resources/arguments.pipeline.zip | Bin 0 -> 767 bytes 11 files changed, 114 insertions(+), 11 deletions(-) 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 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/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 2 && compressedFile[0] == '\x50' && compressedFile[1] == '\x4B' //Signature of zip file is "PK" } @@ -75,13 +79,38 @@ 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 { + return nil, util.NewInvalidInputErrorWithDetails(err, "Error extracting pipeline from the tarball file. Not a valid tarball file.") + } + if isPipelineYamlFile(header.Name) { + //Found the pipeline file. + break + } + } + // 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) + } + 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) { - return nil, util.NewInvalidInputError("Error extracting pipeline from the tarball file. Expecting a YAML file inside the tarball. Got: %v", 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 { @@ -98,10 +127,21 @@ 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) + + // 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 { + if isPipelineYamlFile(file.Name) { + pipelineYamlFile = file + break + } + } + + 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 := 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.") } diff --git a/backend/src/apiserver/server/util_test.go b/backend/src/apiserver/server/util_test.go index 866f76b271a..e188c922cc4 100644 --- a/backend/src/apiserver/server/util_test.go +++ b/backend/src/apiserver/server/util_test.go @@ -148,6 +148,15 @@ func TestReadPipelineFile_Zip_AnyExtension(t *testing.T) { assert.Equal(t, expectedPipelineFile, pipelineFile) } +func TestReadPipelineFile_MultifileZip(t *testing.T) { + file, _ := os.Open("test/pipeline_plus_component/pipeline_plus_component.zip") + pipelineFile, err := ReadPipelineFile("pipeline_plus_component.ai-hub-package", file, MaxFileLength) + assert.Nil(t, err) + + expectedPipelineFile, _ := ioutil.ReadFile("test/pipeline_plus_component/pipeline.yaml") + assert.Equal(t, expectedPipelineFile, pipelineFile) +} + func TestReadPipelineFile_Tarball(t *testing.T) { file, _ := os.Open("test/arguments_tarball/arguments.tar.gz") pipelineFile, err := ReadPipelineFile("arguments.tar.gz", file, MaxFileLength) @@ -166,6 +175,15 @@ func TestReadPipelineFile_Tarball_AnyExtension(t *testing.T) { assert.Equal(t, expectedPipelineFile, pipelineFile) } +func TestReadPipelineFile_MultifileTarball(t *testing.T) { + file, _ := os.Open("test/pipeline_plus_component/pipeline_plus_component.tar.gz") + pipelineFile, err := ReadPipelineFile("pipeline_plus_component.ai-hub-package", file, MaxFileLength) + assert.Nil(t, err) + + expectedPipelineFile, _ := ioutil.ReadFile("test/pipeline_plus_component/pipeline.yaml") + assert.Equal(t, expectedPipelineFile, pipelineFile) +} + func TestReadPipelineFile_UnknownFileFormat(t *testing.T) { file, _ := os.Open("test/unknown_format.foo") _, err := ReadPipelineFile("unknown_format.foo", file, MaxFileLength) diff --git a/backend/test/integration/pipeline_api_test.go b/backend/test/integration/pipeline_api_test.go index fdd07b00451..4ad99ca07d1 100644 --- a/backend/test/integration/pipeline_api_test.go +++ b/backend/test/integration/pipeline_api_test.go @@ -80,7 +80,7 @@ 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) @@ -88,9 +88,9 @@ func (s *PipelineApiTest) TestPipelineAPI() { 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 +111,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 +139,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 +162,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