From 14fcf68edc8669bc1e9313759942aa6ab85adf29 Mon Sep 17 00:00:00 2001
From: Dave Bailey <davebailey@google.com>
Date: Wed, 17 Sep 2014 17:43:44 +0000
Subject: [PATCH] Improve error detection for URL manifests, and skip parsing
 if content unchanged.

---
 pkg/kubelet/config/http.go      | 15 +++++++++++++++
 pkg/kubelet/config/http_test.go | 24 +++++++++++++++++++++---
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/pkg/kubelet/config/http.go b/pkg/kubelet/config/http.go
index be5e0f57c39..b091070e9b6 100644
--- a/pkg/kubelet/config/http.go
+++ b/pkg/kubelet/config/http.go
@@ -18,6 +18,7 @@ limitations under the License.
 package config
 
 import (
+	"bytes"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -34,12 +35,14 @@ import (
 type SourceURL struct {
 	url     string
 	updates chan<- interface{}
+	data    []byte
 }
 
 func NewSourceURL(url string, period time.Duration, updates chan<- interface{}) *SourceURL {
 	config := &SourceURL{
 		url:     url,
 		updates: updates,
+		data:    nil,
 	}
 	glog.Infof("Watching URL %s", url)
 	go util.Forever(config.run, period)
@@ -68,6 +71,11 @@ func (s *SourceURL) extractFromURL() error {
 	if len(data) == 0 {
 		return fmt.Errorf("zero-length data received from %v", s.url)
 	}
+	// Short circuit if the manifest has not changed since the last time it was read.
+	if bytes.Compare(data, s.data) == 0 {
+		return nil
+	}
+	s.data = data
 
 	// First try as if it's a single manifest
 	var manifest api.ContainerManifest
@@ -101,6 +109,13 @@ func (s *SourceURL) extractFromURL() error {
 		}
 	}
 	if multiErr == nil {
+		// A single manifest that did not pass semantic validation will yield an empty
+		// array of manifests (and no error) when unmarshaled as such.  In that case,
+		// if the single manifest at least had a Version, we return the single-manifest
+		// error (if any).
+		if len(manifests) == 0 && manifest.Version != "" {
+			return singleErr
+		}
 		pods := []kubelet.Pod{}
 		for i, manifest := range manifests {
 			pod := kubelet.Pod{Name: manifest.ID, Manifest: manifest}
diff --git a/pkg/kubelet/config/http_test.go b/pkg/kubelet/config/http_test.go
index 992f25b6dc1..ce4fb5a1bc7 100644
--- a/pkg/kubelet/config/http_test.go
+++ b/pkg/kubelet/config/http_test.go
@@ -40,7 +40,7 @@ func TestURLErrorNotExistNoUpdate(t *testing.T) {
 
 func TestExtractFromHttpBadness(t *testing.T) {
 	ch := make(chan interface{}, 1)
-	c := SourceURL{"http://localhost:49575/_not_found_", ch}
+	c := SourceURL{"http://localhost:49575/_not_found_", ch, nil}
 	if err := c.extractFromURL(); err == nil {
 		t.Errorf("Expected error")
 	}
@@ -75,6 +75,24 @@ func TestExtractInvalidManifest(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "Unspecified container name",
+			manifests: []api.ContainerManifest{
+				{
+					Version: "v1beta1",
+					Containers: []api.Container{{Name: ""}},
+				},
+			},
+		},
+		{
+			desc: "Invalid container name",
+			manifests: []api.ContainerManifest{
+				{
+					Version: "v1beta1",
+					Containers: []api.Container{{Name: "_INVALID_"}},
+				},
+			},
+		},
 	}
 	for _, testCase := range testCases {
 		data, err := json.Marshal(testCase.manifests)
@@ -87,7 +105,7 @@ func TestExtractInvalidManifest(t *testing.T) {
 		}
 		testServer := httptest.NewServer(&fakeHandler)
 		ch := make(chan interface{}, 1)
-		c := SourceURL{testServer.URL, ch}
+		c := SourceURL{testServer.URL, ch, nil}
 		if err := c.extractFromURL(); err == nil {
 			t.Errorf("%s: Expected error", testCase.desc)
 		}
@@ -146,7 +164,7 @@ func TestExtractFromHTTP(t *testing.T) {
 		}
 		testServer := httptest.NewServer(&fakeHandler)
 		ch := make(chan interface{}, 1)
-		c := SourceURL{testServer.URL, ch}
+		c := SourceURL{testServer.URL, ch, nil}
 		if err := c.extractFromURL(); err != nil {
 			t.Errorf("%s: Unexpected error: %v", testCase.desc, err)
 		}