Bug 1497839 - copy secrets to transient namespace and always run #473

shawn-hurley · 2017-10-04T15:19:16Z

Describe what this PR does and why we need it:
Removes split code path based on secrets to run the APB in the transient namespace always
Changes proposed in this pull request

remove branch on secrets
copy secrets to the transient namespace
panic the broker if we are attempting to delete its namespace.

Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on
N/A
Which issue this PR fixes (This will close that issue when PR gets merged)
N/A

Fixes: #385

djzager

LGTM with 1 nitpick

djzager · 2017-10-04T15:26:01Z

pkg/apb/executor.go

-		}
+
+	// Using a new UUID is sane, because it will have some gurantee
+	// of uniquenes and will meet DNS name requirements.


nit

Using a new UUID is sane, because it will have some guarantee
of uniqueness and will meet DNS name requirements.

djzager · 2017-10-04T15:29:21Z

pkg/apb/executor.go

+	namespace := v1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Labels: map[string]string{transientNameSpaceKey: spec.FQName},
+			Name:   executionContext.Namespace,


You could use GenerateName

https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#idempotency, but then you would have to get the name after you have created the namespace.

I think then we could give the namespace a name that is more obvious like apb-namespace-(unique suffix).

shawn-hurley · 2017-10-11T15:27:54Z

(2) Update AWS APB workflow on broker to match with transient namespace approach.

djzager · 2017-10-12T17:26:47Z

Example Namespace:

- apiVersion: v1
  kind: Namespace
  metadata:
    annotations:
      openshift.io/sa.scc.mcs: s0:c13,c7
      openshift.io/sa.scc.supplemental-groups: 1000170000/10000
      openshift.io/sa.scc.uid-range: 1000170000/10000
    creationTimestamp: 2017-10-12T17:12:06Z
    generateName: dh-hello-world-apb-prov-
    labels:
      apb-action: provision
      apb-fqname: dh-hello-world-apb
    name: dh-hello-world-apb-prov-84s0z
    namespace: ""
    resourceVersion: "10004"
    selfLink: /api/v1/namespaces/dh-hello-world-apb-prov-84s0z
    uid: 7927f3e5-af70-11e7-a5b7-64006a6fb904
  spec:
    finalizers:
    - kubernetes
    - openshift.io/origin
  status:
    phase: Active

eriknelson

Couple minor requests, overall looks good.

eriknelson · 2017-10-13T13:22:41Z

pkg/apb/executor.go

+	namespace := v1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Labels:       labels,
+			GenerateName: fmt.Sprintf("%s-%.4s-", spec.FQName, action),


What's the upper limit on the size of this field? 64 chars? We should guarantee we're under it.

I can make this change if you like, but my understanding is that GenerateName handles this for you:

Names generated by the system may be requested using metadata.generateName. GenerateName indicates that the name should be made unique by the server prior to persisting it. A non-empty value for the field indicates the name will be made unique (and the name returned to the client will be different than the name passed). The value of this field will be combined with a unique suffix on the server if the Name field has not been provided. The provided value must be valid within the rules for Name, and may be truncated by the length of the suffix required to make the value unique on the server. If this field is specified, and Name is not present, the server will NOT return a 409 if the generated name exists - instead, it will either return 201 Created or 504 with Reason ServerTimeout indicating a unique name could not be found in the time allotted, and the client should retry (optionally after the time indicated in the Retry-After header).

From https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#idempotency

No change needed, that looks good! Thank you for the link 👍

eriknelson · 2017-10-13T13:24:15Z

pkg/apb/svc_acct.go

@@ -236,8 +237,11 @@ func (s *ServiceAccountManager) DestroyApbSandbox(executionContext ExecutionCont
 	if asbNamespace != executionContext.Namespace {
 		s.log.Debug("Deleting namespace %s", executionContext.Namespace)
 		k8scli.CoreV1().Namespaces().Delete(executionContext.Namespace, &metav1.DeleteOptions{})
-
+	} else {
+		//We should not be attempting to run pods in the ASB namespace, if we are, something is seriously wrong.


Nit: Space after //

eriknelson · 2017-10-13T13:30:15Z

pkg/apb/executor.go

@@ -201,3 +200,22 @@ func checkPullPolicy(policy string) (v1.PullPolicy, error) {

 	return value, nil
 }
+
+func copySecretsToNamespace(executionContext ExecutionContext,


Nit: We have a couple stylistic patterns for long function signatures in the codebase right now. Admittedly, it's not super consistent, but we should try to not introduce other forms as well IMO. Personally, I like a vertically favored format:

func copySecretsToNamespace( executionContext ExecutionContext, clusterConfig ClusterConfig, k8scli *clientset.Clientset, secrets []string, ) error { // Return values // Implementation }

Also a longer form format, wrapping after the last full var/type crosses something like 80 chars:

func copySecretsToNamespace(executionContext ExecutionContext, clusterConfig ClusterConfig, k8scli *clientset.Clientset, secrets []string) error { // Implementation }

I would just advocate picking one and sticking with it.

eriknelson · 2017-10-13T13:31:21Z

pkg/apb/svc_acct.go

-
+	} else {
+		//We should not be attempting to run pods in the ASB namespace, if we are, something is seriously wrong.
+		panic(fmt.Errorf("Broker is attempting to delete its own namespace"))


+1, let's not implode 🎉

eriknelson · 2017-10-13T13:32:31Z

@shawn-hurley @djzager meant to request changes, apparently I can't operate github.

the APB in the transient namespace.

Add additional labels to apb pod for use when debugging failed apb execution.

eriknelson · 2017-10-13T19:00:34Z

doublecomboACK

…nshift#473) * Bug 1497839 - copy secrets to transient namespace and always run the APB in the transient namespace. * using kubernetes generatename to get a unique name that is readable. * update based on PR comments * Make transient namespace name user friendly Add additional labels to apb pod for use when debugging failed apb execution.

djzager approved these changes Oct 4, 2017

View reviewed changes

djzager added the needs-review label Oct 4, 2017

jmrodri self-requested a review October 5, 2017 15:10

shawn-hurley force-pushed the copy-secrets branch from dbd1e05 to f9d9b4e Compare October 12, 2017 03:30

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

djzager force-pushed the copy-secrets branch from a9755c6 to 42a5614 Compare October 12, 2017 17:13

eriknelson self-requested a review October 12, 2017 17:18

eriknelson approved these changes Oct 13, 2017

View reviewed changes

shawn-hurley added 3 commits October 13, 2017 10:53

Bug 1497839 - copy secrets to transient namespace and always run

6ed0b2d

the APB in the transient namespace.

using kubernetes generatename to get a unique name that is readable.

06e7fab

update based on PR comments

6a56cee

shawn-hurley force-pushed the copy-secrets branch from 42a5614 to 6a56cee Compare October 13, 2017 14:54

Make transient namespace name user friendly

10ca133

Add additional labels to apb pod for use when debugging failed apb execution.

eriknelson approved these changes Oct 13, 2017

View reviewed changes

eriknelson merged commit 81381b1 into openshift:master Oct 13, 2017

Bug 1497839 - copy secrets to transient namespace and always run #473

Bug 1497839 - copy secrets to transient namespace and always run #473

shawn-hurley commented Oct 4, 2017 •

edited by djzager

djzager left a comment

djzager Oct 4, 2017

djzager Oct 4, 2017

shawn-hurley Oct 4, 2017

shawn-hurley commented Oct 11, 2017

djzager commented Oct 12, 2017

eriknelson left a comment

eriknelson Oct 13, 2017

djzager Oct 13, 2017

eriknelson Oct 13, 2017

eriknelson Oct 13, 2017

eriknelson Oct 13, 2017

eriknelson Oct 13, 2017

eriknelson commented Oct 13, 2017

eriknelson commented Oct 13, 2017

Bug 1497839 - copy secrets to transient namespace and always run #473

Bug 1497839 - copy secrets to transient namespace and always run #473

Conversation

shawn-hurley commented Oct 4, 2017 • edited by djzager

djzager 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

shawn-hurley commented Oct 11, 2017

djzager commented Oct 12, 2017

eriknelson 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

eriknelson commented Oct 13, 2017

eriknelson commented Oct 13, 2017

shawn-hurley commented Oct 4, 2017 •

edited by djzager