RFC-0030 - Add support to Diego for file based service bindings #100

dimitardimitrov13 · 2024-07-25T11:42:56Z

Read the Contributing document.

Summary

This PR is continuation for the RFC-0030

Backward Compatibility

Breaking Change? No

This feature is the first PoC for RFC-0030.
Since the CAPI implementation for RFC-0030 is not done yet, the PR doesn't need to be applied immediately.

Remove ioutil.ReadFile in favor of os.ReadFile. Remove MkdirAll in favor of Mkdir.

PlamenDoychev · 2024-07-26T12:52:36Z

depot/containerstore/service_binding_root_handler.go

+	return nil
+}
+
+func (h *ServiceBindingRootHandler) RemoveDir(logger lager.Logger, container executor.Container) error {


I do not see this function covered with tests, or am i wrong?

Hello here it is the test for RemoveDir
https://github.com/cloudfoundry/executor/pull/100/files#diff-5e39d32f309883624104dc2056437db4a3d782ea689020c000a3848675ca80d8R81

…tests.

ameowlia · 2024-12-13T16:29:31Z

depot/containerstore/storenode.go

@@ -33,6 +33,7 @@ const ContainerMissingMessage = "missing garden container"
 const VolmanMountFailed = "failed to mount volume"
 const BindMountCleanupFailed = "failed to cleanup bindmount artifacts"
 const CredDirFailed = "failed to create credentials directory"
+const VolumeMountedFileFailed = "failed to create service binding root"


I think the future of this feature will be used for more things than just service bindings.

❓ Can you change this error message to be less specific to service bindings and more generic about volume mounted files?

ameowlia · 2024-12-13T16:30:43Z

depot/containerstore/storenode.go

@@ -242,7 +247,19 @@ func (n *storeNode) Create(logger lager.Logger, traceID string) error {
 			n.complete(logger, traceID, true, CredDirFailed, true)
 			return err
 		}
+
+		if len(n.info.VolumeMountedFiles) > 0 {


❓ should this use info instead of n.info like everything else in this function?

ameowlia · 2024-12-13T16:35:55Z

depot/containerstore/volume_mounted_files_handler.go

+		return nil, err
+	}
+
+	logger.Info("creating-dir - volume-mounted-files")


❓ Is "volume-mounted-files" meant to be hard coded? Or should it be "h.volumeMountPath"?

Or maybe "h.containerMountPath"?

ameowlia · 2024-12-13T16:41:08Z

depot/containerstore/volume_mounted_files_handler_test.go

+		It("validates the content of the redis username file", func() {
+			_, err := handler.CreateDir(logger, container)
+			Expect(err).To(Succeed())
+
+			usernameFilePath := filepath.Join(tmpdir, fakeContainerUUID, "redis", "username")
+			Expect(usernameFilePath).To(BeAnExistingFile())
+
+			content, err := os.ReadFile(usernameFilePath)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(string(content)).To(Equal("username"))
+		})
+
+		It("validates the content of the redis password file", func() {
+			_, err := handler.CreateDir(logger, container)
+			Expect(err).To(Succeed())
+
+			usernameFilePath := filepath.Join(tmpdir, fakeContainerUUID, "redis", "password")
+			Expect(usernameFilePath).To(BeAnExistingFile())
+
+			content, err := os.ReadFile(usernameFilePath)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(string(content)).To(Equal("password"))
+		})
+
+		It("validates the content of the httpd username file", func() {
+			_, err := handler.CreateDir(logger, container)
+			Expect(err).To(Succeed())
+
+			usernameFilePath := filepath.Join(tmpdir, fakeContainerUUID, "httpd", "username")
+			Expect(usernameFilePath).To(BeAnExistingFile())
+
+			content, err := os.ReadFile(usernameFilePath)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(string(content)).To(Equal("username"))
+		})
+
+		It("validates the content of the httpd password file", func() {
+			_, err := handler.CreateDir(logger, container)
+			Expect(err).To(Succeed())
+
+			usernameFilePath := filepath.Join(tmpdir, fakeContainerUUID, "httpd", "password")
+			Expect(usernameFilePath).To(BeAnExistingFile())
+
+			content, err := os.ReadFile(usernameFilePath)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(string(content)).To(Equal("password"))
+		})


These are all basically the same test. Can you DRY this up by turning these into table tests?

ameowlia · 2024-12-13T16:42:04Z

depot/containerstore/volume_mounted_files_handler_test.go

+		It("removes volume-mounted-files directory", func() {
+			err := handler.RemoveDir(logger, container)
+			Expect(err).NotTo(HaveOccurred())
+
+			volumeMountedFiles := filepath.Join(tmpdir, fakeContainerUUID)
+			Eventually(volumeMountedFiles).ShouldNot(BeADirectory())
+		})
+


This should be under a Describe for "RemoveDir" not "CreateDir".

ameowlia · 2024-12-13T16:42:33Z

depot/containerstore/volume_mounted_files_handler_test.go

+
+			It("returns an error", func() {
+				_, err := handler.CreateDir(logger, executor.Container{Guid: "fake_guid"})
+				Expect(err).To(HaveOccurred())


❓ Can you add another expect on a more specific error?

ameowlia · 2024-12-13T16:47:01Z

depot/containerstore/volume_mounted_files_handler.go

+	if err != nil {
+		logger.Error("failed-to-remove-volume-mounted-files-directory", err, lager.Data{"directory": path})
+	}


Can you add a test for this?

ameowlia · 2024-12-13T16:48:56Z

depot/containerstore/volume_mounted_files_handler.go

+	}, nil
+}
+
+func (h *VolumeMountedFilesHandler) volumeMountedFilesForServices(


All of these errors are untested. I understand that it can be hard to test file actions and to force them to fail, but is there a way to force a least one failure, so that you can test what happens to CreateDir when this function returns an error?

ameowlia · 2024-12-13T16:49:27Z

depot/containerstore/volume_mounted_files_handler.go

+			err := fmt.Errorf("failed to extract volume-mounted-files directory. format is: /serviceName/fileName")
+			logger.Error(" volume-mounted-files directory is required", err, lager.Data{"dirName": "is empty"})
+
+			continue


Why do some errors only log instead of bubble up? Why would you continue in these cases?

dimitardimitrov13 · 2024-12-20T09:47:14Z

Hello @ameowlia, all comments were addressed.

dimitardimitrov13 added 7 commits July 3, 2024 09:55

Adding of ServiceBindingRoot to executor config.

e44881b

Implement binding root for ServiceBindigRoot

c2e337d

Implement the creation of service binding config files.

6206f0d

Add go.mod and go.sum for testing purpose.

bf2ae01

Extend the creation of service binding config files.

89c2805

Implement container sercice binding root cleanup.

1232e3e

Implementing BDD tests.

6af3112

dimitardimitrov13 requested a review from a team as a code owner July 25, 2024 11:42

dimitardimitrov13 mentioned this pull request Jul 25, 2024

[RFC0030] Add Support for File based Service Binding Information cloudfoundry/community#901

Open

dimitardimitrov13 force-pushed the CFAR-929_VCAP_SERVICES branch from 2c59134 to 2f5337e Compare July 25, 2024 12:04

dimitardimitrov13 added 3 commits July 25, 2024 15:09

Rename filesVariables to fileBasedServiceBinding.

8b3dfbe

Rename fileBasedServiceBinding to serviceBindingFiles.

fc4c7fc

Remove go.mod and go.sum

fa28a86

Remove ioutil.ReadFile in favor of os.ReadFile. Remove MkdirAll in favor of Mkdir.

dimitardimitrov13 force-pushed the CFAR-929_VCAP_SERVICES branch from 2f5337e to fa28a86 Compare July 25, 2024 12:11

Merge branch 'main' into CFAR-929_VCAP_SERVICES

25025ce

PlamenDoychev reviewed Jul 26, 2024

View reviewed changes

dimitardimitrov13 added 3 commits October 29, 2024 13:31

Changes according Updates #1008

9fbecf6

Merge branch 'cloudfoundry:main' into CFAR-929_VCAP_SERVICES

d754e8c

Change Mkdir to MkdirAll. Fixes around those changes. Refactoring of …

0d5855f

…tests.

ameowlia requested changes Dec 13, 2024

View reviewed changes

Changes according to code review.

af98c4d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC-0030 - Add support to Diego for file based service bindings #100

RFC-0030 - Add support to Diego for file based service bindings #100

dimitardimitrov13 commented Jul 25, 2024

PlamenDoychev Jul 26, 2024

dimitardimitrov13 Jul 29, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

ameowlia Dec 13, 2024

dimitardimitrov13 commented Dec 20, 2024

RFC-0030 - Add support to Diego for file based service bindings #100

Are you sure you want to change the base?

RFC-0030 - Add support to Diego for file based service bindings #100

Conversation

dimitardimitrov13 commented Jul 25, 2024

Summary

Backward Compatibility

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

dimitardimitrov13 commented Dec 20, 2024