refactor csr approvals #9711

michaelgugino · 2018-08-22T00:05:38Z

Currently, csr approval process for nodes is quite
fragile.

This commit creates a new custom module oc_csr_client
which facilitates handling the multiple steps involved
for approving pending node certificates.

The module attempts to approve all 'client' csrs
for any nodes provided via node_list, missing csrs
are ignored as long as the missing node is in a
'Ready' status as reported by oc get nodes.

Next, the module approves csrs for 'server' certificates.
Similar to the client process, missing node csrs
are acceptable as long as the node's api endpoint
is reachable without error, indicating a server
certificate is deployed.

In cases of long delay between issuing a csr and
csr approval, there may be several outstanding
'server' csrs. This module will approve any
outstanding csrs.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1571515

vrutkovs · 2018-08-22T13:44:03Z

roles/lib_openshift/library/oc_csr_client.py

+def get_ready_nodes(module, oc_bin, oc_conf):
+    '''Get list of nodes currently ready vi oc'''
+    # plain output is way less to parse.
+    command = "{} {} get nodes".format(oc_bin, oc_conf)


An overengineered version would be:
oc get nodes -o jsonpath="{range .items[*]}{@.metadata.name}:{range @.status.conditions[*]}{@.type}={@.status};{end}{end}" to output smth like
ip-172-18-14-252.ec2.internal:OutOfDisk=False;MemoryPressure=False;DiskPressure=False;PIDPressure=False;Ready=True;
split it and filter out lines with Ready=True.

That's incomprehensible and probably just as much splitting and filtering.

Going back to past discussions where David had stated that oc output is not guaranteed stable I think we should either get yaml output or a template like Vadim is suggesting. Perhaps not as complicated, but it still needs to be a format that we're sure of so if they change column ordering we're not busted.

michaelgugino

I have run this in a few different iterations against a local cluster (removed certs of a node, restarted), though not on a fresh cluster. Hopefully CI passes :)

michaelgugino · 2018-08-22T22:16:11Z

roles/lib_openshift/library/oc_csr_client.py

+
+short_description: Retrieve, approve, and verify node client csrs
+
+version_added: "2.4"


michaelgugino · 2018-08-22T22:16:30Z

roles/lib_openshift/library/oc_csr_client.py

+from ansible.module_utils.basic import AnsibleModule
+
+try:
+    from json.decoder import JSONDecodeError


python2 vs python 3.4+

michaelgugino · 2018-08-22T22:16:47Z

roles/lib_openshift/library/oc_csr_client.py

+  oc_csr_client:
+    oc_bin: "/usr/bin/oc"
+    oc_conf: "/etc/origin/master/admin.kubeconfig"
+    node_list: "['node1.example.com', 'node2.example.com']"


Remove double quotes on this line.

michaelgugino · 2018-08-22T22:17:32Z

roles/lib_openshift/library/t.json

@@ -0,0 +1,6 @@
+{"ANSIBLE_MODULE_ARGS": {


Remove this file.

michaelgugino · 2018-08-22T22:18:08Z

roles/lib_openshift/library/oc_csr_client.py

@@ -0,0 +1,288 @@
+#!/usr/bin/env python
+# pylint: disable=missing-docstring


Remove this line and add some docstrings :)

michaelgugino · 2018-08-22T22:18:17Z

roles/lib_openshift/library/oc_csr_client.py

+
+DOCUMENTATION = '''
+---
+module: oc_csr_client


Rename module?

Yeah, I'm not sure what we should name it though. All the other oc modules actually map pretty much 1:1 with oc foo bar cli interactions.

michaelgugino · 2018-08-22T22:19:21Z

roles/lib_openshift/library/oc_csr_client.py

+        supports_check_mode=False,
+        argument_spec=module_args
+    )
+    oc_bin = module.params['oc_bin']


This stuff gets passed around a lot. Probably should refactor into a class object, this will help will reporting during failures.

sdodson

looks like it's probably functionally correct, just some nits about module name and defaults.

sdodson · 2018-08-23T17:13:04Z

playbooks/openshift-node/private/join.yml

+  - name: Approve node certificates when bootstrapping
+    oc_csr_client:
+      oc_bin: "{{ openshift_client_binary }}"
+      oc_conf: "{{ openshift.common.config_base }}/master/admin.kubeconfig"


If we're naming this module oc_ I'd expect it to default this like all the other oc modules do.

sdodson · 2018-08-23T17:14:06Z

playbooks/openshift-node/private/join.yml

    when:
    - l_nodes_to_join|length > 0

-  - when: approve_out is failed


Unless we're sure this is 100% lets leave in debug log generation.

This is no longer necessary. No module provides actually useful failure information.

ok sounds good

sdodson · 2018-08-23T17:15:44Z

roles/lib_openshift/library/oc_csr_client.py

+
+DOCUMENTATION = '''
+---
+module: oc_csr_client


Yeah, I'm not sure what we should name it though. All the other oc modules actually map pretty much 1:1 with oc foo bar cli interactions.

sdodson · 2018-08-23T17:20:52Z

roles/lib_openshift/library/oc_csr_client.py

+def get_ready_nodes(module, oc_bin, oc_conf):
+    '''Get list of nodes currently ready vi oc'''
+    # plain output is way less to parse.
+    command = "{} {} get nodes".format(oc_bin, oc_conf)


Going back to past discussions where David had stated that oc output is not guaranteed stable I think we should either get yaml output or a template like Vadim is suggesting. Perhaps not as complicated, but it still needs to be a format that we're sure of so if they change column ordering we're not busted.

Currently, csr approval process for nodes is quite fragile. This commit creates a new custom module oc_csr_approve which facilitates handling the multiple steps involved for approving pending node certificates. The module attempts to approve all 'client' csrs for any nodes provided via node_list, missing csrs are ignored as long as the missing node is in a 'Ready' status as reported by oc get nodes. Next, the module approves csrs for 'server' certificates. Similar to the client process, missing node csrs are acceptable as long as the node's api endpoint is reachable without error, indicating a server certificate is deployed. In cases of long delay between issuing a csr and csr approval, there may be several outstanding 'server' csrs. This module will approve any outstanding csrs. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1571515

vrutkovs

/lgtm

Worked perfectly here

michaelgugino · 2018-08-24T14:11:25Z

/test install

openshift-ci-robot · 2018-08-24T14:25:36Z

@michaelgugino: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/gcp-crio	`fef0430`	link	`/test gcp-crio`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

sdodson · 2018-08-24T15:35:44Z

May require tweaking of the retries and time but we can figure that out later.
/lgtm

openshift-ci-robot · 2018-08-24T15:35:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michaelgugino, sdodson, vrutkovs

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~~ [michaelgugino,sdodson,vrutkovs]

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

sdodson · 2018-08-24T15:36:02Z

gcp-crio was an e2e flake.

michaelgugino · 2018-08-24T15:56:19Z

/cherrypick release-3.10

openshift-cherrypick-robot · 2018-08-24T15:56:27Z

@michaelgugino: #9711 failed to apply on top of branch "release-3.10":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	roles/openshift_node/tasks/upgrade.yml
Falling back to patching base and 3-way merge...
Auto-merging roles/openshift_node/tasks/upgrade.yml
CONFLICT (content): Merge conflict in roles/openshift_node/tasks/upgrade.yml
Patch failed at 0001 Refactor csr approvals: oc_csr_approve

In response to this:

/cherrypick release-3.10

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2018

openshift-ci-robot requested review from mtnbikenc and sdodson August 22, 2018 00:05

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 Aug 22, 2018

michaelgugino mentioned this pull request Aug 22, 2018

Timed out accepting certificate signing requests. #9628

Closed

vrutkovs reviewed Aug 22, 2018

View reviewed changes

michaelgugino force-pushed the csr-approve-refactor branch from 3a08096 to 19d554d Compare August 22, 2018 21:09

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2018

michaelgugino force-pushed the csr-approve-refactor branch 2 times, most recently from 76cedc7 to 45a59ee Compare August 22, 2018 22:13

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 22, 2018

michaelgugino changed the title ~~WIP: refactor csr approvals~~ refactor csr approvals Aug 22, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2018

michaelgugino commented Aug 22, 2018

View reviewed changes

michaelgugino force-pushed the csr-approve-refactor branch 2 times, most recently from a7cbcec to 1a96157 Compare August 23, 2018 02:41

sdodson removed the request for review from mtnbikenc August 23, 2018 13:21

michaelgugino force-pushed the csr-approve-refactor branch 2 times, most recently from c87511b to 37f799f Compare August 23, 2018 17:38

sdodson requested changes Aug 23, 2018

View reviewed changes

michaelgugino force-pushed the csr-approve-refactor branch 2 times, most recently from f49176d to a9c01f0 Compare August 23, 2018 20:51

michaelgugino force-pushed the csr-approve-refactor branch from a9c01f0 to fef0430 Compare August 23, 2018 21:39

vrutkovs approved these changes Aug 24, 2018

View reviewed changes

openshift-ci-robot assigned vrutkovs Aug 24, 2018

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

openshift-ci-robot assigned sdodson Aug 24, 2018

sdodson merged commit 6016dea into openshift:master Aug 24, 2018

michaelgugino mentioned this pull request Aug 24, 2018

[3.10] Refactor csr approvals: oc_csr_approve #9751

Merged

michaelgugino mentioned this pull request Sep 6, 2018

Install 3.10 no csr #9672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor csr approvals #9711

refactor csr approvals #9711

michaelgugino commented Aug 22, 2018 •

edited

Loading

vrutkovs Aug 22, 2018 •

edited

Loading

michaelgugino Aug 22, 2018

sdodson Aug 23, 2018

michaelgugino left a comment

michaelgugino Aug 22, 2018

michaelgugino Aug 22, 2018

michaelgugino Aug 22, 2018

michaelgugino Aug 22, 2018

michaelgugino Aug 22, 2018

michaelgugino Aug 22, 2018

sdodson Aug 23, 2018

michaelgugino Aug 22, 2018

sdodson left a comment

sdodson Aug 23, 2018

sdodson Aug 23, 2018

michaelgugino Aug 23, 2018

sdodson Aug 23, 2018

sdodson Aug 23, 2018

sdodson Aug 23, 2018

vrutkovs left a comment

michaelgugino commented Aug 24, 2018

openshift-ci-robot commented Aug 24, 2018 •

edited

Loading

sdodson commented Aug 24, 2018

openshift-ci-robot commented Aug 24, 2018

sdodson commented Aug 24, 2018

michaelgugino commented Aug 24, 2018

openshift-cherrypick-robot commented Aug 24, 2018


		short_description: Retrieve, approve, and verify node client csrs

		version_added: "2.4"

		@@ -0,0 +1,288 @@
		#!/usr/bin/env python
		# pylint: disable=missing-docstring

refactor csr approvals #9711

refactor csr approvals #9711

Conversation

michaelgugino commented Aug 22, 2018 • edited Loading

vrutkovs Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelgugino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdodson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrutkovs left a comment

Choose a reason for hiding this comment

michaelgugino commented Aug 24, 2018

openshift-ci-robot commented Aug 24, 2018 • edited Loading

sdodson commented Aug 24, 2018

openshift-ci-robot commented Aug 24, 2018

sdodson commented Aug 24, 2018

michaelgugino commented Aug 24, 2018

openshift-cherrypick-robot commented Aug 24, 2018

michaelgugino commented Aug 22, 2018 •

edited

Loading

vrutkovs Aug 22, 2018 •

edited

Loading

openshift-ci-robot commented Aug 24, 2018 •

edited

Loading