From c0c5c0f8400a6c523dcc52b87fb869bc57db2578 Mon Sep 17 00:00:00 2001 From: Benny Zlotnik Date: Sun, 3 Dec 2023 18:22:45 +0200 Subject: [PATCH 1/3] ovirt,openstack: use env instead of files for secrets Use envFrom to pass secrets to the populator controller. This simplifies the code, and makes the openstack test introduced in the patch easier to implement. Signed-off-by: Benny Zlotnik --- .../openstack-populator.go | 50 ++++++++----------- cmd/ovirt-populator/ovirt-populator.go | 44 ++++------------ .../populator-machinery/controller.go | 23 ++++----- 3 files changed, 42 insertions(+), 75 deletions(-) diff --git a/cmd/openstack-populator/openstack-populator.go b/cmd/openstack-populator/openstack-populator.go index e4e0ebaa9..89e016b1d 100644 --- a/cmd/openstack-populator/openstack-populator.go +++ b/cmd/openstack-populator/openstack-populator.go @@ -5,7 +5,6 @@ import ( "io" "net/http" "os" - "path/filepath" "strings" "time" @@ -60,17 +59,14 @@ func populate(fileName, identityEndpoint, secretName, imageID string) { klog.Info("Prometheus progress counter registered.") } - options, err := readOptions() - if err != nil { - klog.Fatal(err) - } + options := readOptions() client := &libclient.Client{ URL: identityEndpoint, Options: options, } - err = client.Connect() + err := client.Connect() if err != nil { klog.Fatal(err) } @@ -143,34 +139,30 @@ func writeData(reader io.ReadCloser, file *os.File, imageID string, progress *pr return nil } -func readOptions() (options map[string]string, err error) { +func readOptions() (options map[string]string) { options = map[string]string{} - secretDirPath := "/etc/secret-volume" - dirEntries, err := os.ReadDir(secretDirPath) - if err != nil { - return + + // List of options to read from environment variables + envOptions := []string{ + "regionName", "authType", "username", "userID", "password", + "applicationCredentialID", "applicationCredentialName", "applicationCredentialSecret", + "token", "systemScope", "projectName", "projectID", "userDomainName", + "userDomainID", "projectDomainName", "projectDomainID", "domainName", + "domainID", "defaultDomain", "insecureSkipVerify", "cacert", "availability", } + klog.Info("Options:") - for _, dirEntry := range dirEntries { - if !dirEntry.Type().IsDir() { - option := dirEntry.Name() - if strings.HasPrefix(option, "..") { - continue - } - filePath := filepath.Join(secretDirPath, option) - var fileContent []byte - fileContent, err = os.ReadFile(filePath) - if err != nil { - return - } - value := string(fileContent) - options[option] = value - if option == "password" || option == "applicationCredentialSecret" || option == "token" { - value = strings.Repeat("*", len(value)) - } - klog.Info(" - ", option, " = ", value) + for _, option := range envOptions { + value := os.Getenv(option) + options[option] = value + // Mask sensitive information + if option == "password" || option == "applicationCredentialSecret" || option == "token" { + value = strings.Repeat("*", len(value)) } + + klog.Info(" - ", option, " = ", value) } + return } diff --git a/cmd/ovirt-populator/ovirt-populator.go b/cmd/ovirt-populator/ovirt-populator.go index 64ba24157..4106cd047 100644 --- a/cmd/ovirt-populator/ovirt-populator.go +++ b/cmd/ovirt-populator/ovirt-populator.go @@ -156,50 +156,28 @@ func populate(engineURL, diskID, volPath string) { } func loadEngineConfig(engineURL string) engineConfig { - user, err := os.ReadFile("/etc/secret-volume/user") - if err != nil { - klog.Fatal(err.Error()) - } - pass, err := os.ReadFile("/etc/secret-volume/password") - if err != nil { - klog.Fatal(err.Error()) - } + user := os.Getenv("user") + pass := os.Getenv("password") - var insecureSkipVerify []byte - _, err = os.Stat("/etc/secret-volume/insecureSkipVerify") - if err != nil { - if errors.Is(err, os.ErrNotExist) { - insecureSkipVerify = []byte("false") - } else { - klog.Fatal(err.Error()) - } - } else { - insecureSkipVerify, err = os.ReadFile("/etc/secret-volume/insecureSkipVerify") - if err != nil { - klog.Fatal(err.Error()) - } + var insecureSkipVerify string + insecureSkipVerify, found := os.LookupEnv("insecureSkipVerify") + if !found { + insecureSkipVerify = "false" } insecure, err := strconv.ParseBool(string(insecureSkipVerify)) if err != nil { klog.Fatal(err.Error()) } + //If the insecure option is set, the ca file field in the secret is not required. - var cacert []byte - if insecure { - cacert = []byte("") - } else { - cacert, err = os.ReadFile("/etc/secret-volume/cacert") - if err != nil { - klog.Error(err.Error()) - } - } + cacert := os.Getenv("cacert") return engineConfig{ URL: engineURL, - username: string(user), - password: string(pass), - cacert: string(cacert), + username: user, + password: pass, + cacert: cacert, insecure: insecure, } } diff --git a/pkg/lib-volume-populator/populator-machinery/controller.go b/pkg/lib-volume-populator/populator-machinery/controller.go index e92f690b8..0faab98e8 100644 --- a/pkg/lib-volume-populator/populator-machinery/controller.go +++ b/pkg/lib-volume-populator/populator-machinery/controller.go @@ -628,11 +628,7 @@ func (c *controller) syncPvc(ctx context.Context, key, pvcNamespace, pvcName str }, } } - con.VolumeMounts = append(con.VolumeMounts, corev1.VolumeMount{ - Name: "secret-volume", - ReadOnly: true, - MountPath: "/etc/secret-volume", - }) + if waitForFirstConsumer { pod.Spec.NodeName = nodeName } @@ -940,6 +936,15 @@ func makePopulatePodSpec(pvcPrimeName, secretName string) corev1.PodSpec { Drop: []corev1.Capability{"ALL"}, }, }, + EnvFrom: []corev1.EnvFromSource{ + { + SecretRef: &corev1.SecretEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: secretName, + }, + }, + }, + }, }, }, SecurityContext: &corev1.PodSecurityContext{ @@ -958,14 +963,6 @@ func makePopulatePodSpec(pvcPrimeName, secretName string) corev1.PodSpec { }, }, }, - { - Name: "secret-volume", - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: secretName, - }, - }, - }, }, } } From 7b6a7cd963bd2d2abece73eae80b8527cdfd8724 Mon Sep 17 00:00:00 2001 From: Benny Zlotnik Date: Sun, 3 Dec 2023 18:23:33 +0200 Subject: [PATCH 2/3] openstack: Add simple test for the openstack populator It uses a fake openstack server, but a real file is created Signed-off-by: Benny Zlotnik --- cmd/openstack-populator/BUILD.bazel | 8 +- .../openstack-populator_test.go | 173 ++++++++++++++++++ cmd/ovirt-populator/ovirt-populator.go | 1 - 3 files changed, 180 insertions(+), 2 deletions(-) create mode 100644 cmd/openstack-populator/openstack-populator_test.go diff --git a/cmd/openstack-populator/BUILD.bazel b/cmd/openstack-populator/BUILD.bazel index 0ea7cbece..34f654219 100644 --- a/cmd/openstack-populator/BUILD.bazel +++ b/cmd/openstack-populator/BUILD.bazel @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test") load( "@io_bazel_rules_docker//container:container.bzl", "container_image", @@ -31,3 +31,9 @@ container_image( files = [":openstack-populator"], visibility = ["//visibility:public"], ) + +go_test( + name = "openstack-populator_test", + srcs = ["openstack-populator_test.go"], + embed = [":openstack-populator_lib"], +) diff --git a/cmd/openstack-populator/openstack-populator_test.go b/cmd/openstack-populator/openstack-populator_test.go new file mode 100644 index 000000000..fc3700554 --- /dev/null +++ b/cmd/openstack-populator/openstack-populator_test.go @@ -0,0 +1,173 @@ +package main + +import ( + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "os" + "testing" +) + +func setupMockServer() (*httptest.Server, string, int, error) { + listener, err := net.Listen("tcp", ":0") + if err != nil { + return nil, "", 0, err + } + + mux := http.NewServeMux() + + port := listener.Addr().(*net.TCPAddr).Port + identityServerURL := fmt.Sprintf("http://localhost:%d", port) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + response := fmt.Sprintf(`{ + "versions": { + "values": [ + { + "id": "v3.0", + "links": [ + {"rel": "self", "href": "%s/v3/"} + ], + "status": "stable" + } + ] + } + }`, identityServerURL) + fmt.Fprint(w, response) + }) + + mux.HandleFunc("/v2/images/", func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintln(w, `mock_data`) + }) + + mux.HandleFunc("/v3/auth/tokens", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Subject-Token", "MIIFvgY") + w.WriteHeader(http.StatusCreated) + identityPublicURL := fmt.Sprintf("http://localhost:%d/v3/", port) + identityAdminURL := fmt.Sprintf("http://localhost:%d/v3/", port) + identityInternalURL := fmt.Sprintf("http://localhost:%d/v3/", port) + imageServiceURL := fmt.Sprintf("http://localhost:%d/v2/images", port) + + response := fmt.Sprintf(`{ + "token": { + "methods": ["password"], + "project": { + "domain": { + "id": "default", + "name": "Default" + }, + "id": "8538a3f13f9541b28c2620eb19065e45", + "name": "admin" + }, + "catalog": [ + { + "type": "identity", + "name": "keystone", + "endpoints": [ + { + "url": "%s", + "region": "RegionOne", + "interface": "public", + "id": "identity-public-endpoint-id" + }, + { + "url": "%s", + "region": "RegionOne", + "interface": "admin", + "id": "identity-admin-endpoint-id" + }, + { + "url": "%s", + "region": "RegionOne", + "interface": "internal", + "id": "identity-internal-endpoint-id" + } + ] + }, + { + "type": "image", + "name": "glance", + "endpoints": [ + { + "url": "%s", + "region": "RegionOne", + "interface": "public", + "id": "image-public-endpoint-id" + } + ] + } + ], + "user": { + "domain": { + "id": "default", + "name": "Default" + }, + "id": "3ec3164f750146be97f21559ee4d9c51", + "name": "admin" + }, + "issued_at": "201406-10T20:55:16.806027Z" + } + }`, + identityPublicURL, + identityAdminURL, + identityInternalURL, + imageServiceURL) + + fmt.Fprint(w, response) + }) + + server := httptest.NewUnstartedServer(mux) + server.Listener = listener + + server.Start() + + return server, identityServerURL, port, nil +} + +func TestPopulate(t *testing.T) { + os.Setenv("username", "testuser") + os.Setenv("password", "testpassword") + os.Setenv("projectName", "Default") + os.Setenv("domainName", "Default") + os.Setenv("insecureSkipVerify", "true") + os.Setenv("availability", "public") + os.Setenv("regionName", "RegionOne") + os.Setenv("authType", "password") + + server, identityServerURL, port, err := setupMockServer() + if err != nil { + t.Fatalf("Failed to start mock server: %v", err) + } + defer server.Close() + + fmt.Printf("Mock server running on port: %d\n", port) + + fileName := "disk.img" + secretName := "test-secret" + imageID := "test-image-id" + + fmt.Println("server ", identityServerURL) + populate(fileName, identityServerURL, secretName, imageID) + + file, err := os.Open(fileName) + if err != nil { + t.Fatalf("Failed to open file: %v", err) + } + defer file.Close() // Ensure the file is closed after reading + + content, err := io.ReadAll(file) + if err != nil { + t.Fatalf("Failed to read file: %v", err) + } + + if string(content) != "mock_data\n" { + t.Errorf("Expected %s, got %s", "mock_data\n", string(content)) + } + + os.Remove(fileName) +} diff --git a/cmd/ovirt-populator/ovirt-populator.go b/cmd/ovirt-populator/ovirt-populator.go index 4106cd047..aaeaa4d26 100644 --- a/cmd/ovirt-populator/ovirt-populator.go +++ b/cmd/ovirt-populator/ovirt-populator.go @@ -159,7 +159,6 @@ func loadEngineConfig(engineURL string) engineConfig { user := os.Getenv("user") pass := os.Getenv("password") - var insecureSkipVerify string insecureSkipVerify, found := os.LookupEnv("insecureSkipVerify") if !found { insecureSkipVerify = "false" From c77f297b10d6e7f3c03134f3b50102d47f9d3d70 Mon Sep 17 00:00:00 2001 From: Benny Zlotnik Date: Tue, 19 Dec 2023 12:52:51 +0200 Subject: [PATCH 3/3] remove duplication Signed-off-by: Benny Zlotnik --- .../openstack-populator_test.go | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/cmd/openstack-populator/openstack-populator_test.go b/cmd/openstack-populator/openstack-populator_test.go index fc3700554..53760f75b 100644 --- a/cmd/openstack-populator/openstack-populator_test.go +++ b/cmd/openstack-populator/openstack-populator_test.go @@ -19,7 +19,7 @@ func setupMockServer() (*httptest.Server, string, int, error) { mux := http.NewServeMux() port := listener.Addr().(*net.TCPAddr).Port - identityServerURL := fmt.Sprintf("http://localhost:%d", port) + baseURL := fmt.Sprintf("http://localhost:%d", port) mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -36,7 +36,7 @@ func setupMockServer() (*httptest.Server, string, int, error) { } ] } - }`, identityServerURL) + }`, baseURL) fmt.Fprint(w, response) }) @@ -48,11 +48,9 @@ func setupMockServer() (*httptest.Server, string, int, error) { w.Header().Set("Content-Type", "application/json") w.Header().Set("X-Subject-Token", "MIIFvgY") w.WriteHeader(http.StatusCreated) - identityPublicURL := fmt.Sprintf("http://localhost:%d/v3/", port) - identityAdminURL := fmt.Sprintf("http://localhost:%d/v3/", port) - identityInternalURL := fmt.Sprintf("http://localhost:%d/v3/", port) - imageServiceURL := fmt.Sprintf("http://localhost:%d/v2/images", port) - + identityServer := fmt.Sprintf("%s/v3/", baseURL) + imageServiceURL := fmt.Sprintf("%s/v2/images", baseURL) + fmt.Println("identityServer ", identityServer) response := fmt.Sprintf(`{ "token": { "methods": ["password"], @@ -113,9 +111,9 @@ func setupMockServer() (*httptest.Server, string, int, error) { "issued_at": "201406-10T20:55:16.806027Z" } }`, - identityPublicURL, - identityAdminURL, - identityInternalURL, + identityServer, + identityServer, + identityServer, imageServiceURL) fmt.Fprint(w, response) @@ -126,7 +124,7 @@ func setupMockServer() (*httptest.Server, string, int, error) { server.Start() - return server, identityServerURL, port, nil + return server, baseURL, port, nil } func TestPopulate(t *testing.T) { @@ -166,7 +164,7 @@ func TestPopulate(t *testing.T) { } if string(content) != "mock_data\n" { - t.Errorf("Expected %s, got %s", "mock_data\n", string(content)) + t.Errorf("Expected %s, got %s", "mock_data", string(content)) } os.Remove(fileName)