update policy for pods/exec #3716

deads2k · 2015-07-15T13:15:15Z

Upstream pull: kubernetes/kubernetes#11333

Reacts to upstream changes for kubernetes/kubernetes#10654. Also refactors the test to make sure that project admins are able to run pods/exec by default.

deads2k · 2015-07-15T13:15:22Z

@liggitt

liggitt · 2015-07-15T13:18:36Z

hack/test-end-to-end.sh

 oc project test
+oc whoami


you want to check output of this, or is it just for the test log?

you want to check output of this, or is it just for the test log?

Just for the log.

ncdc · 2015-07-15T13:33:21Z

@deads2k move the port forward test up too?

deads2k · 2015-07-15T14:05:36Z

@deads2k move the port forward test up too?

done.

liggitt · 2015-07-15T14:36:15Z

hack/test-end-to-end.sh

+# Port forwarding
+echo "[INFO] Validating port-forward"
+oc port-forward -p ${frontend_pod} 10080:8080  &> "${LOG_DIR}/port-forward.log" &
+wait_for_url_timed "http://localhost:10080" "[INFO] Frontend says: " $((10*TIME_SEC))


was there something wrong with the healthz check?

was there something wrong with the healthz check?

The /healthz check is for a registry, not a ruby frontend. Registry is in default where e2e-user doesn't have rights. e2e-user checks the ruby front-end.

ah, makes sense

liggitt · 2015-07-15T14:38:39Z

@smarterclayton thinks we should keep our client using GET

ncdc · 2015-07-15T14:39:29Z

Isn't upstream going to move to require POST for exec?

deads2k · 2015-07-15T14:40:27Z

@smarterclayton thinks we should keep our client using GET

How would our client work against an external kube? Also, that would break our compatibility with kubectl.

liggitt · 2015-07-15T14:41:43Z

apparently upstream supports both now?

smarterclayton · 2015-07-15T14:42:02Z

Our clients have to work against 3.0 OpenShift servers. So we have to
support both.

On Wed, Jul 15, 2015 at 10:40 AM, David Eads [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton thinks we should keep
our client using GET

How would our client work against an external kube? Also, that would break
our compatibility with kubectl.

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

Clayton Coleman | Lead Engineer, OpenShift

ncdc · 2015-07-15T14:42:05Z

Yes but afaik they're going to remove GET support

liggitt · 2015-07-15T14:43:10Z

ah, the new-client, old server bit

smarterclayton · 2015-07-15T15:59:25Z

We need to carry the "post" first, then try "get" client patch.

deads2k · 2015-07-15T16:57:57Z

We need to carry the "post" first, then try "get" client patch.

This doesn't make that problem better or worse, but it does allow the current origin code to work. This is necessary, but not sufficient. New issue for the command?

smarterclayton · 2015-07-15T17:04:37Z

On Jul 15, 2015, at 12:58 PM, David Eads [email protected] wrote:

We need to carry the "post" first, then try "get" client patch.

This doesn't make that problem better or worse, but it does allow the
current origin code to work.

Not sure what you mean.

This is necessary, but not sufficient. New issue for the command?

For updating the default role assignments? Yes, new issue.

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

liggitt · 2015-07-15T17:10:42Z

this PR allows both GET and POST for exec and portforward. new issue for the carried patch to try GET, then POST in exec and portforward (I would actually try in the reverse order... "correct" first, then fallback to legacy)

deads2k · 2015-07-15T17:26:50Z

this PR allows both GET and POST for exec and portforward. new issue for the carried patch to try GET, then POST in exec and portforward

That was my intent, but if this upstream patch suits (easy to carry and merge as opposed to nicely factored) then I guess I'm not picky

liggitt · 2015-07-15T17:31:51Z

Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/cmd/exec.go

+	postErr := re.Execute(req, config, args, stdin, cmdOut, cmdErr, tty)
+
+	// if we don't have an error, return.  If we did get an error, try a GET because v3.0.0 shipped with exec running as a GET.
+	if postErr == nil {


should we scope this to method not supported errors? if we get an error here when an exec times out or is terminated by the user and respond to it by opening a second request, that's not what we want.

I guess against a 3.0 server you'd get access denied or method not supported

should we scope this to method not supported errors?

It would have to include forbidden too since the old policy would forbid this for most users. I don't have strong feelings.

It would have to include forbidden too since the old policy would forbid this for most users.

tightened

liggitt · 2015-07-15T18:30:14Z

Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/kubectl/cmd/exec.go

+	}
+
+	// only try the get if the error is either a forbidden or method not supported, otherwise trying with a GET probably won't help
+	if !apierrors.IsForbidden(postErr) && !apierrors.IsMethodNotSupported(postErr) {


looks good... double check that Execute parses forbidden/notsupported statuses correctly?

looks good... double check that Execute parses forbidden/notsupported statuses correctly?

Looks to be passed straight back through: https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/request.go#L662-L665

Looks to be passed straight back through: https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/client/request.go#L662-L665

grrr.. nm

deads2k · 2015-07-15T19:56:20Z

@liggitt now chaining back errors correctly

liggitt · 2015-07-15T20:16:08Z

LGTM

deads2k · 2015-07-15T20:17:16Z

[merge]

openshift-bot · 2015-07-15T20:20:46Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2750/) (Image: devenv-fedora_2007)

openshift-bot · 2015-07-15T20:20:47Z

Evaluated for origin up to 9b94ef6

openshift-bot · 2015-07-15T20:20:47Z

[Test]ing while waiting on the merge queue

openshift-bot · 2015-07-15T20:24:17Z

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

Merged by openshift-bot

liggitt reviewed Jul 15, 2015
View reviewed changes

liggitt added the area/compatibility label Jul 15, 2015

liggitt mentioned this pull request Jul 15, 2015

Track bootstrap policy updates / migration #3717

Closed

update policy for pods/exec

5619b88

deads2k force-pushed the make-exec-work branch from 67f011f to 5619b88 Compare July 15, 2015 14:05

liggitt reviewed Jul 15, 2015
View reviewed changes

deads2k force-pushed the make-exec-work branch from e36f92d to c2fea2c Compare July 15, 2015 17:25

liggitt reviewed Jul 15, 2015
View reviewed changes

deads2k force-pushed the make-exec-work branch from c2fea2c to 3c180b9 Compare July 15, 2015 17:43

liggitt self-assigned this Jul 15, 2015

liggitt reviewed Jul 15, 2015
View reviewed changes

deads2k force-pushed the make-exec-work branch from 3c180b9 to af9f4f7 Compare July 15, 2015 19:52

UPSTREAM: 11333: pass along status errors for upgrades

7873b31

UPSTREAM: carry: support pointing oc exec to old openshift server

9b94ef6

deads2k force-pushed the make-exec-work branch from af9f4f7 to 9b94ef6 Compare July 15, 2015 19:55

openshift-bot pushed a commit that referenced this pull request Jul 16, 2015

Merge pull request #3716 from deads2k/make-exec-work

6ed2b36

Merged by openshift-bot

openshift-bot merged commit 6ed2b36 into openshift:master Jul 16, 2015

deads2k deleted the make-exec-work branch July 31, 2015 18:30

update policy for pods/exec #3716

update policy for pods/exec #3716

Uh oh!

Conversation

deads2k commented Jul 15, 2015

Uh oh!

deads2k commented Jul 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncdc commented Jul 15, 2015

Uh oh!

deads2k commented Jul 15, 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 Jul 15, 2015

Uh oh!

ncdc commented Jul 15, 2015

Uh oh!

deads2k commented Jul 15, 2015

Uh oh!

liggitt commented Jul 15, 2015

Uh oh!

smarterclayton commented Jul 15, 2015

Uh oh!

ncdc commented Jul 15, 2015

Uh oh!

liggitt commented Jul 15, 2015

Uh oh!

smarterclayton commented Jul 15, 2015

Uh oh!

deads2k commented Jul 15, 2015

Uh oh!

smarterclayton commented Jul 15, 2015

Uh oh!

liggitt commented Jul 15, 2015

Uh oh!

deads2k commented Jul 15, 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!

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!

deads2k commented Jul 15, 2015

Uh oh!

liggitt commented Jul 15, 2015

Uh oh!

deads2k commented Jul 15, 2015

Uh oh!

openshift-bot commented Jul 15, 2015

Uh oh!

openshift-bot commented Jul 15, 2015

Uh oh!

openshift-bot commented Jul 15, 2015

Uh oh!

openshift-bot commented Jul 15, 2015

Uh oh!

Uh oh!