Skip to content

Conversation

@abhinavdahiya
Copy link
Contributor

The original feature of syncing the ssh key for authorized users from cluster-config-v1 config map was added in [1]. The original assumption was
that as cluster-config-v1 was being deprecated in favor of global configs [2] and we will be able to find new home for the source of authorized keys for core user.

Based on several discussions like [3], The better UX for users is that machineconfig with authorized keys is not owned by the controller and is rather pushed by the installer at install time.
This allows users to edit the machineconfig directly to remove, add and manage the list of authorized keys.

This requires openshift/installer#1150

/cc @kikisdeliveryservice @crawford @cgwalters

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 30, 2019
@ashcrow
Copy link
Member

ashcrow commented Jan 30, 2019

level=fatal msg="waiting for openshift-console URL: context deadline exceeded"

/test e2e-aws-op

@kikisdeliveryservice
Copy link
Contributor

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 30, 2019
@runcom
Copy link
Member

runcom commented Mar 14, 2019

needs to be rebased as per our conversation on slack - we'll get this in to avoid a migration post release

@runcom
Copy link
Member

runcom commented Mar 14, 2019

/hold cancel

we're waiting on #548 as well before this anyway

@openshift-ci-robot 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 14, 2019
@kikisdeliveryservice kikisdeliveryservice removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2019
@runcom
Copy link
Member

runcom commented Mar 15, 2019

alrighty, this is good to be rebased reviewed and merged

@kikisdeliveryservice
Copy link
Contributor

i do think #541 should go in before this

@runcom
Copy link
Member

runcom commented Mar 23, 2019

#541 went it, @abhinavdahiya can you rebase this so we can review and merge?

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 26, 2019
@abhinavdahiya
Copy link
Contributor Author

#541 went it, @abhinavdahiya can you rebase this so we can review and merge?

@runcom PTAL

@runcom
Copy link
Member

runcom commented Mar 26, 2019

so nice to see this (cc @kikisdeliveryservice )

=== RUN   TestUpdateSSH
--- PASS: TestUpdateSSH (230.63s)

/approve

@kikisdeliveryservice @cgwalters ptal for merging

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2019
@cgwalters
Copy link
Member

LGTM

Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

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

The documentation section is out of date and needs to be updated to reflect current output.

Also, I dont fully understand the flow. If a user makes a new sshkeys config, the 99-worker-ssh will be updated and a new rendered-worker-XXXXX config will be created? or will the 99-worker-ssh NOT be updated and just a new rendered worker config is generated?

I'd like this to be clear in the docs

@cgwalters
Copy link
Member

or will the 99-worker-ssh NOT be updated

AIUI it's more like "99-worker-ssh is created by the installer if a SSH key was provided to it" - after that it's user editable (including deletion). There's no "internal controller" for it like there are for the 00-* configs etc.

The user could create separately named SSH MCs too - the keys would then be unioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

does this sentence make sense anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

The original feature of syncing the ssh key for authorized users from `cluster-config-v1` config map was added in [1]. The original assumption was
that as `cluster-config-v1` was being deprecated in favor of global configs [2] and we will be able to find new home for the source of authorized keys for `core` user.

Based on several discussions like [3], The better UX for users is that machineconfig with authorized keys is not owned by the controller and is rather pushed by the installer at install time.
This allows users to edit the machineconfig directly to remove, add and manage the list of authorized keys.

[1]: openshift@a46fcad
[2]: openshift/installer#680
[3]: https://projects.engineering.redhat.com/browse/COREOS-1059
@kikisdeliveryservice
Copy link
Contributor

It's time! 👍

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [kikisdeliveryservice,runcom]

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

@abhinavdahiya
Copy link
Contributor Author

/hold
would need openshift/installer#1150 to go in so that we are not left without SSH keys being setup.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2019
@abhinavdahiya
Copy link
Contributor Author

openshift/installer#1150 merged
/hold cancel

@openshift-ci-robot 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
@abhinavdahiya
Copy link
Contributor Author

/retest

@abhinavdahiya
Copy link
Contributor Author

Hmm... haven't seen that before :/

could not wait for build: the build machine-config-controller failed after 3m16s with reason PushImageToRegistryFailed: Failed to push the image to the registry.

Registry server Address: 
Registry server User Name: serviceaccount
Registry server Email: [email protected]
Registry server Password: <<non-empty>>
error: build error: Failed to push image: Put https://dock...t canceled (Client.Timeout exceeded while awaiting headers)

/retest

@openshift-merge-robot openshift-merge-robot merged commit 76be890 into openshift:master Mar 28, 2019
@runcom
Copy link
Member

runcom commented Apr 1, 2019

looks like this may have broken upgrades (investigating with QE) - might have a clue related to the "generated by controller version" and the check the MCO does on that field.

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants