-
Notifications
You must be signed in to change notification settings - Fork 1.5k
pkg/asset/installconfig/aws: Make base-domain a select widget #939
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
pkg/asset/installconfig/aws: Make base-domain a select widget #939
Conversation
|
The select widget looks like: $ AWS_PROFILE=openshift-dev openshift-install --dir=wking create install-config
? SSH Public Key <none>
? Platform aws
? Region us-west-1
? Base Domain [Use arrows to move, type to filter, ? for more help]
akamra-ocp.com
> devcluster.openshift.com
mhicks-openshift.com |
0ba0181 to
ce66da6
Compare
|
So in the no-creds case, would you like me to fall back to the free-form input instead of prompting for creds? Or should I ask the user to choose between entering creds and the free-form input? |
ce66da6 to
68368e7
Compare
|
@abhinavdahiya as much as I would also prefer that, we are seeing too many failures due to this being free-form. I think this is okay for now. We may need to allow for free-form input in cases where we can't find valid credentials. @wking what happens if we find multiple sets of credentials? |
|
/approve |
I don't think that's possible. The AWS logic we're using to load credentials probably stops at the first set it finds (the same way it works for the |
This used to be covered in the docs from fa7c9e3 (*: Copy route53 baseDomain advice to openshift-install locations, 2018-09-27, openshift#353), but in order to see those you'd need to have a moment of doubt and think to hit '?'. Even if you read the docs, it's possible you'd typo the base domain or add a trailing period (theoretically trailing periods would be fine, but they may have some issues at the moment [1]). With this commit, we go ahead and fetch available public zones ourselves, so AWS users don't have to. And it also reduces the help noise on the base-domain input for users targeting non-AWS platforms. The empty struct map is slightly more efficient than a boolean map, because the empty struct takes up no space [2]. Although it's hard to imagine an account with enough public zones for that space savings to be significant. The IsForbidden handling lets us fall back to the free-form input if we aren't authorized to list zones for the select widget: $ openshift-install --dir=wking create install-config ? SSH Public Key <none> ? Platform aws ? Region us-west-1 ERROR list hosted zones: AccessDenied: User: arn:aws:iam::...:user/trking is not authorized to perform: route53:ListHostedZones with an explicit deny status code: 403, request id: 1d..29 ? Base Domain [? for help] [1]: openshift#831 (comment) [2]: https://dave.cheney.net/2014/03/25/the-empty-struct
68368e7 to
390cf77
Compare
I've pushed 68368e7 -> 390cf77 to address this. If you restrict your user's privileges (thanks @cuppett for the hint): $ cat <<EOF >policy
{
"Version": "2012-10-17",
"Statement": [
{
"Sid": "VisualEditor0",
"Effect": "Deny",
"Action": "route53:ListHostedZones",
"Resource": "*"
}
]
}
EOF
$ aws iam create-policy --policy-name deny-list-hosted-zones --policy-document file://policy
{
"Policy": {
"PolicyName": "deny-list-hosted-zones",
"CreateDate": "2018-12-19T21:21:29Z",
"AttachmentCount": 0,
"IsAttachable": true,
"PolicyId": "AN...LS",
"DefaultVersionId": "v1",
"Path": "/",
"Arn": "arn:aws:iam::...:policy/deny-list-hosted-zones",
"UpdateDate": "2018-12-19T21:21:29Z"
}
}
$ aws iam attach-user-policy --user-name trking --policy-arn arn:aws:iam::...:policy/deny-list-hosted-zonesyou can see the fallback in action: $ openshift-install --dir=wking create install-config
? SSH Public Key <none>
? Platform aws
? Region us-west-1
ERROR list hosted zones: AccessDenied: User: arn:aws:iam::...:user/trking is not authorized to perform: route53:ListHostedZones with an explicit deny
status code: 403, request id: 1d..29
? Base Domain [? for help] |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, crawford, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixing a typo from 390cf77 (pkg/asset/installconfig/aws: Make base-domain a select widget, 2018-12-18, #939). Marker is the *previous* value [1]. [1]: https://godoc.org/github.com/aws/aws-sdk-go/service/route53#ListHostedZonesOutput
This used to be covered in the docs from fa7c9e3 (#353), but in order to see those you'd need to have a moment of doubt and think to hit
?. Even if you read the docs, it's possible you'd typo the base domain or add a trailing period (theoretically trailing periods would be fine, but they may have some issues at the moment).With this commit, we go ahead and fetch available public zones ourselves, so AWS users don't have to. And it also reduces the help noise on the base-domain input for users targeting non-AWS platforms.