Ignore error when discovering server supported groupVersions and resources #16082

caesarxuchao · 2015-10-22T00:54:56Z

First two commits are #15973.

Let the Discovery Client ignore error when discovering server supported groupVersions

fixes @bgrant0607's comment #15796 (comment).

This one needs to be cherry-picked.

supersede #16066

cc @liggitt @jlowdermilk

k8s-github-robot · 2015-10-22T01:18:21Z

Labelling this PR as size/L

k8s-bot · 2015-10-22T01:23:36Z

GCE e2e test build/test passed for commit a998fc0f04af3d311a56d5cb1c3a41fdc2c8e193.

liggitt · 2015-10-22T18:11:00Z

cc @deads2k for negotiation awareness

liggitt · 2015-10-22T18:36:18Z

pkg/apis/unversioned/codec/json_codec.go

+	"k8s.io/kubernetes/pkg/runtime"
+)
+
+type JSONCodec struct{}


I don't think we want this... why can't we use an existing codec for this? Any of them should be able to handle unversioned objects, right?

If we use the Scheme codec, I think it would probably fail here and here.

Even if the Scheme can handle the unversioned objects, I feel it's less robust to use such a complex codec when we only need a simple one. What do you think?

I don't have a strong opinion. If we find ourselves adding anything more complicated than straight json calls here, we should consider not using this.

But consider changing the name to jsonCodec, and making a func NewCodec() jsonCodec {...} so people can make one? Or a public variable var Codec = jsonCodec{}.

k8s-bot · 2015-10-22T23:47:15Z

GCE e2e build/test failed for commit 273484d8eee631b5d991c2504e8bb3458cd63441.

liggitt · 2015-10-22T23:47:26Z

pkg/client/unversioned/discovery_client.go

-		return nil, fmt.Errorf("unexpected error: %v", err)
+	err = d.Get().AbsPath("/apis").Do().Into(apiGroupList)
+	if err != nil {
+		if !errors.IsNotFound(err) {


A secured 1.0 server could easily return a forbidden error here

I'm not familiar with the secured server. Could you point me to the related code? Thanks.

A server that limits users' access to URLs outside their namespace or specifically whitelisted exceptions (for 1.0, that would likely include /api, but definitely wouldn't include /apis. This is close to possible with the abac authorizer (#13097 is open to make it work completely), but real installations would be likely to deny requests outside the expected URL paths. TL;DR: either a NotFound or Forbidden error could easily be received for /apis from a 1.0 installation.

@deads2k I would actually be very tolerant of errors when requesting /apis if we got a usable response from /api, if we ever expect this to work against a 1.0 server

Now it ignores 403 error as well.

lavalamp · 2015-10-23T00:07:07Z

Only minor comments, thanks.

k8s-bot · 2015-10-23T06:09:35Z

GCE e2e build/test failed for commit 36e55f6a19cd0456a6c82c5bb84e86b94bbeef8a.

caesarxuchao · 2015-10-23T17:10:04Z

e2e failure:

Cluster failed to initialize within 300 seconds.

caesarxuchao · 2015-10-23T17:10:16Z

@k8s-bot test this please.

caesarxuchao · 2015-10-23T17:40:08Z

@k8s-bot test this please.

caesarxuchao · 2015-10-23T17:42:46Z

@liggitt @deads2k could you help review the entire change (including the first 2 commits which are #15973)? We need to cherry-pick this into 1.1 release and lavalamp is not available today. Thanks.

caesarxuchao · 2015-10-23T17:43:16Z

@k8s-bot test this please

caesarxuchao · 2015-10-23T17:48:21Z

@k8s-bot test this

j3ffml · 2015-10-23T17:49:31Z

@caesarxuchao the bot is currently dead, we're trying to revive it #16161.

bgrant0607 · 2015-10-28T00:00:50Z

#16364 has been LGTMed. Thanks.

lavalamp · 2015-10-28T00:20:02Z

Looks good, will put on the label when #16364 merges.

lavalamp · 2015-10-28T22:38:52Z

@caesarxuchao can you make an issue for this, so we can track it for 1.1? Thanks

lavalamp · 2015-10-28T22:39:44Z

Oh wait, you're occupied today-- I'll make one.

Ignore 403 and 404 error when discovering server-supported group/version

caesarxuchao · 2015-10-29T16:56:54Z

@lavalamp rebased and squashed. I'll make a cherry-pick now.

k8s-bot · 2015-10-29T17:27:56Z

GCE e2e test build/test passed for commit 69932ae.

caesarxuchao · 2015-10-29T17:56:05Z

Adding the lgtm label per #16082 (comment). @lavalamp please remove it if you want to.

lavalamp · 2015-10-29T18:12:09Z

That's fine, thanks!

lavalamp · 2015-10-29T23:59:39Z

@k8s-bot unit test this

k8s-github-robot · 2015-10-30T03:53:12Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

…#16082-upstream-release-1.1 Auto commit by PR queue bot

k8s-bot · 2015-10-30T04:24:08Z

GCE e2e test build/test passed for commit 69932ae.

k8s-github-robot · 2015-10-30T06:13:13Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-10-30T06:44:35Z

GCE e2e test build/test passed for commit 69932ae.

k8s-github-robot · 2015-10-30T06:45:15Z

Automatic merge from submit-queue

Auto commit by PR queue bot

…y-pick-of-#16082-upstream-release-1.1 Auto commit by PR queue bot

caesarxuchao added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cherrypick-candidate labels Oct 22, 2015

caesarxuchao assigned lavalamp Oct 22, 2015

googlebot added the cla: yes label Oct 22, 2015

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2015

liggitt reviewed Oct 22, 2015
View reviewed changes

caesarxuchao changed the title ~~Ignore error when discovering server supported groupVersions~~ Ignore error when discovering server supported groupVersions and resources Oct 23, 2015

caesarxuchao closed this Oct 23, 2015

caesarxuchao reopened this Oct 23, 2015

caesarxuchao closed this Oct 23, 2015

caesarxuchao reopened this Oct 23, 2015

This was referenced Oct 28, 2015

Make sure 1.1 API resource discovery tolerates 1.0 and 1.2+ apiservers. #16462

Closed

Add TypeMeta to discovery response objects #16364

Merged

Let discoveryClient use RESTClient;

69932ae

Ignore 403 and 404 error when discovering server-supported group/version

caesarxuchao force-pushed the discover-1.0-server-new branch from 92ccb9b to 69932ae Compare October 29, 2015 16:55

googlebot added cla: yes and removed cla: no labels Oct 29, 2015

caesarxuchao mentioned this pull request Oct 29, 2015

Automated cherry pick of #16082 #16516

Merged

caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. cherrypick-candidate and removed cherrypick-candidate labels Oct 29, 2015

k8s-github-robot referenced this pull request Oct 30, 2015

Merge pull request #16516 from caesarxuchao/automated-cherry-pick-of-…

1636c10

…#16082-upstream-release-1.1 Auto commit by PR queue bot

k8s-github-robot pushed a commit that referenced this pull request Oct 30, 2015

Merge pull request #16082 from caesarxuchao/discover-1.0-server-new

a86d878

Auto commit by PR queue bot

k8s-github-robot merged commit a86d878 into kubernetes:master Oct 30, 2015

eparis removed the cherrypick-candidate label Mar 8, 2016

shyamjvs referenced this pull request in shyamjvs/kubernetes Dec 1, 2016

Merge pull request kubernetes#16516 from caesarxuchao/automated-cherr…

fb01cea

…y-pick-of-#16082-upstream-release-1.1 Auto commit by PR queue bot

shouhong referenced this pull request in shouhong/kubernetes Feb 14, 2017

Merge pull request kubernetes#16516 from caesarxuchao/automated-cherr…

23f866a

…y-pick-of-#16082-upstream-release-1.1 Auto commit by PR queue bot

Ignore error when discovering server supported groupVersions and resources #16082

Ignore error when discovering server supported groupVersions and resources #16082

Conversation

caesarxuchao commented Oct 22, 2015

k8s-github-robot commented Oct 22, 2015

k8s-bot commented Oct 22, 2015

liggitt commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Oct 23, 2015

k8s-bot commented Oct 23, 2015

caesarxuchao commented Oct 23, 2015

caesarxuchao commented Oct 23, 2015

caesarxuchao commented Oct 23, 2015

caesarxuchao commented Oct 23, 2015

caesarxuchao commented Oct 23, 2015

caesarxuchao commented Oct 23, 2015

j3ffml commented Oct 23, 2015

bgrant0607 commented Oct 28, 2015

lavalamp commented Oct 28, 2015

lavalamp commented Oct 28, 2015

lavalamp commented Oct 28, 2015

caesarxuchao commented Oct 29, 2015

k8s-bot commented Oct 29, 2015

caesarxuchao commented Oct 29, 2015

lavalamp commented Oct 29, 2015

lavalamp commented Oct 29, 2015

k8s-github-robot commented Oct 30, 2015

k8s-bot commented Oct 30, 2015

k8s-github-robot commented Oct 30, 2015

k8s-bot commented Oct 30, 2015

k8s-github-robot commented Oct 30, 2015