Add test to guard unset fields in deployer pod #17471
Conversation
openshift-ci-robot
added
the
size/M
openshift-merge-robot
added
the
approved
| 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: |
There was a problem hiding this comment.
@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.
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.
feels like it should be the cluster admin who gets to choose deployers scheduling, not the user
There was a problem hiding this comment.
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.
@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.
mfojtik
force-pushed
the
add-deployer-test
branch
2 times, most recently
from
eb61653 to
961c7e8
Compare
| p.Spec.HostAliases = nil | ||
| p.Spec.Priority = nil | ||
| p.Spec.PriorityClassName = "" | ||
| 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
| 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.DeprecatedServiceAccount = "" | ||
| p.Spec.AutomountServiceAccountToken = nil | ||
| p.Spec.Volumes = nil | ||
| p.Spec.NodeName = "" |
There was a problem hiding this comment.
Probably need to be copied if user want to run the deployment on isolated node?
| 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?
mfojtik
force-pushed
the
add-deployer-test
branch
from
961c7e8 to
9084edf
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mfojtik, tnozicka The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Automatic merge from submit-queue (batch tested with PRs 17299, 17482, 17471). |
|
/cherrypick release-3.7 |
|
@Kargakis: #17471 failed to apply on top of branch "release-3.7": exit status 128 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This should protect is from "missing fields" in deployer pods for those fields we don't copy.