Introduce `POST /v3/roles` endpoint to create space roles #215

danail-branekov · 2021-11-05T10:29:36Z

Is there a related GitHub Issue?

What is this change about?

Introduce the POST /v3/roles API endpoint. Whenever called, the API would create a rolebinding in a space namespace that binds a user to a role. Note that the API would fail to create a space role if the user does not have a role (i.e. a role binding) in the parent org namespace (this is how the API should behave according to the CF API documentation)

Does this PR introduce a breaking change?

Yes

K8s role names differ from the CF roles, for example, space_developer maps to cf-k8s-controllers-space-developer. In order to express this mapping we have introduced a new configuration yaml providing that mapping. We did not want to put that config in cf_k8s_api_config.yaml as that would mean that we would have to carry those mappings across all kustomization overlays.
That is why we now have two config files and we changed the meaning of the CONFIG environment variable to reference a directory, rather than a single file. The main would read the content of the directory and gradually fill the configuration object.
We have amended the .envrc accordingly so that make run works.

Acceptance Steps

Create org
curl -X POST /v3/organizations -H 'Content-Type: application/json' -d '{"name":"my-org"}'
Create a rolebinding in the org namespaces that binds a user to whatever role
kubectl create rolebinding <rolebinding-name> -n <org-ns-name> --clusterrole=some-role --user=some-user
Create space
curl -X POST /v3/spaces -H 'Content-Type: application/json' -d '{ "name": "space-name", "relationships": { "organization": { "data": { "guid": "<org-guid>" } } } }'
Create the role:

curl "https://api.example.org/v3/roles" \
  -X POST \
  -H "Content-Type: application/json" \
  -d '{
      "type": "cf-k8s-controllers-space-developer",
      "relationships": {
        "user": {
          "data": {
            "guid": "some-user>"
          }
        },
        "space": {
          "data": {
            "guid": "<the space GUID>"
          }
        }
      }
    }'

*See success response like:

{
   "guid": "40557c70-d1bd-4976-a2ab-a85f5e882418",
   "created_at": "2019-10-10T17:19:12Z",
   "updated_at": "2019-10-10T17:19:12Z",
   "type": "<the ClusterRole name>",
   "relationships": {
      "user": {
         "data": {
            "guid": "<the username>"
         }
      },
      "space": {
         "data": {
            "guid": "<the space GUID>"
         }
      },
      "organization": {
         "data": null
      }
   },
   "links": {
      "self": {
         "href": "https://api.example.org/v3/roles/40557c70-d1bd-4976-a2ab-a85f5e882418"
      },
      "space": {
         "href": "https://api.example.org/v3/spaces/05c5da3b-6cbc-421c-87c3-20bb3c41ab7c"
      }
   }
}

Tag your pair, your PM, and/or team

@cloudfoundry/cf-k8s

kieron-dev

See inline comments.

Also, where are the changes to the .envrc mentioned in one of the commits?

kieron-dev · 2021-11-09T09:44:17Z

api/apis/role_handler_test.go

+			})
+		})
+
+		When("the request body is invalid with missing required name field", func() {


s/name/type

kieron-dev · 2021-11-09T09:49:52Z

api/repositories/authorization/org_test.go

-	JustBeforeEach(func() {
-		namespaces, getErr = org.GetAuthorizedNamespaces(ctx, identity)
+		roleName1 = generateGUID("org-user-1")
+		roleName2 = generateGUID("org-user-1")


s/org-user-1/org-user-2/

kieron-dev · 2021-11-09T09:51:39Z

api/repositories/authorization/org_test.go

-	It("lists the namespaces with bindings for current user", func() {
-		Expect(getErr).NotTo(HaveOccurred())
-		Expect(namespaces).To(ConsistOf(org1Ns))
+		Expect(k8sClient.Delete(context.Background(), &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: roleName1}})).To(Succeed())


maybe use a ctx var above somewhere and replace all those context.Background() calls?

kieron-dev · 2021-11-09T09:54:37Z

api/repositories/authorization/org_test.go

-		BeforeEach(func() {
-			ctx, cancelCtx = context.WithDeadline(ctx, time.Now().Add(-time.Minute))
+			BeforeEach(func() {
+				ctx, cancelCtx = context.WithDeadline(ctx, time.Now().Add(-time.Minute))


I know it's not from this PR, but I really appreciate using an expired context to make k8s client calls fail :)

kieron-dev · 2021-11-09T09:58:46Z

api/repositories/role_repository.go

+	}
+
+	now := time.Now()
+	role.CreatedAt = now


Should we use the metadata.creationTimestamp from the k8s roleBinding instead of now?

kieron-dev · 2021-11-09T10:07:16Z

api/repositories/role_repository_test.go

+	})
+
+	Describe("CreateSpaceRole", func() {
+		createSubnamespaceAnchor := func(name, parent string) *hnsv1alpha2.SubnamespaceAnchor {


Given we set SpaceNameLabel, this is creating a space subnamespaceanchor. Consider renaming.

There is also some duplication with org/space creation in the org_repository_test code. Potential refactor.

kieron-dev · 2021-11-09T10:08:33Z

api/repositories/role_repository_test.go

+
+		BeforeEach(func() {
+			authorizedInChecker.AuthorizedInReturns(true, nil)
+			orgAnchor = createSubnamespaceAnchor(uuid.NewString(), rootNamespace)


We are creating an org anchor, but setting the SpaceNameLabel on it. Suggest modifying this to remove future confusion

kieron-dev · 2021-11-09T10:15:24Z

api/tests/e2e/e2e_suite_test.go

@@ -59,6 +67,16 @@ var _ = BeforeSuite(func() {
 	ensureServerIsUp()
 })

+var _ = BeforeEach(func() {
+	serviceAccountName = generateGUID("user")
+	token := obtainServiceAccountToken(serviceAccountName)


obtainServiceAccountToken does a fair bit of work, so we're introducing a significant overhead into each test here.

Consider whether this is necessary for every e2e test. If so, it might make more sense to be shared in a synchronised before suite.

kieron-dev · 2021-11-09T10:17:53Z

api/tests/e2e/roles_test.go

+		userName string
+	)
+
+	createRoleWithHeaders := func(roleName, userName, spaceGUID string, headers map[string]string) (*http.Response, error) {


Is this a bit premature? We're currently using the privileged client to create the role binding, so the auth header is not used.

kieron-dev · 2021-11-09T10:18:12Z

api/tests/e2e/roles_test.go

+		})
+
+		It("creates a role binding", func() {
+			response, err := createRoleWithHeaders("space_developer", userName, space.GUID, nil)


nil. See comment above.

* Currently supports only creating space roles * Generate predictable role binding names by doing a sha256sum on the role name and user. The name is also prefixed with `cf-` to ensure it starts witha lowercase letter. * The role will fail to create if the user does not have a role in the parent org namespace (as per the docs for POST /v3/roles) Issue: #160 Co-authored-by: Mario Nitchev <marionitchev@gmail.com> Co-authored-by: Kieron Browne <kbrowne@vmware.com>

We've seen an increased amount of flakes while trying to get the ServiceAccount secret Co-authored-by: Kieron Browne <kbrowne@vmware.com> Co-authored-by: Mario Nitchev <marionitchev@gmail.com>

Although it doesn't work in GitHub Actions, it still saves time when running locally Co-authored-by: Kieron Browne <kbrowne@vmware.com>

* the api now parses all yaml files in a directory pointed by the CONFIG env var and merges them in a single config * adjust the CONFIG in .envrc Co-authored-by: Mario Nitchev <marionitchev@gmail.com>

Co-authored-by: Mario Nitchev <marionitchev@gmail.com>

Co-authored-by: Kieron Browne <kbrowne@vmware.com> Co-authored-by: Mario Nitchev <marionitchev@gmail.com>

danail-branekov force-pushed the wip-create-role branch 2 times, most recently from 33969d0 to 6699804 Compare November 5, 2021 15:12

mnitchev linked an issue Nov 8, 2021 that may be closed by this pull request

[Feature]: API client can assign a space role to a user with POST /v3/roles #160

Closed

kieron-dev suggested changes Nov 9, 2021

View reviewed changes

danail-branekov and others added 6 commits November 9, 2021 12:04

Increase eventually timeout to 120 seconds when waiting for secret

905c460

We've seen an increased amount of flakes while trying to get the ServiceAccount secret Co-authored-by: Kieron Browne <kbrowne@vmware.com> Co-authored-by: Mario Nitchev <marionitchev@gmail.com>

Use go build cache in Dockerfiles

2fe02ad

Although it doesn't work in GitHub Actions, it still saves time when running locally Co-authored-by: Kieron Browne <kbrowne@vmware.com>

Place config files in a directory

d957cf1

* the api now parses all yaml files in a directory pointed by the CONFIG env var and merges them in a single config * adjust the CONFIG in .envrc Co-authored-by: Mario Nitchev <marionitchev@gmail.com>

Fix controllers reference yaml

d861da3

Co-authored-by: Mario Nitchev <marionitchev@gmail.com>

Address PR comments

229aed3

Co-authored-by: Kieron Browne <kbrowne@vmware.com> Co-authored-by: Mario Nitchev <marionitchev@gmail.com>

kieron-dev force-pushed the wip-create-role branch from 6699804 to 229aed3 Compare November 9, 2021 14:08

kieron-dev approved these changes Nov 9, 2021

View reviewed changes

kieron-dev merged commit 05065c1 into main Nov 9, 2021

kieron-dev deleted the wip-create-role branch November 9, 2021 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `POST /v3/roles` endpoint to create space roles #215

Introduce `POST /v3/roles` endpoint to create space roles #215

danail-branekov commented Nov 5, 2021

kieron-dev left a comment

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

kieron-dev Nov 9, 2021

Introduce POST /v3/roles endpoint to create space roles #215

Introduce POST /v3/roles endpoint to create space roles #215

Conversation

danail-branekov commented Nov 5, 2021

Is there a related GitHub Issue?

What is this change about?

Does this PR introduce a breaking change?

Acceptance Steps

Tag your pair, your PM, and/or team

kieron-dev 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce `POST /v3/roles` endpoint to create space roles #215

Introduce `POST /v3/roles` endpoint to create space roles #215