Skip to content

pkg/asset/installconfig/aws: Ask for AWS access key and secret #798

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.

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

Merged

Conversation

wking
Copy link
Member

@wking wking commented Dec 5, 2018

Builds on #688; review that first.

For folks who forgot to setup ~/.aws/credentials before calling the installer. Now we'll give them a nice prompt and a link into the AWS docs instead of dying. I'm also saving any provided creds into the usual location for later AWS access during this and future installer runs.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 5, 2018
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2018
@wking wking force-pushed the aws-credentials-prompts branch from 927f9d7 to cdddb2c Compare December 5, 2018 23:29
@crawford
Copy link
Contributor

crawford commented Dec 5, 2018

The unique commit in this PR looks good to me. Waiting on #688.

@wking wking force-pushed the aws-credentials-prompts branch from cdddb2c to 9dec9df Compare December 6, 2018 01:02
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 6, 2018
@wking wking force-pushed the aws-credentials-prompts branch from 9dec9df to 3359da5 Compare December 6, 2018 01:24
@openshift-ci-robot openshift-ci-robot added 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. labels Dec 6, 2018
@wking wking force-pushed the aws-credentials-prompts branch from 3359da5 to 4aee114 Compare December 6, 2018 04:16
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 6, 2018
@wking
Copy link
Member Author

wking commented Dec 6, 2018

I've pushed 3359da54a -> 4aee114, rebasing onto master so GitHub just shows the one commit.

For folks who forgot to setup ~/.aws/credentials before calling the
installer.  Now we'll give them a nice prompt and a link into the AWS
docs instead of dying.  I'm also saving any provided creds into the
usual location for later AWS access during this and future installer
runs.

I'm constructing my own provider chain so I can drop the
RemoteCredProvider which github.com/aws/aws-sdk-go/aws/defaults wants
to add.  I'm not entirely clear on what that's about (maybe [1]?), but
it was hanging occasionally in my local testing.

[1]: https://aws.amazon.com/blogs/security/how-to-eliminate-the-need-for-hardcoded-aws-credentials-in-devices-by-using-the-aws-iot-credentials-provider/
@wking
Copy link
Member Author

wking commented Dec 6, 2018

e2e-aws:

error: unable to connect to image repository registry.svc.ci.openshift.org/ci-op-5hbh92jn/stable@sha256:5eb1e9b2c4c0e4b7c2beda1372a039c44db4b694f567b91ccaf44df1a16cc6aa: Get https://registry.svc.ci.openshift.org/v2/: net/http: request canceled (Client.Timeout exceeded while awaiting headers) 

/retest

@crawford
Copy link
Contributor

crawford commented Dec 6, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, wking

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wking
Copy link
Member Author

wking commented Dec 6, 2018

Three e2e-aws errors:

fail [github.com/openshift/origin/test/extended/builds/labels.go:59]: Expected
    <bool>: false
to be true

....
failed: (1m47s) 2018-12-06T05:03:58 "[Feature:Builds][Smoke] result image should have proper labels set  S2I build from a template should create a image from \"test-s2i-build.json\" template with proper Docker labels [Suite:openshift/conformance/parallel] [Suite:openshift/smoke-4]"
fail [github.com/openshift/origin/test/extended/builds/labels.go:87]: Expected
    <bool>: false
to be true

...
failed: (1m57s) 2018-12-06T05:04:08 "[Feature:Builds][Smoke] result image should have proper labels set  Docker build from a template should create a image from \"test-docker-build.json\" template with proper Docker labels [Suite:openshift/conformance/parallel] [Suite:openshift/smoke-4]"

and:

fail [github.com/openshift/origin/test/extended/builds/new_app.go:63]: Expected error:
    <*errors.errorString | 0xc420590340>: {
        s: "The build \"a234567890123456789012345678901234567890123456789012345678-1\" status is \"Failed\"",
    }
    The build "a234567890123456789012345678901234567890123456789012345678-1" status is "Failed"
not to have occurred

...
failed: (2m4s) 2018-12-06T05:12:02 "[Feature:Builds][Conformance] oc new-app  should succeed with a --name of 58 characters [Suite:openshift/conformance/parallel/minimal] [Suite:openshift/smoke-4]"

/retest

@wking
Copy link
Member Author

wking commented Dec 6, 2018

Hrm, same failures as last time:


Failing tests:

[Feature:Builds][Conformance] oc new-app  should succeed with a --name of 58 characters [Suite:openshift/conformance/parallel/minimal] [Suite:openshift/smoke-4]
[Feature:Builds][Smoke] result image should have proper labels set  Docker build from a template should create a image from "test-docker-build.json" template with proper Docker labels [Suite:openshift/conformance/parallel] [Suite:openshift/smoke-4]
[Feature:Builds][Smoke] result image should have proper labels set  S2I build from a template should create a image from "test-s2i-build.json" template with proper Docker labels [Suite:openshift/conformance/parallel] [Suite:openshift/smoke-4]

/retest

@wking wking mentioned this pull request Dec 6, 2018
@crawford
Copy link
Contributor

crawford commented Dec 6, 2018

E1206 07:28:33.675471     398 streamwatcher.go:109] Unable to decode an event from the watch stream: http2: server sent GOAWAY and closed the connection; LastStreamID=5, ErrCode=NO_ERROR, debug=""
error: watch closed before Until timeout
error openshift-ingress/deploy/router-default did not come up
Waiting for deployment "router-default" rollout to finish: 0 of 1 updated replicas are available...
E1206 07:31:08.615712     417 streamwatcher.go:109] Unable to decode an event from the watch stream: net/http: request canceled (Client.Timeout exceeded while reading body)
error: watch closed before Until timeout
error openshift-ingress/deploy/router-default did not come up
sleep: invalid option -- '3'
Try 'sleep --help' for more information.
2018/12/06 07:31:09 Container test in pod e2e-aws failed, exit code 1, reason Error

/retest

@wking
Copy link
Member Author

wking commented Dec 6, 2018

sleep: invalid option -- '3'

openshift/release#2321

@jeremyeder
Copy link

Here are my notes and the logs that you requested on slack:
https://gist.github.com/jeremyeder/041b1414711a6442775d494144f08ad7

@wking
Copy link
Member Author

wking commented Dec 7, 2018

/retest

@openshift-merge-robot openshift-merge-robot merged commit 4dcb3bf into openshift:master Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants