Skip to content

Add validation to SDN objects with invalid name funcs #13124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 1, 2017
Merged

Add validation to SDN objects with invalid name funcs #13124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions api/protobuf-spec/github_com_openshift_origin_pkg_sdn_api_v1.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion api/swagger-spec/oapi-v1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26657,7 +26657,7 @@
},
"host": {
"type": "string",
"description": "Host is the name of the node. (This is redundant with the object's name, and this field is not actually used any more.)"
"description": "Host is the name of the node. (This is the same as the object's name, but both fields must be set.)"
},
"hostIP": {
"type": "string",
Expand Down
2 changes: 1 addition & 1 deletion api/swagger-spec/openshift-openapi-spec.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50864,7 +50864,7 @@
"type": "string"
},
"host": {
"description": "Host is the name of the node. (This is redundant with the object's name, and this field is not actually used any more.)",
"description": "Host is the name of the node. (This is the same as the object's name, but both fields must be set.)",
"type": "string"
},
"hostIP": {
Expand Down
2 changes: 1 addition & 1 deletion pkg/openapi/zz_generated.openapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13228,7 +13228,7 @@ var OpenAPIDefinitions *common.OpenAPIDefinitions = &common.OpenAPIDefinitions{
},
"host": {
SchemaProps: spec.SchemaProps{
Description: "Host is the name of the node. (This is redundant with the object's name, and this field is not actually used any more.)",
Description: "Host is the name of the node. (This is the same as the object's name, but both fields must be set.)",
Type: []string{"string"},
Format: "",
},
Expand Down
3 changes: 1 addition & 2 deletions pkg/sdn/api/v1/generated.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/sdn/api/v1/swagger_doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (EgressNetworkPolicySpec) SwaggerDoc() map[string]string {
var map_HostSubnet = map[string]string{
"": "HostSubnet describes the container subnet network on a node. The HostSubnet object must have the same name as the Node object it corresponds to.",
"metadata": "Standard object's metadata.",
"host": "Host is the name of the node. (This is redundant with the object's name, and this field is not actually used any more.)",
"host": "Host is the name of the node. (This is the same as the object's name, but both fields must be set.)",
"hostIP": "HostIP is the IP address to be used as a VTEP by other nodes in the overlay network",
"subnet": "Subnet is the CIDR range of the overlay network assigned to the node for its pods",
}
Expand Down
3 changes: 1 addition & 2 deletions pkg/sdn/api/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@ type HostSubnet struct {
// Standard object's metadata.
kapi.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

// Host is the name of the node. (This is redundant with the object's name, and this
// field is not actually used any more.)
// Host is the name of the node. (This is the same as the object's name, but both fields must be set.)
Host string `json:"host" protobuf:"bytes,2,opt,name=host"`
// HostIP is the IP address to be used as a VTEP by other nodes in the overlay network
HostIP string `json:"hostIP" protobuf:"bytes,3,opt,name=hostIP"`
Expand Down
11 changes: 10 additions & 1 deletion pkg/sdn/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package validation

import (
"fmt"
"net"

"k8s.io/kubernetes/pkg/api/validation"
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we default Host to Name, if it's empty?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would simplify the redundancy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

allErrs = append(allErrs, field.Invalid(field.NewPath("host"), hs.Host, fmt.Sprintf("must be the same as metadata.name: %q", hs.Name)))
}

if hs.Subnet == "" {
// check if annotation exists, then let the Subnet field be empty
if _, ok := hs.Annotations[sdnapi.AssignHostSubnetAnnotation]; !ok {
Expand Down Expand Up @@ -117,8 +122,12 @@ func ValidateHostSubnetUpdate(obj *sdnapi.HostSubnet, old *sdnapi.HostSubnet) fi
func ValidateNetNamespace(netnamespace *sdnapi.NetNamespace) field.ErrorList {
allErrs := validation.ValidateObjectMeta(&netnamespace.ObjectMeta, false, path.ValidatePathSegmentName, field.NewPath("metadata"))

if netnamespace.NetName != netnamespace.Name {
allErrs = append(allErrs, field.Invalid(field.NewPath("netname"), netnamespace.NetName, fmt.Sprintf("must be the same as metadata.name: %q", netnamespace.Name)))
}

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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we're not consistent with that. But most the places I've checked use uppercase ID, so I'd suggest leaving as was.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to match the struct tags.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't notice, sorry for the noise in that case.

}
return allErrs
}
Expand Down
23 changes: 4 additions & 19 deletions test/integration/etcd_storage_path_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"time"

kapi "k8s.io/kubernetes/pkg/api"
kubeerr "k8s.io/kubernetes/pkg/api/errors"
"k8s.io/kubernetes/pkg/api/meta"
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/client/restclient"
Expand Down Expand Up @@ -154,12 +153,12 @@ var etcdStorageData = map[unversioned.GroupVersionResource]struct {
// --

// github.com/openshift/origin/pkg/sdn/api/v1
gvr("", "v1", "netnamespaces"): { // This will fail to delete because meta.name != NetName but it is keyed off NetName
stub: `{"metadata": {"name": "nn1"}, "netid": 100, "netname": "networkname"}`,
gvr("", "v1", "netnamespaces"): {
stub: `{"metadata": {"name": "networkname"}, "netid": 100, "netname": "networkname"}`,
expectedEtcdPath: "openshift.io/registry/sdnnetnamespaces/networkname",
},
gvr("", "v1", "hostsubnets"): { // This will fail to delete because meta.name != Host but it is keyed off Host
stub: `{"host": "hostname", "hostIP": "192.168.1.1", "metadata": {"name": "hs1"}, "subnet": "192.168.1.1/24"}`,
gvr("", "v1", "hostsubnets"): {
stub: `{"host": "hostname", "hostIP": "192.168.1.1", "metadata": {"name": "hostname"}, "subnet": "192.168.1.1/24"}`,
expectedEtcdPath: "openshift.io/registry/sdnsubnets/hostname",
},
gvr("", "v1", "clusternetworks"): {
Expand Down Expand Up @@ -836,12 +835,7 @@ func (c *allClient) cleanup(all *[]cleanupData) error {
mapping := (*all)[i].mapping

if err := c.destroy(obj, mapping); err != nil {
if kubeerr.IsNotFound(err) && isInInvalidNameWhiteList(mapping) {
continue
}
return err
} else if err == nil && isInInvalidNameWhiteList(mapping) {
return fmt.Errorf("Object %#v with mapping %#v should fail to delete if it is in the invalid name whitelist", obj, mapping)
}
}
return nil
Expand Down Expand Up @@ -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 {
switch mapping.GroupVersionKind.GroupVersion().WithResource(mapping.Resource) {
case gvr("", "v1", "netnamespaces"), gvr("", "v1", "hostsubnets"): // TODO figure out how to not whitelist these
return true
}
return false
}

func getFromEtcd(keys etcd.KeysAPI, path string) (*metaObject, error) {
response, err := keys.Get(context.Background(), path, nil)
if err != nil {
Expand Down