Bug 1507617 - Adding SSL and Authentication to etcd #522

shawn-hurley · 2017-10-30T17:57:23Z

Describe what this PR does and why we need it:
adds SSL and auth to etcd
Changes proposed in this pull request

move etcd to its own pod
add SSL for etcd
add cert auth for etcd.
remove support for 3.6 in run_latest_build.sh

Does this PR depend on another PR (Use this to track when PRs should be merged)
fusor/catasb#173
Also, this will need to be released to latest the changes to run_latest_build.sh
Which issue this PR fixes (This will close that issue when PR gets merged)
fixes <issue_number>
BZ#1507617
fixes #514

rthallisey · 2017-10-30T19:12:06Z

templates/deploy-ansible-service-broker.template.yaml

+  value: /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
+- description: Value of the etcd server ca file
+  displayname: etcd trusted ca
+  name: ETCD_TRUESTED_CA


Should ETCD_TRUESTED_CA be ETCD_TRUSTED_CA?

rthallisey · 2017-10-30T19:12:16Z

templates/deploy-ansible-service-broker.template.yaml

+  displayname: etcd trusted ca file path
+  name: BROKER_ETCD_TRUSTED_CA
+  value: /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
+- description: Value of the etcd server ca file


Add a new line

fabianvf · 2017-10-30T20:05:50Z

scripts/run_latest_build.sh

+  -p ETCD_TRUSTED_CA_FILE=/var/run/etcd-auth-secret/ca.crt \
+  -p BROKER_CLIENT_CERT_PATH=/var/run/asb-etcd-auth/client.crt \
+  -p BROKER_CLIENT_KEY_PATH=/var/run/asb-etcd-auth/client.key \
+  -p ETCD_TRUESTED_CA="$ETCD_CA_CERT" \


s/TRUESTED/TRUSTED/

fabianvf · 2017-10-30T20:06:45Z

templates/deploy-ansible-service-broker.template.yaml

+    name: etcd-auth-secret
+    namespace: ansible-service-broker
+  data:
+    ca.crt: ${ETCD_TRUESTED_CA}


s/TRUESTED/TRUSTED/

djzager

I was holding it wrong, testing correctly this time.

Before I realized how wrong I was holding it

After making the changes to scripts/run_latest_build.sh that allows me to use a local template, the broker does not come up.

➜  ansible-service-broker git:(shurley-etcd-secure) ✗ oc get pods
NAME               READY     STATUS             RESTARTS   AGE
asb-1-deploy       1/1       Running            0          1m
asb-1-jf96c        0/1       CrashLoopBackOff   3          1m
asb-etcd-1-dg697   1/1       Running            0          1m
➜  ansible-service-broker git:(shurley-etcd-secure) ✗ oc get pods
NAME               READY     STATUS             RESTARTS   AGE
asb-1-deploy       1/1       Running            0          1m
asb-1-jf96c        0/1       CrashLoopBackOff   4          1m
asb-etcd-1-dg697   1/1       Running            0          1m
➜  ansible-service-broker git:(shurley-etcd-secure) ✗ oc logs asb-1-jf96c
Using config file mounted to /etc/ansible-service-broker/config.yaml
============================================================
==           Starting Ansible Service Broker...           ==
============================================================
[2017-10-31T13:45:36.053Z] [NOTICE] Initializing clients...
[2017-10-31T13:45:36.053Z] [DEBUG] Trying to connect to etcd
[2017-10-31T13:45:36.053Z] [INFO] == ETCD CX ==
[2017-10-31T13:45:36.053Z] [INFO] EtcdHost: asb-etcd.ansible-service-broker.svc
[2017-10-31T13:45:36.053Z] [INFO] EtcdPort: 2379
[2017-10-31T13:45:36.053Z] [INFO] Endpoints: [http://asb-etcd.ansible-service-broker.svc:2379]
[2017-10-31T13:45:36.056Z] [ERROR] client: etcd cluster is unavailable or misconfigured; error #0: malformed HTTP response "\x15\x03\x01\x00\x02\x02"

I suspect I'm holding it wrong.

djzager · 2017-10-31T13:30:38Z

scripts/run_latest_build.sh

@@ -74,23 +74,32 @@ oc new-project ansible-service-broker
 #
 TEMPLATE_URL="https://raw.githubusercontent.com/openshift/ansible-service-broker/master/templates/deploy-ansible-service-broker.template.yaml"


You don't have to make this change..but I would appreciate it, make this value override-able TEMPLATE_URL=${TEMPLATE_URL:-"..."}.

@djzager You will need to use the broker image from the PR when running run_latest

Yeah. That is what I was holding wrong 😎. Still playing with it.

djzager

Tested with run_latest_build.sh + make prepare-local-env + make run and it was running as I would expect.

shawn-hurley · 2017-10-31T16:51:45Z

Please note that we should ignore the gate error here as this PR will require the fusor/catasb#173 to be merged to go to green.

rthallisey · 2017-11-01T14:02:33Z

templates/deploy-ansible-service-broker.template.yaml

@@ -101,7 +120,7 @@ objects:
    selector:
      app: ansible-service-broker
    strategy:
-      type: Recreate
+      type: Rolling


I think we want to leave this as recreate. See #511

Rolling will be better now that we don't have to worry about the PV that etcd is using.

Just some additional notes:

I tested redeploying the broker and etcd 👍

I tested scaling up the broker 👍

I tested scaling up etcd 👎, this is because the replicated pod thinks it's a master...and it is not. I do not think this is something this PR should be concerned with. Dealing with etcd replicas is a pain and the whole reason I think CoreOS came out with the idea of operators. Just wanted to point it out.

this is because the replicated pod thinks it's a master...and it is not. I do not think this is something this PR should be concerned with. Dealing with etcd replicas is a pain and the whole reason I think CoreOS came out with the idea of operators. Just wanted to point it out.

Agree, I could see adding a trello card to make this work better.

rthallisey · 2017-11-01T14:13:57Z

templates/deploy-ansible-service-broker.template.yaml

@@ -215,8 +292,11 @@ objects:
          white_list:
            - ".*-apb$"
      dao:
-        etcd_host: 0.0.0.0
+        etcd_host: asb-etcd.${NAMESPACE}.svc


Why do you need .${NAMESPACE}.svc?

Using ${NAMESPACE} will allow helios broker to pass the aws-service-broker namespace and use the same config. I did not go back and fix every instance but don't see why we can't start now. I can remove it if you feel strongly. also you need ${namespace}.svc for the internal DNS. The signed cert (generated from the openshift annotation) are valid for certain DNS names, I think the valid ones IIRC are

.{namespace}.svc

.{namespace}.cluster.local

.{namespace}.local

If you are just looking the service, you could drop the {namespace.svc} because the DNS will assume you are looking in that namespace but we need SSL to work.

At least that is my understanding of what is going on.

irc discussion: https://docs.openshift.com/online/dev_guide/secrets.html#service-serving-certificate-secrets

eriknelson

VISIACK -- LGTM, although I haven't gotten a chance to test it myself. Sounds like folks are having success.

jmrodri

Acceptable :)

* etcd is now run in its own pod

…ld.sh

shawn-hurley · 2017-11-02T01:19:49Z

Travis is having issues with this PR.

* adding ability to use etcd athenticated * etcd is now run in its own pod * fixing run latest build to use etcd authentication * removing parameters that are not needed * fixing local dev scripts to work with new deployment * typos * allow for the overriding of the deployment-template in run_latest_build.sh * fixing build issue. * fixing deploy script hopefully * fixing gate * trying to remove new line from certs * triming all withspace. * actaully setting the correct value * remove debug statements.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 30, 2017

shawn-hurley force-pushed the etcd-secure branch 2 times, most recently from 2bfd959 to 40d2e95 Compare October 30, 2017 18:40

shawn-hurley added bug needs-review labels Oct 30, 2017

shawn-hurley mentioned this pull request Oct 30, 2017

Changing the default for auto escalate to false #503

Merged

jmrodri requested review from jmrodri, eriknelson and djzager October 30, 2017 19:09

rthallisey reviewed Oct 30, 2017

View reviewed changes

fabianvf reviewed Oct 30, 2017

View reviewed changes

djzager requested changes Oct 31, 2017

View reviewed changes

djzager approved these changes Oct 31, 2017

View reviewed changes

rthallisey suggested changes Nov 1, 2017

View reviewed changes

fabianvf mentioned this pull request Nov 1, 2017

Bug 1507617- Move etcd into its own service/dc with SSL openshift/openshift-ansible#5976

Merged

rthallisey approved these changes Nov 1, 2017

View reviewed changes

eriknelson approved these changes Nov 1, 2017

View reviewed changes

jmrodri approved these changes Nov 1, 2017

View reviewed changes

shawn-hurley and others added 8 commits November 1, 2017 20:14

adding ability to use etcd athenticated

a4315a0

* etcd is now run in its own pod

fixing run latest build to use etcd authentication

d3c0b62

removing parameters that are not needed

1e715bc

fixing local dev scripts to work with new deployment

937150d

typos

175bea7

allow for the overriding of the deployment-template in run_latest_bui…

7050596

…ld.sh

fixing build issue.

a45a22f

fixing deploy script hopefully

77198b5

shawn-hurley force-pushed the etcd-secure branch from 44a42e7 to 77198b5 Compare November 2, 2017 00:16

shawn-hurley added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 2, 2017

shawn-hurley removed the needs-review label Nov 2, 2017

shawn-hurley added 5 commits November 2, 2017 08:25

fixing gate

c9ebd65

trying to remove new line from certs

01b4951

triming all withspace.

ed90a61

actaully setting the correct value

fd8439e

remove debug statements.

ab84848

shawn-hurley merged commit ddb9887 into openshift:master Nov 2, 2017

Bug 1507617 - Adding SSL and Authentication to etcd #522

Bug 1507617 - Adding SSL and Authentication to etcd #522

shawn-hurley commented Oct 30, 2017 •

edited

rthallisey Oct 30, 2017

rthallisey Oct 30, 2017

fabianvf Oct 30, 2017

fabianvf Oct 30, 2017

djzager left a comment •

edited

djzager Oct 31, 2017

shawn-hurley Oct 31, 2017

djzager Oct 31, 2017

djzager left a comment

shawn-hurley commented Oct 31, 2017

rthallisey Nov 1, 2017

shawn-hurley Nov 1, 2017

djzager Nov 1, 2017

shawn-hurley Nov 1, 2017

rthallisey Nov 1, 2017

shawn-hurley Nov 1, 2017 •

edited

rthallisey Nov 1, 2017

eriknelson left a comment

jmrodri left a comment

shawn-hurley commented Nov 2, 2017

		@@ -74,23 +74,32 @@ oc new-project ansible-service-broker
		#
		TEMPLATE_URL="https://raw.githubusercontent.com/openshift/ansible-service-broker/master/templates/deploy-ansible-service-broker.template.yaml"

Bug 1507617 - Adding SSL and Authentication to etcd #522

Bug 1507617 - Adding SSL and Authentication to etcd #522

Conversation

shawn-hurley commented Oct 30, 2017 • edited

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

djzager left a comment • edited

Choose a reason for hiding this comment

Before I realized how wrong I was holding it

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djzager left a comment

Choose a reason for hiding this comment

shawn-hurley commented Oct 31, 2017

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

shawn-hurley Nov 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson left a comment

Choose a reason for hiding this comment

jmrodri left a comment

Choose a reason for hiding this comment

shawn-hurley commented Nov 2, 2017

shawn-hurley commented Oct 30, 2017 •

edited

djzager left a comment •

edited

shawn-hurley Nov 1, 2017 •

edited