Forbid creating VPA objects without targetRef #1785
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign bskiba@ |
| @@ -178,16 +178,22 @@ func validateVPA(vpa *vpa_types.VerticalPodAutoscaler) error { | |||
| } | |||
| } | |||
|
|
|||
| if isCreate { | |||
There was a problem hiding this comment.
You can fold the two ifs into one.
There was a problem hiding this comment.
Sorry, I started with a more complicated code and haven't cleaned this. Fixed.
| @@ -147,7 +147,7 @@ var ( | |||
| } | |||
| ) | |||
|
|
|||
| func validateVPA(vpa *vpa_types.VerticalPodAutoscaler) error { | |||
| func validateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error { | |||
There was a problem hiding this comment.
nit: I am inclined to pass ar.Request.Operation instead of boolean but you can ignore my inclination :)
There was a problem hiding this comment.
Since you don't mind I'll choose "ignore" this time.
|
Nits only |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| @@ -178,16 +178,22 @@ func validateVPA(vpa *vpa_types.VerticalPodAutoscaler) error { | |||
| } | |||
| } | |||
|
|
|||
| if isCreate { | |||
| if vpa.Spec.TargetRef == nil { | |||
| return fmt.Errorf("TargetRef is required") | |||
There was a problem hiding this comment.
Actually one more thing, can you make this more descriptive? Sth like, you are trying to use an unsupported version of the api, please migrate to v1beta2 and use targetRef to specify pods to autoscale
There was a problem hiding this comment.
Reworded.
Since we're not sure of the API version used here, it makes the message a bit long but maybe it's for the best.
kgolab
force-pushed
the
kg-vpa-invalidate-v1beta1
branch
from
ac8288e to
6ec53d4
Compare
k8s-ci-robot
removed
the
lgtm
|
@bskiba , please take another look. |
|
Thanks! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bskiba The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Follow-up to #1783 .
It's a bit of a poor man's solution since we don't have the version nor v1beta1 fields at hand so I've used (lack of) TargetRef for validation.
Moreover, the validation can be by-passed by updating the object but I guess at that point the user has been already warned.
An attempt to create v1beta1 object (or simply v1beta2 w/o TargetRef) ends up with: