Disable v1beta3 in REST API #4992

ironcladlou · 2015-10-07T14:30:35Z

Disable v1beta3 in the REST API.

ironcladlou · 2015-10-07T14:30:45Z

[test]

openshift-bot · 2015-10-07T14:33:07Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5922/)

deads2k · 2015-10-07T17:59:17Z

jenkins issue re[test]

ironcladlou · 2015-10-08T19:58:44Z

@liggitt or @deads2k, if either of you could pull my branch and reproduce this (hack/test-cmd.sh) error that would be really helpful. I'm making slow progress due to unfamiliarity with the client code involved system.

W1008 15:49:32.277822    3169 helper.go:234] Server does not support API version 'v1beta3'. Falling back to 'v1'.
Error from server: error when creating "examples/hello-openshift/hello-pod.json": Pod in version v1beta3 cannot be handled as a Pod: The api
Version in the data (v1beta3) does not match the specified apiVersion(v1)

Even though it appears the correct version (v1) was negotiated (verified through some debugging... I think), the v1beta3 data isn't getting converted on the way out (according to the server log, at least.)

deads2k · 2015-10-09T15:59:30Z

The wrong codec is being used here https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/k8s.io/kubernetes/pkg/kubectl/resource/helper.go#L120 and here https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/k8s.io/kubernetes/pkg/kubectl/cmd/create.go#L109 . You need to be encoding using the codec of the RESTClient. I'm not sure where else you'll hit this.

ironcladlou · 2015-10-09T19:31:54Z

We've found that the origin client factory is using a private client cache which doesn't negotiate versions with the server (upstream clients use https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/k8s.io/kubernetes/pkg/kubectl/cmd/util/clientcache.go).

This causes things like the client using the version specified in resource JSON for encoding.

ironcladlou · 2015-10-09T19:33:12Z

@smarterclayton @deads2k what's the way out, here? Porting the negotiation logic downstream?

deads2k · 2015-10-09T19:36:57Z

@smarterclayton @deads2k what's the way out, here? Porting the negotiation logic downstream?

Is it not re-useable as is if we called into it?

ironcladlou · 2015-10-12T20:18:06Z

I pushed my WIP branch to get feedback on what more we can do here in the immediately future. Things I've done:

Ported server version negotiation to the origin client cache (I found some ways to reuse most of the upstream code, thankfully).
Exposed GetCodec through RESTClient so that resource builder visitors can encode using the codec found on the negotiated client version, which led to the realization that the client made by the builder is based on the mapping for the resource being manipulated rather than the negotiated server version.

It's not entirely clear to me what the correct behavior is supposed to be here. Should the negotiated version from the server be used for all encoding/decoding? If not, when should the client be based on the mapping vs. the server? I'm learning every part of this as I go and there seem to be many rabbit holes at every step.

cc @deads2k @smarterclayton

smarterclayton · 2015-10-13T01:16:12Z

v1beta3 should not show up in the list of potential service targets. That may mean that the code that initializes the rest mapper that is passed to all the CLI commands needs to exclude v1beta3

deads2k · 2015-10-13T11:49:54Z

It's not entirely clear to me what the correct behavior is supposed to be here. Should the negotiated version from the server be used for all encoding/decoding? If not, when should the client be based on the mapping vs. the server? I'm learning every part of this as I go and there seem to be many rabbit holes at every step.

Decoding should try every codec available, so decoding is based on every api-version the client has available. Encoding for transport to the server has to match the level negotiated with the server. Encoding request the by the user for client-side output should match the user's request, so again, any codec available.

smarterclayton · 2015-10-13T12:05:47Z

The codec selected by the client restmapper should be based primarily on
the user input (api version) and then probably on the negotiated
preferences. That means the apiversion value from the restclient should be
seeded into the mapper when mapping a are retrieved.

On Oct 13, 2015, at 7:50 AM, David Eads notifications@github.com wrote:

It's not entirely clear to me what the correct behavior is supposed to be
here. Should the negotiated version from the server be used for all
encoding/decoding? If not, when should the client be based on the mapping
vs. the server? I'm learning every part of this as I go and there seem to
be many rabbit holes at every step.

Decoding should try every codec available, so decoding is based on every
api-version the client has available. Encoding for transport to the server
has to match the level negotiated with the server. Encoding request the by
the user for client-side output should match the user's request, so again,
any codec available.

—
Reply to this email directly or view it on GitHub
#4992 (comment).

ironcladlou · 2015-10-13T15:32:25Z

I updated the client factory so that the restmapper gets its output version from a clientConfig obtained from the clientCache (which now negotiates). The fact that we still have to expose GetCodec via RESTClient and update all uses of the builder visitor upstream to use it is concerning. I could be missing an alternative approach on that point.

ironcladlou · 2015-10-13T17:18:04Z

Tests pass, but my confidence level is extremely low, especially re: #4992 (comment). @smarterclayton @deads2k

ironcladlou · 2015-10-13T17:20:18Z

I haven't implemented any sort of redirect for /osapi. I need some guidance.

ironcladlou · 2015-10-13T17:21:02Z

I'm not splitting the upstream stuff into a separate commit until all the work is vetted.

smarterclayton · 2015-10-13T17:43:49Z

You don't need GetCodec - you need the RESTMapper to target based on a
version you initialize it with. We have wrappers to RESTMapper that
injects a preferred version - which should not be v1beta3

On Tue, Oct 13, 2015 at 1:21 PM, Dan Mace notifications@github.com wrote:

I'm not splitting the upstream stuff into a separate commit until all the
work is vetted.

—
Reply to this email directly or view it on GitHub
#4992 (comment).

ironcladlou · 2015-10-13T17:51:48Z

You don't need GetCodec - you need the RESTMapper to target based on a version you initialize it with. We have wrappers to RESTMapper that injects a preferred version - which should not be v1beta3

Hm, looks like you're right. Setting up the mapper in the factory to use the new negotiated client from the cache seems to have corrected the codec assigned to the mapper, rendering the GetCodec stuff redundant. That's good...

ironcladlou · 2015-10-13T19:31:33Z

@smarterclayton @liggitt @deads2k

From IRC:

liggitt: if a client is checking /osapi for supported versions, and they get silently redirected to /oapi and see "v1", they aren't likely to correctly build "oapi/v1", but use "osapi/v1", which will fail
liggitt: I would rather a request to /osapi return an empty list of supported versions if there are no supported versions under that prefix

In the interest of time, I think you guys should hash out the requirement so I can implement.

liggitt · 2015-10-13T19:40:56Z

the only two uses I know of for pinging /osapi were 1) version negotiation for (very?) old clients, and 2) login connectivity check from old clients. both of those should be satisfied by returning an empty list if v1beta3 is disabled, rather than a 404

ironcladlou · 2015-10-13T19:54:39Z

I modified master.go#initAPIVersionRoute to support returning an empty version list. Querying /osapi now returns:

{"versions": []}

ironcladlou · 2015-10-15T21:33:56Z

I think all feedback's addressed except for one possible test for /oapi to assert 200 and an empty versions list, but we need to decide where that'll go. Seems like something minor to do in a followup immediately after if everything else looks okay.

liggitt · 2015-10-15T23:29:31Z

Just remembered, need to exclude dead versions in the version/kind map generated for the UI at
https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/asset.go#L168
https://github.com/openshift/origin/blob/master/pkg/assets/handlers.go#L186
https://github.com/openshift/origin/blob/master/pkg/cmd/server/origin/asset.go#L165

I could live with a follow up issue for that, worst case the UI would try to submit a resource and get a 404 error

liggitt · 2015-10-15T23:59:47Z

pkg/cmd/util/clientcmd/factory.go

We need a test in place to make sure this continues to check against origin prefix. Unit test against an httptest server that verifies only /oapi is requested. Upstream version negotiation resets the prefix in several places. Want to make sure we catch it if a prefix reset creeps into this path

liggitt · 2015-10-16T00:20:57Z

lgtm, with follow up issues for:

web console config
test for /osapi 200 response returning empty list
test for negotiation continuing to hit /oapi

Fine with me to squash

ironcladlou · 2015-10-16T13:45:37Z

@liggitt squashed, and filtered dead versions here: https://github.com/ironcladlou/origin/blob/remove-v1beta3/pkg/cmd/server/origin/asset.go#L170

I didn't see an existing test for that, PTAL.

ironcladlou · 2015-10-16T14:39:03Z

Jenkins flake looks like #5161. @liggitt, want to give this one more look and tag it?

liggitt · 2015-10-16T14:57:51Z

pkg/cmd/server/origin/asset.go

need to filter version per-kind... our dead levels and k8s' might not be identical

deadOriginVersions := sets.NewString(configapi.DeadOpenShiftAPILevels...) deadKubernetesVersions := sets.NewString(configapi.DeadKubernetesAPILevels...) ... if latest.OriginKind(kind, version) { if !deadOriginVersions.Has(version) { originResources.Insert(resource) } } else { if !deadKubernetesVersions.Has(version) { k8sResources.Insert(resource) } }

also remove MasterLegacyPrefix from WebConsoleConfig and this line from the config template:

"v1beta3": "{{ .MasterLegacyPrefix | js}}",

Reimplemented, thanks

liggitt · 2015-10-16T20:27:34Z

actually, hold on a sec... I'm not seeing negotiation happen against /oapi

This is what happens when I use kube resources:

$ oc get pods --loglevel=6
GET https://192.168.1.114:8443/api 200 OK in 2 milliseconds
GET https://192.168.1.114:8443/api/v1/namespaces/default/pods 200 OK in 1 milliseconds

When I use origin resources:

$ oc get builds --loglevel=6
GET https://192.168.1.114:8443/oapi/v1/namespaces/default/builds 200 OK in 1 milliseconds

ironcladlou · 2015-10-16T20:47:44Z

Added debugging output here and got very weird results, although it does seem to verify that we're negotiating correctly:

https://github.com/ironcladlou/origin/blob/remove-v1beta3/Godeps/_workspace/src/k8s.io/kubernetes/pkg/client/unversioned/client.go#L150

$ oc get pods --loglevel=8
!!! negotiating with https://10.0.1.50:8443/oapi
!!! negotiating with https://10.0.1.50:8443/api
I1016 16:41:44.618077   13349 debugging.go:99] GET https://10.0.1.50:8443/api
I1016 16:41:44.618141   13349 debugging.go:106] Request Headers:
I1016 16:41:44.618154   13349 debugging.go:109]     User-Agent: oc/v0.0.0 (linux/amd64) kubernetes/$Format
I1016 16:41:44.619012   13349 debugging.go:124] Response Status: 200 OK in 0 milliseconds
I1016 16:41:44.619038   13349 debugging.go:127] Response Headers:
I1016 16:41:44.619052   13349 debugging.go:130]     Cache-Control: no-store
I1016 16:41:44.619066   13349 debugging.go:130]     Content-Type: application/json
I1016 16:41:44.619080   13349 debugging.go:130]     Date: Fri, 16 Oct 2015 20:41:44 GMT
I1016 16:41:44.619094   13349 debugging.go:130]     Content-Length: 32
I1016 16:41:44.619127   13349 request.go:788] Response Body: {
  "versions": [
    "v1"
  ]
}
I1016 16:41:44.619562   13349 debugging.go:99] GET https://10.0.1.50:8443/api/v1/namespaces/default/pods
I1016 16:41:44.619583   13349 debugging.go:106] Request Headers:
I1016 16:41:44.619596   13349 debugging.go:109]     User-Agent: oc/v0.0.0 (linux/amd64) kubernetes/$Format
I1016 16:41:44.620919   13349 debugging.go:124] Response Status: 200 OK in 1 milliseconds
I1016 16:41:44.620944   13349 debugging.go:127] Response Headers:
I1016 16:41:44.620957   13349 debugging.go:130]     Cache-Control: no-store
I1016 16:41:44.620971   13349 debugging.go:130]     Content-Type: application/json
I1016 16:41:44.620985   13349 debugging.go:130]     Date: Fri, 16 Oct 2015 20:41:44 GMT
I1016 16:41:44.620998   13349 debugging.go:130]     Content-Length: 129
I1016 16:41:44.621028   13349 request.go:788] Response Body: {"kind":"PodList","apiVersion":"v1","metadata":{"selfLink":"/api/v1/namespaces/default/pods","resourceVersion":"309"},"items":[]}
NAME      READY     STATUS    RESTARTS   AGE

$ oc get builds --loglevel=8
!!! negotiating with https://10.0.1.50:8443/oapi
!!! negotiating with https://10.0.1.50:8443/oapi
I1016 16:43:33.019478   13392 request.go:788] Response Body: {
  "versions": [
    "v1"
  ]
}
I1016 16:43:33.019760   13392 debugging.go:99] GET https://10.0.1.50:8443/oapi/v1/namespaces/default/builds
I1016 16:43:33.019778   13392 debugging.go:106] Request Headers:
I1016 16:43:33.019786   13392 debugging.go:109]     User-Agent: oc/ (linux/amd64) openshift/unknown
I1016 16:43:33.021288   13392 debugging.go:124] Response Status: 200 OK in 1 milliseconds
I1016 16:43:33.021371   13392 debugging.go:127] Response Headers:
I1016 16:43:33.021388   13392 debugging.go:130]     Content-Type: application/json
I1016 16:43:33.021404   13392 debugging.go:130]     Date: Fri, 16 Oct 2015 20:43:33 GMT
I1016 16:43:33.021418   13392 debugging.go:130]     Content-Length: 134
I1016 16:43:33.021431   13392 debugging.go:130]     Cache-Control: no-store
I1016 16:43:33.021480   13392 request.go:788] Response Body: {"kind":"BuildList","apiVersion":"v1","metadata":{"selfLink":"/oapi/v1/namespaces/default/builds","resourceVersion":"320"},"items":[]}
NAME      TYPE      FROM      STATUS    STARTED

I'm not sure why only the response of the negotiation for origin resources is getting logged... also not 100% clear I understand how there are two passes through the negotiation in both cases. Clearly some more debugging is required there to fully understand.

liggitt · 2015-10-16T20:51:53Z

pkg/cmd/util/clientcmd/factory.go

if this is only used for version negotiation, name it that, and put a TODO in the struct to remove it once we figure out negotiation

Changed and commented on the clientCache struct itself.

liggitt · 2015-10-17T00:35:20Z

pkg/cmd/util/clientcmd/factory_test.go

I'm a dope, should be t.Fatalf... verify-govet failed the build

D'oh.. fixed

liggitt · 2015-10-17T03:50:52Z

also fixed UI URL generation spec tests in https://github.com/openshift/origin/pull/5173/files#diff-689b2ab3d7877627d6bc9d184285eefe

liggitt · 2015-10-19T14:09:48Z

lgtm, [merge]

openshift-bot · 2015-10-19T14:16:50Z

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 2

openshift-bot · 2015-10-19T14:16:51Z

Evaluated for origin merge up to 5bb8222

openshift-bot · 2015-10-19T14:48:51Z

Evaluated for origin test up to 5bb8222

ironcladlou · 2015-10-19T20:39:52Z

Cherry-picked into and superceded by #5143

ironcladlou force-pushed the remove-v1beta3 branch from 92a4cc5 to 9d11e61 Compare October 7, 2015 14:33

ironcladlou force-pushed the remove-v1beta3 branch from 9d11e61 to ab4f481 Compare October 7, 2015 20:18

deads2k mentioned this pull request Oct 9, 2015

Change api version in examples #5030

Merged

ironcladlou force-pushed the remove-v1beta3 branch from 338ca69 to fb45a31 Compare October 9, 2015 18:41

ironcladlou force-pushed the remove-v1beta3 branch from fb45a31 to 20a9933 Compare October 12, 2015 20:11

ironcladlou force-pushed the remove-v1beta3 branch from 20a9933 to 38aae1a Compare October 13, 2015 12:57

ironcladlou force-pushed the remove-v1beta3 branch from a1de5d1 to 2cc2397 Compare October 13, 2015 17:20

ironcladlou force-pushed the remove-v1beta3 branch 2 times, most recently from 677e6a7 to 5377390 Compare October 13, 2015 17:57

liggitt reviewed Oct 15, 2015
View reviewed changes

ironcladlou force-pushed the remove-v1beta3 branch from 2a03ad2 to 68ff261 Compare October 16, 2015 13:44

liggitt reviewed Oct 16, 2015
View reviewed changes

ironcladlou force-pushed the remove-v1beta3 branch 2 times, most recently from 6a0e777 to 2d1909c Compare October 16, 2015 20:21

liggitt reviewed Oct 16, 2015
View reviewed changes

ironcladlou force-pushed the remove-v1beta3 branch 2 times, most recently from 147a461 to 3fc9775 Compare October 16, 2015 22:05

liggitt mentioned this pull request Oct 17, 2015

Disable v1beta3 in REST API #5173

Closed

liggitt reviewed Oct 17, 2015
View reviewed changes

ironcladlou force-pushed the remove-v1beta3 branch 2 times, most recently from a09a468 to 78487fa Compare October 19, 2015 13:33

Disable v1beta3 in REST API

5bb8222

ironcladlou force-pushed the remove-v1beta3 branch from 78487fa to 5bb8222 Compare October 19, 2015 14:09

ironcladlou closed this Oct 19, 2015

ironcladlou mentioned this pull request Oct 19, 2015

Refactor server version negotiation and client cache to consolidate with upstream #5216

Closed

deads2k mentioned this pull request Oct 20, 2015

"--config" option doesn't work for CLI #5218

Closed

Disable v1beta3 in REST API #4992

Disable v1beta3 in REST API #4992

Uh oh!

Conversation

ironcladlou commented Oct 7, 2015

Uh oh!

ironcladlou commented Oct 7, 2015

Uh oh!

openshift-bot commented Oct 7, 2015

Uh oh!

deads2k commented Oct 7, 2015

Uh oh!

ironcladlou commented Oct 8, 2015

Uh oh!

deads2k commented Oct 9, 2015

Uh oh!

ironcladlou commented Oct 9, 2015

Uh oh!

ironcladlou commented Oct 9, 2015

Uh oh!

deads2k commented Oct 9, 2015

Uh oh!

ironcladlou commented Oct 12, 2015

Uh oh!

smarterclayton commented Oct 13, 2015

Uh oh!

deads2k commented Oct 13, 2015

Uh oh!

smarterclayton commented Oct 13, 2015

Uh oh!

ironcladlou commented Oct 13, 2015

Uh oh!

ironcladlou commented Oct 13, 2015

Uh oh!

ironcladlou commented Oct 13, 2015

Uh oh!

ironcladlou commented Oct 13, 2015

Uh oh!

smarterclayton commented Oct 13, 2015

Uh oh!

ironcladlou commented Oct 13, 2015

Uh oh!

ironcladlou commented Oct 13, 2015

Uh oh!

liggitt commented Oct 13, 2015

Uh oh!

ironcladlou commented Oct 13, 2015

Uh oh!

ironcladlou commented Oct 15, 2015

Uh oh!

liggitt commented Oct 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Oct 16, 2015

Uh oh!

ironcladlou commented Oct 16, 2015

Uh oh!

ironcladlou commented Oct 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Oct 16, 2015

Uh oh!

ironcladlou commented Oct 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Oct 17, 2015

Uh oh!