Add Get and GetCredentials for CFServiceInstances #3610

klapkov · 2024-11-11T23:20:19Z

Is there a related GitHub Issue?

#3609
#3608

What is this change about?

Here we expose GET /v3/service_instances/:guid and we also add GET /v3/service_instances/:guid/credentials

Does this PR introduce a breaking change?

No

Acceptance Steps

Tag your pair, your PM, and/or team

api/repositories/service_instance_repository.go

api/handlers/service_instance.go

danail-branekov · 2024-12-09T13:45:08Z

api/handlers/service_instance.go

+
+	serviceInstance, err := h.serviceInstanceRepo.GetServiceInstance(r.Context(), authInfo, serviceInstanceGUID)
+	if err != nil {
+		return nil, apierrors.LogAndReturn(logger, err, "failed to get service instance")


We should "mask" the forbidden error as "not found" in order not to leak information. See here for example

danail-branekov · 2024-12-09T13:45:59Z

api/handlers/service_instance_test.go

+			})
+
+			It("returns 404 Not Found", func() {
+				expectNotAuthorizedError()


the It says "returns 404" (correct), but it verifies for not authorised error (not correct :))

The api should mask forbidden error as not found. Therefore the test should expect a not found error rather then unauthorised. I think you did it the other way round :)

danail-branekov · 2024-12-09T14:21:05Z

tools/credentials_test.go

-				}
-			}`),
-		}))
+	When("ToCredentialsSecretData", func() {


This When should be a Describe as it describes a function under test

danail-branekov · 2024-12-09T14:22:53Z

tools/credentials_test.go


-	JustBeforeEach(func() {
-		var err error
 		secretData, err = tools.ToCredentialsSecretData(credsObject)


Invoking the method under test should be performed within the describe for that function. It should not be invoked in the global describe

danail-branekov · 2024-12-09T14:23:18Z

tools/credentials_test.go

-		credsObject any
-		secretData  map[string][]byte
+		credsObject        any
+		secretData         map[string][]byte


secretData can be local to the ToCredentialsSecretData describe

danail-branekov · 2024-12-09T14:24:58Z

tools/credentials_test.go

-		secretData  map[string][]byte
+		credsObject        any
+		secretData         map[string][]byte
+		decodedCredentials map[string]any


decodedCredentials should be pushed down to ToDecodedSecretDataCredentials describe

tools/credentials_test.go

danail-branekov · 2024-12-09T14:51:17Z

Also, e2e tests would be nice for the two new endpoints

klapkov · 2024-12-12T08:24:07Z

@danail-branekov Did some changes, could you take a look.

danail-branekov

I have resolved the comments that have been already addressed, and left the ones that have not been unresolved.
I have also added some more comments, please have a look

danail-branekov · 2024-12-12T11:38:18Z

api/handlers/service_instance.go

+		return nil, apierrors.LogAndReturn(logger, apierrors.ForbiddenAsNotFound(err), "failed to get service instance", "GUID", serviceInstanceGUID)
+	}
+
+	if serviceInstance.Type != korifiv1alpha1.UserProvidedType {


I think it would be better to push this check to the repository (reasoning below). The repository looks up the service instance anyway, so it could return a not found error in case the service instance type is not upsi.

From API point of view, instance credentials are just another resource. We strive to keep api handlers simple and only take care to process the request parameters, delegate to a repository and present the result. Business logic on aspects of getting and processing resources should be "outsourced" to repositories.

Forget about it, after a short discussion, having that decision in the handler is probably ok. Apologies for the noise

danail-branekov · 2024-12-12T11:40:00Z

api/handlers/service_instance.go

+
+	serviceInstance, err := h.serviceInstanceRepo.GetServiceInstance(r.Context(), authInfo, serviceInstanceGUID)
+	if err != nil {
+		return nil, apierrors.LogAndReturn(logger, err, "failed to get service instance")


This is still not resolved, the api should mask forbidden errors as not found

danail-branekov · 2024-12-12T11:41:16Z

api/handlers/service_instance_test.go

+			})
+
+			It("returns 404 Not Found", func() {
+				expectNotAuthorizedError()


The api should mask forbidden error as not found. Therefore the test should expect a not found error rather then unauthorised. I think you did it the other way round :)

danail-branekov · 2024-12-12T11:45:07Z

api/handlers/service_instance_test.go

+			})
+
+			It("returns 404 Not Found", func() {
+				_, actualAuthInfo, actualInstanceGUID := serviceInstanceRepo.GetServiceInstanceCredentialsArgsForCall(0)


From my point of view, checking GetServiceInstanceCredentials invocation and its parameter duplicates the checks in the default case. I think expecting a not found error is enough for this test.

danail-branekov · 2024-12-12T11:51:05Z

api/handlers/service_instance_test.go

+			})
+		})
+
+		When("getting the service instance fails with not found", func() {


the handler should only interpret the "forbidden" error and present it as not found. Any other sort of errors (regiardless not found, general error, whatever) should be just returned.

Having said that, I would alter this When to verify that when the repository returns a generic error, it results into an unexpected error (HTTP 500). In other words, as the handler does not handle the not found error in any particular way, I would go for the least specific test case.

danail-branekov · 2024-12-12T11:53:21Z

api/handlers/service_instance_test.go

+			})
+		})
+
+		When("the unmarshaling of the secret fails", func() {


See the comment on the "not found" error above. The handler does not handle unprocessable entity error in any particular way, it simply returns it. Another context with this particular error does not bring any more value. Therefore I would propose deleting at in order to keep the test simple

danail-branekov · 2024-12-12T11:56:48Z

api/repositories/service_instance_repository_test.go

+				})
+			})
+		})
+	})


Could you also add a When to verify that the repository returns unprocessiable entity error when the secret data is invalid json?

@danail-branekov Maybe this is a stupid question, but how would I achieve that ? I would have to create a secret with an invalid json format, how would I marshal it ?

secret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, Namespace: space.Name, }, Data: map[string][]byte{korifiv1alpha1.CredentialSecretKey, []byte{"not-json"}}, } Expect(k8sClient.Create(ctx, secret)).To(Succeed())

How about that?

Add Get and GetCredentials for CFServiceInstances

d8eaf43

klapkov commented Nov 11, 2024

View reviewed changes

api/repositories/service_instance_repository.go Show resolved Hide resolved

Merge branch 'main' into get_service_is_credentials

60f54c3

danail-branekov reviewed Dec 9, 2024

View reviewed changes

klapkov added 2 commits December 12, 2024 11:04

Address comments

0df3fb3

Remove SecretName from CFServiceInstance record

76ed317

klapkov force-pushed the get_service_is_credentials branch from 612645d to 76ed317 Compare December 12, 2024 09:04

danail-branekov reviewed Dec 12, 2024

View reviewed changes

klapkov and others added 2 commits December 13, 2024 00:03

Some more changes

4eaa4b7

Merge branch 'main' into get_service_is_credentials

cd1c0be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Get and GetCredentials for CFServiceInstances #3610

Add Get and GetCredentials for CFServiceInstances #3610

klapkov commented Nov 11, 2024

danail-branekov Dec 9, 2024

danail-branekov Dec 9, 2024

danail-branekov Dec 12, 2024

danail-branekov Dec 9, 2024

danail-branekov Dec 9, 2024

danail-branekov Dec 9, 2024

danail-branekov Dec 9, 2024

danail-branekov commented Dec 9, 2024

klapkov commented Dec 12, 2024

danail-branekov left a comment

danail-branekov Dec 12, 2024 •

edited

Loading

danail-branekov Dec 12, 2024

danail-branekov Dec 12, 2024

danail-branekov Dec 12, 2024

danail-branekov Dec 12, 2024

danail-branekov Dec 12, 2024

danail-branekov Dec 12, 2024

danail-branekov Dec 12, 2024

klapkov Dec 12, 2024

danail-branekov Dec 12, 2024

Add Get and GetCredentials for CFServiceInstances #3610

Are you sure you want to change the base?

Add Get and GetCredentials for CFServiceInstances #3610

Conversation

klapkov commented Nov 11, 2024

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

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

danail-branekov commented Dec 9, 2024

klapkov commented Dec 12, 2024

danail-branekov left a comment

Choose a reason for hiding this comment

danail-branekov Dec 12, 2024 • edited Loading

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

danail-branekov Dec 12, 2024 •

edited

Loading