Skip to content

preserve err type loginoptions#17138

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
juanvallejo:jvallejo/preserve-err-type-loginoptions
Nov 4, 2017
Merged

preserve err type loginoptions#17138
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
juanvallejo:jvallejo/preserve-err-type-loginoptions

Conversation

@juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Nov 2, 2017 •
edited by enj
Loading

Fixes #17136
Fixes #17150

Preserves the error type returned in https://github.com/openshift/origin/blob/master/pkg/oc/cli/cmd/login/loginoptions.go#L237

cc @openshift/cli-review

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 2, 2017
@juanvallejo juanvallejo changed the title Jvallejo/preserve err type loginoptions preserve err type loginoptions Nov 2, 2017
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Nov 2, 2017

/test extended_gssapi

@juanvallejo juanvallejo force-pushed the jvallejo/preserve-err-type-loginoptions branch from 7f7e973 to 4477cad Compare November 2, 2017 01:13
enj
enj suggested changes Nov 2, 2017
View reviewed changes
test/integration/oauth_request_header_test.go Outdated
StartingKubeConfig: &clientcmdapi.Config{},
Reader: bytes.NewBufferString("myusername\nmypassword\n"),
Out: loginOutput,
ErrOut: loginErrOutput,
Copy link
Contributor

@enj enj Nov 2, 2017

Choose a reason for hiding this comment

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

ioutil.Discard

test/integration/oauth_oidc_test.go Outdated
StartingKubeConfig: &clientcmdapi.Config{},
Reader: bytes.NewBufferString("mylogin\nmypassword\n"),
Out: loginOutput,
ErrOut: loginErrOutput,
Copy link
Contributor

@enj enj Nov 2, 2017

Choose a reason for hiding this comment

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

ioutil.Discard

pkg/oc/bootstrap/docker/up.go Outdated
func (c *ClientStartConfig) Login(out io.Writer) error {
server := c.OpenShiftHelper().Master(c.ServerIP)
return openshift.Login(initialUser, initialPassword, server, c.LocalConfigDir, c.originalFactory, c.command, out)
return openshift.Login(initialUser, initialPassword, server, c.LocalConfigDir, c.originalFactory, c.command, out, ioutil.Discard)
Copy link
Contributor

@enj enj Nov 2, 2017

Choose a reason for hiding this comment

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

It is probably debatable if this should use ioutil.Discard or not.

Copy link
Contributor Author

@juanvallejo juanvallejo Nov 2, 2017

Choose a reason for hiding this comment

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

cc @csrwng

Copy link
Contributor Author

@juanvallejo juanvallejo Nov 2, 2017

Choose a reason for hiding this comment

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

The alternative would be to wire each handler to receive a second io.Writer for errs

Copy link
Contributor

@csrwng csrwng Nov 2, 2017

Choose a reason for hiding this comment

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

why not use the same writer for both? (out)

Copy link
Contributor

@csrwng csrwng Nov 2, 2017

Choose a reason for hiding this comment

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

the more info we can return to users about failures, etc. the better

Copy link
Contributor Author

@juanvallejo juanvallejo Nov 2, 2017

Choose a reason for hiding this comment

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

thanks, went ahead and re-used out for both

pkg/oc/cli/cmd/login/loginoptions.go Outdated
if statusErr, ok := err.(*kerrors.StatusError); ok {
if statusErr.Status().Code == http.StatusInternalServerError {
return fmt.Errorf("error: The server was unable to respond - %v", suggestion)
fmt.Fprintf(o.ErrOut, "error: The server was unable to respond - %v", suggestion)
Copy link
Contributor

@enj enj Nov 2, 2017

Choose a reason for hiding this comment

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

This needs a newline at the end and you probably want to print the error as well. Not special casing StatusInternalServerError would also be reasonable thing to do.

Copy link
Contributor Author

@juanvallejo juanvallejo Nov 2, 2017

Choose a reason for hiding this comment

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

Thanks, will go ahead and remove the special case for internal server err - the error we print is pretty much the same as the one below this

@enj
Copy link
Contributor

enj commented Nov 2, 2017

Do not forget to squash.

@enj enj assigned fabianofranz and unassigned smarterclayton and csrwng Nov 2, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/preserve-err-type-loginoptions branch 4 times, most recently from f235ce3 to 21e025b Compare November 2, 2017 16:03
@enj enj mentioned this pull request Nov 2, 2017
Closed
enj
enj reviewed Nov 2, 2017
View reviewed changes
pkg/oc/cli/cmd/login/loginoptions.go Outdated
token, err := tokencmd.RequestToken(o.Config, o.Reader, o.Username, o.Password)
if err != nil {
suggestion := "verify you have provided the correct host and port and that the server is currently running."
fmt.Fprintf(o.ErrOut, "error: %v - %v\n", err, suggestion)
Copy link
Contributor

@enj enj Nov 2, 2017

Choose a reason for hiding this comment

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

No point in making the suggestion var now.

Copy link
Contributor Author

@juanvallejo juanvallejo Nov 2, 2017

Choose a reason for hiding this comment

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

thought it might help this line not get too long - will update though

@enj
Copy link
Contributor

enj commented Nov 2, 2017

@juanvallejo we need some kind of test (unit preferred) to make sure #17150 does not happen.

@juanvallejo juanvallejo force-pushed the jvallejo/preserve-err-type-loginoptions branch 2 times, most recently from 8e76c1c to b366e91 Compare November 3, 2017 13:58
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 3, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/preserve-err-type-loginoptions branch from d63700e to df1e76b Compare November 3, 2017 17:06
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Nov 3, 2017

@enj thanks, added a unit test. ptal

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Nov 3, 2017

/test extended_conformance_gce

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Nov 3, 2017

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 3, 2017
enj
enj suggested changes Nov 3, 2017
View reviewed changes
pkg/cmd/util/tokencmd/request_token.go Outdated
// https://tools.ietf.org/html/draft-ietf-oauth-discovery-04#section-2
// Copied from pkg/cmd/server/origin/nonapiserver.go
oauthMetadataEndpoint = "/.well-known/oauth-authorization-server"
OauthMetadataEndpoint = "/.well-known/oauth-authorization-server"
Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

Do not export this, it is already a copy from a different file. Hard code the string, it can never change.

pkg/oc/cli/cmd/login/loginoptions_test.go Outdated
"testing"

"github.com/spf13/pflag"

Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

Space seems out of place.

pkg/oc/cli/cmd/login/loginoptions_test.go Outdated
func (r *oauthMetadataResponse) Serialize() string {
b, err := json.Marshal(r.metadata)
if err != nil {
return ""
Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

Fail the test on this.

pkg/oc/cli/cmd/login/loginoptions_test.go Outdated

if r.URL.Path == tokencmd.OauthMetadataEndpoint {
w.WriteHeader(http.StatusOK)
io.WriteString(w, metadataResponse.Serialize())
Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

Can't you just write the raw bytes?

pkg/oc/cli/cmd/login/loginoptions_test.go Outdated
TokenEndpoint: server.URL + "/oauth/token",
CodeChallengeMethodsSupported: []string{"plain", "S256"},
}
defer server.Close()
Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

Move up.

pkg/oc/cli/cmd/login/loginoptions_test.go Outdated
}

if !kapierrs.IsUnauthorized(err) {
t.Fatalf("expecting error of type metav1.StatusReasonUnauthorized, but got %v", reflect.TypeOf(err))
Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

Can just do %T to get the type.

pkg/oc/cli/cmd/login/loginoptions_test.go Outdated
return loginOptions
}

func defaultClientConfig(flags *pflag.FlagSet) kclientcmd.ClientConfig {
Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

Flags, wat?

Copy link
Contributor Author

@juanvallejo juanvallejo Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Heh, had originally grabbed these two helpers from here. Went ahead and cleaned the test up - no longer need them here

pkg/oc/cli/cmd/login/loginoptions_test.go Outdated
clientConfig := defaultClientConfig(flagset)
flagset.Parse(flags)

startingConfig, _ := clientConfig.RawConfig()
Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

If this is an error, it should fail the test.

pkg/oc/cli/cmd/login/loginoptions_test.go Outdated
metadataResponse := &oauthMetadataResponse{}

server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
invoked <- struct{}{}
Copy link
Contributor

@enj enj Nov 3, 2017

Choose a reason for hiding this comment

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

You need a check to fail the test if this would dead lock the buffered channel.

@juanvallejo juanvallejo force-pushed the jvallejo/preserve-err-type-loginoptions branch 3 times, most recently from 5693551 to 1a24880 Compare November 3, 2017 21:00
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Nov 3, 2017

@enj thanks for the review, comments addressed

@enj
Copy link
Contributor

enj commented Nov 3, 2017 •
edited
Loading

/lgtm

Approving since this is all auth code.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2017
@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Nov 3, 2017

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj, juanvallejo
We suggest the following additional approver: fabianofranz

Assign the PR to them by writing /assign @fabianofranz in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@enj enj added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/preserve-err-type-loginoptions branch from 1a24880 to b3b6b08 Compare November 3, 2017 22:20
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2017
@juanvallejo juanvallejo added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/preserve-err-type-loginoptions branch from b3b6b08 to 0c94fae Compare November 3, 2017 22:30
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2017
@juanvallejo juanvallejo added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2017
@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Nov 4, 2017

Automatic merge from submit-queue (batch tested with PRs 17178, 17141, 17138).

@openshift-merge-robot openshift-merge-robot merged commit 12f0ec1 into openshift:master Nov 4, 2017
@juanvallejo juanvallejo deleted the jvallejo/preserve-err-type-loginoptions branch November 6, 2017 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@csrwng csrwng csrwng left review comments

+1 more reviewer

@enj enj enj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fabianofranz fabianofranz

@enj enj

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@juanvallejo @enj @openshift-merge-robot @csrwng @fabianofranz @smarterclayton @openshift-ci-robot