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

Ignore error when discovering server supported groupVersions and resources #16082

Merged
merged 1 commit into from
Oct 30, 2015
Merged

Ignore error when discovering server supported groupVersions and resources #16082

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
70 changes: 70 additions & 0 deletions pkg/client/unversioned/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,76 @@ func TestGetServerVersion(t *testing.T) {
}
}

func TestGetServerGroupsWithV1Server(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
var obj interface{}
switch req.URL.Path {
case "/api":
obj = &unversioned.APIVersions{
Versions: []string{
"v1",
},
}
default:
w.WriteHeader(http.StatusNotFound)
return
}
output, err := json.Marshal(obj)
if err != nil {
t.Errorf("unexpected encoding error: %v", err)
return
}
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
w.Write(output)
}))
client := NewOrDie(&Config{Host: server.URL})
// ServerGroups should not return an error even if server returns error at /api and /apis
apiGroupList, err := client.Discovery().ServerGroups()
if err != nil {
t.Errorf("unexpected error: %v", err)
}
groupVersions := ExtractGroupVersions(apiGroupList)
if !reflect.DeepEqual(groupVersions, []string{"v1"}) {
t.Errorf("expected: %q, got: %q", []string{"v1"}, groupVersions)
}
}

func TestGetServerResourcesWithV1Server(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
var obj interface{}
switch req.URL.Path {
case "/api":
obj = &unversioned.APIVersions{
Versions: []string{
"v1",
},
}
default:
w.WriteHeader(http.StatusNotFound)
return
}
output, err := json.Marshal(obj)
if err != nil {
t.Errorf("unexpected encoding error: %v", err)
return
}
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
w.Write(output)
}))
client := NewOrDie(&Config{Host: server.URL})
// ServerResources should not return an error even if server returns error at /api/v1.
resourceMap, err := client.Discovery().ServerResources()
if err != nil {
t.Errorf("unexpected error: %v", err)
}
if _, found := resourceMap["v1"]; !found {
t.Errorf("missing v1 in resource map")
}

}

func TestGetServerResources(t *testing.T) {
stable := unversioned.APIResourceList{
GroupVersion: "v1",
Expand Down
75 changes: 29 additions & 46 deletions pkg/client/unversioned/discovery_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@ limitations under the License.
package unversioned

import (
"encoding/json"
"fmt"
"net/http"
"net/url"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/errors"
"k8s.io/kubernetes/pkg/api/unversioned"
)

Expand Down Expand Up @@ -50,8 +49,7 @@ type ServerResourcesInterface interface {
// DiscoveryClient implements the functions that dicovery server-supported API groups,
// versions and resources.
type DiscoveryClient struct {
httpClient HTTPClient
baseURL url.URL
*RESTClient
}

// Convert unversioned.APIVersions to unversioned.APIGroup. APIVersions is used by legacy v1, so
Expand All @@ -71,42 +69,29 @@ func apiVersionsToAPIGroup(apiVersions *unversioned.APIVersions) (apiGroup unver
return
}

func (d *DiscoveryClient) get(url string) (resp *http.Response, err error) {
req, err := http.NewRequest("GET", url, nil)
if err != nil {
return nil, err
}
return d.httpClient.Do(req)
}

// ServerGroups returns the supported groups, with information like supported versions and the
// preferred version.
func (d *DiscoveryClient) ServerGroups() (apiGroupList *unversioned.APIGroupList, err error) {
// Get the groupVersions exposed at /api
url := d.baseURL
url.Path = "/api"
resp, err := d.get(url.String())
if err != nil {
return nil, err
v := &unversioned.APIVersions{}
err = d.Get().AbsPath("/api").Do().Into(v)
apiGroup := unversioned.APIGroup{}
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not all errors are ignorable. You probably only want to ignore 404's, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I will fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@deads2k this is fixed. PTAL. Thank you.

apiGroup = apiVersionsToAPIGroup(v)
}
var v unversioned.APIVersions
defer resp.Body.Close()
err = json.NewDecoder(resp.Body).Decode(&v)
if err != nil {
return nil, fmt.Errorf("unexpected error: %v", err)
if err != nil && !errors.IsNotFound(err) && !errors.IsForbidden(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this before the the if err == nil and you won't need to conditionally set apiGroup

Copy link
Contributor

Choose a reason for hiding this comment

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

move this before the the if err == nil and you won't need to conditionally set apiGroup

nm, that was wrong.

return nil, err
}
apiGroup := apiVersionsToAPIGroup(&v)

// Get the groupVersions exposed at /apis
url.Path = "/apis"
resp2, err := d.get(url.String())
if err != nil {
apiGroupList = &unversioned.APIGroupList{}
err = d.Get().AbsPath("/apis").Do().Into(apiGroupList)
if err != nil && !errors.IsNotFound(err) && !errors.IsForbidden(err) {
return nil, err
}
defer resp2.Body.Close()
apiGroupList = &unversioned.APIGroupList{}
if err = json.NewDecoder(resp2.Body).Decode(&apiGroupList); err != nil {
return nil, fmt.Errorf("unexpected error: %v", err)
// to be compatible with a v1.0 server, if it's a 403 or 404, ignore and return whatever we got from /api
if err != nil && (errors.IsNotFound(err) || errors.IsForbidden(err)) {
apiGroupList = &unversioned.APIGroupList{}
}

// append the group retrieved from /api to the list
Expand All @@ -116,20 +101,21 @@ func (d *DiscoveryClient) ServerGroups() (apiGroupList *unversioned.APIGroupList

// ServerResourcesForGroupVersion returns the supported resources for a group and version.
func (d *DiscoveryClient) ServerResourcesForGroupVersion(groupVersion string) (resources *unversioned.APIResourceList, err error) {
url := d.baseURL
url := url.URL{}
if groupVersion == "v1" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Code like this is worrying me. Is the next version of the kube API going to have a group?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow your question, could you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow your question, could you clarify?

When kube gets a v2, will this code have to become?

if groupVersion == "v1" || groupVersion == "v2"{

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. I think the plan is breaking the kube API to smaller groups and each group evolves in their own pace. We won't have a monolithic v2 API.

Copy link
Member

Choose a reason for hiding this comment

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

Does /api/v1 return a list of server resources in 1.0? Is this expected to work against 1.0 servers?

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, that won't work. I will fix it in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, "v1" is the only exception to the group/version format we ever intend to have.

url.Path = "/api/" + groupVersion
} else {
url.Path = "/apis/" + groupVersion
}
resp, err := d.get(url.String())
if err != nil {
return nil, err
}
defer resp.Body.Close()
resources = &unversioned.APIResourceList{}
if err = json.NewDecoder(resp.Body).Decode(resources); err != nil {
return nil, fmt.Errorf("unexpected error: %v", err)
err = d.Get().AbsPath(url.String()).Do().Into(resources)
if err != nil {
// ignore 403 or 404 error to be compatible with an v1.0 server.
if groupVersion == "v1" && (errors.IsNotFound(err) || errors.IsForbidden(err)) {
return resources, nil
} else {
return nil, err
}
}
return resources, nil
}
Expand All @@ -155,6 +141,8 @@ func (d *DiscoveryClient) ServerResources() (map[string]*unversioned.APIResource
func setDiscoveryDefaults(config *Config) error {
config.Prefix = ""
config.Version = ""
Copy link
Member Author

Choose a reason for hiding this comment

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

@deads2k Version is set to empty. The base URL will be to the host root.

// Discovery client deals with unversioned objects, so we use api.Codec.
config.Codec = api.Codec
Copy link
Member Author

Choose a reason for hiding this comment

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

changed to api.Codec

return nil
}

Expand All @@ -165,11 +153,6 @@ func NewDiscoveryClient(c *Config) (*DiscoveryClient, error) {
if err := setDiscoveryDefaults(&config); err != nil {
return nil, err
}
transport, err := TransportFor(c)
if err != nil {
return nil, err
}
client := &http.Client{Transport: transport}
baseURL, err := defaultServerUrlFor(c)
return &DiscoveryClient{client, *baseURL}, nil
client, err := UnversionedRESTClientFor(&config)
return &DiscoveryClient{client}, err
}
25 changes: 25 additions & 0 deletions pkg/client/unversioned/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,31 @@ func RESTClientFor(config *Config) (*RESTClient, error) {
return client, nil
}

// UnversionedRESTClientFor is the same as RESTClientFor, except that it allows
// the config.Version to be empty.
func UnversionedRESTClientFor(config *Config) (*RESTClient, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@deads2k
I forked another constructor that doesn't do the empty version check.

Copy link
Member

Choose a reason for hiding this comment

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

cc @krousey another thing to fix by splitting layers...

if config.Codec == nil {
return nil, fmt.Errorf("Codec is required when initializing a RESTClient")
}

baseURL, err := defaultServerUrlFor(config)
if err != nil {
return nil, err
}

client := NewRESTClient(baseURL, config.Version, config.Codec, config.QPS, config.Burst)

transport, err := TransportFor(config)
if err != nil {
return nil, err
}

if transport != http.DefaultTransport {
client.Client = &http.Client{Transport: transport}
}
return client, nil
}

var (
// tlsTransports stores reusable round trippers with custom TLSClientConfig options
tlsTransports = map[string]*http.Transport{}
Expand Down
1 change: 1 addition & 0 deletions pkg/conversion/decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package conversion
import (
"errors"
"fmt"

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not necessary, but the change is consistent with other files, so I kept this change.

"github.com/ugorji/go/codec"
)

Expand Down