Add validation to SDN objects with invalid name funcs #13124
Conversation
Signed-off-by: Monis Khan <[email protected]>
|
[test] |
|
Evaluated for origin test up to 1d263a2 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/587/) (Base Commit: db96925) |
| if err := sdnapi.ValidVNID(netnamespace.NetID); err != nil { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("netID"), netnamespace.NetID, err.Error())) | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("netid"), netnamespace.NetID, err.Error())) |
There was a problem hiding this comment.
Unfortunately we're not consistent with that. But most the places I've checked use uppercase ID, so I'd suggest leaving as was.
There was a problem hiding this comment.
I changed it to match the struct tags.
There was a problem hiding this comment.
Didn't notice, sorry for the noise in that case.
| @@ -85,6 +86,10 @@ func ValidateClusterNetworkUpdate(obj *sdnapi.ClusterNetwork, old *sdnapi.Cluste | |||
| func ValidateHostSubnet(hs *sdnapi.HostSubnet) field.ErrorList { | |||
| allErrs := validation.ValidateObjectMeta(&hs.ObjectMeta, false, path.ValidatePathSegmentName, field.NewPath("metadata")) | |||
|
|
|||
| if hs.Host != hs.Name { | |||
There was a problem hiding this comment.
Can we default Host to Name, if it's empty?
There was a problem hiding this comment.
This would simplify the redundancy.
There was a problem hiding this comment.
I do not have a strong opinion on if we should or should not default this value. All existing code should be setting both of them to the same thing, so I am leaning towards just leaving this as-is.
| @@ -939,15 +933,6 @@ func createSerializers(config restclient.ContentConfig) (*restclient.Serializers | |||
| return s, nil | |||
| } | |||
|
|
|||
| // do NOT add anything to this - doing so means you wrote something that is broken | |||
| func isInInvalidNameWhiteList(mapping *meta.RESTMapping) bool { | |||
There was a problem hiding this comment.
nice, with this I guess I can get rid of the api group error here as well ;-)
|
LGTM [merge] Do we need this for 1.5? |
|
Evaluated for origin merge up to 1d263a2 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/658/) (Base Commit: 517824a) (Image: devenv-rhel7_6004) |
|
@mfojtik yeah we definitely need this for 1.5 |
Fixes #12697 with openshift/openshift-ansible#3387 as a prerequisite.
Supersedes #12959
I left the two invalid
ObjectNameFuncs alone as this prevents them from being a problem (and @soltysh has already removed them in #12541).cc @openshift/api-review @mfojtik @sdodson @openshift/networking
Signed-off-by: Monis Khan [email protected]