Broker bind output rework #124

rthallisey · 2017-05-24T18:16:01Z

Change the broker bind output from trailing the logs of
the provision or bind container to exec'ing into the container.

jmrodri · 2017-05-24T18:24:04Z

oh this is going to be fun trying to merge this with my changes 😢 your PR makes sense. The changes I had to make was to move getPodName out of extractCredentials call. because I need to get the pod name before the job finishes.

jmrodri · 2017-05-24T18:25:47Z

Good work. I'll look at it probably not until later this week.

jmrodri · 2017-05-24T18:40:01Z

actually doesn't look too bad for me.

rthallisey · 2017-05-24T18:44:33Z

Testing this will require: fusor/apb-examples#25

rthallisey · 2017-06-02T19:51:31Z

@jmrodri rebased fusor/apb-examples#25. So this should be ok to test. Thx for the heads up.

jwmatthews · 2017-06-07T12:33:29Z

I'm apprehensive of merging this until we have a working broker with new service catalog.
My preference is to debug issues with broker<->service catalog, after those are resolved, test this PR and determine if we merge.
Main concern is introducing another variable into the mix when things are not working.

@rthallisey would you help me understand the benefits of this approach with oc exec and reading file opposed to parsing oc logs?

@rthallisey what problems do you see this resolving that we may have faced with the oc logs usage?

rthallisey · 2017-06-07T13:12:24Z

@jwmatthews When we send the credentials to stdout we're making the creds readable by anyone who can view that pod. If we move to exec, the pod will have to be visible and we can use the container environment to secure who can look at the credentials. That could be things like selinux, users, passwords, or tokens to read the credentials that we can't use to protect stdout.

eriknelson · 2017-06-21T01:11:42Z

I'm a little undecided on this. I'm convinced it's more secure than our current implementation given @rthallisey's argument, so even if we merged as is, there's value to be gained. On the other hand, it's not quite one of the three options I remember discussing when credential exfiltration was originally discussed. IIRC we settled on 3 options:

Some kind of pod exit status that was yet to be implemented, but soon to be available?
Broker opens a webhook, networking concerns here.
Some kind of shared volume that the broker and pod can both read/write from.

This PR isn't really any of those options. I don't think that disqualifies it though; is it a 4th option that accomplishes everything we're looking for?

Regardless of the answer, this is definitely better than what we're doing right now.

rthallisey · 2017-06-21T11:24:56Z

@eriknelson Ya it's not one of the original three, but here's my thinking.

Some kind of pod exit status that was yet to be implemented, but soon to be available?

I interpreted this as reading the pod exit status from something like an oc describe. I didn't understand this one because this seems like it would be the same as what we're doing now.

I'm going to look in the API code and see how a pod can return an error code and maybe that's what was meant here.

Broker opens a webhook, networking concerns here.

Networking concerns.

Some kind of shared volume that the broker and pod can both read/write from.

This is nearly identical to exec. In the container we don't allow anyone but the broker to read from the directory that's attached to the volume. But, now we have to create and destroy resources on the broker side. Exec doesn't need that.

jwmatthews · 2017-06-23T20:07:26Z

In regard to this PR, I'd like to chat with some of the AOS folks to run this approach by them and get feedback before we merge.

Plan to address feedback in next sprint, tracking in card: https://trello.com/c/PZJfBeph

jwmatthews · 2017-06-27T19:06:15Z

@rthallisey please rebase this when you can.

I spoke to Clayton and confirmed he's good with this approach.

rthallisey · 2017-06-28T15:09:21Z

Depends on #242

shawn-hurley · 2017-06-28T15:49:33Z

pkg/apb/ext_creds.go

@@ -14,78 +15,56 @@ var stillWaitingError = "status: still waiting to start"
 var timeoutFreq = 6    // Seconds
 var totalTimeout = 900 // 15min

-// ExtractCredentials - Extract credentials from pod in a certain namespace.


Please keep this line in. I am pretty sure this is why the build is failing.

rthallisey · 2017-06-30T16:25:22Z

@jwmatthews sounds good. It hasn't been working for me for the past few days so I'll see if I can figure out the issue.

rthallisey · 2017-07-06T16:17:36Z

I think this is good to go. @eriknelson can you help me test this? I'm having trouble getting a working UI. I've tested this with the environment I have and the bind credentials make it into etcd. After that, the service-catalog injects those from etcd into a running pod. So I'm confident this should work since I don't touch that part of the bind. But I'd like to confirm this with an already working environment.

eriknelson

Had a conversation about this with @jmrodri over lunch and the feeling is that the mechanism used for monitoring output is difficult to reason about and should be simplified. While not technically wrong; it's not easy to answer something as simple as "How long can an APB run for before it times out?" given all the retry logic between the broker, credential extraction script, and bind-init. Here's the proposed changes:

broker-bind-creds Should drop any retry logic at all and be a reasonably dumb cat script. If it's there, cat it and rm, if not, exit 1.
bind-init is effectively "how long should the pod be kept alive waiting for the broker to collect credentials". That's good as is, we probably just want to hardcode a value of 5min there.
Push most of the retry logic into the broker. Hard code retry count/interval and have the interval remain static. (Not a function of the retry count). Can be made configurable if desired in the future.

With these changes, we should be able to confidently say: The broker will attempt to extract credentials for N*interval units, and if the credentials haven't been written by that point in time, things have timed out.

Additional requested changes:

monitorOutput shouldn't be a go routine, and should return output, err, where err is the bubbled oc exec cmd error. We want to make sure err gets logged somewhere in the stack.
Destroy the APB sandbox immediately after Provision

Overall, I think the core of this accomplishes the original goal for the change, and I've seen it passing regression tests. I'd be comfortable merging this and it's apb-examples counterpart pending these changes.

eriknelson · 2017-07-12T14:15:25Z

pkg/apb/provision.go

@@ -67,6 +67,13 @@ func Provision(
 	}

 	creds, err := ExtractCredentials(podName, instance.Context.Namespace, log)
+	// We should not save credentials from an app that finds them and isn't
+	// bindable
+	if creds != nil && !instance.Spec.Bindable {


eriknelson · 2017-07-12T14:20:53Z

pkg/broker/broker.go

@@ -490,6 +475,15 @@ func (a AnsibleBroker) Provision(instanceUUID uuid.UUID, req *ProvisionRequest,
 				return nil, err
 			}
 		}
+
+		sm := apb.NewServiceAccountManager(a.log)


As written, this will leak an APB sandbox if the Dao set errors out. Need to move the destruction above the SetExtractedCredentails stanza and immediately after the apb.Provision call.

eriknelson · 2017-07-12T15:23:04Z

pkg/apb/ext_creds.go

-	var err error
-	creds, err = buildExtractedCredentials(credOut)
+	log.Debug("Calling monitorOutput on " + podname)
+	go monitorOutput(namespace, podname, credOut, log)


I don't see a reason to make this a goroutine. It spawns monitor output and then immediately blocks on a single value, so we don't gain anything. The http server is optimized to continue to accept requests concurrently, so a long monitorOutput isn't going to block anything incoming.

+1 at this time and the way we do the work I don't see the need for a go routine to monitor the output.

eriknelson · 2017-07-12T15:27:26Z

pkg/apb/ext_creds.go

-func buildExtractedCredentials(output []byte) (*ExtractedCredentials, error) {
-	stillWaiting := strings.Contains(string(output), "ContainerCreating") ||
-		strings.Contains(string(output), "NotFound")
+func monitorOutput(namespace string, podname string, mon chan []byte, log *logging.Logger) {


If this isn't going to be run as a goroutine, should remove the channel from the signature and just return (string, error), string being the output from line 60.

eriknelson · 2017-07-12T15:27:48Z

pkg/apb/ext_creds.go

+	// instead of only getting the credentials
+
+	for r := 1; r <= CredentialRetries; r++ {
+		output, _ := RunCommand("oc", "exec", podname, GatherCredentialsCMD, "--namespace="+namespace)


We shouldn't drop the error, should return it as part of the new signature.

💯 this will totally bite us down the road. I can already foresee the exec erroring out for some odd reason and not having any idea what really happened.

Here's the log if I capture err with log.Error(err.Error()) during a provision:

[2017-07-13T10:06:23.988-04:00] [ERROR] exit status 1 [2017-07-13T10:06:23.988-04:00] [WARNING] [apb-5834b548-1611-4e5a-a4ee-a799f2fa3018] Retry attempt 1: Waiting for container to start [2017-07-13T10:06:23.988-04:00] [WARNING] [apb-5834b548-1611-4e5a-a4ee-a799f2fa3018] Retry attempt 1: exec into apb-5834b548-1611-4e5a-a4ee-a799f2fa3018 failed [2017-07-13T10:06:25.614-04:00] [ERROR] exit status 1 [2017-07-13T10:06:25.614-04:00] [WARNING] [apb-5834b548-1611-4e5a-a4ee-a799f2fa3018] Retry attempt 2: Waiting for container to start [2017-07-13T10:06:25.614-04:00] [WARNING] [apb-5834b548-1611-4e5a-a4ee-a799f2fa3018] Retry attempt 2: exec into apb-5834b548-1611-4e5a-a4ee-a799f2fa3018 failed [2017-07-13T10:07:08.242-04:00] [ERROR] exit status 137 [2017-07-13T10:07:08.242-04:00] [WARNING] [apb-5834b548-1611-4e5a-a4ee-a799f2fa3018] Retry attempt 3: exec into apb-5834b548-1611-4e5a-a4ee-a799f2fa3018 failed [2017-07-13T10:07:11.669-04:00] [ERROR] exit status 1 [2017-07-13T10:07:11.669-04:00] [NOTICE] [apb-5834b548-1611-4e5a-a4ee-a799f2fa3018] APB completed [2017-07-13T10:07:11.669-04:00] [INFO] Destroying APB sandbox...

Since we combine stderr and stdout [1] into one stream, both are being pushed to output. err is the return value from the command similar to echo $?. So it's probably not a good idea to capture it as anything but info. Also, I don't want to exit on error because the retry logic is in this function. What I can do is move the retry logic out of this function and return on error.
[1] -

ansible-service-broker/pkg/apb/hack.go

Line 12 in 9f8abd0

output, err := exec.Command(cmd, args...).CombinedOutput()

I looked at moving out the retry logic and it wouldn't be as clean because some of the monitoring pieces would leak into extractCredentials.

jmrodri

Better. See @eriknelson explanation of what should change to finish this up.

meant to be request changes.

jmrodri

requesting changes

Change the broker bind output from trailing the logs of the provision or bind container to exec'ing into the container.

Deprovision was calling ExtractCredentials to get the output of oc logs into the broker logs. The broker was calling provision then ExtractCredentials. We don't need to do that since provision and bind already call it.

rthallisey · 2017-07-13T15:16:14Z

@eriknelson @jmrodri both broker-bind-output patches have been updated.

shawn-hurley

the reason the build is failing

shawn-hurley · 2017-07-13T16:30:55Z

pkg/apb/types.go

+
+	// Bind credential gathering constants
+	// 5 seconds x 60 retries = 5 minute timout
+	WaitTime             = 5


Should add comments for exported constants

eriknelson

I was able to provision images built with cooresponding apb-base changes rhscl-postresql-apb, mediawiki123-apb, and bind the two as expected. All requested changes addressed, I think this is ready to go.

fusor/apb-examples#25

eriknelson · 2017-07-13T20:22:19Z

Travis is failing due to a failing lint:

=================================
              Lint               
=================================
/home/travis/gopath/src/github.com/openshift/ansible-service-broker/pkg/apb/types.go:104:2: comment on exported const WaitTime should be of the form "WaitTime ..."
/home/travis/gopath/src/github.com/openshift/ansible-service-broker/pkg/apb/types.go:107:2: exported const CredentialRetries should have comment (or a comment on this block) or be unexported
The command "./scripts/travis.sh lint" exited with 1.

Going to merge this and immediately follow up with a fix for that. Thank you everyone!

rthallisey · 2017-11-01T16:50:39Z

#104

* Will automatically use proper relist with an APB push following a push and/or bootstrap (unless otherwise specified with --no-relist)

* Broker bind output rework Change the broker bind output from trailing the logs of the provision or bind container to exec'ing into the container. * Fix some things after the rebase * Add namespace to monitorOutput * Remove uneeded calls to ExtractCredentials Deprovision was calling ExtractCredentials to get the output of oc logs into the broker logs. The broker was calling provision then ExtractCredentials. We don't need to do that since provision and bind already call it. * Accept pod completion as success * Catch a few corner cases and improve logging * Remove go routine and add a few fixes

rthallisey mentioned this pull request May 24, 2017

[DO NOT MERGE] Exec into the container to get bind creds #60

Closed

jmrodri self-requested a review May 24, 2017 18:25

jmrodri added enhancement labels May 24, 2017

rthallisey mentioned this pull request May 24, 2017

Broker bind output fusor/apb-examples#25

Merged

jmrodri mentioned this pull request May 31, 2017

Recover jobs when broker restarted #131

Merged

jmrodri added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 2, 2017

jmrodri added feature and removed sprint5 labels Jun 9, 2017

jwmatthews added the post-tech-preview label Jun 14, 2017

eriknelson added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2017

jwmatthews changed the title ~~Broker bind output rework~~ [DO NOT MERGE] Broker bind output rework Jun 23, 2017

rthallisey removed the post-tech-preview label Jun 23, 2017

jwmatthews changed the title ~~[DO NOT MERGE] Broker bind output rework~~ Broker bind output rework Jun 27, 2017

rthallisey force-pushed the broker-bind-output branch from 8cdd532 to 6a41012 Compare June 28, 2017 15:02

shawn-hurley reviewed Jun 28, 2017

View reviewed changes

rthallisey force-pushed the broker-bind-output branch from 6a41012 to a537460 Compare June 28, 2017 15:54

rthallisey force-pushed the broker-bind-output branch from 606f1a0 to 8e25f1c Compare July 6, 2017 16:13

rthallisey force-pushed the broker-bind-output branch 2 times, most recently from e383185 to c0c1c29 Compare July 6, 2017 18:57

eriknelson suggested changes Jul 12, 2017

View reviewed changes

jmrodri previously approved these changes Jul 12, 2017

View reviewed changes

jmrodri suggested changes Jul 12, 2017

View reviewed changes

Ryan Hallisey added 6 commits July 13, 2017 09:39

Broker bind output rework

1f330dd

Change the broker bind output from trailing the logs of the provision or bind container to exec'ing into the container.

Fix some things after the rebase

a009dd0

Add namespace to monitorOutput

c8b9a1e

Remove uneeded calls to ExtractCredentials

7cc1d70

Deprovision was calling ExtractCredentials to get the output of oc logs into the broker logs. The broker was calling provision then ExtractCredentials. We don't need to do that since provision and bind already call it.

Accept pod completion as success

1184337

Catch a few corner cases and improve logging

e5ec2fe

rthallisey force-pushed the broker-bind-output branch from c0c1c29 to b8ecc27 Compare July 13, 2017 14:42

Remove go routine and add a few fixes

ec205ec

rthallisey force-pushed the broker-bind-output branch from b8ecc27 to ec205ec Compare July 13, 2017 14:47

shawn-hurley reviewed Jul 13, 2017

View reviewed changes

eriknelson changed the title ~~[DO NOT MERGE] Broker bind output rework~~ Broker bind output rework Jul 13, 2017

eriknelson approved these changes Jul 13, 2017

View reviewed changes

jmrodri approved these changes Jul 13, 2017

View reviewed changes

eriknelson merged commit 11792e5 into openshift:master Jul 13, 2017

rthallisey mentioned this pull request Jul 14, 2017

Improve the broker bind output by using error returned from RunCommand #276

Merged

rthallisey deleted the broker-bind-output branch July 27, 2017 13:13

shawn-hurley pushed a commit to shawn-hurley/ansible-service-broker that referenced this pull request Nov 2, 2017

Relist support (openshift#124)

13f8e33

* Will automatically use proper relist with an APB push following a push and/or bootstrap (unless otherwise specified with --no-relist)

Broker bind output rework #124

Broker bind output rework #124

Conversation

rthallisey commented May 24, 2017

jmrodri commented May 24, 2017 • edited

jmrodri commented May 24, 2017

jmrodri commented May 24, 2017

rthallisey commented May 24, 2017

rthallisey commented Jun 2, 2017 • edited

jwmatthews commented Jun 7, 2017

rthallisey commented Jun 7, 2017

eriknelson commented Jun 21, 2017

rthallisey commented Jun 21, 2017 • edited

jwmatthews commented Jun 23, 2017

jwmatthews commented Jun 27, 2017

rthallisey commented Jun 28, 2017

Choose a reason for hiding this comment

rthallisey commented Jun 30, 2017

rthallisey commented Jul 6, 2017 • edited

eriknelson left a comment • 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

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

jmrodri left a comment

Choose a reason for hiding this comment

jmrodri left a comment

Choose a reason for hiding this comment

rthallisey commented Jul 13, 2017

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriknelson left a comment • edited

Choose a reason for hiding this comment

eriknelson commented Jul 13, 2017

rthallisey commented Nov 1, 2017

jmrodri commented May 24, 2017 •

edited

rthallisey commented Jun 2, 2017 •

edited

rthallisey commented Jun 21, 2017 •

edited

rthallisey commented Jul 6, 2017 •

edited

eriknelson left a comment •

edited

eriknelson left a comment •

edited