*: unify handling of ssh keys #127

crawford · 2018-08-14T20:22:22Z

Instead of each platform implementing their own mechanism for supplying
SSH keys, use the same method everywhere. Now, instead of using AWS's
metadata service, we inject the keys directly via Ignition.

Note: The stub Ignition configs contain the SSH keys. Eventually, the
keys will need to be moved into MCO, so that they can be rotated.

wking · 2018-08-14T20:30:32Z

installer/pkg/config/types.go

The property is optional (per your config.tf comment), so I think this should be omitempty (like the other Admin properties).

wking · 2018-08-14T20:32:35Z

steps/variables-libvirt.tf

Add default = "qemu:///system"? And add an (optional) prefix to the description?

This is a required field.

This is a required field.

But previously we'd hard-coded qemu://system in a number of places. Do we need to require it if we have a value that usually works that we can use as a default? Or is that default value not as reliable as it seemed?

I think we are going to see too many developers get hit by this if we have a silent default. OSX users, for example, will probably end up using vmware://system (or whatever the URI is). I'd rather be explicit about this for now.

wking · 2018-08-14T20:33:30Z

tests/smoke/aws/README.md

You should drop this line too.

wking · 2018-08-14T20:34:22Z

tests/smoke/aws/README.md

The new setting should be optional, so this line can grow an (optional) prefix (like the later list entries).

This URI shouldn't be hardcoded since we allow the user to change the libvirt connection URI.

crawford · 2018-08-14T23:34:42Z

retest this please

wking · 2018-08-14T23:38:01Z

tests/run.sh

$(< ...) is a Bashism. For POSIX compat, can we use $(cat ~/.ssh/id_rsa.pub) (which also replaces $HOME with POSIX-compatible ~ expansion)?

wking · 2018-08-14T23:52:42Z

The Jenkins error was:

23:39:55 4 error(s) occurred:
23:39:55 
23:39:55 * module.vpc.aws_nat_gateway.nat_gw[2]: 1 error(s) occurred:
23:39:55 
23:39:55 * aws_nat_gateway.nat_gw.2: Error creating NAT Gateway: NatGatewayLimitExceeded: Performing this operation would exceed the limit of 5 NAT gateways
23:39:55 	status code: 400, request id: 8ee48aea-49ac-4bb7-8e96-a0a8b4a74620
23:39:55 * module.vpc.aws_nat_gateway.nat_gw[0]: 1 error(s) occurred:
23:39:55 
23:39:55 * aws_nat_gateway.nat_gw.0: Error creating NAT Gateway: NatGatewayLimitExceeded: Performing this operation would exceed the limit of 5 NAT gateways
23:39:55 	status code: 400, request id: 2889206b-f069-43ed-9e4f-854ba05e2849
23:39:55 * module.vpc.aws_nat_gateway.nat_gw[1]: 1 error(s) occurred:
23:39:55 
23:39:55 * aws_nat_gateway.nat_gw.1: Error creating NAT Gateway: NatGatewayLimitExceeded: Performing this operation would exceed the limit of 5 NAT gateways
23:39:55 	status code: 400, request id: fab6d9d4-deda-408f-845d-6e7594abcc3b
23:39:55 * module.vpc.aws_elb.api_internal: 1 error(s) occurred:
23:39:55 
23:39:55 * aws_elb.api_internal: TooManyLoadBalancers: Exceeded quota of account 846518947292
23:39:55 	status code: 400, request id: 2566dfdb-a01b-11e8-8a09-0737bdcebc48

I'll work through the Jenkins account tonight and try and clean up leaked resources.

yifan-gu

/lgtm

wking · 2018-08-15T13:27:57Z

retest this please

wking · 2018-08-15T14:06:38Z

The new Jenkins error:

1 error(s) occurred:

* module.masters.aws_launch_configuration.master_conf: 1 error(s) occurred:

* aws_launch_configuration.master_conf: Error creating launch configuration: ValidationError: The key pair 'ssh-ed25519 AAAA...' does not exist
  status code: 400, request id: ff093110-a08f-11e8-9ca1-0366660da85d

looks legitamate. We may need more smoke-test changes to either generate a real test keypair, or remove the key from our smoke config.

crawford · 2018-08-15T16:46:02Z

/hold

crawford · 2018-08-15T16:47:28Z

@yifan-gu please don't lgtm PRs that other team members have commented on. That will cause tide to merge the PR.

Instead of each platform implementing their own mechanism for supplying SSH keys, use the same method everywhere. Now, instead of using AWS's metadata service, we inject the keys directly via Ignition. Note: The stub Ignition configs contain the SSH keys. Eventually, the keys will need to be moved into MCO, so that they can be rotated.

crawford · 2018-08-15T18:11:37Z

/hold cancel

yifan-gu · 2018-08-15T19:16:13Z

@yifan-gu please don't lgtm PRs that other team members have commented on. That will cause tide to merge the PR.

@crawford Ahh, sorry about that :P

yifan-gu · 2018-08-15T19:20:38Z

/lgtm

openshift-ci-robot · 2018-08-15T19:20:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, yifan-gu

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [crawford,yifan-gu]

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

The variable is used to represent the key_pair. The name doesn't match the use case. This will add confusion when sshKey is added in openshift#127.

Catching up with openshift/installer@37f623c5 (*: unify handling of ssh keys, 2018-08-14, openshift/installer#127).

37f623c (*: unify handling of ssh keys, 2018-08-14, openshift#127) replaced our old key-pair upload with the TF_VAR_tectonic_admin_ssh_key export, and updated the message from "Uploading SSH key-pair to AWS..." to our current "Generation SSH key-pair..." message. But while we used to *always* upload a key to AWS, we've only ever generated a new key if ~/.ssh/id_rsa.pub was missing. This commit moves the Generating... mesage into the if block to avoid freaking out callers who may think we're clobbering their SSH key ;). While I'm in the area, I've also dropped the SSH variable and its associated SC2034 (unused variable) disable. The output of ssh-keygen isn't particularly interesting, so I've just set -q to quiet it instead. We'd had the old SSH and SC2034 disable since the script landed in a2405e4 (run smoke tests with bash script, 2018-06-18, coreos/tectonic-installer#3284).

The variable is used to represent the key_pair. The name doesn't match the use case. This will add confusion when sshKey is added in openshift#127.

crawford added the run-smoke-tests label Aug 14, 2018

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 14, 2018

wking reviewed Aug 14, 2018

View reviewed changes

steps/tnc_dns: use the specified libvirt uri

1611a65

This URI shouldn't be hardcoded since we allow the user to change the libvirt connection URI.

wking reviewed Aug 14, 2018

View reviewed changes

openshift-ci-robot assigned yifan-gu Aug 15, 2018

yifan-gu approved these changes Aug 15, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2018

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 15, 2018

abhinavdahiya approved these changes Aug 15, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2018

openshift-merge-robot merged commit 873f609 into openshift:master Aug 15, 2018

jstuever mentioned this pull request Aug 15, 2018

*openstack: rename variable sshKey to keyPair flaper87/installer#5

Merged

wking mentioned this pull request Aug 17, 2018

cluster/test-deploy/aws/openshift: Move sshKey to the admin block openshift/release#1206

Merged

wking added a commit to wking/openshift-release that referenced this pull request Aug 17, 2018

cluster/test-deploy/aws/openshift: Move sshKey to the admin block

0b24ab0

Catching up with openshift/installer@37f623c5 (*: unify handling of ssh keys, 2018-08-14, openshift/installer#127).

This was referenced Aug 17, 2018

tests/run: Shift "Generating SSH key-pair..." message into if block #147

Merged

modules/aws: Drop auto-scaling groups (ASG) #88

Merged

crawford deleted the ssh branch August 21, 2018 16:42

wking mentioned this pull request Aug 21, 2018

libvirt-howto: fix sshKey section name #154

Merged

lucab mentioned this pull request Aug 24, 2018

Add coreos-metadata, or don't openshift/os#125

Closed

wking mentioned this pull request Aug 24, 2018

Create asset graph engine #120

Merged

abhinavdahiya mentioned this pull request Aug 27, 2018

installconfig: Replace 'KeyPairName' with 'SSHKey', and fix golint, uuid #188

Merged

wking mentioned this pull request Sep 5, 2018

default values for some fields in install_config #212

Closed

*: unify handling of ssh keys #127

*: unify handling of ssh keys #127

Uh oh!

Conversation

crawford commented Aug 14, 2018

Uh oh!

wking Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

wking Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crawford Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

wking Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

crawford Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

wking Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

wking Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

crawford commented Aug 14, 2018

Uh oh!

wking Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

wking commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yifan-gu left a comment

Choose a reason for hiding this comment

Uh oh!

wking commented Aug 15, 2018

Uh oh!

wking commented Aug 15, 2018

Uh oh!

crawford commented Aug 15, 2018

Uh oh!

crawford commented Aug 15, 2018

Uh oh!

crawford commented Aug 15, 2018

Uh oh!

yifan-gu commented Aug 15, 2018

Uh oh!

yifan-gu commented Aug 15, 2018

Uh oh!

openshift-ci-robot commented Aug 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wking Aug 14, 2018 •

edited

Loading

wking commented Aug 14, 2018 •

edited

Loading