Implemented GET /v3/service_credential_bindings/:guid/details endpoint #3762

ddraganovv · 2025-01-27T22:28:54Z

Is there a related GitHub Issue?

No

What is this change about?

Implementation of an endpoint according to the CF api documentation:
GET /v3/service_credential_bindings/:guid/details

Does this PR introduce a breaking change?

No

danail-branekov · 2025-01-28T11:56:53Z

api/handlers/service_binding.go

-	ServiceBindingPath  = "/v3/service_credential_bindings/{guid}"
+	ServiceBindingsPath       = "/v3/service_credential_bindings"
+	ServiceBindingPath        = "/v3/service_credential_bindings/{guid}"
+	ServiceBindingDetailsPath = ServiceBindingPath + "/details"


I would recommend not using string concatenation - plain strings make reasoning about what endpoints are implemented easier and is machine friendlier should we want to have a tool to generate a list of supported endpoints

danail-branekov · 2025-01-28T12:01:29Z

api/repositories/service_binding_repository.go

+type Credentials map[string]any
+type ServiceBindingDetailsRecord struct {
+	Credentials    Credentials
+	SyslogDrainURL *string


Provided that the repository never populates SyslogDrainURL and VolumeMounts, I would suggest removing them from the record. We can introduce them when we properly implement them.

danail-branekov · 2025-01-28T12:01:56Z

api/repositories/service_binding_repository.go

@@ -75,6 +77,12 @@ func (r ServiceBindingRecord) Relationships() map[string]string {
 	}
 }

+type Credentials map[string]any


Is there any reason to alias map[string]any as Credentials? I.e. is the Credentials type needed?

danail-branekov · 2025-01-28T12:03:44Z

api/presenter/service_binding.go

@@ -41,6 +41,12 @@ type ServiceBindingLinks struct {
 	Details         Link `json:"details"`
 }

+type ServiceBindingDetailsResponse struct {
+	Credentials    map[string]any `json:"credentials"`
+	SyslogDrainURL *string        `json:"syslog_drain_url,omitempty"`


Similar to the ServiceBindingDetailsRecord, we do not support SyslogDrainURL and VolumeMounts, so I would recommend removing them from the response type.

danail-branekov · 2025-01-28T12:07:00Z

api/repositories/service_binding_repository.go

@@ -272,6 +280,52 @@ func (r *ServiceBindingRepo) GetServiceBinding(ctx context.Context, authInfo aut
 	return serviceBindingToRecord(*serviceBinding), nil
 }

+func (r *ServiceBindingRepo) GetServiceBindingDetails(ctx context.Context, authInfo authorization.Info, bindingGUID string) (ServiceBindingDetailsRecord, error) {
+	userClient, err := r.userClientFactory.BuildClient(authInfo)


Getting the CFServiceBinding here is the same as getting it in GetServiceBinding. I would suggest extracting a reusable utility function to remove code duplication

danail-branekov · 2025-01-28T12:16:21Z

api/repositories/service_binding_repository_test.go

+			credentialBytes, err := json.Marshal(credentials)
+			Expect(err).To(BeNil())
+
+			creds := map[string][]byte{


You could use ToCredentialsSecretData to create the secret data in the correct format

danail-branekov · 2025-01-28T12:18:53Z

api/repositories/service_binding_repository_test.go

+
+			It("returns details", func() {
+				Expect(getErr).To(BeNil())
+				Expect(serviceBindingRecord.Credentials).To(HaveKeyWithValue("bar", "val2"))


A more concise way to assert two things would be

Expect(serviceBindingRecord.Credentials).To(SatisfyAll( HaveKeyWithValue("bar", "val2"), HaveKeyWithValue("foo", "val1"), ))

Or even simpler:

Expect(serviceBindingRecord.Credentials).To(Equal(map[string]any{ "bar": "val2", "foo": "val1", }))

danail-branekov · 2025-01-28T12:20:33Z

api/repositories/service_binding_repository_test.go

+		BeforeEach(func() {
+			serviceInstanceGUID := prefixedGUID("instance")
+			cfServiceInstance = createServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, "service-instance-name", serviceInstanceGUID)
+			serviceBindingGUID = prefixedGUID("binding")


We generally prefer uuid.NewString() instead of those prefixedXXX utilities. We are trying to slowly get rid of them alltogether, let's not use them in new code.

Any reason to user prefixedGUID instead of uuid.NewString()?

api/repositories/service_binding_repository_test.go

danail-branekov · 2025-01-28T12:23:33Z

controllers/api/v1alpha1/cfserviceinstance_types.go

-	ManagedType      = "managed"
+	UserProvidedType     = "user-provided"
+	ManagedType          = "managed"
+	CredentialsSecretKey = "credentials"


We already have a constant for that key, no need of a new one. But if you use the utilities from the tools/secrets.go file, you should probably not need to reference it.

danail-branekov · 2025-01-28T12:27:24Z

As this is a new endpoint, I would also ask you to add an E2E test (in tests/e2e/service_bindings_test.go). In e2e tests we test the very-very happy path to verify that the invocation of the endpoint succeeds and maybe a field or two in the response. Thus we verify that endpoints are properly wired in the api main. No need to test error cases there, they are already tested with unit/integration tests.

georgethebeatle · 2025-02-06T12:48:22Z

api/handlers/service_binding_test.go

+			Expect(rr).To(HaveHTTPHeaderWithValue("Content-Type", "application/json"))
+			Expect(rr).To(HaveHTTPBody(SatisfyAll(
+				MatchJSONPath("$.credentials.connection", "mydb://user@password:example.com"),
+				MatchJSONPath("$.syslog_drain_url", "http://syslog.example.com/drain"),


With regards to syslog_drain_url and volume_mounts, we would better not return anything. Furnthermore these values are not stubbed in the before each, so this test would probably fail.

georgethebeatle · 2025-02-06T12:49:24Z

api/handlers/service_binding_test.go

+				MatchJSONPath("$.volume_mounts[0]", "mount1"),
+				MatchJSONPath("$.volume_mounts[1]", "mount2"),
+			)))
+			Expect(serviceBindingRepo.GetServiceBindingDetailsCallCount()).To(Equal(1))


We should also add a check of the arguments passed to GetServiceBindingDetails

georgethebeatle · 2025-02-06T12:51:12Z

api/handlers/service_binding_test.go

+				serviceBindingRepo.GetServiceBindingDetailsReturns(repositories.ServiceBindingDetailsRecord{}, apierrors.NewForbiddenError(nil, repositories.ServiceBindingResourceType))
+			})
+
+			It("returns 404 NotFound when binding not found", func() {


the context of the test is already stated in the When above, so this would be more concise as:

It("returns 404 Not Found")

georgethebeatle · 2025-02-06T12:55:31Z

api/repositories/service_binding_repository.go

 	if err != nil {
-		return ServiceBindingRecord{}, apierrors.FromK8sError(err, ServiceBindingResourceType)
+		return ServiceBindingDetailsRecord{}, fmt.Errorf("failed to get credentials: %w", apierrors.ForbiddenAsNotFound(apierrors.FromK8sError(err, ServiceBindingResourceType))) //


Two things:

We usually do the ForbiddenAsNotFound masking in the handler layer only

The trailing comment should go

georgethebeatle · 2025-02-06T12:59:32Z

api/repositories/service_binding_repository.go

+func (r *ServiceBindingRepo) GetServiceBindingDetails(ctx context.Context, authInfo authorization.Info, guid string) (ServiceBindingDetailsRecord, error) {
+	binding, err := r.getServiceBinding(ctx, authInfo, guid)
+	if err != nil {
+		return ServiceBindingDetailsRecord{}, fmt.Errorf("get-service-binding-details failed to retrieve the service binding: %w", err)


This error is already wrapped in the function, so no need to wrap it again, as we do not add any significant info here.

georgethebeatle · 2025-02-06T13:00:09Z

api/repositories/service_binding_repository.go

 	if err != nil {
-		return ServiceBindingRecord{}, err
+		return ServiceBindingRecord{}, fmt.Errorf("get-service-binding failed to create user client: %w", err)


This error is already wrapped, no need to wrap again, furthermore the description is incorrect.

georgethebeatle · 2025-02-06T13:01:28Z

api/repositories/service_binding_repository_test.go

+		BeforeEach(func() {
+			serviceInstanceGUID := prefixedGUID("instance")
+			cfServiceInstance = createServiceInstanceCR(ctx, k8sClient, serviceInstanceGUID, space.Name, "service-instance-name", serviceInstanceGUID)
+			serviceBindingGUID = prefixedGUID("binding")


Any reason to user prefixedGUID instead of uuid.NewString()?

georgethebeatle · 2025-02-06T13:06:08Z

tests/e2e/service_bindings_test.go

+		var result credentialsResponse
+
+		BeforeEach(func() {
+			brokerGUID := createBroker(serviceBrokerURL)


This test would be much simpler if we create a binding to a user provided service instance. The details endpoint is valid for both types of service binding.

ddraganovv force-pushed the add_service_binding_getDetails_handler branch 2 times, most recently from e8183f3 to dedb18a Compare January 27, 2025 22:49

danail-branekov reviewed Jan 28, 2025

View reviewed changes

ddraganovv force-pushed the add_service_binding_getDetails_handler branch from dedb18a to f34a991 Compare February 4, 2025 23:55

Added getDetails handler and repo implementation

fd51307

ddraganovv force-pushed the add_service_binding_getDetails_handler branch from f34a991 to fd51307 Compare February 5, 2025 22:40

georgethebeatle reviewed Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented GET /v3/service_credential_bindings/:guid/details endpoint #3762

Implemented GET /v3/service_credential_bindings/:guid/details endpoint #3762

ddraganovv commented Jan 27, 2025

danail-branekov Jan 28, 2025

danail-branekov Jan 28, 2025

danail-branekov Jan 28, 2025

danail-branekov Jan 28, 2025

danail-branekov Jan 28, 2025

danail-branekov Jan 28, 2025

danail-branekov Jan 28, 2025

danail-branekov Jan 28, 2025

georgethebeatle Feb 6, 2025

danail-branekov Jan 28, 2025

danail-branekov commented Jan 28, 2025

georgethebeatle Feb 6, 2025

georgethebeatle Feb 6, 2025

georgethebeatle Feb 6, 2025

georgethebeatle Feb 6, 2025

georgethebeatle Feb 6, 2025

georgethebeatle Feb 6, 2025

georgethebeatle Feb 6, 2025

georgethebeatle Feb 6, 2025

Implemented GET /v3/service_credential_bindings/:guid/details endpoint #3762

Are you sure you want to change the base?

Implemented GET /v3/service_credential_bindings/:guid/details endpoint #3762

Conversation

ddraganovv commented Jan 27, 2025

Is there a related GitHub Issue?

What is this change about?

Does this PR introduce a breaking change?

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 commented Jan 28, 2025

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