Proposal to improve bind credential extraction #550

djzager · 2017-11-13T19:30:20Z

A proposal to remove (really deprecate) kubectl exec from bind
credential extraction process.

A proposal to remove (really deprecate) `kubectl exec` from bind credential extraction process.

djzager · 2017-11-13T19:36:17Z

Improve Ansible Broker's Bind Credential Extraction Process

jmrodri

One minor sentence change in the problem description. Otherwise, ACK.

jmrodri · 2017-11-13T19:49:57Z

docs/proposals/prop-apb-gen-creds.md

+[Kubernetes Downward
+API](https://kubernetes.io/docs/tasks/inject-data-application/downward-api-volume-expose-pod-information/),
+APBs have all they need to generate secrets inside the APB sandbox namespace to
+be retrieved later by the broker.


I would split the last sentence fragment so that it is more clear:

APBs will have all they need to generate secrets inside the APB sandbox namespace. The secret will be retrieved by the broker after the pod finishes.

jmrodri · 2017-11-13T19:51:01Z

docs/proposals/prop-apb-gen-creds.md

+
+- [ ] Update the [`asb_encode_binding`
+module](https://github.com/ansibleplaybookbundle/ansible-asb-modules/blob/master/library/asb_encode_binding.py)
+to create a kubernetes secret in the APB sandbox namespace.


Are there any error conditions we need to be able to handle? Anything that could cause the information not to appear in the secret?

Could you elaborate more on this. Off the top of my head the only scenario that I would be concerned about would be if we fail to create the secret. I'm struggling to think of a scenario where we would be able to create the secret but it would be empty.

I don't know :) just trying to come up with worst case scenarios. Can creating a secret fail? if we get errors what will happen? will we mark the pod as failed? today if we don't write the creds in the file we return an error code that exec can see. That way the broker knows when to stop looking. If the pod completes, but no secret exists does this mean there are no secrets or that we failed to write them?

Yes. The APB would fail if we couldn't create the secret.

jmrodri · 2017-11-13T19:51:21Z

docs/proposals/prop-apb-gen-creds.md

+to create a kubernetes secret in the APB sandbox namespace.
+- [ ] Update
+  [`apb-base`](https://github.com/ansibleplaybookbundle/apb-base/tree/master/files/usr/bin):
+  we no longer need to run `bind-init` or `broker-bind-creds`.


👍 to removing the need for these scripts

jmrodri · 2017-11-13T19:52:19Z

docs/proposals/prop-apb-gen-creds.md

+- [ ] Update
+  [`apb-base`](https://github.com/ansibleplaybookbundle/apb-base/tree/master/files/usr/bin):
+  we no longer need to run `bind-init` or `broker-bind-creds`.
+- [ ] Bump the APB version to `1.1`. This will prevent older broker's from grabbing


Are you bumping the min and the max? or something else? Should newer brokers be able to handle older APBs?

jmrodri · 2017-11-13T19:52:49Z

docs/proposals/prop-apb-gen-creds.md

+  ExtractCredentials`](https://github.com/openshift/ansible-service-broker/blob/8dda3277/pkg/apb/ext_creds.go#L33):
+  If APB version is `1.0`, do it the way we are currently doing it.
+  If APB version is `1.1`, 1) watch pod and wait for it to complete 2) evaluate
+  success/failure of APB execution 3) read credentials from secret.


This answers my question above.

coveralls · 2017-11-13T20:08:59Z

Coverage remained the same at 26.319% when pulling a3f6cfb on djzager:prop-apb-gen-creds into 8dda327 on openshift:master.

coveralls · 2017-11-13T21:31:27Z

Coverage remained the same at 26.319% when pulling ed901d8 on djzager:prop-apb-gen-creds into 8dda327 on openshift:master.

eriknelson · 2017-11-13T21:51:32Z

docs/proposals/prop-apb-gen-creds.md

+  [`pkg/apb/ext_creds.go
+  ExtractCredentials`](https://github.com/openshift/ansible-service-broker/blob/8dda3277/pkg/apb/ext_creds.go#L33):
+  If APB version is `1.0`, do it the way we are currently doing it.
+  If APB version is `1.1`, 1) watch pod and wait for it to complete 2) evaluate


Do we want to deprecate and warn v1.0 APBs?

Would this be a major version bump since it breaks compatibility with the 1.0 Broker?

It would be good to have a table somewhere tracking which Broker goes with which APBs that go with which release. Might come in handy.

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. Implements openshift#544 and the proposal openshift#550. Also addresses the potential issue in openshift#553. 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.

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.

* 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 #544 Fixes #553 Implements the proposal #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

* 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

Proposal to improve bind credential extraction

a3f6cfb

A proposal to remove (really deprecate) `kubectl exec` from bind credential extraction process.

djzager added 3.8 proposal labels Nov 13, 2017

djzager requested review from jmrodri and eriknelson November 13, 2017 19:30

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 13, 2017

jmrodri suggested changes Nov 13, 2017

View reviewed changes

Address review comments

ed901d8

jmrodri approved these changes Nov 13, 2017

View reviewed changes

eriknelson approved these changes Nov 13, 2017

View reviewed changes

jmrodri merged commit ea7b117 into openshift:master Nov 14, 2017

djzager mentioned this pull request Nov 15, 2017

Broker should extract credentials from secret #555

Merged

Proposal to improve bind credential extraction #550

Proposal to improve bind credential extraction #550

djzager commented Nov 13, 2017

djzager commented Nov 13, 2017

jmrodri left a comment

jmrodri Nov 13, 2017

jmrodri Nov 13, 2017

djzager Nov 13, 2017

jmrodri Nov 13, 2017

djzager Nov 13, 2017

jmrodri Nov 13, 2017

jmrodri Nov 13, 2017

jmrodri Nov 13, 2017

coveralls commented Nov 13, 2017

coveralls commented Nov 13, 2017

eriknelson Nov 13, 2017

rthallisey Nov 14, 2017

Proposal to improve bind credential extraction #550

Proposal to improve bind credential extraction #550

Conversation

djzager commented Nov 13, 2017

djzager commented Nov 13, 2017

jmrodri 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

coveralls commented Nov 13, 2017

coveralls commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment