Skip to content
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

Pass ssh keys from clusterconfig to machineconfig #164

Merged

Conversation

kikisdeliveryservice
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice commented Nov 10, 2018

Closes openshift/installer#578

Summary of issue: the ClusterConfig has the SSH key and while that key is being passed into the MCO, the MCO isn't properly adding it to the MachineConfig. This prevents us from being able add functionality to MCD updating existing SSH keys in a MachineConfig's Spec.Config.Passwd.Users.

Comments & feedback welcome!
cc: @abhinavdahiya @wking

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2018
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 10, 2018
@kikisdeliveryservice
Copy link
Contributor Author

/assign @abhinavdahiya

@kikisdeliveryservice
Copy link
Contributor Author

The best that I can tell is that the sshkeys need to move this way to make it to the MachineConfig: installerConfig(Admin.SSHKey)-> MCOConfig --> RenderConfig/ControllerConfig --> MachineConfig (Passwd.Users)

Is this flow correct? I've traced through quite a few times, but in need of expertise on this. Can someone PTAL and let me know if my logic is correct?
cc: @ashcrow @abhinavdahiya @wking

@abhinavdahiya
Copy link
Contributor

The best that I can tell is that the sshkeys need to move this way to make it to the MachineConfig: installerConfig(Admin.SSHKey)-> MCOConfig --> RenderConfig/ControllerConfig --> MachineConfig (Passwd.Users)

Yes, that looks like state of the art right now.

@jlebon
Copy link
Member

jlebon commented Nov 20, 2018

Looks like this needs a rebase.

@kikisdeliveryservice
Copy link
Contributor Author

Rebased! @jlebon :)

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Looks sane at a high level (lots of debug print statements left over I see :)). Just one comment.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 29, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2018
@openshift-bot
Copy link
Contributor

@kikisdeliveryservice: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2018
@kikisdeliveryservice
Copy link
Contributor Author

/retest

@kikisdeliveryservice
Copy link
Contributor Author

Update:
Confirming that the SSH Key made it all the way from installerConfig(Admin.SSHKey)-> MCOConfig --> RenderConfig/ControllerConfig --> MachineConfig (Passwd.Users).

Now checking the machineconfigs for both master and worker shows (example)

$ oc get machineconfig 00-worker  -o yaml
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  creationTimestamp: 2018-12-04T01:07:56Z
  generation: 1
  labels:
    machineconfiguration.openshift.io/role: worker
  name: 00-worker
  ownerReferences:
  - apiVersion: machineconfiguration.openshift.io/v1
    blockOwnerDeletion: true
    controller: true
    kind: ControllerConfig
    name: machine-config-controller
    uid: fae0efad-f760-11e8-b3cc-42f92e967bf5
  resourceVersion: "23232"
  selfLink: /apis/machineconfiguration.openshift.io/v1/machineconfigs/00-worker
  uid: 088750c9-f761-11e8-b3cc-42f92e967bf5
spec:
  config:
    ignition:
      config: {}
      security:
        tls: {}
      timeouts: {}
      version: 2.2.0
    networkd: {}
    passwd:
      users:
      - name: core
        sshAuthorizedKeys:
        - |
          ssh-rsa ABC123....

@kikisdeliveryservice
Copy link
Contributor Author

kikisdeliveryservice commented Dec 4, 2018

However, I am seeing the daemon degrade and I'm not sure if this is this a product of when and how I'm applying my binaries and their timing or if I also need to allow reconcilable to account for this initialization of the configs... I'm going to try to test this with better timing tomorrow (assuming I can get a cluster up again) as I think the mcd got a headstart on the mco/mcc when I applied my binaries. I also put a sketch of what I might add to reconcilable if necessary, but it's untested.

This is the last thing needed to wrap up the PR.

$ oc logs -f -n openshift-machine-config-operator machine-config-daemon-289tf
I1204 01:30:42.968291    5400 start.go:51] Version: 3.11.0-301-g13e15273
I1204 01:30:42.968962    5400 start.go:88] starting node writer
I1204 01:30:42.974021    5400 run.go:22] Running captured: chroot /rootfs rpm-ostree status --json
I1204 01:30:43.054368    5400 daemon.go:120] Booted osImageURL: registry.svc.ci.openshift.org/rhcos/maipo@sha256:e4b05527d762ba159c821d53bc8b2478c38b717b0fcfb4b76c57787c2f46ee2c (47.177)
I1204 01:30:43.072829    5400 start.go:139] Calling chroot("/rootfs")
I1204 01:30:43.072944    5400 daemon.go:294] CheckStateOnBoot
I1204 01:30:43.085725    5400 update.go:87] Checking if configs are reconcilable!
I1204 01:30:43.085837    5400 update.go:100] Old Config: {[] []}
I1204 01:30:43.085911    5400 update.go:101] New Config: {[] []}
I1204 01:30:43.085947    5400 daemon.go:565] No target osImageURL provided
I1204 01:30:43.095044    5400 start.go:158] Starting MachineConfigDaemon
I1204 01:30:43.095059    5400 daemon.go:195] Enabling Kubelet Healthz Monitor
I1204 01:30:52.359987    5400 daemon.go:371] handleNodeUpdate
I1204 01:30:52.629908    5400 daemon.go:371] handleNodeUpdate
I1204 01:33:09.537006    5400 update.go:33] Updating node with new config
I1204 01:33:09.538536    5400 update.go:87] Checking if configs are reconcilable!
I1204 01:33:09.538767    5400 update.go:100] Old Config: {[] []}
I1204 01:33:09.538859    5400 update.go:101] New Config: {[] [{<nil>  []  core false false false <nil>  [ssh-rsa ABC123]  false <nil>}]}
W1204 01:33:09.538950    5400 update.go:133] daemon can't reconcile state!
W1204 01:33:09.538990    5400 update.go:134] Ignition passwd section contains changes
I1204 01:33:09.539039    5400 daemon.go:385] Unable to apply update: daemon can't reconcile this config
I1204 01:33:09.539076    5400 daemon.go:371] handleNodeUpdate
E1204 01:33:09.539286    5400 writer.go:85] Marking degraded due to: daemon can't reconcile this config
F1204 01:33:09.548695    5400 start.go:163] failed to run: daemon can't reconcile this config

@kikisdeliveryservice
Copy link
Contributor Author

cc @ashcrow @abhinavdahiya (some updates and questions above)

@ashcrow
Copy link
Member

ashcrow commented Dec 4, 2018

@kikisdeliveryservice it looks like the reconcilable needs updating to allow for a different Password.User section: https://github.com/openshift/machine-config-operator/pull/164/files#diff-06961b075f1753956d802ba954d2cfb5R133

pkg/operator/sync.go Outdated Show resolved Hide resolved
@kikisdeliveryservice
Copy link
Contributor Author

@ashcrow @abhinavdahiya Was definitely planning on squashing, but wanted to get a review settled before I removed all of my commits. :)

Taking a look at the comments now, thank you!

@kikisdeliveryservice kikisdeliveryservice force-pushed the pass-mco-keys branch 3 times, most recently from 03900f5 to bf658e8 Compare January 3, 2019 23:41
@yuqi-zhang
Copy link
Contributor

/retest

@kikisdeliveryservice
Copy link
Contributor Author

kikisdeliveryservice commented Jan 4, 2019

@abhinavdahiya is this what you had in mind re: creating the ssh mc for each role: new commit

Don't worry I'll squash this commit too once it's all settled 😇

@ashcrow
Copy link
Member

ashcrow commented Jan 4, 2019

/test e2e-aws

@ashcrow
Copy link
Member

ashcrow commented Jan 4, 2019

/retest

@ashcrow
Copy link
Member

ashcrow commented Jan 4, 2019

/test e2e-aws

@ashcrow
Copy link
Member

ashcrow commented Jan 4, 2019

Flakes. Unable to pull images.

@abhinavdahiya
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 4, 2019
@abhinavdahiya
Copy link
Contributor

/retest

allow the installer to use the mco to distribute ssh keys. this
is the first step in adding the ability to update existing ssh keys.

closes installer issue openshift#578
@kikisdeliveryservice
Copy link
Contributor Author

FYI: just squashed my last commit in since I got @abhinavdahiya 's final review. :)

@kikisdeliveryservice
Copy link
Contributor Author

@ashcrow I know you already approved this, so could I get a LGTM so this can be merged? Tests aren't flaking anymore.

@ashcrow
Copy link
Member

ashcrow commented Jan 7, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, ashcrow, kikisdeliveryservice

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:
  • OWNERS [abhinavdahiya,ashcrow]

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

@openshift-merge-robot openshift-merge-robot merged commit 7773bac into openshift:master Jan 7, 2019
@kikisdeliveryservice kikisdeliveryservice deleted the pass-mco-keys branch January 7, 2019 17:44
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants