diff --git a/pkg/api/helpers.go b/pkg/api/helpers.go index 75ceff3a7cf..5dff4d7c02b 100644 --- a/pkg/api/helpers.go +++ b/pkg/api/helpers.go @@ -18,7 +18,6 @@ package api import ( "reflect" - "strings" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource" "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" @@ -60,27 +59,4 @@ var Semantic = conversion.EqualitiesOrDie( } return a.Amount.Cmp(b.Amount) == 0 }, - pullPoliciesEqual, ) - -// TODO: Address these per #1502 - -func IsPullAlways(p PullPolicy) bool { - return pullPoliciesEqual(p, PullAlways) -} - -func IsPullNever(p PullPolicy) bool { - return pullPoliciesEqual(p, PullNever) -} - -func IsPullIfNotPresent(p PullPolicy) bool { - // Default to pull if not present - if len(p) == 0 { - return true - } - return pullPoliciesEqual(p, PullIfNotPresent) -} - -func pullPoliciesEqual(p1, p2 PullPolicy) bool { - return strings.ToLower(string(p1)) == strings.ToLower(string(p2)) -} diff --git a/pkg/api/helpers_test.go b/pkg/api/helpers_test.go index 5d15ced93c0..c8cde624c31 100644 --- a/pkg/api/helpers_test.go +++ b/pkg/api/helpers_test.go @@ -59,8 +59,6 @@ func TestSemantic(t *testing.T) { true, }, {resource.MustParse("2m"), resource.MustParse("1m"), false}, - {PullPolicy("NEVER"), PullPolicy("neveR"), true}, - {PullPolicy("NEVER"), PullPolicy("neveRi"), false}, } for index, item := range table { diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 48af28fda29..1ac27118f64 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -266,6 +266,29 @@ func validateLifecycle(lifecycle *api.Lifecycle) errs.ValidationErrorList { return allErrs } +// TODO(dchen1107): Move this along with other defaulting values +func validatePullPolicyWithDefault(ctr *api.Container) errs.ValidationErrorList { + allErrors := errs.ValidationErrorList{} + + // TODO(dchen1107): Move ParseImageName code to pkg/util + if len(ctr.ImagePullPolicy) == 0 { + parts := strings.Split(ctr.Image, ":") + // Check image tag + if parts[len(parts)-1] == "latest" { + ctr.ImagePullPolicy = api.PullAlways + } else { + ctr.ImagePullPolicy = api.PullIfNotPresent + } + } + if ctr.ImagePullPolicy != api.PullAlways && + ctr.ImagePullPolicy != api.PullIfNotPresent && + ctr.ImagePullPolicy != api.PullNever { + allErrors = append(allErrors, errs.NewFieldNotSupported("", ctr.ImagePullPolicy)) + } + + return allErrors +} + func validateContainers(containers []api.Container, volumes util.StringSet) errs.ValidationErrorList { allErrs := errs.ValidationErrorList{} @@ -294,6 +317,7 @@ func validateContainers(containers []api.Container, volumes util.StringSet) errs cErrs = append(cErrs, validatePorts(ctr.Ports).Prefix("ports")...) cErrs = append(cErrs, validateEnv(ctr.Env).Prefix("env")...) cErrs = append(cErrs, validateVolumeMounts(ctr.VolumeMounts, volumes).Prefix("volumeMounts")...) + cErrs = append(cErrs, validatePullPolicyWithDefault(ctr).Prefix("pullPolicy")...) allErrs = append(allErrs, cErrs.PrefixIndex(i)...) } // Check for colliding ports across all containers. diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 755eced505c..537a5e9aa41 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -220,6 +220,54 @@ func TestValidateVolumeMounts(t *testing.T) { } } +func TestValidatePullPolicy(t *testing.T) { + type T struct { + Container api.Container + ExpectedPolicy api.PullPolicy + } + testCases := map[string]T{ + "NotPresent1": { + api.Container{Name: "abc", Image: "image:latest", ImagePullPolicy: "PullIfNotPresent"}, + api.PullIfNotPresent, + }, + "NotPresent2": { + api.Container{Name: "abc1", Image: "image", ImagePullPolicy: "PullIfNotPresent"}, + api.PullIfNotPresent, + }, + "Always1": { + api.Container{Name: "123", Image: "image:latest", ImagePullPolicy: "PullAlways"}, + api.PullAlways, + }, + "Always2": { + api.Container{Name: "1234", Image: "image", ImagePullPolicy: "PullAlways"}, + api.PullAlways, + }, + "Never1": { + api.Container{Name: "abc-123", Image: "image:latest", ImagePullPolicy: "PullNever"}, + api.PullNever, + }, + "Never2": { + api.Container{Name: "abc-1234", Image: "image", ImagePullPolicy: "PullNever"}, + api.PullNever, + }, + "DefaultToNotPresent": {api.Container{Name: "notPresent", Image: "image"}, api.PullIfNotPresent}, + "DefaultToNotPresent2": {api.Container{Name: "notPresent1", Image: "image:sometag"}, api.PullIfNotPresent}, + "DefaultToAlways1": {api.Container{Name: "always", Image: "image:latest"}, api.PullAlways}, + "DefaultToAlways2": {api.Container{Name: "always", Image: "foo.bar.com:5000/my/image:latest"}, api.PullAlways}, + } + for k, v := range testCases { + ctr := &v.Container + errs := validatePullPolicyWithDefault(ctr) + if len(errs) != 0 { + t.Errorf("case[%s] expected success, got %#v", k, errs) + } + if ctr.ImagePullPolicy != v.ExpectedPolicy { + t.Errorf("case[%s] expected policy %v, got %v", k, v.ExpectedPolicy, ctr.ImagePullPolicy) + } + } + +} + func TestValidateContainers(t *testing.T) { volumes := util.StringSet{} capabilities.SetForTests(capabilities.Capabilities{ diff --git a/pkg/kubelet/config/http_test.go b/pkg/kubelet/config/http_test.go index f0e0f08b71f..ca69aab1608 100644 --- a/pkg/kubelet/config/http_test.go +++ b/pkg/kubelet/config/http_test.go @@ -156,7 +156,8 @@ func TestExtractFromHTTP(t *testing.T) { Containers: []api.Container{{ Name: "1", Image: "foo", - TerminationMessagePath: "/dev/termination-log"}}, + TerminationMessagePath: "/dev/termination-log", + ImagePullPolicy: "PullIfNotPresent"}}, }, }, api.BoundPod{ @@ -169,7 +170,8 @@ func TestExtractFromHTTP(t *testing.T) { Containers: []api.Container{{ Name: "1", Image: "foo", - TerminationMessagePath: "/dev/termination-log"}}, + TerminationMessagePath: "/dev/termination-log", + ImagePullPolicy: "PullIfNotPresent"}}, }, }), }, diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 69d76eff3d2..c2be5fe94b8 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -1095,9 +1095,8 @@ func (kl *Kubelet) syncPod(pod *api.BoundPod, dockerContainers dockertools.Docke if err != nil { glog.Errorf("Couldn't make a ref to pod %v, container %v: '%v'", pod.Name, container.Name, err) } - if !api.IsPullNever(container.ImagePullPolicy) { + if container.ImagePullPolicy != api.PullNever { present, err := kl.dockerPuller.IsImagePresent(container.Image) - latest := dockertools.RequireLatestImage(container.Image) if err != nil { if ref != nil { record.Eventf(ref, "failed", "Failed to inspect image %q", container.Image) @@ -1105,8 +1104,8 @@ func (kl *Kubelet) syncPod(pod *api.BoundPod, dockerContainers dockertools.Docke glog.Errorf("Failed to inspect image %q: %v; skipping pod %q container %q", container.Image, err, podFullName, container.Name) continue } - if api.IsPullAlways(container.ImagePullPolicy) || - (api.IsPullIfNotPresent(container.ImagePullPolicy) && (!present || latest)) { + if container.ImagePullPolicy == api.PullAlways || + (container.ImagePullPolicy == api.PullIfNotPresent && (!present)) { if err := kl.pullImage(container.Image, ref); err != nil { continue } diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 4739d6c09bb..6e4548cb873 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/pkg/kubelet/kubelet_test.go @@ -487,7 +487,7 @@ func TestSyncPodsCreatesNetAndContainerPullsImage(t *testing.T) { }, Spec: api.PodSpec{ Containers: []api.Container{ - {Name: "bar"}, + {Name: "bar", Image: "something", ImagePullPolicy: "PullIfNotPresent"}, }, }, }, @@ -502,7 +502,7 @@ func TestSyncPodsCreatesNetAndContainerPullsImage(t *testing.T) { fakeDocker.Lock() - if !reflect.DeepEqual(puller.ImagesPulled, []string{"custom_image_name", ""}) { + if !reflect.DeepEqual(puller.ImagesPulled, []string{"custom_image_name", "something"}) { t.Errorf("Unexpected pulled containers: %v", puller.ImagesPulled) } @@ -1924,7 +1924,7 @@ func TestSyncPodsWithPullPolicy(t *testing.T) { fakeDocker.Lock() - if !reflect.DeepEqual(puller.ImagesPulled, []string{"custom_image_name", "pull_always_image", "pull_if_not_present_image", "want:latest"}) { + if !reflect.DeepEqual(puller.ImagesPulled, []string{"custom_image_name", "pull_always_image", "pull_if_not_present_image"}) { t.Errorf("Unexpected pulled containers: %v", puller.ImagesPulled) }