Broker should extract credentials from secret #555

djzager · 2017-11-15T21:10:38Z

This change makes it so the broker can handle secrets that are created
by APBs when using the asb_encode_binding module.

Update the broker so that it can handle secrets generated by the APB
when asb_encode_binding module is used from the asb-modules.
Update executor::ExecuteApb to wait for pod to complete, since the
pod is no longer kept alive for credential extraction.
Clean up some of the log messages and code format related to apb
actions.

Fixes #544
Fixes #553
Implements the proposal #550

Depends on the following PRs:

ansible-asb-modules#8 This is how the secret gets generated.
apb-base#7 Remove scripts related to extracting credentials from the containers filesystem.
ansible-playbook-bundle#163 Bump the APB versions so freshly built APBs will pass version validation checks.

coveralls · 2017-11-15T22:00:04Z

Changes Unknown when pulling f25784d on djzager:bind-creds into ** on openshift:master**.

coveralls · 2017-11-15T22:02:09Z

Changes Unknown when pulling f25784d on djzager:bind-creds into ** on openshift:master**.

coveralls · 2017-11-16T14:40:00Z

Changes Unknown when pulling bda09b1 on djzager:bind-creds into ** on openshift:master**.

rthallisey · 2017-11-16T14:46:57Z

Note to reviewers: This will fail CI until all the deps are merged.

dymurray · 2017-11-16T15:10:02Z

I'm a little concerned with bumping the APB spec version. The versioning doc explains how that version should be bumped. It is meant to track changes to the spec file itself and not meant to track dependency changes.

I've raised the concern with David but I cannot think of a better way to track dependency changes that will break broker functionality unless we strive to be backwards compatible. In that case I would like to see the broker support the old mechanism of execing in if the secret doesn't exist or simply read from the secret if we are using the newest asb_encode_binding.

jmrodri · 2017-11-16T16:02:53Z

@dymurray I tend to agree with you that the version is for the spec. This PR isn't changing the spec definition.

jmrodri · 2017-11-16T16:05:05Z

At one point I suggested we remain backwards compatible, but we seemed to have dropped that for some reason. @djzager do you remember why we stopped being compatible?

djzager · 2017-11-16T16:05:32Z

The lesson learned here for me is that I should have included @dymurray on the original proposal. My bad.

djzager · 2017-11-16T16:07:54Z

The reason we stopped the backwards compatibility train @jmrodri was @shawn-hurley wise intervention about 1-off if statements that change behavior and I think that remains true. I tend to agree with @dymurray's point(s), we'll have to do a meaningful rework of the ExecuteApb architecture and flow to handle backwards compatibility correctly. (Call me out if anything I just said was stupid).

jmrodri · 2017-11-16T16:10:26Z

@djzager ok i'm not afraid of some if statements, unless of course they are EVERYWHERE. Then let's re-evaluate the ExecuteApb then to see how to best handle this new requirement. I seriously think we're going to piss people off if we don't allow old APBs to work. Thanks for the summary.

dymurray · 2017-11-16T16:32:03Z

Based on comments in scrum I think this would be a good approach:

Keep APB spec version at 1.0.
Add a new label to the Dockerfile for apb-base titled apb-base-version and set it to 1.0.
In the broker, add a new check during spec bootstrap to check apb-base version
If apb-base version exists and is 1.0 then we can assume it has the newer asb_encode_binding module and will look for a secret for the bind creds
if apb-base version does not exist then assume it doesn't have the newer asb_encode_binding module and revert to execing into the pod

shawn-hurley · 2017-11-16T18:11:25Z

@jmrodri What I was very concerned with was that for now, 2-3 if statements are not bad but will be hard to maintain in the future. If we did not have to support backwards compatibility my point was we should not complicate the code.

I agree that we should support backwards compatibility after talking to everyone.

I think that we should make it so that future changes to this code path are not fraught with code paths that suddenly change drastically with an if statement deep in the function. I find this just as hard to figure out what is going on as the interface redirection of other projects.

All: I think the concept that we choose to go with here should be applied to the multiple spec version parsing.

coveralls · 2017-11-20T15:04:00Z

Changes Unknown when pulling c700798 on djzager:bind-creds into ** on openshift:master**.

coveralls · 2017-11-21T15:53:46Z

Changes Unknown when pulling 072490e on djzager:bind-creds into ** on openshift:master**.

coveralls · 2017-11-21T22:06:13Z

Changes Unknown when pulling ab5dcc3 on djzager:bind-creds into ** on openshift:master**.

coveralls · 2017-11-22T04:29:35Z

Changes Unknown when pulling 0069ffb on djzager:bind-creds into ** on openshift:master**.

djzager · 2017-11-22T16:47:58Z

@dymurray @shawn-hurley @jmrodri @eriknelson

A bunch of changes have been made for the sake of 1) introducing a new APB runtime version (now at version 2) and 2) providing backwards compatibility between the 2 runtime versions, specifically with respect to secrets. I still need to update the proposal document to line up with what was actually done but I've included below my method for testing.

Testing

Setup

NOTE Started with catasb to bring up cluster.

Build and tag the image:

$ make build-image
$ docker tag docker.io/ansibleplaybookbundle/origin-ansible-service-broker:latest \
   docker.io/djzager/origin-ansible-service-broker:latest

Update the broker-config to include djzager org:

$ oc edit configmap broker-config
+      - type: "dockerhub"
+        name: "dz"
+        url: "docker.io"
+        org: "djzager"
+        tag: "latest"
+        white_list:
+          - ".*rhscl-postgresql-apb$"

Update the broker deployment config to deploy the new broker

$ oc edit dc asb
-        image: docker.io/ansibleplaybookbundle/origin-ansible-service-broker:latest
+        image: docker.io/djzager/origin-ansible-service-broker:latest

Test

Provision PostgreSQL into oldproject
Provision MediaWiki into oldproject
Bind PostgreSQL to MediaWiki in oldproject
Verify MediaWiki is installed correctly
Unbind PostgreSQL
Deprovision PostgreSQL && MediaWiki
Provision PostgreSQL V2 (from djzager) into newproject
Provision MediaWiki into newproject
Bind PostgreSQL V2 to MediaWiki in newproject
Verify MediaWiki is installed correctly
Unbind PostgreSQL V2
Deprovision PostgreSQL V2 && MediaWiki --> Noticing an issue with deprovision, investigating.
😎

shawn-hurley

I think that we should continue to use the pattern of trying to fit methods declarations on 1 line, if you have to break it up because it goes beyond ~95 characters then we break it up on the last parameter. I thought that is what we had all decided to go with moving forward?

shawn-hurley · 2017-12-01T14:19:15Z

pkg/apb/ext_creds.go

+	podname string,
+	namespace string,
+	log *logging.Logger,
+	k8s *clients.KubernetesClient,


Why are we passing in the k8s client? I think the right thing to do here is getting it from the method.

Fair point.

I understand this to mean why pass it when you can just ask for a reference to the client with k8s, err := clients.Kubernetes(log). Is that right?

yes, that is exactly what I was thinking

I hate it that I agree with you b/c then I have to change it. 😎

I'm going to wait to push this change until there are thoughts on the method declarations.

I'm fine with either. I leave it up to the writer to choose as long as the function wouldn't normally fit on one line.

I don't think we need to block on this since we haven't really established a style here. And we would have to change it everywhere to match the style we go with. We can discuss in a separate issue if we want to pursue it.

djzager · 2017-12-01T14:52:30Z

@shawn-hurley I've got the feeling that I missed that discussion about breaking up method declarations.

Before I start, let me just say this...I may be a little OCD (just ask me about scratches on my sunglasses) and may need to just get over myself. Personally, I would prefer either break them up in the way I had done it or shove it all on one line regardless of length. I say that because one dangling parameter + return list + { makes me a sad 🐼

I'll do whatever is agreed on.

I need to read the actual go style guide.
We should probably document where we diverge from that style so newcomers to the project can use that as reference.

Should have done this on the first try: adding @eriknelson @jmrodri @rthallisey since I trust they'll have an opinion.

coveralls · 2017-12-01T15:02:23Z

Changes Unknown when pulling 55cf1cc on djzager:bind-creds into ** on openshift:master**.

rthallisey · 2017-12-01T16:20:25Z

I'm fine with either. I leave it up to the writer to choose as long as the function wouldn't normally fit
on one line.

I don't think we need to block on this since we haven't really established a style here. And we would
have to change it everywhere to match the style we go with. We can discuss in a separate issue if
we want to pursue it.

Wrote my comments in the wrong place.

coveralls · 2017-12-01T16:54:44Z

Changes Unknown when pulling e14bdda on djzager:bind-creds into ** on openshift:master**.

coveralls · 2017-12-04T14:57:12Z

Changes Unknown when pulling 91fd234 on djzager:bind-creds into ** on openshift:master**.

This change makes it so the broker can handle secrets that are created by APBs when using the `asb_encode_binding` module. - Update the broker so that it can handle secrets generated by the APB when `asb_encode_binding` module is used from the asb-modules. - Update `executor::ExecuteApb` to wait for pod to complete, since the pod is no longer kept alive for credential extraction. - Clean up some of the log messages and code format related to apb actions. Fixes openshift#544 Fixes openshift#553 Implements the proposal openshift#550 Depends on the following PRs: - [ansible-asb-modules#8](ansibleplaybookbundle/ansible-asb-modules#8) This is how the secret gets generated. - [apb-base#7](ansibleplaybookbundle/apb-base#7) Remove scripts related to extracting credentials from the containers filesystem. - [ansible-playbook-bundle#163](ansibleplaybookbundle/ansible-playbook-bundle#163) Bump the APB versions so freshly built APBs will pass version validation checks.

Update the broker to evaluate the `com.redhat.apb.runtime` label on APBs (default to `1` when there is no label). Add version checking of this new min/max apb runtime version and update associated tests.

Update apb execution to be handle multiple apb runtime versions.

Secrets retrieved using client-go are already decoded. So the extract credentials function has been updated to only attempt to decode the credentials if they need it. Also changed the `log.Error` when the APB completed to a `log.Notice` since it is not a failure.

coveralls · 2017-12-04T17:01:23Z

Changes Unknown when pulling 34fe506 on djzager:bind-creds into ** on openshift:master**.

coveralls · 2017-12-04T18:22:15Z

Changes Unknown when pulling 34fe506 on djzager:bind-creds into ** on openshift:master**.

rthallisey · 2017-12-04T18:32:25Z

scripts/broker-ci/setup-cluster.sh

@@ -13,6 +13,7 @@ function cluster-setup () {
 dockerhub_org: ansibleplaybookbundle
 broker_tag: latest
 broker_kind: ClusterServiceBroker
+apbtab: canary
 EOF


You can remove this. It doesn't get used by the gate.

* Broker should extract credentials from secret This change makes it so the broker can handle secrets that are created by APBs when using the `asb_encode_binding` module. - Update the broker so that it can handle secrets generated by the APB when `asb_encode_binding` module is used from the asb-modules. - Update `executor::ExecuteApb` to wait for pod to complete, since the pod is no longer kept alive for credential extraction. - Clean up some of the log messages and code format related to apb actions. Fixes openshift#544 Fixes openshift#553 Implements the proposal openshift#550 Depends on the following PRs: - [ansible-asb-modules#8](ansibleplaybookbundle/ansible-asb-modules#8) This is how the secret gets generated. - [apb-base#7](ansibleplaybookbundle/apb-base#7) Remove scripts related to extracting credentials from the containers filesystem. - [ansible-playbook-bundle#163](ansibleplaybookbundle/ansible-playbook-bundle#163) Bump the APB versions so freshly built APBs will pass version validation checks. * Add APB runtime version to our APB Spec object Update the broker to evaluate the `com.redhat.apb.runtime` label on APBs (default to `1` when there is no label). Add version checking of this new min/max apb runtime version and update associated tests. * Handle bind credential extraction based on runtime Update apb execution to be handle multiple apb runtime versions. * Fixing log statements * Only decode bind creds if encoded Secrets retrieved using client-go are already decoded. So the extract credentials function has been updated to only attempt to decode the credentials if they need it. Also changed the `log.Error` when the APB completed to a `log.Notice` since it is not a failure. * Pull k8s API call to get pod status into k8s client * Cleanup extract credentials based on comments * Update proposal based on what is implemented * Improve adapter log info on apb runtime eval * Fixes after rebase * Address review comments and fix unit tests * Ask for the k8s client when we need it * Use canary APB images

djzager added 3.8 do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. work-in-progress labels Nov 15, 2017

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

djzager force-pushed the bind-creds branch 2 times, most recently from 6d94c42 to f25784d Compare November 15, 2017 21:17

djzager changed the title ~~Bind creds~~ Broker should extract credentials from secret Nov 15, 2017

djzager force-pushed the bind-creds branch from f25784d to bda09b1 Compare November 16, 2017 13:58

djzager added needs-review and removed work-in-progress labels Nov 16, 2017

djzager force-pushed the bind-creds branch from bda09b1 to c700798 Compare November 20, 2017 14:21

djzager force-pushed the bind-creds branch from 072490e to ab5dcc3 Compare November 21, 2017 21:37

djzager force-pushed the bind-creds branch from 0069ffb to e5d681b Compare November 22, 2017 16:17

shawn-hurley requested changes Dec 1, 2017

View reviewed changes

eriknelson approved these changes Dec 1, 2017

View reviewed changes

shawn-hurley approved these changes Dec 1, 2017

View reviewed changes

djzager removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Dec 1, 2017

djzager added 12 commits December 4, 2017 11:18

Add APB runtime version to our APB Spec object

8af1418

Update the broker to evaluate the `com.redhat.apb.runtime` label on APBs (default to `1` when there is no label). Add version checking of this new min/max apb runtime version and update associated tests.

Handle bind credential extraction based on runtime

4a7b7c2

Update apb execution to be handle multiple apb runtime versions.

Fixing log statements

52847f2

Pull k8s API call to get pod status into k8s client

a3cfd5d

Cleanup extract credentials based on comments

2a09c62

Update proposal based on what is implemented

4e6fd31

Improve adapter log info on apb runtime eval

57e3191

Fixes after rebase

72c3316

Address review comments and fix unit tests

6cc870d

Ask for the k8s client when we need it

b4b655e

djzager force-pushed the bind-creds branch 2 times, most recently from 2a5c0f2 to 34fe506 Compare December 4, 2017 16:31

rthallisey reviewed Dec 4, 2017

View reviewed changes

Use canary APB images

66572b5

djzager force-pushed the bind-creds branch from 34fe506 to 66572b5 Compare December 4, 2017 18:33

rthallisey merged commit 8502d47 into openshift:master Dec 4, 2017

Broker should extract credentials from secret #555

Broker should extract credentials from secret #555

djzager commented Nov 15, 2017 •

edited

coveralls commented Nov 15, 2017

coveralls commented Nov 15, 2017

coveralls commented Nov 16, 2017

rthallisey commented Nov 16, 2017

dymurray commented Nov 16, 2017

jmrodri commented Nov 16, 2017

jmrodri commented Nov 16, 2017

djzager commented Nov 16, 2017

djzager commented Nov 16, 2017

jmrodri commented Nov 16, 2017

dymurray commented Nov 16, 2017

shawn-hurley commented Nov 16, 2017

coveralls commented Nov 20, 2017

coveralls commented Nov 21, 2017

coveralls commented Nov 21, 2017

coveralls commented Nov 22, 2017

djzager commented Nov 22, 2017 •

edited

shawn-hurley left a comment

shawn-hurley Dec 1, 2017

djzager Dec 1, 2017

djzager Dec 1, 2017 •

edited

shawn-hurley Dec 1, 2017

djzager Dec 1, 2017

djzager Dec 1, 2017

rthallisey Dec 1, 2017

rthallisey Dec 1, 2017

djzager commented Dec 1, 2017 •

edited

coveralls commented Dec 1, 2017

rthallisey commented Dec 1, 2017 •

edited

coveralls commented Dec 1, 2017 •

edited

coveralls commented Dec 4, 2017 •

edited

coveralls commented Dec 4, 2017

coveralls commented Dec 4, 2017

rthallisey Dec 4, 2017

Broker should extract credentials from secret #555

Broker should extract credentials from secret #555

Conversation

djzager commented Nov 15, 2017 • edited

coveralls commented Nov 15, 2017

coveralls commented Nov 15, 2017

coveralls commented Nov 16, 2017

rthallisey commented Nov 16, 2017

dymurray commented Nov 16, 2017

jmrodri commented Nov 16, 2017

jmrodri commented Nov 16, 2017

djzager commented Nov 16, 2017

djzager commented Nov 16, 2017

jmrodri commented Nov 16, 2017

dymurray commented Nov 16, 2017

shawn-hurley commented Nov 16, 2017

coveralls commented Nov 20, 2017

coveralls commented Nov 21, 2017

coveralls commented Nov 21, 2017

coveralls commented Nov 22, 2017

djzager commented Nov 22, 2017 • edited

Testing

Setup

Test

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djzager Dec 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djzager commented Dec 1, 2017 • edited

coveralls commented Dec 1, 2017

rthallisey commented Dec 1, 2017 • edited

coveralls commented Dec 1, 2017 • edited

coveralls commented Dec 4, 2017 • edited

coveralls commented Dec 4, 2017

coveralls commented Dec 4, 2017

Choose a reason for hiding this comment

djzager commented Nov 15, 2017 •

edited

djzager commented Nov 22, 2017 •

edited

djzager Dec 1, 2017 •

edited

djzager commented Dec 1, 2017 •

edited

rthallisey commented Dec 1, 2017 •

edited

coveralls commented Dec 1, 2017 •

edited

coveralls commented Dec 4, 2017 •

edited