machines: add the authorized keys for a pool using a machine config #1150

abhinavdahiya · 2019-01-30T01:36:18Z

cluster-config-v1 is being deprecated in favor of global configs (#680) and Machine Config Operator needs to drop using the SSHKey in install-config to setup the SSHAuthorizedKeys for core user.

This pushes a machineconfig with the SSHAuthorizedKeys sourced from here for each machinepool, so that Machine Config Operator can drop generating the machineconfig using the cluster-config-v1 config map in the cluster.

/cc @crawford

cgwalters · 2019-01-30T15:52:19Z

Can you elaborate in the commit message why we're making this change, what problem it's solving?

This is basically the inverse of the existing MCO ssh key support - the installer uses its own config to inject MC objects, rather than the MCO scraping the install config and generating MC fragments. Are we going forward with both of these approaches?

wking · 2019-01-30T16:29:26Z

Can you elaborate in the commit message why we're making this change, what problem it's solving?

It would be good to mention #680 and cluster-config-v1 deprecation.

abhinavdahiya · 2019-01-30T18:33:03Z

Can you elaborate in the commit message why we're making this change, what problem it's solving?

with 5c4dac6 -> 87f5c3d

i have tried to add the why.

Also created openshift/machine-config-operator#356 that drops creating machineconfig with ssh key

runcom · 2019-03-25T12:46:39Z

@abhinavdahiya can you rebase this so we can get it in along with openshift/machine-config-operator#356

sdodson · 2019-03-25T14:22:08Z

Lets make sure we summarize the why in the PR description before this merges. Even if that's just a copy of the non vendor commit.

wking · 2019-03-25T14:59:02Z

I've updated the PR topic post.

abhinavdahiya · 2019-03-25T22:59:06Z

5412e88 ... 3ad4777 is extracted from #1392 modified-local-branch to reuse the plumbing that we need to push machineconfig for hyperthreading...

/cc @staebler

Users can push manifests during bootstrap that of the form: ```yaml --- ``` Especially for the installer: setting authorizes_keys [1] and setting hyperthreading [2] will push a manifest that includes multiple machineconfig objects for control-plane (master) and compute (worker) roles. Single file with multiple k8s objects separated by `---` is also a supported structure for `oc create|apply` ie. there is a high chance that users trying to push machineconfigs at install time might create such files. This commit allows bootstrap controller to read all k8s objects, even ones described above to find all the `machineconfiguration.openshift.io` Objects. [1]: openshift/installer#1150 [2]: openshift/installer#1392

abhinavdahiya · 2019-03-26T00:44:34Z

@abhinavdahiya can you rebase this so we can get it in along with openshift/machine-config-operator#356

@runcom PTAL

cgwalters · 2019-03-26T14:30:26Z

This LGTM. (I debated some bikeshedding on the 99-%s-ssh naming but eh, not worth it)

This PR should land first - in that case we'll have two things injecting SSH (the installer's MCs and the MCO), and then we land openshift/machine-config-operator#356 to bring us back down to 1.

/approve

pkg/asset/machines/machineconfig/authorizedkeys.go

wking · 2019-03-27T00:00:44Z

pkg/asset/manifests/openshift.go

@@ -135,6 +133,7 @@ func (o *Openshift) Generate(dependencies asset.Parents) error {
 			Data:     data,
 		})
 	}
+	o.FileList = append(o.FileList, worker.Files()...)


This makes me jumpy about reloading/regenerating these when they're loaded from disk during a multi-stage install. Can we move Worker out from under Openshift into it's own writeable asset (like we did for masters in 7a396d9, #1211)? CC @staebler

This makes me jumpy about reloading/regenerating these when they're loaded from disk during a multi-stage install. Can we move Worker out from under Openshift into it's own writeable asset (like we did for masters in 7a396d9, #1211)? CC @staebler

master's we use as input to cluster asset and that's why it makes sense to split that out in #1211 that worker machinesets are opaque to creating infra resources for now and I don't think we would need to do something like #1211 for the time being.

... worker machinesets are opaque to creating infra resources for now...

So should we stop writing them at all? Drop them from Openshift without making them writable assets? This is getting into #1119.

So should we stop writing them at all?

why, people can edit the manifest as a file. doesn't mean we don't write them 😕

why, people can edit the manifest as a file. doesn't mean we don't write them confused

Because then we won't have structured info about chosen worker availability zones and such. I'll work up some test-cases tonight.

whatever we decide on that ^^ , I don't think it should be part of this PR.

pkg/asset/machines/master.go

pkg/asset/machines/machineconfig/manifest.go

abhinavdahiya · 2019-03-27T05:23:39Z

Ok, I've left a few more comments inline. I'm ok landing this as it stands if you don't share my concerns or are ok carrying them as technical debt, so I'll set this up for you to admit when you feel comfortable:

replied to the comments. PTAL @wking
#1150 (comment) frankly does not belong in this PR.

wking · 2019-03-27T07:59:05Z

e2e-aws:

Flaky tests:

[Feature:DeploymentConfig] deploymentconfigs when run iteratively [Conformance] should only deploy the last deployment [Suite:openshift/conformance/parallel/minimal]

Failing tests:

[sig-storage] In-tree Volumes [Driver: nfs] [Testpattern: Dynamic PV (default fs)] subPath should be able to unmount after the subpath directory is deleted [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] In-tree Volumes [Driver: nfs] [Testpattern: Inline-volume (default fs)] subPath should be able to unmount after the subpath directory is deleted [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] In-tree Volumes [Driver: nfs] [Testpattern: Pre-provisioned PV (default fs)] subPath should be able to unmount after the subpath directory is deleted [Suite:openshift/conformance/parallel] [Suite:k8s]

/lgtm
/retest

wking · 2019-03-27T08:26:04Z

Failing tests are openshift/origin#22412, and may be happening every time.

wking · 2019-03-27T19:00:37Z

/hold cancel

Needs a rebase, though

Vendor openshift/machine-config-operator to have access to the MachineConfig type. The MachineConfig type is used to send supplement the ignition config for machine pools that have hyperthreading enabled. This also adds some more packages from coreos/ignition.

Provide the means by which a machines asset can add MachineConfig manifests. This is needed so that the ignition configs can be supplemented for machine pools for example have hyperthreading disabled, include authorized_keys for user.

…machinepool This sets up the master machine asset to allow adding MachineConfigs. A list of machine configs can be added to master machines that will be created alongside the user-data and machine objects.

…epools This sets up the worker machine asset to allow adding MachineConfigs. A list of machine configs can be added to compute machinepools that will be created alongside the user-data and machineset objects.

`cluster-config-v1` is being deprecated in favor of global configs [1] and Machine Config Operator needs to drop using the `SSHKey` in install-config [2] to setup the `SSHAuthorizedKeys` for `core` user. This pushes a machineconfig with the `SSHAuthorizedKeys` sourced from [2] for each machinepool, so that Machine Config Operator can drop generating the machineconfig using the `cluster-config-v1` config map in the cluster. [1]: openshift#680 [2]: https://godoc.org/github.com/openshift/installer/pkg/types#InstallConfig

abhinavdahiya · 2019-03-27T19:03:22Z

/hold cancel

Needs a rebase, though

done.

wking · 2019-03-27T19:05:28Z

/lgtm

openshift-ci-robot · 2019-03-27T19:05:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, cgwalters, 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:

~~OWNERS~~ [abhinavdahiya,wking]

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

abhinavdahiya · 2019-03-27T21:13:23Z

Failing tests:

[sig-apps] StatefulSet [k8s.io] Basic StatefulSet functionality [StatefulSetBasic] should not deadlock when a pod's predecessor fails [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] Dynamic Provisioning DynamicProvisioner should provision storage with different parameters [Suite:openshift/conformance/parallel] [Suite:k8s]

/retest

Since f2eacf3 (asset/machines/master: allow adding MachineConfigs for control-plane machinepool, 2019-03-25, openshift#1150), we no longer need to filet a File slice to get a Master object's Machine files. Moving the method to a stand-alone function sets the stage for structured worker Machine objects. f2eacf3 also removed the only (*Master).Machines() consumer, so remove that method completely.

wking · 2019-03-27T22:21:15Z

e2e-aws:

level=error msg="\t* aws_internet_gateway.igw: error attaching EC2 Internet Gateway (igw-07d40ca7af759b41a): timeout while waiting for state to become 'success' (timeout: 2m0s)"

/retest

…hines Since f2eacf3 (asset/machines/master: allow adding MachineConfigs for control-plane machinepool, 2019-03-25, openshift#1150), we no longer need to filter a File slice to get a Master object's Machine files. Drop the obsoleted Machines() implemenation and rename the previous StructuredMachines implmentation to take its place.

openshift-ci-robot requested a review from crawford January 30, 2019 01:36

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 30, 2019

abhinavdahiya mentioned this pull request Jan 30, 2019

operator: remove the generated ssh key machineconfig openshift/machine-config-operator#356

Merged

abhinavdahiya force-pushed the ssh_mc branch from 8a9ccdc to bbf866c Compare January 30, 2019 18:30

abhinavdahiya mentioned this pull request Jan 30, 2019

Support providing MachineConfig objects directly #1144

Closed

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2019

wking mentioned this pull request Feb 4, 2019

pkg/start/asset: Add support for post-pod-manifests openshift/cluster-bootstrap#13

Closed

abhinavdahiya force-pushed the ssh_mc branch from bbf866c to 362106c Compare March 25, 2019 22:57

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2019

openshift-ci-robot requested a review from staebler March 25, 2019 22:59

This was referenced Mar 26, 2019

controller/bootstrap: use files with multiple yaml documents openshift/machine-config-operator#577

Merged

types: option to disable hyperthreading #1392

Merged

wking reviewed Mar 26, 2019

View reviewed changes

pkg/asset/machines/machineconfig/authorizedkeys.go Outdated Show resolved Hide resolved

wking reviewed Mar 26, 2019

View reviewed changes

pkg/asset/machines/machineconfig/authorizedkeys.go Outdated Show resolved Hide resolved

wking reviewed Mar 27, 2019

View reviewed changes

abhinavdahiya force-pushed the ssh_mc branch from 362106c to 8967ccd Compare March 27, 2019 00:49

wking reviewed Mar 27, 2019

View reviewed changes

pkg/asset/machines/master.go Show resolved Hide resolved

wking reviewed Mar 27, 2019

View reviewed changes

pkg/asset/machines/master.go Outdated Show resolved Hide resolved

wking reviewed Mar 27, 2019

View reviewed changes

pkg/asset/machines/machineconfig/manifest.go Show resolved Hide resolved

openshift-ci-robot assigned wking Mar 27, 2019

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 27, 2019

abhinavdahiya force-pushed the ssh_mc branch from 8967ccd to c1ae013 Compare March 27, 2019 04:55

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019

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

staebler and others added 5 commits March 27, 2019 12:01

asset/machines: add manifests for MachineConfig

8683b8e

Provide the means by which a machines asset can add MachineConfig manifests. This is needed so that the ignition configs can be supplemented for machine pools for example have hyperthreading disabled, include authorized_keys for user.

asset/machines/master: allow adding MachineConfigs for control-plane …

f2eacf3

…machinepool This sets up the master machine asset to allow adding MachineConfigs. A list of machine configs can be added to master machines that will be created alongside the user-data and machine objects.

asset/machines/worker: allow adding MachineConfigs for compute machin…

e4d5ec9

…epools This sets up the worker machine asset to allow adding MachineConfigs. A list of machine configs can be added to compute machinepools that will be created alongside the user-data and machineset objects.

abhinavdahiya force-pushed the ssh_mc branch from c1ae013 to 5623cbd Compare March 27, 2019 19:03

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019

wking mentioned this pull request Mar 27, 2019

pkg/asset/machines/worker: Structured workers #1481

Merged

openshift-merge-robot merged commit 6a50329 into openshift:master Mar 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

machines: add the authorized keys for a pool using a machine config #1150

machines: add the authorized keys for a pool using a machine config #1150

abhinavdahiya commented Jan 30, 2019 •

edited by wking

Loading

cgwalters commented Jan 30, 2019

wking commented Jan 30, 2019

abhinavdahiya commented Jan 30, 2019

runcom commented Mar 25, 2019

sdodson commented Mar 25, 2019 •

edited

Loading

wking commented Mar 25, 2019

abhinavdahiya commented Mar 25, 2019 •

edited

Loading

abhinavdahiya commented Mar 26, 2019

cgwalters commented Mar 26, 2019

wking Mar 27, 2019

abhinavdahiya Mar 27, 2019

wking Mar 27, 2019 •

edited

Loading

abhinavdahiya Mar 27, 2019

wking Mar 27, 2019

abhinavdahiya Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

wking commented Mar 27, 2019

wking commented Mar 27, 2019

wking commented Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

wking commented Mar 27, 2019

openshift-ci-robot commented Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

wking commented Mar 27, 2019

machines: add the authorized keys for a pool using a machine config #1150

machines: add the authorized keys for a pool using a machine config #1150

Conversation

abhinavdahiya commented Jan 30, 2019 • edited by wking Loading

cgwalters commented Jan 30, 2019

wking commented Jan 30, 2019

abhinavdahiya commented Jan 30, 2019

runcom commented Mar 25, 2019

sdodson commented Mar 25, 2019 • edited Loading

wking commented Mar 25, 2019

abhinavdahiya commented Mar 25, 2019 • edited Loading

abhinavdahiya commented Mar 26, 2019

cgwalters commented Mar 26, 2019

wking Mar 27, 2019

Choose a reason for hiding this comment

abhinavdahiya Mar 27, 2019

Choose a reason for hiding this comment

wking Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

abhinavdahiya Mar 27, 2019

Choose a reason for hiding this comment

wking Mar 27, 2019

Choose a reason for hiding this comment

abhinavdahiya Mar 27, 2019

Choose a reason for hiding this comment

abhinavdahiya commented Mar 27, 2019

wking commented Mar 27, 2019

wking commented Mar 27, 2019

wking commented Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

wking commented Mar 27, 2019

openshift-ci-robot commented Mar 27, 2019

abhinavdahiya commented Mar 27, 2019

wking commented Mar 27, 2019

abhinavdahiya commented Jan 30, 2019 •

edited by wking

Loading

sdodson commented Mar 25, 2019 •

edited

Loading

abhinavdahiya commented Mar 25, 2019 •

edited

Loading

wking Mar 27, 2019 •

edited

Loading