From 2837ff5659669de826996fa730ad63af22503274 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Tue, 8 Oct 2024 09:40:49 +0530 Subject: [PATCH 01/11] Use proper URLs for initial-advertise-peer-urls & advertise-client-urls instead of @ separator --- pkg/member/member_control.go | 9 +- pkg/member/member_control_test.go | 31 ++- pkg/miscellaneous/miscellaneous.go | 97 +++++-- pkg/miscellaneous/miscellaneous_test.go | 260 +++++++++++++++++-- pkg/miscellaneous/testdata/valid_config.yaml | 13 +- pkg/server/httpAPI.go | 34 +-- test/e2e/integration/cloud_backup_test.go | 15 +- 7 files changed, 372 insertions(+), 87 deletions(-) diff --git a/pkg/member/member_control.go b/pkg/member/member_control.go index 2575fc50b..97b884d67 100644 --- a/pkg/member/member_control.go +++ b/pkg/member/member_control.go @@ -116,7 +116,9 @@ func NewMemberControl(etcdConnConfig *brtypes.EtcdConnectionConfig) Control { // AddMemberAsLearner add a member as a learner to the etcd cluster func (m *memberControl) AddMemberAsLearner(ctx context.Context) error { //Add member as learner to cluster - memberURL, err := miscellaneous.GetMemberPeerURL(m.configFile, m.podName) + // TODO: Need to handle multiple peer URLs once etcd config is updated to support it. + // It is required in the context of Gardener usecase to support live control plane migration. + memberURL, err := miscellaneous.GetAdvertisePeerURLs(m.configFile) if err != nil { m.logger.Fatalf("Error fetching etcd member URL : %v", err) } @@ -205,8 +207,9 @@ func (m *memberControl) IsMemberInCluster(ctx context.Context) (bool, error) { func (m *memberControl) doUpdateMemberPeerAddress(ctx context.Context, cli etcdClient.ClusterCloser, id uint64) error { // Already existing clusters or cluster after restoration have `http://localhost:2380` as the peer address. This needs to explicitly updated to the correct peer address. m.logger.Infof("Updating member peer URL for %s", m.podName) - - memberPeerURL, err := miscellaneous.GetMemberPeerURL(m.configFile, m.podName) + // TODO: Need to handle multiple peer URLs once etcd config is updated to support it. + // It is required in the context of Gardener usecase to support live control plane migration. + memberPeerURL, err := miscellaneous.GetAdvertisePeerURLs(m.configFile) if err != nil { return fmt.Errorf("could not fetch member URL : %v", err) } diff --git a/pkg/member/member_control_test.go b/pkg/member/member_control_test.go index 33ac98e14..d4d71785b 100644 --- a/pkg/member/member_control_test.go +++ b/pkg/member/member_control_test.go @@ -47,20 +47,23 @@ var _ = Describe("Membercontrol", func() { outfile := "/tmp/etcd.conf.yaml" etcdConfigYaml := `# Human-readable name for this member. - name: etcd1 - data-dir: ` + os.Getenv("ETCD_DATA_DIR") + ` - metrics: extensive - snapshot-count: 75000 - enable-v2: false - quota-backend-bytes: 1073741824 - listen-client-urls: http://0.0.0.0:2379 - advertise-client-urls: http://0.0.0.0:2379 - initial-advertise-peer-urls: http@etcd-main-peer@default@2380 - initial-cluster: etcd1=http://0.0.0.0:2380 - initial-cluster-token: new - initial-cluster-state: new - auto-compaction-mode: periodic - auto-compaction-retention: 30m` +name: etcd1 +data-dir: ` + os.Getenv("ETCD_DATA_DIR") + ` +metrics: extensive +snapshot-count: 75000 +enable-v2: false +quota-backend-bytes: 1073741824 +listen-client-urls: http://0.0.0.0:2379 +advertise-client-urls: + ` + podName + `: http://0.0.0.0:2379 +initial-advertise-peer-urls: + ` + podName + `: + - http://etcd-main-peer.default:2380 +initial-cluster: etcd1=http://0.0.0.0:2380 +initial-cluster-token: new +initial-cluster-state: new +auto-compaction-mode: periodic +auto-compaction-retention: 30m` err := os.WriteFile(outfile, []byte(etcdConfigYaml), 0755) Expect(err).ShouldNot(HaveOccurred()) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 1cdc50614..007616a98 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -535,41 +535,100 @@ func ReadConfigFileAsMap(path string) (map[string]interface{}, error) { return c, nil } -// ParsePeerURL forms a PeerURL, given podName by parsing the initial-advertise-peer-urls -func ParsePeerURL(initialAdvertisePeerURLs, podName string) (string, error) { - tokens := strings.Split(initialAdvertisePeerURLs, "@") - if len(tokens) < 4 { - return "", fmt.Errorf("invalid peer URL : %s", initialAdvertisePeerURLs) +// GetAdvertisePeerURLs returns the advertise peer URLs for the etcd member. +func GetAdvertisePeerURLs(configFile string) (string, error) { + memberName, err := GetEnvVarOrError("POD_NAME") + if err != nil { + return "", err } - domaiName := fmt.Sprintf("%s.%s.%s", tokens[1], tokens[2], "svc") - return fmt.Sprintf("%s://%s.%s:%s", tokens[0], podName, domaiName, tokens[3]), nil -} - -// IsPeerURLTLSEnabled checks whether the peer address is TLS enabled or not. -func IsPeerURLTLSEnabled() (bool, error) { - podName, err := GetEnvVarOrError("POD_NAME") + config, err := ReadConfigFileAsMap(configFile) if err != nil { - return false, err + return "", err + } + initAdPeerURL := config["initial-advertise-peer-urls"] + if initAdPeerURL == nil { + return "", fmt.Errorf("initial-advertise-peer-urls must be set in etcd config") } - configFile := GetConfigFilePath() + peerUrlsMap, ok := initAdPeerURL.(map[interface{}]interface{}) + if !ok { + return "", fmt.Errorf("initial-advertise-peer-urls is not in the expected format") + } + resultMap := make(map[string][]string) + for pod, urls := range peerUrlsMap { + podName, ok := pod.(string) + if !ok { + return "", fmt.Errorf("pod name is not a string") + } + urlsList, ok := urls.([]interface{}) + if !ok { + return "", fmt.Errorf("urls is not a list") + } + for _, url := range urlsList { + urlStr, ok := url.(string) + if !ok { + return "", fmt.Errorf("url is not a string") + } + resultMap[podName] = append(resultMap[podName], urlStr) + } + } + peerUrls, ok := resultMap[memberName] + if !ok { + return "", fmt.Errorf("peer url not found for pod %s", memberName) + } + return strings.Join(peerUrls, ","), nil +} +// GetAdvertiseClientURL returns the advertise client URL for the etcd member. +func GetAdvertiseClientURL(configFile string) (string, error) { + memberName, err := GetEnvVarOrError("POD_NAME") + if err != nil { + return "", err + } config, err := ReadConfigFileAsMap(configFile) if err != nil { - return false, err + return "", err } - initAdPeerURL := config["initial-advertise-peer-urls"] + initAdClientURL := config["advertise-client-urls"] + if initAdClientURL == nil { + return "", fmt.Errorf("advertise-client-urls must be set in etcd config") + } + clientUrlsMap, ok := initAdClientURL.(map[interface{}]interface{}) + if !ok { + return "", fmt.Errorf("advertise-client-urls is not in the expected format") + } + resultMap := make(map[string]string) + for pod, url := range clientUrlsMap { + podName, ok := pod.(string) + if !ok { + return "", fmt.Errorf("pod name is not a string") + } + urlStr, ok := url.(string) + if !ok { + return "", fmt.Errorf("url is not a string") + } + resultMap[podName] = urlStr + } + clientURL, ok := resultMap[memberName] + if !ok { + return "", fmt.Errorf("client url not found for pod %s", memberName) + } + return clientURL, nil +} - memberPeerURL, err := ParsePeerURL(initAdPeerURL.(string), podName) +// IsPeerURLTLSEnabled checks whether the peer address is TLS enabled or not. +func IsPeerURLTLSEnabled() (bool, error) { + configFile := GetConfigFilePath() + // TODO: Need to handle multiple peer URLs once etcd config is updated to support it. + // It is required in the context of Gardener usecase to support live control plane migration. + memberPeerURL, err := GetAdvertisePeerURLs(configFile) if err != nil { return false, err } - peerURL, err := url.Parse(memberPeerURL) if err != nil { return false, err } - return peerURL.Scheme == https, nil } diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index 7b0d13cdb..6fe3a20e9 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -11,12 +11,14 @@ import ( "os" "path/filepath" "reflect" + "strings" "time" mockfactory "github.com/gardener/etcd-backup-restore/pkg/mock/etcdutil/client" "github.com/gardener/etcd-backup-restore/pkg/snapstore" brtypes "github.com/gardener/etcd-backup-restore/pkg/types" "go.uber.org/mock/gomock" + "sigs.k8s.io/yaml" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -586,28 +588,224 @@ var _ = Describe("Miscellaneous Tests", func() { }) }) - Describe("parse peer urls config", func() { - var ( - initialAdPeerURL string - podName string + Describe("GetAdvertisePeerURLs", func() { + const ( + configFile = "/tmp/etcd-config.yaml" + podName = "test-pod" ) + Context("When POD_NAME environment variable is not set", func() { + It("should return an error", func() { + _, err := GetAdvertisePeerURLs(configFile) + Expect(err).NotTo(BeNil()) + }) + }) - BeforeEach(func() { - podName = "etcd-test-pod-0" + Context("When POD_NAME environment variable is set", func() { + BeforeEach(func() { + Expect(os.Setenv("POD_NAME", podName)).To(Succeed()) + Expect(os.Setenv("ETCD_CONF", configFile)).To(Succeed()) + }) + AfterEach(func() { + Expect(os.Unsetenv("POD_NAME")).To(Succeed()) + Expect(os.Unsetenv("ETCD_CONF")).To(Succeed()) + }) + + Context("When the config file cannot be read", func() { + It("should return an error", func() { + _, err := GetAdvertisePeerURLs(configFile) + Expect(err).NotTo(BeNil()) + }) + }) + + Context("When initial-advertise-peer-urls is not set in the config file", func() { + var config map[string]interface{} + + BeforeEach(func() { + config = map[string]interface{}{ + "name": "etcd-test", + } + writeConfigToFile(configFile, config) + }) + + AfterEach(func() { + Expect(os.Remove(configFile)).To(Succeed()) + }) + + It("should return an error", func() { + _, err := GetAdvertisePeerURLs(configFile) + Expect(err).NotTo(BeNil()) + }) + }) + + Context("When initial-advertise-peer-urls is set in the config file", func() { + var config map[string]interface{} + podUrlsMap := make(map[string]interface{}) + + AfterEach(func() { + Expect(os.Remove(configFile)).To(Succeed()) + }) + + Context("When the initial-advertise-peer-urls is not in the expected format", func() { + BeforeEach(func() { + config = map[string]interface{}{ + "name": "etcd-test", + "initial-advertise-peer-urls": "invalid-format", + } + writeConfigToFile(configFile, config) + }) + + It("should return an error", func() { + _, err := GetAdvertisePeerURLs(configFile) + Expect(err).NotTo(BeNil()) + }) + }) + + Context("When the pod name is not present in the config file", func() { + BeforeEach(func() { + otherPodPeerURLs := []string{"http://pod1:2380", "http://pod1:2381"} + podUrlsMap["other-pod"] = otherPodPeerURLs + + config = map[string]interface{}{ + "name": "etcd-test", + "initial-advertise-peer-urls": podUrlsMap, + } + writeConfigToFile(configFile, config) + }) + + It("should return an error", func() { + _, err := GetAdvertisePeerURLs(configFile) + Expect(err).NotTo(BeNil()) + }) + }) + + Context("When the pod name is present in the config file", func() { + var podPeerURLs []string + BeforeEach(func() { + podPeerURLs = []string{"http://pod:2380", "http://pod:2381"} + podUrlsMap[podName] = podPeerURLs + + config = map[string]interface{}{ + "name": "etcd-test", + "initial-advertise-peer-urls": podUrlsMap, + } + writeConfigToFile(configFile, config) + }) + + It("should return the peer URLs", func() { + peerURLs, err := GetAdvertisePeerURLs(configFile) + Expect(err).NotTo(HaveOccurred()) + Expect(peerURLs).To(Equal(strings.Join(podPeerURLs, ","))) + }) + }) + }) }) - Context("parse peer url", func() { - It("parsing well-defined initial-advertise-peer-urls", func() { - initialAdPeerURL = "https@etcd-events-peer@shoot--dev--test@2380" - peerURL, err := ParsePeerURL(initialAdPeerURL, podName) - Expect(err).To(BeNil()) - Expect(peerURL).To(Equal("https://etcd-test-pod-0.etcd-events-peer.shoot--dev--test.svc:2380")) + }) + + Describe("GetAdvertiseClientURL", func() { + const ( + configFile = "/tmp/etcd-config.yaml" + podName = "test-pod" + ) + + Context("When POD_NAME environment variable is not set", func() { + It("should return an error", func() { + url, err := GetAdvertiseClientURL(configFile) + Expect(err).To(HaveOccurred()) + Expect(url).To(BeEmpty()) + }) + }) + + Context("When POD_NAME environment variable is set", func() { + BeforeEach(func() { + Expect(os.Setenv("POD_NAME", podName)).To(Succeed()) + Expect(os.Setenv("ETCD_CONF", configFile)).To(Succeed()) }) - It("parsing malformed initial-advertise-peer-urls", func() { - initialAdPeerURL = "https@etcd-events-peer@shoot--dev--test" - _, err := ParsePeerURL(initialAdPeerURL, podName) - Expect(err).ToNot(BeNil()) + AfterEach(func() { + Expect(os.Unsetenv("POD_NAME")).To(Succeed()) + Expect(os.Unsetenv("ETCD_CONF")).To(Succeed()) + }) + + Context("When the config file cannot be read", func() { + It("should return an error", func() { + _, err := GetAdvertiseClientURL(configFile) + Expect(err).To(Not(BeNil())) + }) + }) + + Context("When advertise-client-urls is not set in the etcd config", func() { + BeforeEach(func() { + config := map[string]interface{}{ + "name": "etcd-test", + } + writeConfigToFile(configFile, config) + }) + + AfterEach(func() { + Expect(os.Remove(configFile)).To(Succeed()) + }) + + It("should return an error", func() { + url, err := GetAdvertiseClientURL(configFile) + Expect(err).To(HaveOccurred()) + Expect(url).To(BeEmpty()) + }) + }) + + Context("When advertise-client-urls is set in the etcd config", func() { + podUrlsMap := make(map[string]interface{}) + AfterEach(func() { + Expect(os.Remove(configFile)).To(Succeed()) + }) + + Context("When advertise-client-urls is not in the expected format", func() { + BeforeEach(func() { + config := map[string]interface{}{ + "name": "etcd-test", + "advertise-client-urls": "invalid-format", + } + writeConfigToFile(configFile, config) + }) + + It("should return an error", func() { + url, err := GetAdvertiseClientURL(configFile) + Expect(err).To(HaveOccurred()) + Expect(url).To(BeEmpty()) + }) + }) + + Context("When the client URL is not found for the pod", func() { + BeforeEach(func() { + podUrlsMap["other-pod"] = "http://localhost:2379" + config := map[string]interface{}{ + "advertise-client-urls": podUrlsMap, + } + writeConfigToFile(configFile, config) + }) + + It("should return an error", func() { + url, err := GetAdvertiseClientURL(configFile) + Expect(err).To(HaveOccurred()) + Expect(url).To(BeEmpty()) + }) + }) + + Context("When the client URL is found for the pod", func() { + BeforeEach(func() { + podUrlsMap[podName] = "http://localhost:2379" + config := map[string]interface{}{ + "advertise-client-urls": podUrlsMap, + } + writeConfigToFile(configFile, config) + }) + + It("should return the client URL", func() { + url, err := GetAdvertiseClientURL(configFile) + Expect(err).ToNot(HaveOccurred()) + Expect(url).To(Equal("http://localhost:2379")) + }) + }) }) }) }) @@ -714,7 +912,16 @@ var _ = Describe("Miscellaneous Tests", func() { Context("with non-TLS enabled peer url", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 -initial-advertise-peer-urls: http@etcd-main-peer@default@2380 +initial-advertise-peer-urls: + test_pod: + - http://etcd-main-peer.default:2380 + - http://etcd-main-peer.default:2381 + test_pod2: + - http://etcd-main-peer.default:2380 + - http://etcd-main-peer.default:2381 + test_pod3: + - http://etcd-main-peer.default:2380 + - http://etcd-main-peer.default:2381 initial-cluster: etcd1=http://0.0.0.0:2380` err := os.WriteFile(outfile, []byte(etcdConfigYaml), 0755) Expect(err).ShouldNot(HaveOccurred()) @@ -730,7 +937,16 @@ initial-cluster: etcd1=http://0.0.0.0:2380` Context("with TLS enabled peer url", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 -initial-advertise-peer-urls: https@etcd-main-peer@default@2380 +initial-advertise-peer-urls: + test_pod: + - https://etcd-main-peer.default:2380 + - https://etcd-main-peer.default:2381 + test_pod2: + - https://etcd-main-peer.default:2380 + - https://etcd-main-peer.default:2381 + test_pod3: + - https://etcd-main-peer.default:2380 + - https://etcd-main-peer.default:2381 initial-cluster: etcd1=https://0.0.0.0:2380` err := os.WriteFile(outfile, []byte(etcdConfigYaml), 0755) Expect(err).ShouldNot(HaveOccurred()) @@ -832,3 +1048,11 @@ func (ds *DummyStore) Save(snap brtypes.Snapshot, rc io.ReadCloser) error { func (ds *DummyStore) Fetch(snap brtypes.Snapshot) (io.ReadCloser, error) { return nil, nil } + +func writeConfigToFile(configFile string, config map[string]interface{}) { + byteSlice, err := yaml.Marshal(config) + Expect(err).NotTo(HaveOccurred()) + + err = os.WriteFile(configFile, byteSlice, 0644) + Expect(err).NotTo(HaveOccurred()) +} diff --git a/pkg/miscellaneous/testdata/valid_config.yaml b/pkg/miscellaneous/testdata/valid_config.yaml index f711d5ccd..dbf6437da 100644 --- a/pkg/miscellaneous/testdata/valid_config.yaml +++ b/pkg/miscellaneous/testdata/valid_config.yaml @@ -31,7 +31,10 @@ listen-client-urls: https://0.0.0.0:2379 # List of this member's client URLs to advertise to the public. # The URLs needed to be a comma-separated list. -advertise-client-urls: https@etcd-events-peer@shoot--dev--test@2379 +advertise-client-urls: + etcd-events-0: https://etcd-events-0.etcd-events-peer.shoot--dev--test.svc:2379 + etcd-events-1: https://etcd-events-1.etcd-events-peer.shoot--dev--test.svc:2379 + etcd-events-2: https://etcd-events-2.etcd-events-peer.shoot--dev--test.svc:2379 peer-transport-security: # Path to the peer server TLS cert file. cert-file: /var/etcd/ssl/peer/server/tls.crt @@ -52,7 +55,13 @@ listen-peer-urls: https://0.0.0.0:2380 # List of this member's peer URLs to advertise to the public. # The URLs needed to be a comma-separated list. -initial-advertise-peer-urls: https@etcd-events-peer@shoot--dev--test@2380 +initial-advertise-peer-urls: + etcd-events-0: + - https://etcd-events-0.etcd-events-peer.shoot--dev--test.svc:2380 + etcd-events-1: + - https://etcd-events-1.etcd-events-peer.shoot--dev--test.svc:2380 + etcd-events-2: + - https://etcd-events-2.etcd-events-peer.shoot--dev--test.svc:2380 # Initial cluster token for the etcd cluster during bootstrap. initial-cluster-token: etcd-cluster diff --git a/pkg/server/httpAPI.go b/pkg/server/httpAPI.go index b02ced95b..51298a862 100644 --- a/pkg/server/httpAPI.go +++ b/pkg/server/httpAPI.go @@ -428,25 +428,23 @@ func (h *HTTPHandler) serveConfig(rw http.ResponseWriter, req *http.Request) { config["name"] = podName - initAdPeerURL := config["initial-advertise-peer-urls"] - protocol, svcName, namespace, peerPort, err := parsePeerURL(fmt.Sprint(initAdPeerURL)) + // fetch initial-advertise-peer-urls from etcd config file + initAdPeerURL, err := miscellaneous.GetAdvertisePeerURLs(inputFileName) if err != nil { - h.Logger.Warnf("Unable to determine service name, namespace, peer port from advertise peer urls : %v", err) + h.Logger.Warnf("Unable to get initial-advertise-peer-urls from etcd config file: %v", err) rw.WriteHeader(http.StatusInternalServerError) return } - domaiName := fmt.Sprintf("%s.%s.%s", svcName, namespace, "svc") - config["initial-advertise-peer-urls"] = fmt.Sprintf("%s://%s.%s:%s", protocol, podName, domaiName, peerPort) + config["initial-advertise-peer-urls"] = initAdPeerURL - advClientURL := config["advertise-client-urls"] - protocol, svcName, namespace, clientPort, err := parseAdvClientURL(fmt.Sprint(advClientURL)) + // fetch advertise-client-urls from etcd config file + advClientURL, err := miscellaneous.GetAdvertiseClientURL(inputFileName) if err != nil { - h.Logger.Warnf("Unable to determine service name, namespace, peer port from advertise client url : %v", err) + h.Logger.Warnf("Unable to get advertise-client-urls : %v", err) rw.WriteHeader(http.StatusInternalServerError) return } - domaiName = fmt.Sprintf("%s.%s.%s", svcName, namespace, "svc") - config["advertise-client-urls"] = fmt.Sprintf("%s://%s.%s:%s", protocol, podName, domaiName, clientPort) + config["advertise-client-urls"] = advClientURL config["initial-cluster"] = getInitialCluster(req.Context(), fmt.Sprint(config["initial-cluster"]), *h.EtcdConnectionConfig, *h.Logger, podName) @@ -568,22 +566,6 @@ func getInitialCluster(ctx context.Context, initialCluster string, etcdConn brty return initialCluster } -func parsePeerURL(peerURL string) (string, string, string, string, error) { - tokens := strings.Split(peerURL, "@") - if len(tokens) < 4 { - return "", "", "", "", fmt.Errorf("total length of tokens is less than four") - } - return tokens[0], tokens[1], tokens[2], tokens[3], nil -} - -func parseAdvClientURL(advClientURL string) (string, string, string, string, error) { - tokens := strings.Split(advClientURL, "@") - if len(tokens) < 4 { - return "", "", "", "", fmt.Errorf("total length of tokens is less than four") - } - return tokens[0], tokens[1], tokens[2], tokens[3], nil -} - // delegateReqToLeader forwards the incoming http/https request to BackupLeader. func (h *HTTPHandler) delegateReqToLeader(rw http.ResponseWriter, req *http.Request) { // Get the BackupLeader URL diff --git a/test/e2e/integration/cloud_backup_test.go b/test/e2e/integration/cloud_backup_test.go index 255b5cfd8..9af1d1f3b 100644 --- a/test/e2e/integration/cloud_backup_test.go +++ b/test/e2e/integration/cloud_backup_test.go @@ -118,6 +118,11 @@ var _ = Describe("CloudBackup", func() { Container: os.Getenv("TEST_ID"), Prefix: path.Join("v2"), } + // Required as the config file for embedded ETCD fetches ETCD instance name from the POD_NAME variable + podName := "etcd1" + podNamespace := "etcd-test" + Expect(os.Setenv("POD_NAME", podName)).To(Succeed()) + Expect(os.Setenv("POD_NAMESPACE", podNamespace)).To(Succeed()) // Create and place a ETCD config yaml outfile := "/tmp/etcd.conf.yaml" etcdConfigYaml := `# Human-readable name for this member. @@ -128,8 +133,11 @@ snapshot-count: 75000 enable-v2: false quota-backend-bytes: 1073741824 listen-client-urls: http://0.0.0.0:2379 -advertise-client-urls: http://0.0.0.0:2379 -initial-advertise-peer-urls: http://0.0.0.0:2380 +advertise-client-urls: + ` + podName + `: http://0.0.0.0:2379 +initial-advertise-peer-urls: + ` + podName + `: + - http://etcd-main-peer.default:2380 initial-cluster: etcd1=http://0.0.0.0:2380 initial-cluster-token: new initial-cluster-state: new @@ -139,9 +147,6 @@ auto-compaction-retention: 30m` err := os.WriteFile(outfile, []byte(etcdConfigYaml), 0755) Expect(err).ShouldNot(HaveOccurred()) Expect(os.Setenv("ETCD_CONF", outfile)).To(Succeed()) - // Required as the config file for embedded ETCD fetches ETCD instance name from the POD_NAME variable - Expect(os.Setenv("POD_NAME", "etcd1")).To(Succeed()) - Expect(os.Setenv("POD_NAMESPACE", "etcd-test")).To(Succeed()) }) Describe("Regular backups", func() { From 316107bace9e2e1f86955a6812b7a70f0cd5fc01 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Thu, 17 Oct 2024 16:21:58 +0530 Subject: [PATCH 02/11] changes after rebase --- pkg/miscellaneous/miscellaneous.go | 17 ----------------- pkg/server/backuprestoreserver.go | 7 +------ 2 files changed, 1 insertion(+), 23 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 007616a98..37c355412 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -670,23 +670,6 @@ func RemoveDir(dir string) error { return nil } -// GetMemberPeerURL returns the peerURL from fiven configuration file provided to etcd member. -func GetMemberPeerURL(configFile string, podName string) (string, error) { - config, err := ReadConfigFileAsMap(configFile) - if err != nil { - return "", err - } - initAdPeerURL := config["initial-advertise-peer-urls"] - if initAdPeerURL == nil { - return "", fmt.Errorf("initial-advertise-peer-urls must be set in etcd config") - } - peerURL, err := ParsePeerURL(initAdPeerURL.(string), podName) - if err != nil { - return "", fmt.Errorf("could not parse peer URL from the config file : %v", err) - } - return peerURL, nil -} - // RestartEtcdWrapper is to call the "/stop" endpoint of etcd-wrapper to restart the etcd-wrapper container. func RestartEtcdWrapper(ctx context.Context, tlsEnabled bool, etcdConnectionConfig *brtypes.EtcdConnectionConfig) error { client := &http.Client{} diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index a90dc4691..3c11478b6 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -571,12 +571,7 @@ func handleSsrStopRequest(ctx context.Context, handler *HTTPHandler, _ *snapshot } func hasPeerURLChanged(ctx context.Context, m member.Control, cli client.ClusterCloser) (bool, error) { - podName, err := miscellaneous.GetEnvVarOrError("POD_NAME") - if err != nil { - return false, fmt.Errorf("error reading POD_NAME env var : %v", err) - } - - peerURLsFromEtcdConfig, err := miscellaneous.GetMemberPeerURL(miscellaneous.GetConfigFilePath(), podName) + peerURLsFromEtcdConfig, err := miscellaneous.GetAdvertisePeerURLs(miscellaneous.GetConfigFilePath()) if err != nil { return false, err } From c0b243e6fa7ad2680908de7601a124c015e9d34c Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Mon, 21 Oct 2024 17:42:56 +0530 Subject: [PATCH 03/11] Use list for advertise-client-urls as well --- pkg/member/member_control.go | 17 +- pkg/miscellaneous/miscellaneous.go | 64 ++------ pkg/miscellaneous/miscellaneous_test.go | 164 ++++--------------- pkg/miscellaneous/testdata/valid_config.yaml | 9 +- pkg/server/backuprestoreserver.go | 6 +- pkg/server/httpAPI.go | 8 +- test/e2e/integration/cloud_backup_test.go | 3 +- 7 files changed, 66 insertions(+), 205 deletions(-) diff --git a/pkg/member/member_control.go b/pkg/member/member_control.go index 97b884d67..78b1cc816 100644 --- a/pkg/member/member_control.go +++ b/pkg/member/member_control.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "strconv" + "strings" "time" utilError "github.com/gardener/etcd-backup-restore/pkg/errors" @@ -116,9 +117,7 @@ func NewMemberControl(etcdConnConfig *brtypes.EtcdConnectionConfig) Control { // AddMemberAsLearner add a member as a learner to the etcd cluster func (m *memberControl) AddMemberAsLearner(ctx context.Context) error { //Add member as learner to cluster - // TODO: Need to handle multiple peer URLs once etcd config is updated to support it. - // It is required in the context of Gardener usecase to support live control plane migration. - memberURL, err := miscellaneous.GetAdvertisePeerURLs(m.configFile) + memberPeerURLs, err := miscellaneous.GetAdvertiseURLs("initial-advertise-peer-urls", m.configFile) if err != nil { m.logger.Fatalf("Error fetching etcd member URL : %v", err) } @@ -131,11 +130,12 @@ func (m *memberControl) AddMemberAsLearner(ctx context.Context) error { memAddCtx, cancel := context.WithTimeout(ctx, EtcdTimeout) defer cancel() + memberPeerURLsList := strings.Split(memberPeerURLs, ",") start := time.Now() - response, err := cli.MemberAddAsLearner(memAddCtx, []string{memberURL}) + response, err := cli.MemberAddAsLearner(memAddCtx, memberPeerURLsList) if err != nil { if errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCPeerURLExist)) || errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCMemberExist)) { - m.logger.Infof("Member %s already part of etcd cluster", memberURL) + m.logger.Infof("Member %s already part of etcd cluster", memberPeerURLs) return nil } else if errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCTooManyLearners)) { m.logger.Infof("Unable to add member %s as a learner because the cluster already has a learner", m.podName) @@ -207,17 +207,16 @@ func (m *memberControl) IsMemberInCluster(ctx context.Context) (bool, error) { func (m *memberControl) doUpdateMemberPeerAddress(ctx context.Context, cli etcdClient.ClusterCloser, id uint64) error { // Already existing clusters or cluster after restoration have `http://localhost:2380` as the peer address. This needs to explicitly updated to the correct peer address. m.logger.Infof("Updating member peer URL for %s", m.podName) - // TODO: Need to handle multiple peer URLs once etcd config is updated to support it. - // It is required in the context of Gardener usecase to support live control plane migration. - memberPeerURL, err := miscellaneous.GetAdvertisePeerURLs(m.configFile) + memberPeerURLs, err := miscellaneous.GetAdvertiseURLs("initial-advertise-peer-urls", m.configFile) if err != nil { return fmt.Errorf("could not fetch member URL : %v", err) } + memberPeerURLsList := strings.Split(memberPeerURLs, ",") memberUpdateCtx, cancel := context.WithTimeout(ctx, EtcdTimeout) defer cancel() - if _, err = cli.MemberUpdate(memberUpdateCtx, id, []string{memberPeerURL}); err == nil { + if _, err = cli.MemberUpdate(memberUpdateCtx, id, memberPeerURLsList); err == nil { m.logger.Info("Successfully updated the member peer URL") return nil } diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 37c355412..20d26570b 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -535,8 +535,8 @@ func ReadConfigFileAsMap(path string) (map[string]interface{}, error) { return c, nil } -// GetAdvertisePeerURLs returns the advertise peer URLs for the etcd member. -func GetAdvertisePeerURLs(configFile string) (string, error) { +// GetAdvertiseURLs returns the advertise URLs of desired type for the etcd member. +func GetAdvertiseURLs(advURLType string, configFile string) (string, error) { memberName, err := GetEnvVarOrError("POD_NAME") if err != nil { return "", err @@ -545,17 +545,17 @@ func GetAdvertisePeerURLs(configFile string) (string, error) { if err != nil { return "", err } - initAdPeerURL := config["initial-advertise-peer-urls"] - if initAdPeerURL == nil { - return "", fmt.Errorf("initial-advertise-peer-urls must be set in etcd config") + advertiseURLs := config[advURLType] + if advertiseURLs == nil { + return "", fmt.Errorf("%s must be set in etcd config", advURLType) } - peerUrlsMap, ok := initAdPeerURL.(map[interface{}]interface{}) + podUrlsMap, ok := advertiseURLs.(map[interface{}]interface{}) if !ok { - return "", fmt.Errorf("initial-advertise-peer-urls is not in the expected format") + return "", fmt.Errorf("%s is not in the expected format", advURLType) } resultMap := make(map[string][]string) - for pod, urls := range peerUrlsMap { + for pod, urls := range podUrlsMap { podName, ok := pod.(string) if !ok { return "", fmt.Errorf("pod name is not a string") @@ -572,60 +572,22 @@ func GetAdvertisePeerURLs(configFile string) (string, error) { resultMap[podName] = append(resultMap[podName], urlStr) } } - peerUrls, ok := resultMap[memberName] + podUrls, ok := resultMap[memberName] if !ok { return "", fmt.Errorf("peer url not found for pod %s", memberName) } - return strings.Join(peerUrls, ","), nil -} - -// GetAdvertiseClientURL returns the advertise client URL for the etcd member. -func GetAdvertiseClientURL(configFile string) (string, error) { - memberName, err := GetEnvVarOrError("POD_NAME") - if err != nil { - return "", err - } - config, err := ReadConfigFileAsMap(configFile) - if err != nil { - return "", err - } - initAdClientURL := config["advertise-client-urls"] - if initAdClientURL == nil { - return "", fmt.Errorf("advertise-client-urls must be set in etcd config") - } - clientUrlsMap, ok := initAdClientURL.(map[interface{}]interface{}) - if !ok { - return "", fmt.Errorf("advertise-client-urls is not in the expected format") - } - resultMap := make(map[string]string) - for pod, url := range clientUrlsMap { - podName, ok := pod.(string) - if !ok { - return "", fmt.Errorf("pod name is not a string") - } - urlStr, ok := url.(string) - if !ok { - return "", fmt.Errorf("url is not a string") - } - resultMap[podName] = urlStr - } - clientURL, ok := resultMap[memberName] - if !ok { - return "", fmt.Errorf("client url not found for pod %s", memberName) - } - return clientURL, nil + return strings.Join(podUrls, ","), nil } // IsPeerURLTLSEnabled checks whether the peer address is TLS enabled or not. func IsPeerURLTLSEnabled() (bool, error) { configFile := GetConfigFilePath() - // TODO: Need to handle multiple peer URLs once etcd config is updated to support it. - // It is required in the context of Gardener usecase to support live control plane migration. - memberPeerURL, err := GetAdvertisePeerURLs(configFile) + memberPeerURLs, err := GetAdvertiseURLs("initial-advertise-peer-urls", configFile) if err != nil { return false, err } - peerURL, err := url.Parse(memberPeerURL) + memberPeerURLsList := strings.Split(memberPeerURLs, ",") + peerURL, err := url.Parse(memberPeerURLsList[0]) if err != nil { return false, err } diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index 6fe3a20e9..3262ec509 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -588,15 +588,16 @@ var _ = Describe("Miscellaneous Tests", func() { }) }) - Describe("GetAdvertisePeerURLs", func() { + Describe("GetAdvertiseURLs", func() { const ( - configFile = "/tmp/etcd-config.yaml" - podName = "test-pod" + configFile = "/tmp/etcd-config.yaml" + podName = "test-pod" + customAdvURLfield = "custom-advertise-urls" ) Context("When POD_NAME environment variable is not set", func() { It("should return an error", func() { - _, err := GetAdvertisePeerURLs(configFile) - Expect(err).NotTo(BeNil()) + _, err := GetAdvertiseURLs(customAdvURLfield, configFile) + Expect(err).To(HaveOccurred()) }) }) @@ -612,12 +613,12 @@ var _ = Describe("Miscellaneous Tests", func() { Context("When the config file cannot be read", func() { It("should return an error", func() { - _, err := GetAdvertisePeerURLs(configFile) - Expect(err).NotTo(BeNil()) + _, err := GetAdvertiseURLs(customAdvURLfield, configFile) + Expect(err).To(HaveOccurred()) }) }) - Context("When initial-advertise-peer-urls is not set in the config file", func() { + Context("When custom-advertise-urls is not set in the config file", func() { var config map[string]interface{} BeforeEach(func() { @@ -632,31 +633,32 @@ var _ = Describe("Miscellaneous Tests", func() { }) It("should return an error", func() { - _, err := GetAdvertisePeerURLs(configFile) - Expect(err).NotTo(BeNil()) + _, err := GetAdvertiseURLs(customAdvURLfield, configFile) + Expect(err).To(HaveOccurred()) }) }) - Context("When initial-advertise-peer-urls is set in the config file", func() { + Context("When custom-advertise-urls is set in the config file", func() { var config map[string]interface{} podUrlsMap := make(map[string]interface{}) AfterEach(func() { Expect(os.Remove(configFile)).To(Succeed()) + podUrlsMap = make(map[string]interface{}) }) - Context("When the initial-advertise-peer-urls is not in the expected format", func() { + Context("When the custom-advertise-urls is not in the expected format", func() { BeforeEach(func() { config = map[string]interface{}{ - "name": "etcd-test", - "initial-advertise-peer-urls": "invalid-format", + "name": "etcd-test", + "custom-advertise-urls": "invalid-format", } writeConfigToFile(configFile, config) }) It("should return an error", func() { - _, err := GetAdvertisePeerURLs(configFile) - Expect(err).NotTo(BeNil()) + _, err := GetAdvertiseURLs(customAdvURLfield, configFile) + Expect(err).To(HaveOccurred()) }) }) @@ -666,15 +668,15 @@ var _ = Describe("Miscellaneous Tests", func() { podUrlsMap["other-pod"] = otherPodPeerURLs config = map[string]interface{}{ - "name": "etcd-test", - "initial-advertise-peer-urls": podUrlsMap, + "name": "etcd-test", + "custom-advertise-urls": podUrlsMap, } writeConfigToFile(configFile, config) }) It("should return an error", func() { - _, err := GetAdvertisePeerURLs(configFile) - Expect(err).NotTo(BeNil()) + _, err := GetAdvertiseURLs(customAdvURLfield, configFile) + Expect(err).To(HaveOccurred()) }) }) @@ -685,15 +687,15 @@ var _ = Describe("Miscellaneous Tests", func() { podUrlsMap[podName] = podPeerURLs config = map[string]interface{}{ - "name": "etcd-test", - "initial-advertise-peer-urls": podUrlsMap, + "name": "etcd-test", + "custom-advertise-urls": podUrlsMap, } writeConfigToFile(configFile, config) }) It("should return the peer URLs", func() { - peerURLs, err := GetAdvertisePeerURLs(configFile) - Expect(err).NotTo(HaveOccurred()) + peerURLs, err := GetAdvertiseURLs(customAdvURLfield, configFile) + Expect(err).To(Not(HaveOccurred())) Expect(peerURLs).To(Equal(strings.Join(podPeerURLs, ","))) }) }) @@ -702,114 +704,6 @@ var _ = Describe("Miscellaneous Tests", func() { }) - Describe("GetAdvertiseClientURL", func() { - const ( - configFile = "/tmp/etcd-config.yaml" - podName = "test-pod" - ) - - Context("When POD_NAME environment variable is not set", func() { - It("should return an error", func() { - url, err := GetAdvertiseClientURL(configFile) - Expect(err).To(HaveOccurred()) - Expect(url).To(BeEmpty()) - }) - }) - - Context("When POD_NAME environment variable is set", func() { - BeforeEach(func() { - Expect(os.Setenv("POD_NAME", podName)).To(Succeed()) - Expect(os.Setenv("ETCD_CONF", configFile)).To(Succeed()) - }) - - AfterEach(func() { - Expect(os.Unsetenv("POD_NAME")).To(Succeed()) - Expect(os.Unsetenv("ETCD_CONF")).To(Succeed()) - }) - - Context("When the config file cannot be read", func() { - It("should return an error", func() { - _, err := GetAdvertiseClientURL(configFile) - Expect(err).To(Not(BeNil())) - }) - }) - - Context("When advertise-client-urls is not set in the etcd config", func() { - BeforeEach(func() { - config := map[string]interface{}{ - "name": "etcd-test", - } - writeConfigToFile(configFile, config) - }) - - AfterEach(func() { - Expect(os.Remove(configFile)).To(Succeed()) - }) - - It("should return an error", func() { - url, err := GetAdvertiseClientURL(configFile) - Expect(err).To(HaveOccurred()) - Expect(url).To(BeEmpty()) - }) - }) - - Context("When advertise-client-urls is set in the etcd config", func() { - podUrlsMap := make(map[string]interface{}) - AfterEach(func() { - Expect(os.Remove(configFile)).To(Succeed()) - }) - - Context("When advertise-client-urls is not in the expected format", func() { - BeforeEach(func() { - config := map[string]interface{}{ - "name": "etcd-test", - "advertise-client-urls": "invalid-format", - } - writeConfigToFile(configFile, config) - }) - - It("should return an error", func() { - url, err := GetAdvertiseClientURL(configFile) - Expect(err).To(HaveOccurred()) - Expect(url).To(BeEmpty()) - }) - }) - - Context("When the client URL is not found for the pod", func() { - BeforeEach(func() { - podUrlsMap["other-pod"] = "http://localhost:2379" - config := map[string]interface{}{ - "advertise-client-urls": podUrlsMap, - } - writeConfigToFile(configFile, config) - }) - - It("should return an error", func() { - url, err := GetAdvertiseClientURL(configFile) - Expect(err).To(HaveOccurred()) - Expect(url).To(BeEmpty()) - }) - }) - - Context("When the client URL is found for the pod", func() { - BeforeEach(func() { - podUrlsMap[podName] = "http://localhost:2379" - config := map[string]interface{}{ - "advertise-client-urls": podUrlsMap, - } - writeConfigToFile(configFile, config) - }) - - It("should return the client URL", func() { - url, err := GetAdvertiseClientURL(configFile) - Expect(err).ToNot(HaveOccurred()) - Expect(url).To(Equal("http://localhost:2379")) - }) - }) - }) - }) - }) - Describe("Etcd Statefulset", func() { var ( sts *appsv1.StatefulSet @@ -912,7 +806,7 @@ var _ = Describe("Miscellaneous Tests", func() { Context("with non-TLS enabled peer url", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 -initial-advertise-peer-urls: +custom-advertise-urls: test_pod: - http://etcd-main-peer.default:2380 - http://etcd-main-peer.default:2381 @@ -937,7 +831,7 @@ initial-cluster: etcd1=http://0.0.0.0:2380` Context("with TLS enabled peer url", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 -initial-advertise-peer-urls: +custom-advertise-urls: test_pod: - https://etcd-main-peer.default:2380 - https://etcd-main-peer.default:2381 @@ -961,7 +855,7 @@ initial-cluster: etcd1=https://0.0.0.0:2380` Context("with empty peer url passed", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 -initial-advertise-peer-urls: "" +custom-advertise-urls: "" initial-cluster: etcd1=http://0.0.0.0:2380` err := os.WriteFile(outfile, []byte(etcdConfigYaml), 0755) Expect(err).ShouldNot(HaveOccurred()) diff --git a/pkg/miscellaneous/testdata/valid_config.yaml b/pkg/miscellaneous/testdata/valid_config.yaml index dbf6437da..584dc02c5 100644 --- a/pkg/miscellaneous/testdata/valid_config.yaml +++ b/pkg/miscellaneous/testdata/valid_config.yaml @@ -32,9 +32,12 @@ listen-client-urls: https://0.0.0.0:2379 # List of this member's client URLs to advertise to the public. # The URLs needed to be a comma-separated list. advertise-client-urls: - etcd-events-0: https://etcd-events-0.etcd-events-peer.shoot--dev--test.svc:2379 - etcd-events-1: https://etcd-events-1.etcd-events-peer.shoot--dev--test.svc:2379 - etcd-events-2: https://etcd-events-2.etcd-events-peer.shoot--dev--test.svc:2379 + etcd-events-0: + - https://etcd-events-0.etcd-events-peer.shoot--dev--test.svc:2379 + etcd-events-1: + - https://etcd-events-1.etcd-events-peer.shoot--dev--test.svc:2379 + etcd-events-2: + - https://etcd-events-2.etcd-events-peer.shoot--dev--test.svc:2379 peer-transport-security: # Path to the peer server TLS cert file. cert-file: /var/etcd/ssl/peer/server/tls.crt diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index 3c11478b6..a939ce687 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "net/http" + "strings" "sync" "sync/atomic" "time" @@ -571,13 +572,14 @@ func handleSsrStopRequest(ctx context.Context, handler *HTTPHandler, _ *snapshot } func hasPeerURLChanged(ctx context.Context, m member.Control, cli client.ClusterCloser) (bool, error) { - peerURLsFromEtcdConfig, err := miscellaneous.GetAdvertisePeerURLs(miscellaneous.GetConfigFilePath()) + peerURLsFromEtcdConfig, err := miscellaneous.GetAdvertiseURLs("initial-advertise-peer-urls", miscellaneous.GetConfigFilePath()) if err != nil { return false, err } + peerURLsList := strings.Split(peerURLsFromEtcdConfig, ",") existingPeerURLs, err := m.GetPeerURLs(ctx, cli) if err != nil { return false, err } - return sets.New[string](peerURLsFromEtcdConfig).Difference(sets.New[string](existingPeerURLs...)).Len() > 0, nil + return sets.New[string](peerURLsList...).Difference(sets.New[string](existingPeerURLs...)).Len() > 0, nil } diff --git a/pkg/server/httpAPI.go b/pkg/server/httpAPI.go index 51298a862..3f31d6b51 100644 --- a/pkg/server/httpAPI.go +++ b/pkg/server/httpAPI.go @@ -429,22 +429,22 @@ func (h *HTTPHandler) serveConfig(rw http.ResponseWriter, req *http.Request) { config["name"] = podName // fetch initial-advertise-peer-urls from etcd config file - initAdPeerURL, err := miscellaneous.GetAdvertisePeerURLs(inputFileName) + initAdPeerURLs, err := miscellaneous.GetAdvertiseURLs("initial-advertise-peer-urls", inputFileName) if err != nil { h.Logger.Warnf("Unable to get initial-advertise-peer-urls from etcd config file: %v", err) rw.WriteHeader(http.StatusInternalServerError) return } - config["initial-advertise-peer-urls"] = initAdPeerURL + config["initial-advertise-peer-urls"] = initAdPeerURLs // fetch advertise-client-urls from etcd config file - advClientURL, err := miscellaneous.GetAdvertiseClientURL(inputFileName) + advClientURLs, err := miscellaneous.GetAdvertiseURLs("advertise-client-urls", inputFileName) if err != nil { h.Logger.Warnf("Unable to get advertise-client-urls : %v", err) rw.WriteHeader(http.StatusInternalServerError) return } - config["advertise-client-urls"] = advClientURL + config["advertise-client-urls"] = advClientURLs config["initial-cluster"] = getInitialCluster(req.Context(), fmt.Sprint(config["initial-cluster"]), *h.EtcdConnectionConfig, *h.Logger, podName) diff --git a/test/e2e/integration/cloud_backup_test.go b/test/e2e/integration/cloud_backup_test.go index 9af1d1f3b..b49c47e48 100644 --- a/test/e2e/integration/cloud_backup_test.go +++ b/test/e2e/integration/cloud_backup_test.go @@ -134,7 +134,8 @@ enable-v2: false quota-backend-bytes: 1073741824 listen-client-urls: http://0.0.0.0:2379 advertise-client-urls: - ` + podName + `: http://0.0.0.0:2379 + ` + podName + `: + - http://0.0.0.0:2379 initial-advertise-peer-urls: ` + podName + `: - http://etcd-main-peer.default:2380 From 5044335d66d376658ba8d1d0e32d7cd0ec0beac4 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Mon, 21 Oct 2024 18:07:07 +0530 Subject: [PATCH 04/11] fix tests --- pkg/member/member_control_test.go | 3 ++- pkg/miscellaneous/miscellaneous_test.go | 6 +++--- test/e2e/integration/cloud_backup_test.go | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/member/member_control_test.go b/pkg/member/member_control_test.go index d4d71785b..8475696d2 100644 --- a/pkg/member/member_control_test.go +++ b/pkg/member/member_control_test.go @@ -55,7 +55,8 @@ enable-v2: false quota-backend-bytes: 1073741824 listen-client-urls: http://0.0.0.0:2379 advertise-client-urls: - ` + podName + `: http://0.0.0.0:2379 + ` + podName + `: + - http://0.0.0.0:2379 initial-advertise-peer-urls: ` + podName + `: - http://etcd-main-peer.default:2380 diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index 3262ec509..80c856de8 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -806,7 +806,7 @@ var _ = Describe("Miscellaneous Tests", func() { Context("with non-TLS enabled peer url", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 -custom-advertise-urls: +initial-advertise-peer-urls: test_pod: - http://etcd-main-peer.default:2380 - http://etcd-main-peer.default:2381 @@ -831,7 +831,7 @@ initial-cluster: etcd1=http://0.0.0.0:2380` Context("with TLS enabled peer url", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 -custom-advertise-urls: +initial-advertise-peer-urls: test_pod: - https://etcd-main-peer.default:2380 - https://etcd-main-peer.default:2381 @@ -855,7 +855,7 @@ initial-cluster: etcd1=https://0.0.0.0:2380` Context("with empty peer url passed", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 -custom-advertise-urls: "" +initial-advertise-peer-urls: "" initial-cluster: etcd1=http://0.0.0.0:2380` err := os.WriteFile(outfile, []byte(etcdConfigYaml), 0755) Expect(err).ShouldNot(HaveOccurred()) diff --git a/test/e2e/integration/cloud_backup_test.go b/test/e2e/integration/cloud_backup_test.go index b49c47e48..04b6d60bf 100644 --- a/test/e2e/integration/cloud_backup_test.go +++ b/test/e2e/integration/cloud_backup_test.go @@ -134,8 +134,8 @@ enable-v2: false quota-backend-bytes: 1073741824 listen-client-urls: http://0.0.0.0:2379 advertise-client-urls: - ` + podName + `: - - http://0.0.0.0:2379 + ` + podName + `: + - http://0.0.0.0:2379 initial-advertise-peer-urls: ` + podName + `: - http://etcd-main-peer.default:2380 From 2c62302cb0ee7f9c5ed70ab3d2ba18b333c373a1 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Thu, 24 Oct 2024 22:01:11 +0530 Subject: [PATCH 05/11] Address review comments --- .../templates/etcd-configmap.yaml | 2 +- .../templates/etcd-statefulset.yaml | 2 +- pkg/member/member_control.go | 13 +- pkg/miscellaneous/miscellaneous.go | 92 +++++---- pkg/miscellaneous/miscellaneous_test.go | 177 +++++++++++------- pkg/miscellaneous/testdata/valid_config.yaml | 14 +- pkg/server/backuprestoreserver.go | 6 +- pkg/server/httpAPI.go | 10 +- 8 files changed, 189 insertions(+), 127 deletions(-) diff --git a/chart/etcd-backup-restore/templates/etcd-configmap.yaml b/chart/etcd-backup-restore/templates/etcd-configmap.yaml index 45bab60f6..68fcfb7cc 100644 --- a/chart/etcd-backup-restore/templates/etcd-configmap.yaml +++ b/chart/etcd-backup-restore/templates/etcd-configmap.yaml @@ -2,7 +2,7 @@ apiVersion: v1 kind: ConfigMap metadata: - name: {{ .Release.Name }}-etcd-bootstrap + name: {{ .Release.Name }}-etcd-config namespace: {{ .Release.Namespace }} labels: app.kubernetes.io/name: etcd diff --git a/chart/etcd-backup-restore/templates/etcd-statefulset.yaml b/chart/etcd-backup-restore/templates/etcd-statefulset.yaml index 6cc33f94e..9d586b389 100644 --- a/chart/etcd-backup-restore/templates/etcd-statefulset.yaml +++ b/chart/etcd-backup-restore/templates/etcd-statefulset.yaml @@ -314,7 +314,7 @@ spec: volumes: - name: etcd-config-file configMap: - name: {{ .Release.Name }}-etcd-bootstrap + name: {{ .Release.Name }}-etcd-config defaultMode: 0644 items: - key: etcd.conf.yaml diff --git a/pkg/member/member_control.go b/pkg/member/member_control.go index 78b1cc816..c1c07b1c2 100644 --- a/pkg/member/member_control.go +++ b/pkg/member/member_control.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "strconv" - "strings" "time" utilError "github.com/gardener/etcd-backup-restore/pkg/errors" @@ -117,7 +116,7 @@ func NewMemberControl(etcdConnConfig *brtypes.EtcdConnectionConfig) Control { // AddMemberAsLearner add a member as a learner to the etcd cluster func (m *memberControl) AddMemberAsLearner(ctx context.Context) error { //Add member as learner to cluster - memberPeerURLs, err := miscellaneous.GetAdvertiseURLs("initial-advertise-peer-urls", m.configFile) + memberPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(m.configFile) if err != nil { m.logger.Fatalf("Error fetching etcd member URL : %v", err) } @@ -130,12 +129,11 @@ func (m *memberControl) AddMemberAsLearner(ctx context.Context) error { memAddCtx, cancel := context.WithTimeout(ctx, EtcdTimeout) defer cancel() - memberPeerURLsList := strings.Split(memberPeerURLs, ",") start := time.Now() - response, err := cli.MemberAddAsLearner(memAddCtx, memberPeerURLsList) + response, err := cli.MemberAddAsLearner(memAddCtx, memberPeerURLs) if err != nil { if errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCPeerURLExist)) || errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCMemberExist)) { - m.logger.Infof("Member %s already part of etcd cluster", memberPeerURLs) + m.logger.Infof("Member %s already part of etcd cluster", m.podName) return nil } else if errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCTooManyLearners)) { m.logger.Infof("Unable to add member %s as a learner because the cluster already has a learner", m.podName) @@ -207,16 +205,15 @@ func (m *memberControl) IsMemberInCluster(ctx context.Context) (bool, error) { func (m *memberControl) doUpdateMemberPeerAddress(ctx context.Context, cli etcdClient.ClusterCloser, id uint64) error { // Already existing clusters or cluster after restoration have `http://localhost:2380` as the peer address. This needs to explicitly updated to the correct peer address. m.logger.Infof("Updating member peer URL for %s", m.podName) - memberPeerURLs, err := miscellaneous.GetAdvertiseURLs("initial-advertise-peer-urls", m.configFile) + memberPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(m.configFile) if err != nil { return fmt.Errorf("could not fetch member URL : %v", err) } - memberPeerURLsList := strings.Split(memberPeerURLs, ",") memberUpdateCtx, cancel := context.WithTimeout(ctx, EtcdTimeout) defer cancel() - if _, err = cli.MemberUpdate(memberUpdateCtx, id, memberPeerURLsList); err == nil { + if _, err = cli.MemberUpdate(memberUpdateCtx, id, memberPeerURLs); err == nil { m.logger.Info("Successfully updated the member peer URL") return nil } diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 20d26570b..d583fb776 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -62,6 +62,11 @@ const ( etcdWrapperPort = "9095" ) +type advertiseURLsConfig struct { + AdvertiseClientURLs map[string][]string `yaml:"advertise-client-urls"` + InitialAdvertisePeerURLs map[string][]string `yaml:"initial-advertise-peer-urls"` +} + // GetLatestFullSnapshotAndDeltaSnapList returns the latest snapshot func GetLatestFullSnapshotAndDeltaSnapList(store brtypes.SnapStore) (*brtypes.Snapshot, brtypes.SnapList, error) { var ( @@ -535,59 +540,76 @@ func ReadConfigFileAsMap(path string) (map[string]interface{}, error) { return c, nil } -// GetAdvertiseURLs returns the advertise URLs of desired type for the etcd member. -func GetAdvertiseURLs(advURLType string, configFile string) (string, error) { - memberName, err := GetEnvVarOrError("POD_NAME") +// parseAdvertiseURLsConfig reads and parses the config file and returns the advertise URLs config. +func parseAdvertiseURLsConfig(configFile string) (*advertiseURLsConfig, error) { + var advURLsConfig advertiseURLsConfig + config, err := os.ReadFile(configFile) if err != nil { - return "", err + return nil, fmt.Errorf("unable to read etcd config file at path: %s : %w", configFile, err) } - config, err := ReadConfigFileAsMap(configFile) + + if err := yaml.Unmarshal(config, &advURLsConfig); err != nil { + return nil, fmt.Errorf("unable to unmarshal etcd config yaml file at path: %s : %w", configFile, err) + } + return &advURLsConfig, nil +} + +func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) { + memberName, err := GetEnvVarOrError("POD_NAME") if err != nil { - return "", err + return nil, err } - advertiseURLs := config[advURLType] - if advertiseURLs == nil { - return "", fmt.Errorf("%s must be set in etcd config", advURLType) + + advURLsConfig, err := parseAdvertiseURLsConfig(configFile) + if err != nil { + return nil, err } - podUrlsMap, ok := advertiseURLs.(map[interface{}]interface{}) - if !ok { - return "", fmt.Errorf("%s is not in the expected format", advURLType) + peerURLs, ok := advURLsConfig.InitialAdvertisePeerURLs[memberName] + if !ok || len(peerURLs) == 0 { + return nil, fmt.Errorf("peer url not found for pod %s", memberName) } - resultMap := make(map[string][]string) - for pod, urls := range podUrlsMap { - podName, ok := pod.(string) - if !ok { - return "", fmt.Errorf("pod name is not a string") - } - urlsList, ok := urls.([]interface{}) - if !ok { - return "", fmt.Errorf("urls is not a list") - } - for _, url := range urlsList { - urlStr, ok := url.(string) - if !ok { - return "", fmt.Errorf("url is not a string") - } - resultMap[podName] = append(resultMap[podName], urlStr) + + for _, peerURL := range peerURLs { + if _, err := url.Parse(peerURL); err != nil { + return nil, fmt.Errorf("peer url %s is not valid", peerURL) } } - podUrls, ok := resultMap[memberName] - if !ok { - return "", fmt.Errorf("peer url not found for pod %s", memberName) + return peerURLs, nil +} + +func GetAdvertiseClientURLs(configFile string) ([]string, error) { + memberName, err := GetEnvVarOrError("POD_NAME") + if err != nil { + return nil, err + } + + advURLsConfig, err := parseAdvertiseURLsConfig(configFile) + if err != nil { + return nil, err + } + + clientURLs, ok := advURLsConfig.AdvertiseClientURLs[memberName] + if !ok || len(clientURLs) == 0 { + return nil, fmt.Errorf("client url not found for pod %s", memberName) + } + + for _, clientURL := range clientURLs { + if _, err := url.Parse(clientURL); err != nil { + return nil, fmt.Errorf("client url %s is not valid", clientURL) + } } - return strings.Join(podUrls, ","), nil + return clientURLs, nil } // IsPeerURLTLSEnabled checks whether the peer address is TLS enabled or not. func IsPeerURLTLSEnabled() (bool, error) { configFile := GetConfigFilePath() - memberPeerURLs, err := GetAdvertiseURLs("initial-advertise-peer-urls", configFile) + memberPeerURLs, err := GetInitialAdvertisePeerURLs(configFile) if err != nil { return false, err } - memberPeerURLsList := strings.Split(memberPeerURLs, ",") - peerURL, err := url.Parse(memberPeerURLsList[0]) + peerURL, err := url.Parse(memberPeerURLs[0]) if err != nil { return false, err } diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index 80c856de8..ba4788f3e 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -11,7 +11,6 @@ import ( "os" "path/filepath" "reflect" - "strings" "time" mockfactory "github.com/gardener/etcd-backup-restore/pkg/mock/etcdutil/client" @@ -588,120 +587,166 @@ var _ = Describe("Miscellaneous Tests", func() { }) }) - Describe("GetAdvertiseURLs", func() { + Describe("Get Advertise URLs", func() { const ( - configFile = "/tmp/etcd-config.yaml" - podName = "test-pod" - customAdvURLfield = "custom-advertise-urls" + configFile = "/tmp/etcd-config.yaml" + podName = "test-pod" ) + type testCase struct { + name string + field string + function func(string) ([]string, error) + } + testCases := []testCase{ + { + name: "GetInitialAdvertisePeerURLs", + field: "initial-advertise-peer-urls", + function: GetInitialAdvertisePeerURLs, + }, + { + name: "GetAdvertiseClientURLs", + field: "advertise-client-urls", + function: GetAdvertiseClientURLs, + }, + } Context("When POD_NAME environment variable is not set", func() { - It("should return an error", func() { - _, err := GetAdvertiseURLs(customAdvURLfield, configFile) - Expect(err).To(HaveOccurred()) - }) + for _, tc := range testCases { + tc := tc + It(fmt.Sprintf("should return an error for %s", tc.name), func() { + _, err := tc.function(configFile) + Expect(err).To(HaveOccurred()) + }) + } }) Context("When POD_NAME environment variable is set", func() { + var config map[string]interface{} + var podUrlsMap map[string][]string + BeforeEach(func() { Expect(os.Setenv("POD_NAME", podName)).To(Succeed()) Expect(os.Setenv("ETCD_CONF", configFile)).To(Succeed()) }) - AfterEach(func() { - Expect(os.Unsetenv("POD_NAME")).To(Succeed()) - Expect(os.Unsetenv("ETCD_CONF")).To(Succeed()) - }) Context("When the config file cannot be read", func() { - It("should return an error", func() { - _, err := GetAdvertiseURLs(customAdvURLfield, configFile) - Expect(err).To(HaveOccurred()) - }) + for _, tc := range testCases { + tc := tc + It(fmt.Sprintf("should return an error for %s", tc.name), func() { + _, err := tc.function(configFile) + Expect(err).To(HaveOccurred()) + }) + } }) - Context("When custom-advertise-urls is not set in the config file", func() { - var config map[string]interface{} - + Context("When advertise-urls is not set in the config file", func() { BeforeEach(func() { config = map[string]interface{}{ "name": "etcd-test", } - writeConfigToFile(configFile, config) }) AfterEach(func() { Expect(os.Remove(configFile)).To(Succeed()) }) - It("should return an error", func() { - _, err := GetAdvertiseURLs(customAdvURLfield, configFile) - Expect(err).To(HaveOccurred()) - }) + for _, tc := range testCases { + tc := tc + It(fmt.Sprintf("should return an error for %s", tc.name), func() { + writeConfigToFile(configFile, config) + + _, err := tc.function(configFile) + Expect(err).To(HaveOccurred()) + }) + } }) - Context("When custom-advertise-urls is set in the config file", func() { - var config map[string]interface{} - podUrlsMap := make(map[string]interface{}) + Context("When advertise-urls is set in the config file", func() { + + BeforeEach(func() { + config = map[string]interface{}{ + "name": "etcd-test", + } + podUrlsMap = make(map[string][]string) + }) AfterEach(func() { Expect(os.Remove(configFile)).To(Succeed()) - podUrlsMap = make(map[string]interface{}) }) - Context("When the custom-advertise-urls is not in the expected format", func() { - BeforeEach(func() { - config = map[string]interface{}{ - "name": "etcd-test", - "custom-advertise-urls": "invalid-format", - } - writeConfigToFile(configFile, config) - }) + Context("When the advertise urls is not in the expected format", func() { + for _, tc := range testCases { + tc := tc + It(fmt.Sprintf("should return an error for %s", tc.name), func() { + config[tc.field] = "invalid-format" + writeConfigToFile(configFile, config) - It("should return an error", func() { - _, err := GetAdvertiseURLs(customAdvURLfield, configFile) - Expect(err).To(HaveOccurred()) - }) + _, err := tc.function(configFile) + Expect(err).To(HaveOccurred()) + }) + } }) Context("When the pod name is not present in the config file", func() { BeforeEach(func() { - otherPodPeerURLs := []string{"http://pod1:2380", "http://pod1:2381"} - podUrlsMap["other-pod"] = otherPodPeerURLs - - config = map[string]interface{}{ - "name": "etcd-test", - "custom-advertise-urls": podUrlsMap, + podUrlsMap = map[string][]string{ + "other-pod": {"http://pod1:2380", "http://pod1:2381"}, } - writeConfigToFile(configFile, config) }) - It("should return an error", func() { - _, err := GetAdvertiseURLs(customAdvURLfield, configFile) - Expect(err).To(HaveOccurred()) - }) + for _, tc := range testCases { + tc := tc + It(fmt.Sprintf("should return an error for %s", tc.name), func() { + config[tc.field] = podUrlsMap + writeConfigToFile(configFile, config) + + _, err := tc.function(configFile) + Expect(err).To(HaveOccurred()) + }) + } }) Context("When the pod name is present in the config file", func() { - var podPeerURLs []string - BeforeEach(func() { - podPeerURLs = []string{"http://pod:2380", "http://pod:2381"} - podUrlsMap[podName] = podPeerURLs - - config = map[string]interface{}{ - "name": "etcd-test", - "custom-advertise-urls": podUrlsMap, + Context("When an url is not in valid format", func() { + BeforeEach(func() { + podUrlsMap = map[string][]string{ + podName: {"http://pod:2380", "ht@tp://invalid-url"}, + } + }) + + for _, tc := range testCases { + tc := tc + It(fmt.Sprintf("should return an error for %s", tc.name), func() { + config[tc.field] = podUrlsMap + writeConfigToFile(configFile, config) + + _, err := tc.function(configFile) + Expect(err).To(HaveOccurred()) + }) } - writeConfigToFile(configFile, config) }) - It("should return the peer URLs", func() { - peerURLs, err := GetAdvertiseURLs(customAdvURLfield, configFile) - Expect(err).To(Not(HaveOccurred())) - Expect(peerURLs).To(Equal(strings.Join(podPeerURLs, ","))) + Context("When the urls are in valid format", func() { + BeforeEach(func() { + podUrlsMap = map[string][]string{ + podName: {"http://pod:2380", "http://pod:2381"}, + } + }) + + for _, tc := range testCases { + tc := tc + It(fmt.Sprintf("should return the peer URLs for %s", tc.field), func() { + config[tc.field] = podUrlsMap + writeConfigToFile(configFile, config) + + urls, err := tc.function(configFile) + Expect(err).To(Not(HaveOccurred())) + Expect(urls).To(Equal(podUrlsMap[podName])) + }) + } }) }) }) }) - }) Describe("Etcd Statefulset", func() { diff --git a/pkg/miscellaneous/testdata/valid_config.yaml b/pkg/miscellaneous/testdata/valid_config.yaml index 584dc02c5..4df6088b7 100644 --- a/pkg/miscellaneous/testdata/valid_config.yaml +++ b/pkg/miscellaneous/testdata/valid_config.yaml @@ -33,11 +33,11 @@ listen-client-urls: https://0.0.0.0:2379 # The URLs needed to be a comma-separated list. advertise-client-urls: etcd-events-0: - - https://etcd-events-0.etcd-events-peer.shoot--dev--test.svc:2379 + - https://etcd-events-0.etcd-events-peer.namespace.svc:2379 etcd-events-1: - - https://etcd-events-1.etcd-events-peer.shoot--dev--test.svc:2379 + - https://etcd-events-1.etcd-events-peer.namespace.svc:2379 etcd-events-2: - - https://etcd-events-2.etcd-events-peer.shoot--dev--test.svc:2379 + - https://etcd-events-2.etcd-events-peer.namespace.svc:2379 peer-transport-security: # Path to the peer server TLS cert file. cert-file: /var/etcd/ssl/peer/server/tls.crt @@ -60,11 +60,11 @@ listen-peer-urls: https://0.0.0.0:2380 # The URLs needed to be a comma-separated list. initial-advertise-peer-urls: etcd-events-0: - - https://etcd-events-0.etcd-events-peer.shoot--dev--test.svc:2380 + - https://etcd-events-0.etcd-events-peer.namespace.svc:2380 etcd-events-1: - - https://etcd-events-1.etcd-events-peer.shoot--dev--test.svc:2380 + - https://etcd-events-1.etcd-events-peer.namespace.svc:2380 etcd-events-2: - - https://etcd-events-2.etcd-events-peer.shoot--dev--test.svc:2380 + - https://etcd-events-2.etcd-events-peer.namespace.svc:2380 # Initial cluster token for the etcd cluster during bootstrap. initial-cluster-token: etcd-cluster @@ -73,7 +73,7 @@ initial-cluster-token: etcd-cluster initial-cluster-state: new # Initial cluster -initial-cluster: etcd-events-0=https://etcd-events-0.etcd-events-peer.shoot--dev--test.svc:2380,etcd-events-1=https://etcd-events-1.etcd-events-peer.shoot--dev--test.svc:2380,etcd-events-2=https://etcd-events-2.etcd-events-peer.shoot--dev--test.svc:2380 +initial-cluster: etcd-events-0=https://etcd-events-0.etcd-events-peer.namespace.svc:2380,etcd-events-1=https://etcd-events-1.etcd-events-peer.namespace.svc:2380,etcd-events-2=https://etcd-events-2.etcd-events-peer.namespace.svc:2380 # auto-compaction-mode ("periodic" or "revision"). auto-compaction-mode: periodic diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index a939ce687..e49bc0297 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -8,7 +8,6 @@ import ( "context" "fmt" "net/http" - "strings" "sync" "sync/atomic" "time" @@ -572,14 +571,13 @@ func handleSsrStopRequest(ctx context.Context, handler *HTTPHandler, _ *snapshot } func hasPeerURLChanged(ctx context.Context, m member.Control, cli client.ClusterCloser) (bool, error) { - peerURLsFromEtcdConfig, err := miscellaneous.GetAdvertiseURLs("initial-advertise-peer-urls", miscellaneous.GetConfigFilePath()) + peerURLsFromEtcdConfig, err := miscellaneous.GetInitialAdvertisePeerURLs(miscellaneous.GetConfigFilePath()) if err != nil { return false, err } - peerURLsList := strings.Split(peerURLsFromEtcdConfig, ",") existingPeerURLs, err := m.GetPeerURLs(ctx, cli) if err != nil { return false, err } - return sets.New[string](peerURLsList...).Difference(sets.New[string](existingPeerURLs...)).Len() > 0, nil + return sets.New[string](peerURLsFromEtcdConfig...).Difference(sets.New[string](existingPeerURLs...)).Len() > 0, nil } diff --git a/pkg/server/httpAPI.go b/pkg/server/httpAPI.go index 3f31d6b51..53a69e727 100644 --- a/pkg/server/httpAPI.go +++ b/pkg/server/httpAPI.go @@ -429,22 +429,22 @@ func (h *HTTPHandler) serveConfig(rw http.ResponseWriter, req *http.Request) { config["name"] = podName // fetch initial-advertise-peer-urls from etcd config file - initAdPeerURLs, err := miscellaneous.GetAdvertiseURLs("initial-advertise-peer-urls", inputFileName) + initAdPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(inputFileName) if err != nil { h.Logger.Warnf("Unable to get initial-advertise-peer-urls from etcd config file: %v", err) rw.WriteHeader(http.StatusInternalServerError) return } - config["initial-advertise-peer-urls"] = initAdPeerURLs + config["initial-advertise-peer-urls"] = strings.Join(initAdPeerURLs, ",") // fetch advertise-client-urls from etcd config file - advClientURLs, err := miscellaneous.GetAdvertiseURLs("advertise-client-urls", inputFileName) + advClientURLs, err := miscellaneous.GetAdvertiseClientURLs(inputFileName) if err != nil { - h.Logger.Warnf("Unable to get advertise-client-urls : %v", err) + h.Logger.Warnf("Unable to get advertise-client-urls from etcd config file: %v", err) rw.WriteHeader(http.StatusInternalServerError) return } - config["advertise-client-urls"] = advClientURLs + config["advertise-client-urls"] = strings.Join(advClientURLs, ",") config["initial-cluster"] = getInitialCluster(req.Context(), fmt.Sprint(config["initial-cluster"]), *h.EtcdConnectionConfig, *h.Logger, podName) From 4a24864649e3934ea21cd2114639e6cb56548f30 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Thu, 24 Oct 2024 22:13:55 +0530 Subject: [PATCH 06/11] fix check error & tests --- pkg/miscellaneous/miscellaneous.go | 2 ++ pkg/miscellaneous/miscellaneous_test.go | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index d583fb776..aef02e546 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -554,6 +554,7 @@ func parseAdvertiseURLsConfig(configFile string) (*advertiseURLsConfig, error) { return &advURLsConfig, nil } +// GetInitialAdvertisePeerURLs retrieves the initial advertise peer URLs for the etcd member. func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) { memberName, err := GetEnvVarOrError("POD_NAME") if err != nil { @@ -578,6 +579,7 @@ func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) { return peerURLs, nil } +// GetAdvertiseClientURLs retrieves the advertise client URLs for the etcd member. func GetAdvertiseClientURLs(configFile string) ([]string, error) { memberName, err := GetEnvVarOrError("POD_NAME") if err != nil { diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index ba4788f3e..750350715 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -840,7 +840,7 @@ var _ = Describe("Miscellaneous Tests", func() { outfile = "/tmp/etcd.conf.yaml" ) BeforeEach(func() { - Expect(os.Setenv("POD_NAME", "test_pod")).To(Succeed()) + Expect(os.Setenv("POD_NAME", "test_pod1")).To(Succeed()) Expect(os.Setenv("ETCD_CONF", outfile)).To(Succeed()) }) AfterEach(func() { @@ -852,7 +852,7 @@ var _ = Describe("Miscellaneous Tests", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 initial-advertise-peer-urls: - test_pod: + test_pod1: - http://etcd-main-peer.default:2380 - http://etcd-main-peer.default:2381 test_pod2: @@ -877,7 +877,7 @@ initial-cluster: etcd1=http://0.0.0.0:2380` BeforeEach(func() { etcdConfigYaml := `name: etcd1 initial-advertise-peer-urls: - test_pod: + test_pod1: - https://etcd-main-peer.default:2380 - https://etcd-main-peer.default:2381 test_pod2: From 4c31696e043a8bdf2a05b349dc4f04dc29306f52 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Mon, 28 Oct 2024 15:55:27 +0530 Subject: [PATCH 07/11] Address review comments by @seshachalam-yv --- pkg/miscellaneous/miscellaneous.go | 36 ++++---- pkg/miscellaneous/miscellaneous_test.go | 117 +++++++++++------------- pkg/server/backuprestoreserver.go | 2 +- 3 files changed, 75 insertions(+), 80 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index aef02e546..06d9ba78c 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -56,7 +56,7 @@ const ( // ScaledToMultiNodeAnnotationKey defines annotation key for scale-up to multi-node cluster. ScaledToMultiNodeAnnotationKey = "gardener.cloud/scaled-to-multi-node" - https = "https" + httpProtocol = "http" // etcdWrapperPort defines the port no. used by etcd-wrapper. etcdWrapperPort = "9095" @@ -554,7 +554,7 @@ func parseAdvertiseURLsConfig(configFile string) (*advertiseURLsConfig, error) { return &advURLsConfig, nil } -// GetInitialAdvertisePeerURLs retrieves the initial advertise peer URLs for the etcd member. +// GetInitialAdvertisePeerURLs retrieves the initial advertise peer URLs for the etcd member using the POD_NAME environment variable. func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) { memberName, err := GetEnvVarOrError("POD_NAME") if err != nil { @@ -563,23 +563,23 @@ func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) { advURLsConfig, err := parseAdvertiseURLsConfig(configFile) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse advertise URLs config: %w", err) } peerURLs, ok := advURLsConfig.InitialAdvertisePeerURLs[memberName] if !ok || len(peerURLs) == 0 { - return nil, fmt.Errorf("peer url not found for pod %s", memberName) + return nil, fmt.Errorf("no peer URLs found for pod %s", memberName) } for _, peerURL := range peerURLs { if _, err := url.Parse(peerURL); err != nil { - return nil, fmt.Errorf("peer url %s is not valid", peerURL) + return nil, fmt.Errorf("invalid peer URL %s: %w", peerURL, err) } } return peerURLs, nil } -// GetAdvertiseClientURLs retrieves the advertise client URLs for the etcd member. +// GetAdvertiseClientURLs retrieves the advertise client URLs for the etcd member using the POD_NAME environment variable. func GetAdvertiseClientURLs(configFile string) ([]string, error) { memberName, err := GetEnvVarOrError("POD_NAME") if err != nil { @@ -588,17 +588,17 @@ func GetAdvertiseClientURLs(configFile string) ([]string, error) { advURLsConfig, err := parseAdvertiseURLsConfig(configFile) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse advertise URLs config: %w", err) } clientURLs, ok := advURLsConfig.AdvertiseClientURLs[memberName] if !ok || len(clientURLs) == 0 { - return nil, fmt.Errorf("client url not found for pod %s", memberName) + return nil, fmt.Errorf("no client URLs found for pod %s", memberName) } for _, clientURL := range clientURLs { if _, err := url.Parse(clientURL); err != nil { - return nil, fmt.Errorf("client url %s is not valid", clientURL) + return nil, fmt.Errorf("invalid client URL %s: %w", clientURL, err) } } return clientURLs, nil @@ -606,16 +606,20 @@ func GetAdvertiseClientURLs(configFile string) ([]string, error) { // IsPeerURLTLSEnabled checks whether the peer address is TLS enabled or not. func IsPeerURLTLSEnabled() (bool, error) { - configFile := GetConfigFilePath() - memberPeerURLs, err := GetInitialAdvertisePeerURLs(configFile) + memberPeerURLs, err := GetInitialAdvertisePeerURLs(GetConfigFilePath()) if err != nil { - return false, err + return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err) } - peerURL, err := url.Parse(memberPeerURLs[0]) - if err != nil { - return false, err + for _, peerURL := range memberPeerURLs { + parsedPeerURL, err := url.Parse(peerURL) + if err != nil { + return false, fmt.Errorf("failed to parse peer URL %s: %w", peerURL, err) + } + if parsedPeerURL.Scheme == httpProtocol { + return false, nil + } } - return peerURL.Scheme == https, nil + return true, nil } // GetPrevScheduledSnapTime returns the previous schedule snapshot time. diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index 750350715..f8c692806 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -592,31 +592,15 @@ var _ = Describe("Miscellaneous Tests", func() { configFile = "/tmp/etcd-config.yaml" podName = "test-pod" ) - type testCase struct { - name string - field string - function func(string) ([]string, error) - } - testCases := []testCase{ - { - name: "GetInitialAdvertisePeerURLs", - field: "initial-advertise-peer-urls", - function: GetInitialAdvertisePeerURLs, - }, - { - name: "GetAdvertiseClientURLs", - field: "advertise-client-urls", - function: GetAdvertiseClientURLs, - }, - } Context("When POD_NAME environment variable is not set", func() { - for _, tc := range testCases { - tc := tc - It(fmt.Sprintf("should return an error for %s", tc.name), func() { - _, err := tc.function(configFile) + DescribeTable("should return an error", + func(field string, function func(string) ([]string, error)) { + _, err := function(configFile) Expect(err).To(HaveOccurred()) - }) - } + }, + Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), + Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + ) }) Context("When POD_NAME environment variable is set", func() { @@ -629,13 +613,14 @@ var _ = Describe("Miscellaneous Tests", func() { }) Context("When the config file cannot be read", func() { - for _, tc := range testCases { - tc := tc - It(fmt.Sprintf("should return an error for %s", tc.name), func() { - _, err := tc.function(configFile) + DescribeTable("should return an error", + func(field string, function func(string) ([]string, error)) { + _, err := function(configFile) Expect(err).To(HaveOccurred()) - }) - } + }, + Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), + Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + ) }) Context("When advertise-urls is not set in the config file", func() { @@ -649,15 +634,16 @@ var _ = Describe("Miscellaneous Tests", func() { Expect(os.Remove(configFile)).To(Succeed()) }) - for _, tc := range testCases { - tc := tc - It(fmt.Sprintf("should return an error for %s", tc.name), func() { + DescribeTable("should return an error", + func(field string, function func(string) ([]string, error)) { writeConfigToFile(configFile, config) - _, err := tc.function(configFile) + _, err := function(configFile) Expect(err).To(HaveOccurred()) - }) - } + }, + Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), + Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + ) }) Context("When advertise-urls is set in the config file", func() { @@ -674,16 +660,18 @@ var _ = Describe("Miscellaneous Tests", func() { }) Context("When the advertise urls is not in the expected format", func() { - for _, tc := range testCases { - tc := tc - It(fmt.Sprintf("should return an error for %s", tc.name), func() { - config[tc.field] = "invalid-format" + + DescribeTable("should return an error", + func(field string, function func(string) ([]string, error)) { + config[field] = "invalid-format" writeConfigToFile(configFile, config) - _, err := tc.function(configFile) + _, err := function(configFile) Expect(err).To(HaveOccurred()) - }) - } + }, + Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), + Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + ) }) Context("When the pod name is not present in the config file", func() { @@ -693,16 +681,17 @@ var _ = Describe("Miscellaneous Tests", func() { } }) - for _, tc := range testCases { - tc := tc - It(fmt.Sprintf("should return an error for %s", tc.name), func() { - config[tc.field] = podUrlsMap + DescribeTable("should return an error", + func(field string, function func(string) ([]string, error)) { + config[field] = podUrlsMap writeConfigToFile(configFile, config) - _, err := tc.function(configFile) + _, err := function(configFile) Expect(err).To(HaveOccurred()) - }) - } + }, + Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), + Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + ) }) Context("When the pod name is present in the config file", func() { @@ -713,16 +702,17 @@ var _ = Describe("Miscellaneous Tests", func() { } }) - for _, tc := range testCases { - tc := tc - It(fmt.Sprintf("should return an error for %s", tc.name), func() { - config[tc.field] = podUrlsMap + DescribeTable("should return an error", + func(field string, function func(string) ([]string, error)) { + config[field] = podUrlsMap writeConfigToFile(configFile, config) - _, err := tc.function(configFile) + _, err := function(configFile) Expect(err).To(HaveOccurred()) - }) - } + }, + Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), + Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + ) }) Context("When the urls are in valid format", func() { @@ -732,17 +722,18 @@ var _ = Describe("Miscellaneous Tests", func() { } }) - for _, tc := range testCases { - tc := tc - It(fmt.Sprintf("should return the peer URLs for %s", tc.field), func() { - config[tc.field] = podUrlsMap + DescribeTable("should return the peer URLs", + func(field string, function func(string) ([]string, error)) { + config[field] = podUrlsMap writeConfigToFile(configFile, config) - urls, err := tc.function(configFile) + urls, err := function(configFile) Expect(err).To(Not(HaveOccurred())) Expect(urls).To(Equal(podUrlsMap[podName])) - }) - } + }, + Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), + Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + ) }) }) }) diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index e49bc0297..44fb68953 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -573,7 +573,7 @@ func handleSsrStopRequest(ctx context.Context, handler *HTTPHandler, _ *snapshot func hasPeerURLChanged(ctx context.Context, m member.Control, cli client.ClusterCloser) (bool, error) { peerURLsFromEtcdConfig, err := miscellaneous.GetInitialAdvertisePeerURLs(miscellaneous.GetConfigFilePath()) if err != nil { - return false, err + return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err) } existingPeerURLs, err := m.GetPeerURLs(ctx, cli) if err != nil { From ab83692c86f32eda8765f3c41046eaecd6b15023 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Thu, 31 Oct 2024 15:26:55 +0530 Subject: [PATCH 08/11] Address review comments by @seshachalam-yv --- pkg/miscellaneous/miscellaneous.go | 19 ++++++++++++++----- pkg/miscellaneous/miscellaneous_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 06d9ba78c..e7161af6c 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -56,7 +56,7 @@ const ( // ScaledToMultiNodeAnnotationKey defines annotation key for scale-up to multi-node cluster. ScaledToMultiNodeAnnotationKey = "gardener.cloud/scaled-to-multi-node" - httpProtocol = "http" + https = "https" // etcdWrapperPort defines the port no. used by etcd-wrapper. etcdWrapperPort = "9095" @@ -610,16 +610,25 @@ func IsPeerURLTLSEnabled() (bool, error) { if err != nil { return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err) } + + peerURLsSchemes := make([]string, 0) for _, peerURL := range memberPeerURLs { parsedPeerURL, err := url.Parse(peerURL) if err != nil { return false, fmt.Errorf("failed to parse peer URL %s: %w", peerURL, err) } - if parsedPeerURL.Scheme == httpProtocol { - return false, nil - } + peerURLsSchemes = append(peerURLsSchemes, parsedPeerURL.Scheme) } - return true, nil + + sort.Strings(peerURLsSchemes) + + if peerURLsSchemes[0] != peerURLsSchemes[len(peerURLsSchemes)-1] { + return false, fmt.Errorf("peer URLs have different schemes") + } + if peerURLsSchemes[0] == https { + return true, nil + } + return false, nil } // GetPrevScheduledSnapTime returns the previous schedule snapshot time. diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index f8c692806..9eeb195ab 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -888,6 +888,30 @@ initial-cluster: etcd1=https://0.0.0.0:2380` }) }) + Context("with both TLS and non-TLS enabled peer url for the same pod", func() { + BeforeEach(func() { + etcdConfigYaml := `name: etcd1 +initial-advertise-peer-urls: + test_pod1: + - https://etcd-main-peer.default:2380 + - http://etcd-main-peer.default:2381 + test_pod2: + - https://etcd-main-peer.default:2380 + - https://etcd-main-peer.default:2381 + test_pod3: + - https://etcd-main-peer.default:2380 + - https://etcd-main-peer.default:2381 +initial-cluster: etcd1=https://0.0.0.0:2380` + err := os.WriteFile(outfile, []byte(etcdConfigYaml), 0755) + Expect(err).ShouldNot(HaveOccurred()) + }) + It("should return error", func() { + enabled, err := IsPeerURLTLSEnabled() + Expect(err).Should(HaveOccurred()) + Expect(enabled).To(BeFalse()) + }) + }) + Context("with empty peer url passed", func() { BeforeEach(func() { etcdConfigYaml := `name: etcd1 From f303bb3634213a4a4fe4e8bbfbcc33e8f145f1ee Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Thu, 7 Nov 2024 11:48:42 +0530 Subject: [PATCH 09/11] Address review comments by @seshachalam-yv --- pkg/miscellaneous/miscellaneous.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index e7161af6c..a37bdb505 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -604,30 +604,32 @@ func GetAdvertiseClientURLs(configFile string) ([]string, error) { return clientURLs, nil } -// IsPeerURLTLSEnabled checks whether the peer address is TLS enabled or not. +// IsPeerURLTLSEnabled checks whether all peer URLs are TLS-enabled (i.e., use the "https" scheme). func IsPeerURLTLSEnabled() (bool, error) { memberPeerURLs, err := GetInitialAdvertisePeerURLs(GetConfigFilePath()) if err != nil { return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err) } - peerURLsSchemes := make([]string, 0) + peerURLSchemes := make(map[string]bool) for _, peerURL := range memberPeerURLs { parsedPeerURL, err := url.Parse(peerURL) if err != nil { return false, fmt.Errorf("failed to parse peer URL %s: %w", peerURL, err) } - peerURLsSchemes = append(peerURLsSchemes, parsedPeerURL.Scheme) + peerURLSchemes[parsedPeerURL.Scheme] = true } - sort.Strings(peerURLsSchemes) - - if peerURLsSchemes[0] != peerURLsSchemes[len(peerURLsSchemes)-1] { + // Check for mixed schemes + if len(peerURLSchemes) > 1 { return false, fmt.Errorf("peer URLs have different schemes") } - if peerURLsSchemes[0] == https { + + // Check if the single scheme is "https" + if _, isTLSEnabled := peerURLSchemes[https]; isTLSEnabled { return true, nil } + return false, nil } From 2f788e2c751203dc13a8b6a2ccf40b3048ab7c21 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Fri, 8 Nov 2024 23:08:50 +0530 Subject: [PATCH 10/11] Address review comments by @ishan16696 --- pkg/member/member_control.go | 6 +++--- pkg/miscellaneous/miscellaneous.go | 14 ++++++------- pkg/miscellaneous/miscellaneous_test.go | 28 ++++++++++++------------- pkg/server/backuprestoreserver.go | 2 +- pkg/server/httpAPI.go | 4 ++-- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/pkg/member/member_control.go b/pkg/member/member_control.go index c1c07b1c2..89c66593c 100644 --- a/pkg/member/member_control.go +++ b/pkg/member/member_control.go @@ -116,7 +116,7 @@ func NewMemberControl(etcdConnConfig *brtypes.EtcdConnectionConfig) Control { // AddMemberAsLearner add a member as a learner to the etcd cluster func (m *memberControl) AddMemberAsLearner(ctx context.Context) error { //Add member as learner to cluster - memberPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(m.configFile) + memberPeerURLs, err := miscellaneous.GetMemberPeerURLs(m.configFile) if err != nil { m.logger.Fatalf("Error fetching etcd member URL : %v", err) } @@ -133,7 +133,7 @@ func (m *memberControl) AddMemberAsLearner(ctx context.Context) error { response, err := cli.MemberAddAsLearner(memAddCtx, memberPeerURLs) if err != nil { if errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCPeerURLExist)) || errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCMemberExist)) { - m.logger.Infof("Member %s already part of etcd cluster", m.podName) + m.logger.Infof("Member %s with peer urls %v already part of etcd cluster", m.podName, memberPeerURLs) return nil } else if errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCTooManyLearners)) { m.logger.Infof("Unable to add member %s as a learner because the cluster already has a learner", m.podName) @@ -205,7 +205,7 @@ func (m *memberControl) IsMemberInCluster(ctx context.Context) (bool, error) { func (m *memberControl) doUpdateMemberPeerAddress(ctx context.Context, cli etcdClient.ClusterCloser, id uint64) error { // Already existing clusters or cluster after restoration have `http://localhost:2380` as the peer address. This needs to explicitly updated to the correct peer address. m.logger.Infof("Updating member peer URL for %s", m.podName) - memberPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(m.configFile) + memberPeerURLs, err := miscellaneous.GetMemberPeerURLs(m.configFile) if err != nil { return fmt.Errorf("could not fetch member URL : %v", err) } diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index a37bdb505..13b934bb0 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -63,8 +63,8 @@ const ( ) type advertiseURLsConfig struct { - AdvertiseClientURLs map[string][]string `yaml:"advertise-client-urls"` - InitialAdvertisePeerURLs map[string][]string `yaml:"initial-advertise-peer-urls"` + AdvertiseClientURLs map[string][]string `json:"advertise-client-urls"` + InitialAdvertisePeerURLs map[string][]string `json:"initial-advertise-peer-urls"` } // GetLatestFullSnapshotAndDeltaSnapList returns the latest snapshot @@ -554,8 +554,8 @@ func parseAdvertiseURLsConfig(configFile string) (*advertiseURLsConfig, error) { return &advURLsConfig, nil } -// GetInitialAdvertisePeerURLs retrieves the initial advertise peer URLs for the etcd member using the POD_NAME environment variable. -func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) { +// GetMemberPeerURLs retrieves the initial advertise peer URLs for the etcd member using the POD_NAME environment variable. +func GetMemberPeerURLs(configFile string) ([]string, error) { memberName, err := GetEnvVarOrError("POD_NAME") if err != nil { return nil, err @@ -579,8 +579,8 @@ func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) { return peerURLs, nil } -// GetAdvertiseClientURLs retrieves the advertise client URLs for the etcd member using the POD_NAME environment variable. -func GetAdvertiseClientURLs(configFile string) ([]string, error) { +// GetMemberClientURLs retrieves the advertise client URLs for the etcd member using the POD_NAME environment variable. +func GetMemberClientURLs(configFile string) ([]string, error) { memberName, err := GetEnvVarOrError("POD_NAME") if err != nil { return nil, err @@ -606,7 +606,7 @@ func GetAdvertiseClientURLs(configFile string) ([]string, error) { // IsPeerURLTLSEnabled checks whether all peer URLs are TLS-enabled (i.e., use the "https" scheme). func IsPeerURLTLSEnabled() (bool, error) { - memberPeerURLs, err := GetInitialAdvertisePeerURLs(GetConfigFilePath()) + memberPeerURLs, err := GetMemberPeerURLs(GetConfigFilePath()) if err != nil { return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err) } diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index 9eeb195ab..ae7925e77 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -598,8 +598,8 @@ var _ = Describe("Miscellaneous Tests", func() { _, err := function(configFile) Expect(err).To(HaveOccurred()) }, - Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), - Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs), + Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs), ) }) @@ -618,8 +618,8 @@ var _ = Describe("Miscellaneous Tests", func() { _, err := function(configFile) Expect(err).To(HaveOccurred()) }, - Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), - Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs), + Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs), ) }) @@ -641,8 +641,8 @@ var _ = Describe("Miscellaneous Tests", func() { _, err := function(configFile) Expect(err).To(HaveOccurred()) }, - Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), - Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs), + Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs), ) }) @@ -669,8 +669,8 @@ var _ = Describe("Miscellaneous Tests", func() { _, err := function(configFile) Expect(err).To(HaveOccurred()) }, - Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), - Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs), + Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs), ) }) @@ -689,8 +689,8 @@ var _ = Describe("Miscellaneous Tests", func() { _, err := function(configFile) Expect(err).To(HaveOccurred()) }, - Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), - Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs), + Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs), ) }) @@ -710,8 +710,8 @@ var _ = Describe("Miscellaneous Tests", func() { _, err := function(configFile) Expect(err).To(HaveOccurred()) }, - Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), - Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs), + Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs), ) }) @@ -731,8 +731,8 @@ var _ = Describe("Miscellaneous Tests", func() { Expect(err).To(Not(HaveOccurred())) Expect(urls).To(Equal(podUrlsMap[podName])) }, - Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs), - Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs), + Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs), + Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs), ) }) }) diff --git a/pkg/server/backuprestoreserver.go b/pkg/server/backuprestoreserver.go index 44fb68953..edcccb046 100644 --- a/pkg/server/backuprestoreserver.go +++ b/pkg/server/backuprestoreserver.go @@ -571,7 +571,7 @@ func handleSsrStopRequest(ctx context.Context, handler *HTTPHandler, _ *snapshot } func hasPeerURLChanged(ctx context.Context, m member.Control, cli client.ClusterCloser) (bool, error) { - peerURLsFromEtcdConfig, err := miscellaneous.GetInitialAdvertisePeerURLs(miscellaneous.GetConfigFilePath()) + peerURLsFromEtcdConfig, err := miscellaneous.GetMemberPeerURLs(miscellaneous.GetConfigFilePath()) if err != nil { return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err) } diff --git a/pkg/server/httpAPI.go b/pkg/server/httpAPI.go index 53a69e727..fb2cabd7b 100644 --- a/pkg/server/httpAPI.go +++ b/pkg/server/httpAPI.go @@ -429,7 +429,7 @@ func (h *HTTPHandler) serveConfig(rw http.ResponseWriter, req *http.Request) { config["name"] = podName // fetch initial-advertise-peer-urls from etcd config file - initAdPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(inputFileName) + initAdPeerURLs, err := miscellaneous.GetMemberPeerURLs(inputFileName) if err != nil { h.Logger.Warnf("Unable to get initial-advertise-peer-urls from etcd config file: %v", err) rw.WriteHeader(http.StatusInternalServerError) @@ -438,7 +438,7 @@ func (h *HTTPHandler) serveConfig(rw http.ResponseWriter, req *http.Request) { config["initial-advertise-peer-urls"] = strings.Join(initAdPeerURLs, ",") // fetch advertise-client-urls from etcd config file - advClientURLs, err := miscellaneous.GetAdvertiseClientURLs(inputFileName) + advClientURLs, err := miscellaneous.GetMemberClientURLs(inputFileName) if err != nil { h.Logger.Warnf("Unable to get advertise-client-urls from etcd config file: %v", err) rw.WriteHeader(http.StatusInternalServerError) From 7fe7c299d3e2268de0bacec9daa2c2d012202959 Mon Sep 17 00:00:00 2001 From: Anvesh Reddy Pinnapureddy Date: Mon, 11 Nov 2024 12:51:53 +0530 Subject: [PATCH 11/11] Address review comments by @ishan16696 --- pkg/miscellaneous/miscellaneous.go | 17 ++++------------- pkg/miscellaneous/miscellaneous_test.go | 4 ++-- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/pkg/miscellaneous/miscellaneous.go b/pkg/miscellaneous/miscellaneous.go index 13b934bb0..dd35cff93 100644 --- a/pkg/miscellaneous/miscellaneous.go +++ b/pkg/miscellaneous/miscellaneous.go @@ -611,26 +611,17 @@ func IsPeerURLTLSEnabled() (bool, error) { return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err) } - peerURLSchemes := make(map[string]bool) for _, peerURL := range memberPeerURLs { parsedPeerURL, err := url.Parse(peerURL) if err != nil { return false, fmt.Errorf("failed to parse peer URL %s: %w", peerURL, err) } - peerURLSchemes[parsedPeerURL.Scheme] = true - } - - // Check for mixed schemes - if len(peerURLSchemes) > 1 { - return false, fmt.Errorf("peer URLs have different schemes") - } - - // Check if the single scheme is "https" - if _, isTLSEnabled := peerURLSchemes[https]; isTLSEnabled { - return true, nil + if parsedPeerURL.Scheme != https { + return false, nil + } } - return false, nil + return true, nil } // GetPrevScheduledSnapTime returns the previous schedule snapshot time. diff --git a/pkg/miscellaneous/miscellaneous_test.go b/pkg/miscellaneous/miscellaneous_test.go index ae7925e77..754d2a4ee 100644 --- a/pkg/miscellaneous/miscellaneous_test.go +++ b/pkg/miscellaneous/miscellaneous_test.go @@ -905,9 +905,9 @@ initial-cluster: etcd1=https://0.0.0.0:2380` err := os.WriteFile(outfile, []byte(etcdConfigYaml), 0755) Expect(err).ShouldNot(HaveOccurred()) }) - It("should return error", func() { + It("should return false", func() { enabled, err := IsPeerURLTLSEnabled() - Expect(err).Should(HaveOccurred()) + Expect(err).To(BeNil()) Expect(enabled).To(BeFalse()) }) })