Add test to guard unset fields in deployer pod #17471
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,22 +2,29 @@ package deployment | |
|
|
||
| import ( | ||
| "fmt" | ||
| "math/rand" | ||
| "reflect" | ||
| "sort" | ||
| "testing" | ||
|
|
||
| fuzz "github.com/google/gofuzz" | ||
|
|
||
| "k8s.io/api/core/v1" | ||
| kapiv1 "k8s.io/api/core/v1" | ||
| kerrors "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/api/resource" | ||
| "k8s.io/apimachinery/pkg/api/testing/fuzzer" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/util/diff" | ||
| kinformers "k8s.io/client-go/informers" | ||
| kclientset "k8s.io/client-go/kubernetes" | ||
| "k8s.io/client-go/kubernetes/fake" | ||
| clientgotesting "k8s.io/client-go/testing" | ||
| "k8s.io/client-go/tools/cache" | ||
| kapi "k8s.io/kubernetes/pkg/api" | ||
| kapihelper "k8s.io/kubernetes/pkg/api/helper" | ||
| kapitesting "k8s.io/kubernetes/pkg/api/testing" | ||
|
|
||
| deployapi "github.com/openshift/origin/pkg/apps/apis/apps" | ||
| _ "github.com/openshift/origin/pkg/apps/apis/apps/install" | ||
|
|
@@ -1075,3 +1082,76 @@ func TestDeployerCustomLabelsAndAnnotations(t *testing.T) { | |
| expectMapContains(t, podTemplate.Annotations, test.annotations, "annotations") | ||
| } | ||
| } | ||
|
|
||
| func TestMakeDeployerPod(t *testing.T) { | ||
| client := &fake.Clientset{} | ||
| controller := okDeploymentController(client, nil, nil, true, v1.PodUnknown) | ||
| config := deploytest.OkDeploymentConfig(1) | ||
| deployment, _ := deployutil.MakeDeploymentV1(config, codec) | ||
| container := controller.makeDeployerContainer(&config.Spec.Strategy) | ||
| container.Resources = deployutil.CopyApiResourcesToV1Resources(&config.Spec.Strategy.Resources) | ||
| defaultGracePeriod := int64(10) | ||
| maxDeploymentDurationSeconds := deployapi.MaxDeploymentDurationSeconds | ||
|
|
||
| for i := 1; i <= 25; i++ { | ||
| seed := rand.Int63() | ||
| f := fuzzer.FuzzerFor(kapitesting.FuzzerFuncs, rand.NewSource(seed), kapi.Codecs) | ||
| f.Funcs( | ||
| func(p *kapiv1.PodTemplateSpec, c fuzz.Continue) { | ||
| c.FuzzNoCustom(p) | ||
| p.Spec.InitContainers = nil | ||
|
|
||
| // These are specific for deployer pod container: | ||
| p.Spec.Containers = []kapiv1.Container{*container} | ||
| p.Spec.Containers[0].Name = "deployment" | ||
| p.Spec.Containers[0].Command = container.Command | ||
| p.Spec.Containers[0].Args = container.Args | ||
| p.Spec.Containers[0].Image = container.Image | ||
| p.Spec.Containers[0].Env = append(p.Spec.Containers[0].Env, kapiv1.EnvVar{Name: "OPENSHIFT_DEPLOYMENT_NAME", Value: "config-1"}) | ||
| p.Spec.Containers[0].Env = append(p.Spec.Containers[0].Env, kapiv1.EnvVar{Name: "OPENSHIFT_DEPLOYMENT_NAMESPACE", Value: "default"}) | ||
| p.Spec.Containers[0].Resources = container.Resources | ||
| p.Spec.Containers[0].ImagePullPolicy = kapiv1.PullIfNotPresent | ||
|
|
||
| // These are hardcoded for deployer pod spec | ||
| p.Spec.RestartPolicy = kapiv1.RestartPolicyNever | ||
| p.Spec.TerminationGracePeriodSeconds = &defaultGracePeriod | ||
| p.Spec.ActiveDeadlineSeconds = &maxDeploymentDurationSeconds | ||
| p.Spec.ServiceAccountName = "sa:test" | ||
|
|
||
| // FIXME: These are weird or missing. If you get an error below, consider | ||
| // adding this field into deployer controller or to this list: | ||
| p.Spec.DeprecatedServiceAccount = "" | ||
| p.Spec.AutomountServiceAccountToken = nil | ||
| p.Spec.Tolerations = nil | ||
| p.Spec.Volumes = nil | ||
| p.Spec.NodeName = "" | ||
| p.Spec.HostNetwork = false | ||
| p.Spec.HostPID = false | ||
| p.Spec.HostIPC = false | ||
| p.Spec.Hostname = "" | ||
| p.Spec.Subdomain = "" | ||
| p.Spec.Affinity = nil | ||
|
There was a problem hiding this comment. Probably need to be copied as well? |
||
| p.Spec.SchedulerName = "" | ||
| p.Spec.HostAliases = nil | ||
| p.Spec.Priority = nil | ||
| p.Spec.PriorityClassName = "" | ||
|
There was a problem hiding this comment. @smarterclayton reminds me that we should maybe populate this to SYSTEM for deployer pods? |
||
| p.Spec.SecurityContext = nil | ||
|
There was a problem hiding this comment. @php-coder @pweil- is this something we want to copy into deployer pod? There was a problem hiding this comment. user shouldn't be able to set this on deployer pods I think; we control the image hence this as well |
||
| }, | ||
| ) | ||
| inputPodTemplate := &kapiv1.PodTemplateSpec{} | ||
| f.Fuzz(&inputPodTemplate) | ||
| deployment.Spec.Template = inputPodTemplate | ||
|
|
||
| outputPodTemplate, err := controller.makeDeployerPod(deployment) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(inputPodTemplate.Spec, outputPodTemplate.Spec) { | ||
| t.Fatalf("Deployer pod is missing fields:\n%s\n\n%s", | ||
| diff.ObjectReflectDiff(inputPodTemplate.Spec, outputPodTemplate.Spec), | ||
| diff.ObjectDiff(inputPodTemplate.Spec, outputPodTemplate.Spec), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kargakis @tnozicka @smarterclayton we probably need to decide which fields we want to copy from the deployment config template to deployer pod (some of these are not copied for reason (like Volume...), some of them we should copy? (like NodeName?)..))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of these should be copied except for volumes only in case of customStrategy otherwise that would be a security hole
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like it should be the cluster admin who gets to choose deployers scheduling, not the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running pods on the users behalf is always fraught - we have to be careful what we allow. We have gradually given users more control over deployer pods including location of scheduling, but our current API won’t scale by one off adding fields.
Toleration improve scheduling (give the pod more options) in most cases, so it’s probably correct to copy them. However, we might have to filter some of the conditions to be safe. For now, I’m a bit concerned about adding them without some design time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton i'm fine keeping this test as it is (it will alert us if new fields are added to PodSpec) with a follow up issue where we can discuss what fields we want to copy.