Parameterize gateway namespace used by nb-ctrlr #14

cam-garrison · 2023-10-18T13:30:48Z

This PR aims to fix Issue #617 by replacing the default namespace of the gateway with the application namespace before applying the notebook controller manifests.

bartoszmajsak

Thanks for making the gateway name dynamic. This way we are 1-1 with the v1 manifests we provided.

One additional thing - adding some tests wouldn't harm :)

components/workbenches/workbenches.go

bartoszmajsak · 2023-10-23T13:32:57Z

components/workbenches/workbenches.go

+	ossmEnv := filepath.Join(kfnotebookControllerServiceMeshPath, "ossm.env")
+	namespace := dscispec.ApplicationsNamespace
+
+	pattern := `ISTIO_GATEWAY=(.*)/odh-gateway`


I think this can be a const.

set as a const here but didnt pull out of the function - will do this when moving into a separate package 9613807

bartoszmajsak · 2023-10-23T14:38:58Z

pkg/common/common.go

@@ -45,3 +46,29 @@ func ReplaceStringsInFile(fileName string, replacements map[string]string) error

 	return nil
 }
+
+// ReplacePatternsInFile uses regexes to replace patterns with replacements in manifests during runtime.
+func ReplacePatternsInFile(fileName string, replacements map[string]string) error {


I don't think we need two separate funcs to handle strings or regular expressions. One should suffice. See example https://go.dev/play/p/-5A8-bkarvS

Also, instead of regexp.MustCompile which panics perhaps it's better use regexp.Compile and push the error down the call stack so that it can be managed appropriately at a higher level like in other cases.

nice, I like this solution, working on implementing this now

implemented here - consolidated both into one with a helper func 9613807

bartoszmajsak · 2023-10-23T14:41:05Z

components/workbenches/workbenches.go

@@ -180,3 +185,16 @@ func (w *Workbenches) DeepCopyInto(target *Workbenches) {
 	*target = *w
 	target.Component = w.Component
 }
+
+func (w *Workbenches) SetGatewayName(dscispec *dsci.DSCInitializationSpec) error {
+	ossmEnv := filepath.Join(kfnotebookControllerServiceMeshPath, "ossm.env")


Should we fail if the file does not exist?

We fail in the ReplaceInFile helper function if it fails to read file for some reason - don't think it should be in both places.

bartoszmajsak · 2023-10-23T14:42:03Z

components/workbenches/workbenches.go

@@ -180,3 +185,16 @@ func (w *Workbenches) DeepCopyInto(target *Workbenches) {
 	*target = *w
 	target.Component = w.Component
 }
+
+func (w *Workbenches) SetGatewayName(dscispec *dsci.DSCInitializationSpec) error {


I am not sure we need this as exported func. Keeping it private should suffice. This in fact makes me wonder if this really belongs here. It's a specific OSSM manifests customization so perhaps making it its own func and package would be a bit cleaner? That would also simplify testing it.

moved out into a separate package "ossmcommon" within the servicemesh directory - let me know what you think of location / naming

bartoszmajsak · 2023-10-25T11:24:03Z

pkg/servicemesh/ossmcommon/operations.go

@@ -0,0 +1,19 @@
+package ossmcommon


I am not a big fan of using generic packages like "common" or "util". It is troublesome on a few fronts.

ambiguity: generic names don't convey specific functionality, making it difficult to understand the purpose of the package at a glance.

lack of cohesion: they're often dumping ground for anything which we can find a better place for.

maintenance: for the reason above they grow out of control with misc utilities, so it's harder to spot redunant code/functionalities, but also any change risks a side effect due to widespread use across the whole code base.

Consider naming more descriptively and grouping related functionalities.

bartoszmajsak · 2023-10-25T11:25:00Z

pkg/servicemesh/ossmcommon/operations.go

+
+const gatewayPattern = `ISTIO_GATEWAY=(.*)/odh-gateway`
+
+func SetGatewayName(dscispec *dsci.DSCInitializationSpec, kfnotebookControllerServiceMeshPath string) error {


We only need to pass namespace, not the whole dsci.DSCInitializationSpec struct pointer, so perhaps better to keep it smaller/more focused.

I am wondering if we shouldn't name SetGatewayName to OverwriteGatewayName, as that's what is effectively happening.

bartoszmajsak · 2023-10-25T11:25:53Z

pkg/servicemesh/ossmcommon/operations_unit_test.go

+	"strings"
+)
+
+var _ = Describe("SetGatewayName", func() {


Name test after feature/behaviour you want to test, not the func name.

bartoszmajsak · 2023-10-25T11:27:19Z

pkg/servicemesh/ossmcommon/operations_unit_test.go

+	BeforeEach(func() {
+		var err error
+
+		tempDir, err = os.MkdirTemp("", "ossm-test")


Starting from Go 1.15, the standard testing package includes T.TempDir(), perhaps worth giving a try, especially that it's fully supported by Ginkgo GinkgoT().TempDir(). You will at least save few lines of code for cleanup :)

bartoszmajsak · 2023-10-25T11:56:33Z

pkg/servicemesh/ossmcommon/operations_unit_test.go

+		Expect(err).NotTo(HaveOccurred())
+
+		expected := "ISTIO_GATEWAY=testnamespace/odh-gateway"
+		Expect(strings.Contains(string(updatedContents), expected)).To(BeTrue(), "Expected content to contain %q, got %q", expected, updatedContents)


I think we can leverage built-in Gomega matchers for simplicity https://onsi.github.io/gomega/#working-with-strings-json-and-yaml

- intialization of Service Mesh Auth(z) as part of DSCI - support for Dashboard and Workbenches components

Update components/workbenches/workbenches.go Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> consolidate regex and string replace funcs into one w helper move to package, add testing move out gateway const rename to ossmcommon clean up unit tests rename pkg, move io funcs, rename gw func

bartoszmajsak · 2023-10-26T11:55:51Z

Merged in 724837d

cam-garrison requested review from aslakknutsen and bartoszmajsak October 18, 2023 13:30

bartoszmajsak requested changes Oct 23, 2023

View reviewed changes

bartoszmajsak force-pushed the service-mesh-integration branch from 290f1b1 to e0f68b7 Compare October 24, 2023 12:03

cam-garrison force-pushed the parameterize-gw-ns branch 2 times, most recently from dc96a6e to 5c07176 Compare October 24, 2023 13:45

cam-garrison requested a review from bartoszmajsak October 24, 2023 18:35

bartoszmajsak requested changes Oct 25, 2023

View reviewed changes

bartoszmajsak force-pushed the service-mesh-integration branch 2 times, most recently from e015bb4 to 876e587 Compare October 25, 2023 16:54

feat(service-mesh): adds service mesh support

454edb4

- intialization of Service Mesh Auth(z) as part of DSCI - support for Dashboard and Workbenches components

bartoszmajsak force-pushed the service-mesh-integration branch from 876e587 to 454edb4 Compare October 25, 2023 17:04

cam-garrison added 2 commits October 25, 2023 14:47

update imports, feature io

e16c812

cam-garrison force-pushed the parameterize-gw-ns branch from 3127fe6 to e16c812 Compare October 25, 2023 18:49

move io pkg out to feature level, rm raw_resource

92af072

bartoszmajsak force-pushed the service-mesh-integration branch from 454edb4 to 724837d Compare October 26, 2023 11:54

bartoszmajsak closed this Oct 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameterize gateway namespace used by nb-ctrlr #14

Parameterize gateway namespace used by nb-ctrlr #14

cam-garrison commented Oct 18, 2023

bartoszmajsak left a comment

bartoszmajsak Oct 23, 2023

cam-garrison Oct 23, 2023

bartoszmajsak Oct 23, 2023

cam-garrison Oct 23, 2023

cam-garrison Oct 23, 2023

bartoszmajsak Oct 23, 2023

cam-garrison Oct 23, 2023

bartoszmajsak Oct 23, 2023

cam-garrison Oct 24, 2023 •

edited

Loading

bartoszmajsak Oct 25, 2023

bartoszmajsak Oct 25, 2023

bartoszmajsak Oct 25, 2023

bartoszmajsak Oct 25, 2023

bartoszmajsak Oct 25, 2023

bartoszmajsak commented Oct 26, 2023


		const gatewayPattern = `ISTIO_GATEWAY=(.*)/odh-gateway`

		func SetGatewayName(dscispec *dsci.DSCInitializationSpec, kfnotebookControllerServiceMeshPath string) error {

Parameterize gateway namespace used by nb-ctrlr #14

Parameterize gateway namespace used by nb-ctrlr #14

Conversation

cam-garrison commented Oct 18, 2023

bartoszmajsak 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

cam-garrison Oct 24, 2023 • 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

bartoszmajsak commented Oct 26, 2023

cam-garrison Oct 24, 2023 •

edited

Loading