diff --git a/pkg/kubelet/config/http.go b/pkg/kubelet/config/http.go index 2a2533d04a3..baed2c3ffcc 100644 --- a/pkg/kubelet/config/http.go +++ b/pkg/kubelet/config/http.go @@ -66,23 +66,18 @@ func (s *SourceURL) extractFromURL() error { } // First try as if it's a single manifest - var pod kubelet.Pod - singleErr := yaml.Unmarshal(data, &pod.Manifest) - // TODO: replace with validation - if singleErr == nil && pod.Manifest.Version == "" { - // If data is a []ContainerManifest, trying to put it into a ContainerManifest - // will not give an error but also won't set any of the fields. - // Our docs say that the version field is mandatory, so using that to judge wether - // this was actually successful. - singleErr = fmt.Errorf("got blank version field") - } - + var manifest api.ContainerManifest + singleErr := yaml.Unmarshal(data, &manifest) if singleErr == nil { - name := pod.Manifest.ID - if name == "" { - name = "1" + if errs := api.ValidateManifest(&manifest); len(errs) > 0 { + singleErr = fmt.Errorf("invalid manifest: %v", errs) + } + } + if singleErr == nil { + pod := kubelet.Pod{Name: manifest.ID, Manifest: manifest} + if pod.Name == "" { + pod.Name = "1" } - pod.Name = name s.updates <- kubelet.PodUpdate{[]kubelet.Pod{pod}, kubelet.SET} return nil } @@ -93,18 +88,21 @@ func (s *SourceURL) extractFromURL() error { // We're not sure if the person reading the logs is going to care about the single or // multiple manifest unmarshalling attempt, so we need to put both in the logs, as is // done at the end. Hence not returning early here. - if multiErr == nil && len(manifests) > 0 && manifests[0].Version == "" { - multiErr = fmt.Errorf("got blank version field") + if multiErr == nil { + for _, manifest := range manifests { + if errs := api.ValidateManifest(&manifest); len(errs) > 0 { + multiErr = fmt.Errorf("invalid manifest: %v", errs) + break + } + } } if multiErr == nil { pods := []kubelet.Pod{} - for i := range manifests { - pod := kubelet.Pod{Manifest: manifests[i]} - name := pod.Manifest.ID - if name == "" { - name = fmt.Sprintf("%d", i+1) + for i, manifest := range manifests { + pod := kubelet.Pod{Name: manifest.ID, Manifest: manifest} + if pod.Name == "" { + pod.Name = fmt.Sprintf("%d", i+1) } - pod.Name = name pods = append(pods, pod) } s.updates <- kubelet.PodUpdate{pods, kubelet.SET} @@ -113,5 +111,5 @@ func (s *SourceURL) extractFromURL() error { return fmt.Errorf("%v: received '%v', but couldn't parse as a "+ "single manifest (%v: %+v) or as multiple manifests (%v: %+v).\n", - s.url, string(data), singleErr, pod.Manifest, multiErr, manifests) + s.url, string(data), singleErr, manifest, multiErr, manifests) } diff --git a/pkg/kubelet/config/http_test.go b/pkg/kubelet/config/http_test.go index 4e08b45fc79..6bf466c0441 100644 --- a/pkg/kubelet/config/http_test.go +++ b/pkg/kubelet/config/http_test.go @@ -41,97 +41,119 @@ func TestURLErrorNotExistNoUpdate(t *testing.T) { func TestExtractFromHttpBadness(t *testing.T) { ch := make(chan interface{}, 1) c := SourceURL{"http://localhost:49575/_not_found_", ch} - err := c.extractFromURL() - if err == nil { + if err := c.extractFromURL(); err == nil { t.Errorf("Expected error") } expectEmptyChannel(t, ch) } -func TestExtractFromHttpSingle(t *testing.T) { - manifests := []api.ContainerManifest{ - {Version: "v1beta1", ID: "foo"}, +func TestExtractInvalidManifest(t *testing.T) { + var testCases = []struct { + desc string + manifests interface{} + }{ + { + desc: "No version", + manifests: []api.ContainerManifest{{Version: ""}}, + }, + { + desc: "Invalid version", + manifests: []api.ContainerManifest{{Version: "v1betta2"}}, + }, + { + desc: "Invalid volume name", + manifests: []api.ContainerManifest{ + {Version: "v1beta1", Volumes: []api.Volume{{Name: "_INVALID_"}}}, + }, + }, + { + desc: "Duplicate volume names", + manifests: []api.ContainerManifest{ + { + Version: "v1beta1", + Volumes: []api.Volume{{Name: "repeated"}, {Name: "repeated"}}, + }, + }, + }, } - // Taking a single-manifest from a URL allows kubelet to be used - // in the implementation of google's container VM image. - data, err := json.Marshal(manifests[0]) - - fakeHandler := util.FakeHandler{ - StatusCode: 200, - ResponseBody: string(data), - } - testServer := httptest.NewServer(&fakeHandler) - ch := make(chan interface{}, 1) - c := SourceURL{testServer.URL, ch} - - err = c.extractFromURL() - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - update := (<-ch).(kubelet.PodUpdate) - expected := CreatePodUpdate(kubelet.SET, kubelet.Pod{Name: "foo", Manifest: manifests[0]}) - if !reflect.DeepEqual(expected, update) { - t.Errorf("Expected: %#v, Got: %#v", expected, update) - } -} - -func TestExtractFromHttpMultiple(t *testing.T) { - manifests := []api.ContainerManifest{ - {Version: "v1beta1", ID: "", Containers: []api.Container{{Name: "1", Image: "foo"}}}, - {Version: "v1beta1", ID: "bar", Containers: []api.Container{{Name: "1", Image: "foo"}}}, - } - data, err := json.Marshal(manifests) - if err != nil { - t.Fatalf("Some weird json problem: %v", err) - } - - fakeHandler := util.FakeHandler{ - StatusCode: 200, - ResponseBody: string(data), - } - testServer := httptest.NewServer(&fakeHandler) - ch := make(chan interface{}, 1) - c := SourceURL{testServer.URL, ch} - - err = c.extractFromURL() - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - - update := (<-ch).(kubelet.PodUpdate) - expected := CreatePodUpdate(kubelet.SET, kubelet.Pod{Name: "1", Manifest: manifests[0]}, kubelet.Pod{Name: "bar", Manifest: manifests[1]}) - if !reflect.DeepEqual(expected, update) { - t.Errorf("Expected: %#v, Got: %#v", expected, update) - } - for i := range update.Pods { - if errs := kubelet.ValidatePod(&update.Pods[i]); len(errs) != 0 { - t.Errorf("Expected no validation errors on %#v, Got %#v", update.Pods[i], errs) + for _, testCase := range testCases { + data, err := json.Marshal(testCase.manifests) + if err != nil { + t.Fatalf("%s: Some weird json problem: %v", testCase.desc, err) + } + fakeHandler := util.FakeHandler{ + StatusCode: 200, + ResponseBody: string(data), + } + testServer := httptest.NewServer(&fakeHandler) + ch := make(chan interface{}, 1) + c := SourceURL{testServer.URL, ch} + if err := c.extractFromURL(); err == nil { + t.Errorf("%s: Expected error", testCase.desc) } } } -func TestExtractFromHttpEmptyArray(t *testing.T) { - manifests := []api.ContainerManifest{} - data, err := json.Marshal(manifests) - if err != nil { - t.Fatalf("Some weird json problem: %v", err) +func TestExtractFromHTTP(t *testing.T) { + var testCases = []struct { + desc string + manifests interface{} + expected kubelet.PodUpdate + }{ + { + desc: "Single manifest", + manifests: api.ContainerManifest{Version: "v1beta1", ID: "foo"}, + expected: CreatePodUpdate(kubelet.SET, + kubelet.Pod{ + Name: "foo", + Manifest: api.ContainerManifest{Version: "v1beta1", ID: "foo"}, + }), + }, + { + desc: "Multiple manifests", + manifests: []api.ContainerManifest{ + {Version: "v1beta1", ID: "", Containers: []api.Container{{Name: "1", Image: "foo"}}}, + {Version: "v1beta1", ID: "bar", Containers: []api.Container{{Name: "1", Image: "foo"}}}, + }, + expected: CreatePodUpdate(kubelet.SET, + kubelet.Pod{ + Name: "1", + Manifest: api.ContainerManifest{Version: "v1beta1", ID: "", Containers: []api.Container{{Name: "1", Image: "foo"}}}, + }, + kubelet.Pod{ + Name: "bar", + Manifest: api.ContainerManifest{Version: "v1beta1", ID: "bar", Containers: []api.Container{{Name: "1", Image: "foo"}}}, + }), + }, + { + desc: "Empty Array", + manifests: []api.ContainerManifest{}, + expected: CreatePodUpdate(kubelet.SET), + }, } - - fakeHandler := util.FakeHandler{ - StatusCode: 200, - ResponseBody: string(data), - } - testServer := httptest.NewServer(&fakeHandler) - ch := make(chan interface{}, 1) - c := SourceURL{testServer.URL, ch} - - err = c.extractFromURL() - if err != nil { - t.Errorf("Unexpected error: %v", err) - } - update := (<-ch).(kubelet.PodUpdate) - expected := CreatePodUpdate(kubelet.SET) - if !reflect.DeepEqual(expected, update) { - t.Errorf("Expected: %#v, Got: %#v", expected, update) + for _, testCase := range testCases { + data, err := json.Marshal(testCase.manifests) + if err != nil { + t.Fatalf("%s: Some weird json problem: %v", testCase.desc, err) + } + fakeHandler := util.FakeHandler{ + StatusCode: 200, + ResponseBody: string(data), + } + testServer := httptest.NewServer(&fakeHandler) + ch := make(chan interface{}, 1) + c := SourceURL{testServer.URL, ch} + if err := c.extractFromURL(); err != nil { + t.Errorf("%s: Unexpected error: %v", testCase.desc, err) + } + update := (<-ch).(kubelet.PodUpdate) + if !reflect.DeepEqual(testCase.expected, update) { + t.Errorf("%s: Expected: %#v, Got: %#v", testCase.desc, testCase.expected, update) + } + for i := range update.Pods { + if errs := kubelet.ValidatePod(&update.Pods[i]); len(errs) != 0 { + t.Errorf("%s: Expected no validation errors on %#v, Got %#v", testCase.desc, update.Pods[i], errs) + } + } } }