From 5b0a856ae833d97d415f7eabbb5c24aa0b400001 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Tue, 26 Jan 2021 09:02:22 -0500 Subject: [PATCH 1/4] add parseURI func Signed-off-by: Stephanie --- pkg/devfile/parser/context/context.go | 18 +++++++++- pkg/devfile/parser/parse.go | 52 +++++++++++++++++++++------ 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/pkg/devfile/parser/context/context.go b/pkg/devfile/parser/context/context.go index c00763c6..b0ab0061 100644 --- a/pkg/devfile/parser/context/context.go +++ b/pkg/devfile/parser/context/context.go @@ -1,6 +1,7 @@ package parser import ( + "fmt" "net/url" "github.com/devfile/library/pkg/testingutil/filesystem" @@ -8,6 +9,8 @@ import ( "k8s.io/klog" ) +var URIMap = make(map[string]bool) + // DevfileCtx stores context info regarding devfile type DevfileCtx struct { @@ -67,6 +70,10 @@ func (d *DevfileCtx) Populate() (err error) { return err } klog.V(4).Infof("absolute devfile path: '%s'", d.absPath) + if _, exist := URIMap[d.absPath]; exist { + return fmt.Errorf("URI %v is recursively referenced", d.absPath) + } + URIMap[d.absPath] = true // Read and save devfile content if err := d.SetDevfileContent(); err != nil { return err @@ -81,7 +88,10 @@ func (d *DevfileCtx) PopulateFromURL() (err error) { if err != nil { return err } - + if _, exist := URIMap[d.url]; exist { + return fmt.Errorf("URI %v is recursively referenced", d.url) + } + URIMap[d.url] = true // Read and save devfile content if err := d.SetDevfileContent(); err != nil { return err @@ -101,10 +111,16 @@ func (d *DevfileCtx) Validate() error { return d.ValidateDevfileSchema() } +// GetAbsPath func returns current devfile absolute path func (d *DevfileCtx) GetAbsPath() string { return d.absPath } +// GetURL func returns current devfile absolute URL address +func (d *DevfileCtx) GetURL() string { + return d.url +} + // SetAbsPath sets absolute file path for devfile func (d *DevfileCtx) SetAbsPath() (err error) { // Set devfile absolute path diff --git a/pkg/devfile/parser/parse.go b/pkg/devfile/parser/parse.go index 109e2e29..a97e498c 100644 --- a/pkg/devfile/parser/parse.go +++ b/pkg/devfile/parser/parse.go @@ -3,6 +3,9 @@ package parser import ( "encoding/json" "fmt" + "net/url" + "path" + "strings" devfileCtx "github.com/devfile/library/pkg/devfile/parser/context" "github.com/devfile/library/pkg/devfile/parser/data" @@ -16,7 +19,7 @@ import ( "github.com/pkg/errors" ) -var URLMap = make(map[string]bool) + // ParseDevfile func validates the devfile integrity. // Creates devfile context and runtime objects @@ -46,8 +49,8 @@ func parseDevfile(d DevfileObj, flattenedDevfile bool) (DevfileObj, error) { return DevfileObj{}, err } } - for url := range URLMap { - delete(URLMap, url) + for uri := range devfileCtx.URIMap { + delete(devfileCtx.URIMap, uri) } // Successful return d, nil @@ -84,11 +87,6 @@ func ParseRawDevfile(path string) (d DevfileObj, err error) { // ParseFromURL func parses and validates the devfile integrity. // Creates devfile context and runtime objects func ParseFromURL(url string) (d DevfileObj, err error) { - if _, exist := URLMap[url]; !exist { - URLMap[url] = true - } else { - return d, fmt.Errorf("URI %v is recursively referenced", url) - } d.Ctx = devfileCtx.NewURLDevfileCtx(url) // Fill the fields of DevfileCtx struct err = d.Ctx.PopulateFromURL() @@ -122,7 +120,7 @@ func parseParentAndPlugin(d DevfileObj) (err error) { parent := d.Data.GetParent() var parentDevfileObj DevfileObj if d.Data.GetParent().Uri != "" { - parentDevfileObj, err = ParseFromURL(parent.Uri) + parentDevfileObj, err = parseFromURI(parent.Uri, d) if err != nil { return err } @@ -153,7 +151,7 @@ func parseParentAndPlugin(d DevfileObj) (err error) { plugin := component.Plugin var pluginDevfileObj DevfileObj if plugin.Uri != "" { - pluginDevfileObj, err = ParseFromURL(plugin.Uri) + pluginDevfileObj, err = parseFromURI(plugin.Uri, d) if err != nil { return err } @@ -181,3 +179,37 @@ func parseParentAndPlugin(d DevfileObj) (err error) { return nil } + + + +func parseFromURI(uri string, curDevfile DevfileObj) (DevfileObj, error){ + // validate URI + + // absolute URL address + if strings.HasPrefix(uri, "http://") || strings.HasPrefix(uri, "https://") { + return ParseFromURL(uri) + } + + // absolute path on disk + if strings.HasPrefix(uri, "file://") { + return Parse(strings.TrimPrefix(uri, "file://")) + } + + // relative path on disk + if curDevfile.Ctx.GetAbsPath() != ""{ + // parse uri receives a relative path and resolves abspath + return Parse(uri) + } + + if curDevfile.Ctx.GetURL() != ""{ + u, err := url.Parse(curDevfile.Ctx.GetURL()) + if err!= nil { + return DevfileObj{}, err + } + u.Path = path.Join(u.Path, uri) + jointURL := u.String() + return ParseFromURL(jointURL) + } + + return DevfileObj{}, fmt.Errorf("fail to parse from uri: %s", uri) +} \ No newline at end of file From 78e6b2802ea49367ccbbfc47efb72beefbb5bcf4 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Mon, 1 Feb 2021 17:04:28 -0500 Subject: [PATCH 2/4] add unit test Signed-off-by: Stephanie --- pkg/devfile/parser/parse.go | 26 +-- pkg/devfile/parser/parse_test.go | 329 ++++++++++++++++++++++++++++--- 2 files changed, 311 insertions(+), 44 deletions(-) diff --git a/pkg/devfile/parser/parse.go b/pkg/devfile/parser/parse.go index a97e498c..f6d2c4c3 100644 --- a/pkg/devfile/parser/parse.go +++ b/pkg/devfile/parser/parse.go @@ -19,8 +19,6 @@ import ( "github.com/pkg/errors" ) - - // ParseDevfile func validates the devfile integrity. // Creates devfile context and runtime objects func parseDevfile(d DevfileObj, flattenedDevfile bool) (DevfileObj, error) { @@ -180,9 +178,7 @@ func parseParentAndPlugin(d DevfileObj) (err error) { return nil } - - -func parseFromURI(uri string, curDevfile DevfileObj) (DevfileObj, error){ +func parseFromURI(uri string, curDevfile DevfileObj) (DevfileObj, error) { // validate URI // absolute URL address @@ -190,26 +186,22 @@ func parseFromURI(uri string, curDevfile DevfileObj) (DevfileObj, error){ return ParseFromURL(uri) } - // absolute path on disk - if strings.HasPrefix(uri, "file://") { - return Parse(strings.TrimPrefix(uri, "file://")) - } - // relative path on disk - if curDevfile.Ctx.GetAbsPath() != ""{ - // parse uri receives a relative path and resolves abspath - return Parse(uri) + if curDevfile.Ctx.GetAbsPath() != "" { + + return Parse(path.Join(path.Dir(curDevfile.Ctx.GetAbsPath()), uri)) } - if curDevfile.Ctx.GetURL() != ""{ + if curDevfile.Ctx.GetURL() != "" { u, err := url.Parse(curDevfile.Ctx.GetURL()) - if err!= nil { + if err != nil { return DevfileObj{}, err } - u.Path = path.Join(u.Path, uri) + + u.Path = path.Join(path.Dir(u.Path), uri) jointURL := u.String() return ParseFromURL(jointURL) } return DevfileObj{}, fmt.Errorf("fail to parse from uri: %s", uri) -} \ No newline at end of file +} diff --git a/pkg/devfile/parser/parse_test.go b/pkg/devfile/parser/parse_test.go index 86e2b8e4..ecce858f 100644 --- a/pkg/devfile/parser/parse_test.go +++ b/pkg/devfile/parser/parse_test.go @@ -2,10 +2,14 @@ package parser import ( "fmt" + "io/ioutil" "net" "net/http" "net/http/httptest" + "os" + "path" "reflect" + "strings" "testing" v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -17,7 +21,6 @@ import ( ) const schemaV200 = "2.0.0" -const devfileTempPath = "devfile.yaml" func Test_parseParentAndPlugin(t *testing.T) { @@ -38,7 +41,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 1: it should override the requested parent's data and add the local devfile's data", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -253,7 +256,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 2: handle a parent'data without any local override and add the local devfile's data", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -436,7 +439,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 3: it should error out when the override is invalid", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -502,7 +505,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 4: error out if the same parent command is defined again in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -555,7 +558,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 5: error out if the same parent component is defined again in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -612,7 +615,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 6: should not have error if the same event is defined again in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -669,7 +672,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 7: error out if the parent project is defined again in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -714,7 +717,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 8: it should merge the plugin's uri data and add the local devfile's data", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -897,7 +900,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 9: it should override the plugin's data with local overrides and add the local devfile's data", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1107,7 +1110,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 10: it should error out when the plugin devfile is invalid", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{}, @@ -1152,7 +1155,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 11: error out if the same plugin command is defined again in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1205,7 +1208,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 12: error out if the same plugin component is defined again in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1262,7 +1265,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 13: error out if the plugin project is defined again in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1307,7 +1310,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 14: error out if the same project is defined in the both plugin devfile and parent", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1371,7 +1374,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 15: error out if the same command is defined in both plugin devfile and parent devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1447,7 +1450,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 16: error out if the same component is defined in both plugin devfile and parent devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1529,7 +1532,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 17: it should override the requested parent's data and plugin's data, and add the local devfile's data", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1784,7 +1787,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 18: error out if the plugin component is defined with a different component type in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1841,7 +1844,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 19: it should override with no errors if the plugin component is defined with a different component type in the plugin override", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{}, }, @@ -1914,7 +1917,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 20: error out if the parent component is defined with a different component type in the local devfile", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -1971,7 +1974,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 21: it should override with no errors if the parent component is defined with a different component type in the parent override", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -2049,7 +2052,7 @@ func Test_parseParentAndPlugin(t *testing.T) { name: "case 22: error out if the URI is recursively referenced", args: args{ devFileObj: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{}, }, @@ -2189,7 +2192,7 @@ func Test_parseParentAndPlugin_RecursivelyReference_withMultipleURI(t *testing.T const httpPrefix = "http://" devFileObj := DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ @@ -2219,7 +2222,7 @@ func Test_parseParentAndPlugin_RecursivelyReference_withMultipleURI(t *testing.T }, } parentDevfile1 := DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevfileHeader: devfilepkg.DevfileHeader{ @@ -2252,7 +2255,7 @@ func Test_parseParentAndPlugin_RecursivelyReference_withMultipleURI(t *testing.T }, } parentDevfile2 := DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevfileHeader: devfilepkg.DevfileHeader{ @@ -2280,7 +2283,7 @@ func Test_parseParentAndPlugin_RecursivelyReference_withMultipleURI(t *testing.T }, } parentDevfile3 := DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(devfileTempPath), + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ DevfileHeader: devfilepkg.DevfileHeader{ @@ -2395,3 +2398,275 @@ func Test_parseParentAndPlugin_RecursivelyReference_withMultipleURI(t *testing.T }) } + +func Test_parseFromURI(t *testing.T) { + const uri1 = "127.0.0.1:8080" + const httpPrefix = "http://" + const localRelativeURI = "testTmp/dir/devfile.yaml" + const notExistURI = "notexist/devfile.yaml" + uri2 := path.Join(uri1, localRelativeURI) + + localDevfile := DevfileObj{ + Ctx: devfileCtx.NewDevfileCtx(localRelativeURI), + Data: &v2.DevfileV2{ + Devfile: v1.Devfile{ + DevfileHeader: devfilepkg.DevfileHeader{ + SchemaVersion: schemaV200, + }, + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Components: []v1.Component{ + { + Name: "runtime", + ComponentUnion: v1.ComponentUnion{ + Container: &v1.ContainerComponent{ + Container: v1.Container{ + Image: "nodejs", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + // prepare for local file + err := os.MkdirAll(path.Dir(localRelativeURI), 0755) + if err != nil { + t.Errorf("failed to create folder: %v, error: %v", path.Dir(localRelativeURI), err) + return + } + yamlData, err := yaml.Marshal(localDevfile.Data) + if err != nil { + t.Errorf("failed to marshall devfile data: %v", err) + return + } + err = ioutil.WriteFile(localRelativeURI, yamlData, 0644) + if err != nil { + t.Errorf("fail to write to file: %v", err) + return + } + defer os.RemoveAll("testTmp/") + + parentDevfile := DevfileObj{ + Ctx: devfileCtx.NewURLDevfileCtx(httpPrefix + uri1), + Data: &v2.DevfileV2{ + Devfile: v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + Parent: &v1.Parent{ + ImportReference: v1.ImportReference{ + ImportReferenceUnion: v1.ImportReferenceUnion{ + Uri: localRelativeURI, + }, + }, + }, + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Components: []v1.Component{ + { + Name: "runtime2", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{ + Volume: v1.Volume{ + Size: "500Mi", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + relativeParentDevfile := DevfileObj{ + Ctx: devfileCtx.NewURLDevfileCtx(httpPrefix + uri2), + Data: &v2.DevfileV2{ + Devfile: v1.Devfile{ + DevfileHeader: devfilepkg.DevfileHeader{ + SchemaVersion: schemaV200, + }, + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Components: []v1.Component{ + { + Name: "runtime", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{ + Volume: v1.Volume{ + Size: "500Mi", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + testServer := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "notexist") { + w.WriteHeader(http.StatusNotFound) + return + } + var data []byte + var err error + if strings.Contains(r.URL.Path, "devfile.yaml") { + data, err = yaml.Marshal(relativeParentDevfile.Data) + } else { + data, err = yaml.Marshal(parentDevfile.Data) + } + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + _, err = w.Write(data) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + })) + // create a listener with the desired port. + l, err := net.Listen("tcp", uri1) + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + + // NewUnstartedServer creates a listener. Close that listener and replace + // with the one we created. + testServer.Listener.Close() + testServer.Listener = l + + testServer.Start() + defer testServer.Close() + + tests := []struct { + name string + curDevfile DevfileObj + uri string + wantDevFile DevfileObj + wantErr bool + }{ + { + name: "case 1: should be able to parse from relative uri on local disk", + curDevfile: DevfileObj{ + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), + Data: &v2.DevfileV2{ + Devfile: v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + Parent: &v1.Parent{ + ImportReference: v1.ImportReference{ + ImportReferenceUnion: v1.ImportReferenceUnion{ + Uri: localRelativeURI, + }, + }, + }, + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, + }, + }, + }, + }, + wantDevFile: localDevfile, + uri: localRelativeURI, + }, + { + name: "case 2: should be able to parse relative uri from URL", + curDevfile: parentDevfile, + wantDevFile: relativeParentDevfile, + uri: localRelativeURI, + }, + { + name: "case 3: should fail if no path or url has been set for devfile ctx", + curDevfile: DevfileObj{ + Data: &v2.DevfileV2{ + Devfile: v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + Parent: &v1.Parent{ + ImportReference: v1.ImportReference{ + ImportReferenceUnion: v1.ImportReferenceUnion{ + Uri: localRelativeURI, + }, + }, + }, + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "case 4: should fail if file not exist", + curDevfile: DevfileObj{ + Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), + Data: &v2.DevfileV2{ + Devfile: v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + Parent: &v1.Parent{ + ImportReference: v1.ImportReference{ + ImportReferenceUnion: v1.ImportReferenceUnion{ + Uri: "notexist/devfile.yaml", + }, + }, + }, + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, + }, + }, + }, + }, + uri: "notexist/devfile.yaml", + wantErr: true, + }, + { + name: "case 5: should fail if url not exist", + curDevfile: DevfileObj{ + Ctx: devfileCtx.NewURLDevfileCtx(httpPrefix + uri1), + Data: &v2.DevfileV2{ + Devfile: v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + Parent: &v1.Parent{ + ImportReference: v1.ImportReference{ + ImportReferenceUnion: v1.ImportReferenceUnion{ + Uri: notExistURI, + }, + }, + }, + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, + }, + }, + }, + }, + uri: notExistURI, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // if the main devfile is from local, need to set absolute path + if tt.curDevfile.Ctx.GetURL() == "" { + err := tt.curDevfile.Ctx.SetAbsPath() + if err != nil { + t.Errorf("Test_parseFromURI() unexpected error = %v", err) + return + } + } + got, err := parseFromURI(tt.uri, tt.curDevfile) + if (err != nil) != tt.wantErr { + t.Errorf("Test_parseFromURI() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if err != nil && tt.wantErr { + return + } + + if !reflect.DeepEqual(got.Data, tt.wantDevFile.Data) { + t.Errorf("wanted: %v, got: %v, difference at %v", tt.wantDevFile, got, pretty.Compare(tt.wantDevFile, got)) + } + }) + } +} From 83f10f5d7f3aca902f6756979a49b57f4d136ba5 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Tue, 2 Feb 2021 13:54:13 -0500 Subject: [PATCH 3/4] add validateURI before paring uri Signed-off-by: Stephanie --- go.mod | 2 +- go.sum | 4 ++-- pkg/devfile/parser/parse.go | 6 +++++- pkg/devfile/parser/parse_test.go | 23 +++++++++++++++++++++++ 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 2be5b7e9..0b91943a 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/devfile/library go 1.13 require ( - github.com/devfile/api/v2 v2.0.0-20210121164412-49ba915897f4 + github.com/devfile/api/v2 v2.0.0-20210202172954-6424f4139ac7 github.com/fatih/color v1.7.0 github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32 github.com/gobwas/glob v0.2.3 diff --git a/go.sum b/go.sum index b6b09b9a..d8ff706f 100644 --- a/go.sum +++ b/go.sum @@ -44,8 +44,8 @@ github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7Do github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/devfile/api/v2 v2.0.0-20210121164412-49ba915897f4 h1:jDzWFpF/BoyaemoPjAzw5SmlX172JsSsh+Frujm5Ww4= -github.com/devfile/api/v2 v2.0.0-20210121164412-49ba915897f4/go.mod h1:Cot4snybn3qhIh48oIFi9McocnIx7zY5fFbjfrIpPvg= +github.com/devfile/api/v2 v2.0.0-20210202172954-6424f4139ac7 h1:bQGUVLEGQtVkvS94K4gQbu57Rk/npcZQmgORmCWYNy8= +github.com/devfile/api/v2 v2.0.0-20210202172954-6424f4139ac7/go.mod h1:Cot4snybn3qhIh48oIFi9McocnIx7zY5fFbjfrIpPvg= github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ= github.com/docker/docker v0.7.3-0.20190327010347-be7ac8be2ae0/go.mod h1:eEKB0N0r5NX/I1kEveEz05bcu8tLC/8azJZsviup8Sk= github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= diff --git a/pkg/devfile/parser/parse.go b/pkg/devfile/parser/parse.go index f6d2c4c3..2eecf5b9 100644 --- a/pkg/devfile/parser/parse.go +++ b/pkg/devfile/parser/parse.go @@ -16,6 +16,7 @@ import ( v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" apiOverride "github.com/devfile/api/v2/pkg/utils/overriding" + "github.com/devfile/api/v2/pkg/validation" "github.com/pkg/errors" ) @@ -180,6 +181,10 @@ func parseParentAndPlugin(d DevfileObj) (err error) { func parseFromURI(uri string, curDevfile DevfileObj) (DevfileObj, error) { // validate URI + err := validation.ValidateURI(uri) + if err != nil { + return DevfileObj{}, err + } // absolute URL address if strings.HasPrefix(uri, "http://") || strings.HasPrefix(uri, "https://") { @@ -188,7 +193,6 @@ func parseFromURI(uri string, curDevfile DevfileObj) (DevfileObj, error) { // relative path on disk if curDevfile.Ctx.GetAbsPath() != "" { - return Parse(path.Join(path.Dir(curDevfile.Ctx.GetAbsPath()), uri)) } diff --git a/pkg/devfile/parser/parse_test.go b/pkg/devfile/parser/parse_test.go index ecce858f..9d41c69b 100644 --- a/pkg/devfile/parser/parse_test.go +++ b/pkg/devfile/parser/parse_test.go @@ -2404,6 +2404,7 @@ func Test_parseFromURI(t *testing.T) { const httpPrefix = "http://" const localRelativeURI = "testTmp/dir/devfile.yaml" const notExistURI = "notexist/devfile.yaml" + const invalidURL = "http//invalid.com" uri2 := path.Join(uri1, localRelativeURI) localDevfile := DevfileObj{ @@ -2643,6 +2644,28 @@ func Test_parseFromURI(t *testing.T) { uri: notExistURI, wantErr: true, }, + { + name: "case 6: should fail if with invalid URI format", + curDevfile: DevfileObj{ + Ctx: devfileCtx.NewURLDevfileCtx(OutputDevfileYamlPath), + Data: &v2.DevfileV2{ + Devfile: v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + Parent: &v1.Parent{ + ImportReference: v1.ImportReference{ + ImportReferenceUnion: v1.ImportReferenceUnion{ + Uri: invalidURL, + }, + }, + }, + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, + }, + }, + }, + }, + uri: invalidURL, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From fb8405c93a339140873335f8f42aef8969bdb5b2 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Thu, 4 Feb 2021 12:47:14 -0500 Subject: [PATCH 4/4] address review comments Signed-off-by: Stephanie --- pkg/devfile/parser/context/context.go | 4 +- pkg/devfile/parser/parse.go | 18 ++-- pkg/devfile/parser/parse_test.go | 150 ++++++-------------------- 3 files changed, 46 insertions(+), 126 deletions(-) diff --git a/pkg/devfile/parser/context/context.go b/pkg/devfile/parser/context/context.go index b0ab0061..252a920c 100644 --- a/pkg/devfile/parser/context/context.go +++ b/pkg/devfile/parser/context/context.go @@ -70,7 +70,7 @@ func (d *DevfileCtx) Populate() (err error) { return err } klog.V(4).Infof("absolute devfile path: '%s'", d.absPath) - if _, exist := URIMap[d.absPath]; exist { + if URIMap[d.absPath] { return fmt.Errorf("URI %v is recursively referenced", d.absPath) } URIMap[d.absPath] = true @@ -88,7 +88,7 @@ func (d *DevfileCtx) PopulateFromURL() (err error) { if err != nil { return err } - if _, exist := URIMap[d.url]; exist { + if URIMap[d.url] { return fmt.Errorf("URI %v is recursively referenced", d.url) } URIMap[d.url] = true diff --git a/pkg/devfile/parser/parse.go b/pkg/devfile/parser/parse.go index 2eecf5b9..a86459b4 100644 --- a/pkg/devfile/parser/parse.go +++ b/pkg/devfile/parser/parse.go @@ -119,7 +119,7 @@ func parseParentAndPlugin(d DevfileObj) (err error) { parent := d.Data.GetParent() var parentDevfileObj DevfileObj if d.Data.GetParent().Uri != "" { - parentDevfileObj, err = parseFromURI(parent.Uri, d) + parentDevfileObj, err = parseFromURI(parent.Uri, d.Ctx) if err != nil { return err } @@ -150,7 +150,7 @@ func parseParentAndPlugin(d DevfileObj) (err error) { plugin := component.Plugin var pluginDevfileObj DevfileObj if plugin.Uri != "" { - pluginDevfileObj, err = parseFromURI(plugin.Uri, d) + pluginDevfileObj, err = parseFromURI(plugin.Uri, d.Ctx) if err != nil { return err } @@ -179,7 +179,7 @@ func parseParentAndPlugin(d DevfileObj) (err error) { return nil } -func parseFromURI(uri string, curDevfile DevfileObj) (DevfileObj, error) { +func parseFromURI(uri string, curDevfileCtx devfileCtx.DevfileCtx) (DevfileObj, error) { // validate URI err := validation.ValidateURI(uri) if err != nil { @@ -192,19 +192,19 @@ func parseFromURI(uri string, curDevfile DevfileObj) (DevfileObj, error) { } // relative path on disk - if curDevfile.Ctx.GetAbsPath() != "" { - return Parse(path.Join(path.Dir(curDevfile.Ctx.GetAbsPath()), uri)) + if curDevfileCtx.GetAbsPath() != "" { + return Parse(path.Join(path.Dir(curDevfileCtx.GetAbsPath()), uri)) } - if curDevfile.Ctx.GetURL() != "" { - u, err := url.Parse(curDevfile.Ctx.GetURL()) + if curDevfileCtx.GetURL() != "" { + u, err := url.Parse(curDevfileCtx.GetURL()) if err != nil { return DevfileObj{}, err } u.Path = path.Join(path.Dir(u.Path), uri) - jointURL := u.String() - return ParseFromURL(jointURL) + // u.String() is the joint absolute URL path + return ParseFromURL(u.String()) } return DevfileObj{}, fmt.Errorf("fail to parse from uri: %s", uri) diff --git a/pkg/devfile/parser/parse_test.go b/pkg/devfile/parser/parse_test.go index 9d41c69b..b1df4bd5 100644 --- a/pkg/devfile/parser/parse_test.go +++ b/pkg/devfile/parser/parse_test.go @@ -2456,6 +2456,9 @@ func Test_parseFromURI(t *testing.T) { Ctx: devfileCtx.NewURLDevfileCtx(httpPrefix + uri1), Data: &v2.DevfileV2{ Devfile: v1.Devfile{ + DevfileHeader: devfilepkg.DevfileHeader{ + SchemaVersion: schemaV200, + }, DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ Parent: &v1.Parent{ ImportReference: v1.ImportReference{ @@ -2546,147 +2549,64 @@ func Test_parseFromURI(t *testing.T) { defer testServer.Close() tests := []struct { - name string - curDevfile DevfileObj - uri string - wantDevFile DevfileObj - wantErr bool + name string + curDevfileCtx devfileCtx.DevfileCtx + uri string + wantDevFile DevfileObj + wantErr bool }{ { - name: "case 1: should be able to parse from relative uri on local disk", - curDevfile: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), - Data: &v2.DevfileV2{ - Devfile: v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - Parent: &v1.Parent{ - ImportReference: v1.ImportReference{ - ImportReferenceUnion: v1.ImportReferenceUnion{ - Uri: localRelativeURI, - }, - }, - }, - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, - }, - }, - }, - }, - wantDevFile: localDevfile, - uri: localRelativeURI, + name: "case 1: should be able to parse from relative uri on local disk", + curDevfileCtx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), + wantDevFile: localDevfile, + uri: localRelativeURI, }, { - name: "case 2: should be able to parse relative uri from URL", - curDevfile: parentDevfile, - wantDevFile: relativeParentDevfile, - uri: localRelativeURI, + name: "case 2: should be able to parse relative uri from URL", + curDevfileCtx: parentDevfile.Ctx, + wantDevFile: relativeParentDevfile, + uri: localRelativeURI, }, { - name: "case 3: should fail if no path or url has been set for devfile ctx", - curDevfile: DevfileObj{ - Data: &v2.DevfileV2{ - Devfile: v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - Parent: &v1.Parent{ - ImportReference: v1.ImportReference{ - ImportReferenceUnion: v1.ImportReferenceUnion{ - Uri: localRelativeURI, - }, - }, - }, - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, - }, - }, - }, - }, - wantErr: true, + name: "case 3: should fail if no path or url has been set for devfile ctx", + curDevfileCtx: devfileCtx.DevfileCtx{}, + wantErr: true, }, { - name: "case 4: should fail if file not exist", - curDevfile: DevfileObj{ - Ctx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), - Data: &v2.DevfileV2{ - Devfile: v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - Parent: &v1.Parent{ - ImportReference: v1.ImportReference{ - ImportReferenceUnion: v1.ImportReferenceUnion{ - Uri: "notexist/devfile.yaml", - }, - }, - }, - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, - }, - }, - }, - }, - uri: "notexist/devfile.yaml", - wantErr: true, + name: "case 4: should fail if file not exist", + curDevfileCtx: devfileCtx.NewDevfileCtx(OutputDevfileYamlPath), + uri: notExistURI, + wantErr: true, }, { - name: "case 5: should fail if url not exist", - curDevfile: DevfileObj{ - Ctx: devfileCtx.NewURLDevfileCtx(httpPrefix + uri1), - Data: &v2.DevfileV2{ - Devfile: v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - Parent: &v1.Parent{ - ImportReference: v1.ImportReference{ - ImportReferenceUnion: v1.ImportReferenceUnion{ - Uri: notExistURI, - }, - }, - }, - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, - }, - }, - }, - }, - uri: notExistURI, - wantErr: true, + name: "case 5: should fail if url not exist", + curDevfileCtx: devfileCtx.NewURLDevfileCtx(httpPrefix + uri1), + uri: notExistURI, + wantErr: true, }, { - name: "case 6: should fail if with invalid URI format", - curDevfile: DevfileObj{ - Ctx: devfileCtx.NewURLDevfileCtx(OutputDevfileYamlPath), - Data: &v2.DevfileV2{ - Devfile: v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - Parent: &v1.Parent{ - ImportReference: v1.ImportReference{ - ImportReferenceUnion: v1.ImportReferenceUnion{ - Uri: invalidURL, - }, - }, - }, - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{}, - }, - }, - }, - }, - uri: invalidURL, - wantErr: true, + name: "case 6: should fail if with invalid URI format", + curDevfileCtx: devfileCtx.NewURLDevfileCtx(OutputDevfileYamlPath), + uri: invalidURL, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // if the main devfile is from local, need to set absolute path - if tt.curDevfile.Ctx.GetURL() == "" { - err := tt.curDevfile.Ctx.SetAbsPath() + if tt.curDevfileCtx.GetURL() == "" { + err := tt.curDevfileCtx.SetAbsPath() if err != nil { t.Errorf("Test_parseFromURI() unexpected error = %v", err) return } } - got, err := parseFromURI(tt.uri, tt.curDevfile) - if (err != nil) != tt.wantErr { + got, err := parseFromURI(tt.uri, tt.curDevfileCtx) + if tt.wantErr == (err == nil) { t.Errorf("Test_parseFromURI() error = %v, wantErr %v", err, tt.wantErr) return } - if err != nil && tt.wantErr { - return - } - if !reflect.DeepEqual(got.Data, tt.wantDevFile.Data) { t.Errorf("wanted: %v, got: %v, difference at %v", tt.wantDevFile, got, pretty.Compare(tt.wantDevFile, got)) }