Skip to content
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

Jump to bottom

Use unversioned list options in server #17369

Merged
merged 1 commit into from
Nov 24, 2015
Merged

Use unversioned list options in server #17369

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 54 additions & 0 deletions pkg/api/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,60 @@ func init() {
*out = (*in).String()
return nil
},
func(in *string, out *unversioned.LabelSelector, s conversion.Scope) error {
selector, err := labels.Parse(*in)
if err != nil {
return err
}
*out = unversioned.LabelSelector{selector}
return nil
},
func(in *string, out *unversioned.FieldSelector, s conversion.Scope) error {
selector, err := fields.ParseSelector(*in)
if err != nil {
return err
}
*out = unversioned.FieldSelector{selector}
return nil
},
func(in *[]string, out *unversioned.LabelSelector, s conversion.Scope) error {
selectorString := ""
if len(*in) > 0 {
selectorString = (*in)[0]
}
selector, err := labels.Parse(selectorString)
if err != nil {
return err
}
*out = unversioned.LabelSelector{selector}
return nil
},
func(in *[]string, out *unversioned.FieldSelector, s conversion.Scope) error {
selectorString := ""
if len(*in) > 0 {
selectorString = (*in)[0]
}
selector, err := fields.ParseSelector(selectorString)
if err != nil {
return err
}
*out = unversioned.FieldSelector{selector}
return nil
},
func(in *unversioned.LabelSelector, out *string, s conversion.Scope) error {
if in.Selector == nil {
return nil
}
*out = in.Selector.String()
return nil
},
func(in *unversioned.FieldSelector, out *string, s conversion.Scope) error {
if in.Selector == nil {
return nil
}
*out = in.Selector.String()
return nil
},
func(in *resource.Quantity, out *resource.Quantity, s conversion.Scope) error {
// Cannot deep copy these, because inf.Dec has unexported fields.
*out = *in.Copy()
Expand Down
5 changes: 3 additions & 2 deletions pkg/api/rest/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"net/url"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/runtime"
"k8s.io/kubernetes/pkg/watch"
)
Expand Down Expand Up @@ -60,7 +61,7 @@ type Lister interface {
// This object must be a pointer type for use with Codec.DecodeInto([]byte, runtime.Object)
NewList() runtime.Object
// List selects resources in the storage which match to the selector. 'options' can be nil.
List(ctx api.Context, options *api.ListOptions) (runtime.Object, error)
List(ctx api.Context, options *unversioned.ListOptions) (runtime.Object, error)
}

// Getter is an object that can retrieve a named RESTful resource.
Expand Down Expand Up @@ -181,7 +182,7 @@ type Watcher interface {
// are supported; an error should be returned if 'field' tries to select on a field that
// isn't supported. 'resourceVersion' allows for continuing/starting a watch at a
// particular version.
Watch(ctx api.Context, options *api.ListOptions) (watch.Interface, error)
Watch(ctx api.Context, options *unversioned.ListOptions) (watch.Interface, error)
}

// StandardStorage is an interface covering the common verbs. Provided for testing whether a
Expand Down
10 changes: 5 additions & 5 deletions pkg/api/rest/resttest/resttest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,7 +864,7 @@ func (t *Tester) testListMatchLabels(obj runtime.Object, assignFn AssignFunc) {
filtered := []runtime.Object{objs[1]}

selector := labels.SelectorFromSet(labels.Set(testLabels))
options := &api.ListOptions{LabelSelector: selector}
options := &unversioned.ListOptions{LabelSelector: unversioned.LabelSelector{selector}}
listObj, err := t.storage.(rest.Lister).List(ctx, options)
if err != nil {
t.Errorf("unexpected error: %v", err)
Expand Down Expand Up @@ -906,7 +906,7 @@ func (t *Tester) testWatchFields(obj runtime.Object, emitFn EmitFunc, fieldsPass

for _, field := range fieldsPass {
for _, action := range actions {
options := &api.ListOptions{FieldSelector: field.AsSelector(), ResourceVersion: "1"}
options := &unversioned.ListOptions{FieldSelector: unversioned.FieldSelector{field.AsSelector()}, ResourceVersion: "1"}
watcher, err := t.storage.(rest.Watcher).Watch(ctx, options)
if err != nil {
t.Errorf("unexpected error: %v, %v", err, action)
Expand All @@ -930,7 +930,7 @@ func (t *Tester) testWatchFields(obj runtime.Object, emitFn EmitFunc, fieldsPass

for _, field := range fieldsFail {
for _, action := range actions {
options := &api.ListOptions{FieldSelector: field.AsSelector(), ResourceVersion: "1"}
options := &unversioned.ListOptions{FieldSelector: unversioned.FieldSelector{field.AsSelector()}, ResourceVersion: "1"}
watcher, err := t.storage.(rest.Watcher).Watch(ctx, options)
if err != nil {
t.Errorf("unexpected error: %v", err)
Expand All @@ -955,7 +955,7 @@ func (t *Tester) testWatchLabels(obj runtime.Object, emitFn EmitFunc, labelsPass

for _, label := range labelsPass {
for _, action := range actions {
options := &api.ListOptions{LabelSelector: label.AsSelector(), ResourceVersion: "1"}
options := &unversioned.ListOptions{LabelSelector: unversioned.LabelSelector{label.AsSelector()}, ResourceVersion: "1"}
watcher, err := t.storage.(rest.Watcher).Watch(ctx, options)
if err != nil {
t.Errorf("unexpected error: %v", err)
Expand All @@ -978,7 +978,7 @@ func (t *Tester) testWatchLabels(obj runtime.Object, emitFn EmitFunc, labelsPass

for _, label := range labelsFail {
for _, action := range actions {
options := &api.ListOptions{LabelSelector: label.AsSelector(), ResourceVersion: "1"}
options := &unversioned.ListOptions{LabelSelector: unversioned.LabelSelector{label.AsSelector()}, ResourceVersion: "1"}
watcher, err := t.storage.(rest.Watcher).Watch(ctx, options)
if err != nil {
t.Errorf("unexpected error: %v", err)
Expand Down
7 changes: 7 additions & 0 deletions pkg/api/testing/fuzzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,13 @@ func FuzzerFor(t *testing.T, version string, src rand.Source) *fuzz.Fuzzer {
j.LabelSelector, _ = labels.Parse("a=b")
j.FieldSelector, _ = fields.ParseSelector("a=b")
},
func(j *unversioned.ListOptions, c fuzz.Continue) {
// TODO: add some parsing
label, _ := labels.Parse("a=b")
j.LabelSelector = unversioned.LabelSelector{label}
field, _ := fields.ParseSelector("a=b")
j.FieldSelector = unversioned.FieldSelector{field}
},
func(s *api.PodSpec, c fuzz.Continue) {
c.FuzzNoCustom(s)
// has a default value
Expand Down
59 changes: 16 additions & 43 deletions pkg/apiserver/apiserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,50 +112,23 @@ func newMapper() *meta.DefaultRESTMapper {
}

func addGrouplessTypes() {
type ListOptions struct {
runtime.Object
unversioned.TypeMeta `json:",inline"`
LabelSelector string `json:"labelSelector,omitempty"`
FieldSelector string `json:"fieldSelector,omitempty"`
Watch bool `json:"watch,omitempty"`
ResourceVersion string `json:"resourceVersion,omitempty"`
TimeoutSeconds *int64 `json:"timeoutSeconds,omitempty"`
}
api.Scheme.AddKnownTypes(
grouplessGroupVersion.String(), &apiservertesting.Simple{}, &apiservertesting.SimpleList{}, &unversioned.Status{},
&ListOptions{}, &api.DeleteOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
&unversioned.ListOptions{}, &api.DeleteOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
api.Scheme.AddKnownTypes(grouplessGroupVersion.String(), &api.Pod{})
}

func addTestTypes() {
type ListOptions struct {
runtime.Object
unversioned.TypeMeta `json:",inline"`
LabelSelector string `json:"labels,omitempty"`
FieldSelector string `json:"fields,omitempty"`
Watch bool `json:"watch,omitempty"`
ResourceVersion string `json:"resourceVersion,omitempty"`
TimeoutSeconds *int64 `json:"timeoutSeconds,omitempty"`
}
api.Scheme.AddKnownTypes(
testGroupVersion.String(), &apiservertesting.Simple{}, &apiservertesting.SimpleList{}, &unversioned.Status{},
&ListOptions{}, &api.DeleteOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
&unversioned.ListOptions{}, &api.DeleteOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
api.Scheme.AddKnownTypes(testGroupVersion.String(), &api.Pod{})
}

func addNewTestTypes() {
type ListOptions struct {
runtime.Object
unversioned.TypeMeta `json:",inline"`
LabelSelector string `json:"labelSelector,omitempty"`
FieldSelector string `json:"fieldSelector,omitempty"`
Watch bool `json:"watch,omitempty"`
ResourceVersion string `json:"resourceVersion,omitempty"`
TimeoutSeconds *int64 `json:"timeoutSeconds,omitempty"`
}
api.Scheme.AddKnownTypes(
newGroupVersion.String(), &apiservertesting.Simple{}, &apiservertesting.SimpleList{}, &unversioned.Status{},
&ListOptions{}, &api.DeleteOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
&unversioned.ListOptions{}, &api.DeleteOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
}

func init() {
Expand All @@ -165,7 +138,7 @@ func init() {
// "internal" version
api.Scheme.AddKnownTypes(
"", &apiservertesting.Simple{}, &apiservertesting.SimpleList{}, &unversioned.Status{},
&api.ListOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
&unversioned.ListOptions{}, &apiservertesting.SimpleGetOptions{}, &apiservertesting.SimpleRoot{})
addGrouplessTypes()
addTestTypes()
addNewTestTypes()
Expand Down Expand Up @@ -363,18 +336,18 @@ type SimpleRESTStorage struct {
injectedFunction func(obj runtime.Object) (returnObj runtime.Object, err error)
}

func (storage *SimpleRESTStorage) List(ctx api.Context, options *api.ListOptions) (runtime.Object, error) {
func (storage *SimpleRESTStorage) List(ctx api.Context, options *unversioned.ListOptions) (runtime.Object, error) {
storage.checkContext(ctx)
result := &apiservertesting.SimpleList{
Items: storage.list,
}
storage.requestedLabelSelector = labels.Everything()
if options != nil && options.LabelSelector != nil {
storage.requestedLabelSelector = options.LabelSelector
if options != nil && options.LabelSelector.Selector != nil {
storage.requestedLabelSelector = options.LabelSelector.Selector
}
storage.requestedFieldSelector = fields.Everything()
if options != nil && options.FieldSelector != nil {
storage.requestedFieldSelector = options.FieldSelector
if options != nil && options.FieldSelector.Selector != nil {
storage.requestedFieldSelector = options.FieldSelector.Selector
}
return result, storage.errors["list"]
}
Expand Down Expand Up @@ -472,15 +445,15 @@ func (storage *SimpleRESTStorage) Update(ctx api.Context, obj runtime.Object) (r
}

// Implement ResourceWatcher.
func (storage *SimpleRESTStorage) Watch(ctx api.Context, options *api.ListOptions) (watch.Interface, error) {
func (storage *SimpleRESTStorage) Watch(ctx api.Context, options *unversioned.ListOptions) (watch.Interface, error) {
storage.checkContext(ctx)
storage.requestedLabelSelector = labels.Everything()
if options != nil && options.LabelSelector != nil {
storage.requestedLabelSelector = options.LabelSelector
if options != nil && options.LabelSelector.Selector != nil {
storage.requestedLabelSelector = options.LabelSelector.Selector
}
storage.requestedFieldSelector = fields.Everything()
if options != nil && options.FieldSelector != nil {
storage.requestedFieldSelector = options.FieldSelector
if options != nil && options.FieldSelector.Selector != nil {
storage.requestedFieldSelector = options.FieldSelector.Selector
}
storage.requestedResourceVersion = ""
if options != nil {
Expand Down Expand Up @@ -931,7 +904,7 @@ func TestList(t *testing.T) {
legacy: true,
},
{
url: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/simple?namespace=other&labels=a%3Db&fields=c%3Dd",
url: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/simple?namespace=other&labelSelector=a%3Db&fieldSelector=c%3Dd",
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm not familiar with the background of this PR. Is labels=a%3Db&fields=c%3Dd a query sent by old clients? If so, we probably should keep the original line to test the backward compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

No - it's not previous format - it's only the format that is introduced in the test - see ListOptions struct that I'm removing from addTestTypes(). In all production code we have labelSelector & fieldSelector.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks.

namespace: "",
selfLink: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/simple",
legacy: true,
Expand All @@ -952,7 +925,7 @@ func TestList(t *testing.T) {
legacy: true,
},
{
url: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/other/simple?labels=a%3Db&fields=c%3Dd",
url: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/other/simple?labelSelector=a%3Db&fieldSelector=c%3Dd",
namespace: "other",
selfLink: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/other/simple",
legacy: true,
Expand Down
37 changes: 15 additions & 22 deletions pkg/apiserver/resthandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,31 +240,24 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch
ctx := scope.ContextFunc(req)
ctx = api.WithNamespace(ctx, namespace)

versioned, err := scope.Creater.New(scope.ServerAPIVersion, "ListOptions")
if err != nil {
errorJSON(err, scope.Codec, w)
return
}
if err := scope.Codec.DecodeParametersInto(req.Request.URL.Query(), versioned); err != nil {
errorJSON(err, scope.Codec, w)
return
}
opts := api.ListOptions{}
if err := scope.Convertor.Convert(versioned, &opts); err != nil {
opts := unversioned.ListOptions{}
if err := scope.Codec.DecodeParametersInto(req.Request.URL.Query(), &opts); err != nil {
errorJSON(err, scope.Codec, w)
return
}

// transform fields
// TODO: Should this be done as part of convertion?
fn := func(label, value string) (newLabel, newValue string, err error) {
return scope.Convertor.ConvertFieldLabel(scope.APIVersion, scope.Kind, label, value)
}
if opts.FieldSelector, err = opts.FieldSelector.Transform(fn); err != nil {
// TODO: allow bad request to set field causes based on query parameters
err = errors.NewBadRequest(err.Error())
errorJSON(err, scope.Codec, w)
return
// TODO: DecodeParametersInto should do this.
if opts.FieldSelector.Selector != nil {
fn := func(label, value string) (newLabel, newValue string, err error) {
return scope.Convertor.ConvertFieldLabel(scope.APIVersion, scope.Kind, label, value)
}
if opts.FieldSelector.Selector, err = opts.FieldSelector.Selector.Transform(fn); err != nil {
// TODO: allow bad request to set field causes based on query parameters
err = errors.NewBadRequest(err.Error())
errorJSON(err, scope.Codec, w)
return
}
}

if hasName {
Expand All @@ -273,7 +266,7 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch
// a request for a single object and optimize the
// storage query accordingly.
nameSelector := fields.OneTermEqualSelector("metadata.name", name)
if opts.FieldSelector != nil && !opts.FieldSelector.Empty() {
if opts.FieldSelector.Selector != nil && !opts.FieldSelector.Selector.Empty() {
// It doesn't make sense to ask for both a name
// and a field selector, since just the name is
// sufficient to narrow down the request to a
Expand All @@ -285,7 +278,7 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch
)
return
}
opts.FieldSelector = nameSelector
opts.FieldSelector.Selector = nameSelector
}

if (opts.Watch || forceWatch) && rw != nil {
Expand Down
8 changes: 4 additions & 4 deletions pkg/apiserver/watch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,14 @@ func TestWatchParamParsing(t *testing.T) {
namespace: api.NamespaceAll,
}, {
path: rootPath,
rawQuery: "resourceVersion=314159&fields=Host%3D&labels=name%3Dfoo",
rawQuery: "resourceVersion=314159&fieldSelector=Host%3D&labelSelector=name%3Dfoo",
resourceVersion: "314159",
labelSelector: "name=foo",
fieldSelector: "Host=",
namespace: api.NamespaceAll,
}, {
path: rootPath,
rawQuery: "fields=id%3dfoo&resourceVersion=1492",
rawQuery: "fieldSelector=id%3dfoo&resourceVersion=1492",
resourceVersion: "1492",
labelSelector: "",
fieldSelector: "id=foo",
Expand All @@ -227,14 +227,14 @@ func TestWatchParamParsing(t *testing.T) {
namespace: "other",
}, {
path: namespacedPath,
rawQuery: "resourceVersion=314159&fields=Host%3D&labels=name%3Dfoo",
rawQuery: "resourceVersion=314159&fieldSelector=Host%3D&labelSelector=name%3Dfoo",
resourceVersion: "314159",
labelSelector: "name=foo",
fieldSelector: "Host=",
namespace: "other",
}, {
path: namespacedPath,
rawQuery: "fields=id%3dfoo&resourceVersion=1492",
rawQuery: "fieldSelector=id%3dfoo&resourceVersion=1492",
resourceVersion: "1492",
labelSelector: "",
fieldSelector: "id=foo",
Expand Down
3 changes: 2 additions & 1 deletion pkg/registry/componentstatus/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"net/http"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/apiserver"
"k8s.io/kubernetes/pkg/probe"
"k8s.io/kubernetes/pkg/runtime"
Expand Down Expand Up @@ -49,7 +50,7 @@ func (rs *REST) NewList() runtime.Object {

// Returns the list of component status. Note that the label and field are both ignored.
// Note that this call doesn't support labels or selectors.
func (rs *REST) List(ctx api.Context, options *api.ListOptions) (runtime.Object, error) {
func (rs *REST) List(ctx api.Context, options *unversioned.ListOptions) (runtime.Object, error) {
servers := rs.GetServersToValidate()

// TODO: This should be parallelized.
Expand Down