From b906279a002ae1d8284ceaa481dc74f5694ab0c8 Mon Sep 17 00:00:00 2001 From: batthebee Date: Sun, 5 Feb 2023 18:27:20 +0100 Subject: [PATCH 1/3] Bugfix: fix handle poolcoordinator certificates in case of restarting yurt-controller-manager --- .../poolcoordinator/cert/certificate.go | 9 +---- .../cert/poolcoordinator_cert_manager.go | 37 +++++++++++-------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/pkg/controller/poolcoordinator/cert/certificate.go b/pkg/controller/poolcoordinator/cert/certificate.go index f8c0b4f091e..0105cf1ff18 100644 --- a/pkg/controller/poolcoordinator/cert/certificate.go +++ b/pkg/controller/poolcoordinator/cert/certificate.go @@ -98,13 +98,8 @@ func NewSignedCert(client client.Interface, cfg *CertConfig, key crypto.Signer, return nil, errors.Wrapf(err, "init cert %s fail", cfg.CertName) } - for _, ip := range ips { - cfg.IPs = append(cfg.IPs, ip) - } - for _, dnsName := range dnsNames { - cfg.DNSNames = append(cfg.DNSNames, dnsName) - } - + cfg.IPs = append(cfg.IPs, ips...) + cfg.DNSNames = append(cfg.DNSNames, dnsNames...) } // prepare cert serial number diff --git a/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go b/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go index 605580df8c2..79e12dbcec8 100644 --- a/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go +++ b/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go @@ -21,7 +21,6 @@ import ( "crypto/x509" "fmt" "net" - "reflect" "time" "github.com/pkg/errors" @@ -107,16 +106,6 @@ type CertConfig struct { certInit certInitFunc } -func (c *CertConfig) init(clientSet client.Interface, stopCh <-chan struct{}) (err error) { - if c.certInit != nil { - c.IPs, c.DNSNames, err = c.certInit(clientSet, stopCh) - if err != nil { - return errors.Wrapf(err, "fail to init cert %s", c.CertName) - } - } - return nil -} - var allSelfSignedCerts []CertConfig = []CertConfig{ { CertName: "apiserver-etcd-client", @@ -296,13 +285,31 @@ func initPoolCoordinator(clientSet client.Interface, stopCh <-chan struct{}) err // 1.3 check has dynamic attrs changed if certConf.certInit != nil { - if err := certConf.init(clientSet, stopCh); err != nil { + // receive dynamic IP addresses + ips, _, err := certConf.certInit(clientSet, stopCh) + if err != nil { // if cert init failed, skip this cert - klog.Errorf("fail to init cert when checking dynamic attrs: %v", err) + klog.Errorf("fail to init cert %s when checking dynamic attrs: %v", certConf.CertName, err) continue } else { - // check if dynamic IP address has changed - if !reflect.DeepEqual(certConf.IPs, cert.IPAddresses) { + // check if dynamic IP addresses arleady exist in cert + changed := false + for _, fromService := range ips { + contains := false + for _, fromSecret := range cert.IPAddresses { + // use Equal to compare IP address instead of deep equal + // deep equal does not work for IP address + if fromService.Equal(fromSecret) { + contains = true + break + } + } + if !contains { + changed = true + break + } + } + if changed { klog.Infof("cert %s IP has changed", certConf.CertName) selfSignedCerts = append(selfSignedCerts, certConf) continue From 9dc9ba19a59ac2aa380fc9e9264c2bb89b62f51b Mon Sep 17 00:00:00 2001 From: batthebee Date: Mon, 6 Feb 2023 23:21:35 +0100 Subject: [PATCH 2/3] Test: unittests for poolcoordinator ip address comparison --- .../cert/poolcoordinator_cert_manager.go | 19 +------- pkg/controller/poolcoordinator/cert/util.go | 20 +++++++++ .../poolcoordinator/cert/util_test.go | 45 +++++++++++++++++++ 3 files changed, 67 insertions(+), 17 deletions(-) diff --git a/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go b/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go index 79e12dbcec8..cc0e182f749 100644 --- a/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go +++ b/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go @@ -292,23 +292,8 @@ func initPoolCoordinator(clientSet client.Interface, stopCh <-chan struct{}) err klog.Errorf("fail to init cert %s when checking dynamic attrs: %v", certConf.CertName, err) continue } else { - // check if dynamic IP addresses arleady exist in cert - changed := false - for _, fromService := range ips { - contains := false - for _, fromSecret := range cert.IPAddresses { - // use Equal to compare IP address instead of deep equal - // deep equal does not work for IP address - if fromService.Equal(fromSecret) { - contains = true - break - } - } - if !contains { - changed = true - break - } - } + // check if dynamic IP addresses already exist in cert + changed := searchAllIP(cert.IPAddresses, ips) if changed { klog.Infof("cert %s IP has changed", certConf.CertName) selfSignedCerts = append(selfSignedCerts, certConf) diff --git a/pkg/controller/poolcoordinator/cert/util.go b/pkg/controller/poolcoordinator/cert/util.go index d7c39abc42f..8f27fd98f43 100644 --- a/pkg/controller/poolcoordinator/cert/util.go +++ b/pkg/controller/poolcoordinator/cert/util.go @@ -74,3 +74,23 @@ func waitUntilSVCReady(clientSet client.Interface, serviceName string, stopCh <- return ips, dnsnames, nil } + +// searchIP returns true if ip is in ipList +func searchIP(ipList []net.IP, ip net.IP) bool { + for _, ipItem := range ipList { + if ipItem.Equal(ip) { + return true + } + } + return false +} + +// searchAllIP returns true if all ips are in ipList +func searchAllIP(ipList []net.IP, ips []net.IP) bool { + for _, ip := range ips { + if !searchIP(ipList, ip) { + return false + } + } + return true +} diff --git a/pkg/controller/poolcoordinator/cert/util_test.go b/pkg/controller/poolcoordinator/cert/util_test.go index ea95c1c7078..00f012b3be8 100644 --- a/pkg/controller/poolcoordinator/cert/util_test.go +++ b/pkg/controller/poolcoordinator/cert/util_test.go @@ -17,6 +17,7 @@ limitations under the License. package cert import ( + "net" "testing" "github.com/pkg/errors" @@ -111,3 +112,47 @@ func TestGetAPIServerSVCURL(t *testing.T) { assert.Equal(t, nil, err) assert.Equal(t, "https://xxxx:644", url) } + +func TestSearchIPs(t *testing.T) { + + // empty ip list + ipList := []net.IP{} + assert.False(t, searchIP(ipList, net.ParseIP("10.0.0.1"))) + + // ip list with multiple ips + ipList = []net.IP{ + net.ParseIP("10.0.0.1"), + net.ParseIP("10.0.0.2"), + } + assert.True(t, searchIP(ipList, net.ParseIP("10.0.0.1"))) + assert.True(t, searchIP(ipList, net.ParseIP("10.0.0.2"))) + assert.False(t, searchIP(ipList, net.ParseIP("10.0.0.3"))) + + // search one exiting ip + ips := []net.IP{ + net.ParseIP("10.0.0.1"), + } + assert.True(t, searchAllIP(ipList, ips)) + + // search multiple existing ips + ips = []net.IP{ + net.ParseIP("10.0.0.1"), + net.ParseIP("10.0.0.2"), + } + assert.True(t, searchAllIP(ipList, ips)) + + // search multiple existing ips with one missing ip + ips = []net.IP{ + net.ParseIP("10.0.0.3"), + } + assert.False(t, searchAllIP(ipList, ips)) + + // search multiple existing ips with multiple missing ips + ips = []net.IP{ + net.ParseIP("10.0.0.1"), + net.ParseIP("10.0.0.4"), + net.ParseIP("10.0.0.3"), + net.ParseIP("10.0.0.2"), + } + assert.False(t, searchAllIP(ipList, ips)) +} From f6693fbb9451b4291e116eab4b23c6154a6b2fd0 Mon Sep 17 00:00:00 2001 From: batthebee Date: Mon, 6 Feb 2023 23:31:54 +0100 Subject: [PATCH 3/3] Test: unittests for poolcoordinator ip address comparison --- .../cert/poolcoordinator_cert_manager.go | 3 +- pkg/controller/poolcoordinator/cert/util.go | 20 -------- .../poolcoordinator/cert/util_test.go | 45 ------------------ pkg/util/ip/ip.go | 20 ++++++++ pkg/util/ip/ip_test.go | 46 +++++++++++++++++++ 5 files changed, 68 insertions(+), 66 deletions(-) diff --git a/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go b/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go index cc0e182f749..eb97eff7601 100644 --- a/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go +++ b/pkg/controller/poolcoordinator/cert/poolcoordinator_cert_manager.go @@ -35,6 +35,7 @@ import ( "k8s.io/klog/v2" certfactory "github.com/openyurtio/openyurt/pkg/util/certmanager/factory" + ip "github.com/openyurtio/openyurt/pkg/util/ip" ) const ( @@ -293,7 +294,7 @@ func initPoolCoordinator(clientSet client.Interface, stopCh <-chan struct{}) err continue } else { // check if dynamic IP addresses already exist in cert - changed := searchAllIP(cert.IPAddresses, ips) + changed := ip.SearchAllIP(cert.IPAddresses, ips) if changed { klog.Infof("cert %s IP has changed", certConf.CertName) selfSignedCerts = append(selfSignedCerts, certConf) diff --git a/pkg/controller/poolcoordinator/cert/util.go b/pkg/controller/poolcoordinator/cert/util.go index 8f27fd98f43..d7c39abc42f 100644 --- a/pkg/controller/poolcoordinator/cert/util.go +++ b/pkg/controller/poolcoordinator/cert/util.go @@ -74,23 +74,3 @@ func waitUntilSVCReady(clientSet client.Interface, serviceName string, stopCh <- return ips, dnsnames, nil } - -// searchIP returns true if ip is in ipList -func searchIP(ipList []net.IP, ip net.IP) bool { - for _, ipItem := range ipList { - if ipItem.Equal(ip) { - return true - } - } - return false -} - -// searchAllIP returns true if all ips are in ipList -func searchAllIP(ipList []net.IP, ips []net.IP) bool { - for _, ip := range ips { - if !searchIP(ipList, ip) { - return false - } - } - return true -} diff --git a/pkg/controller/poolcoordinator/cert/util_test.go b/pkg/controller/poolcoordinator/cert/util_test.go index 00f012b3be8..ea95c1c7078 100644 --- a/pkg/controller/poolcoordinator/cert/util_test.go +++ b/pkg/controller/poolcoordinator/cert/util_test.go @@ -17,7 +17,6 @@ limitations under the License. package cert import ( - "net" "testing" "github.com/pkg/errors" @@ -112,47 +111,3 @@ func TestGetAPIServerSVCURL(t *testing.T) { assert.Equal(t, nil, err) assert.Equal(t, "https://xxxx:644", url) } - -func TestSearchIPs(t *testing.T) { - - // empty ip list - ipList := []net.IP{} - assert.False(t, searchIP(ipList, net.ParseIP("10.0.0.1"))) - - // ip list with multiple ips - ipList = []net.IP{ - net.ParseIP("10.0.0.1"), - net.ParseIP("10.0.0.2"), - } - assert.True(t, searchIP(ipList, net.ParseIP("10.0.0.1"))) - assert.True(t, searchIP(ipList, net.ParseIP("10.0.0.2"))) - assert.False(t, searchIP(ipList, net.ParseIP("10.0.0.3"))) - - // search one exiting ip - ips := []net.IP{ - net.ParseIP("10.0.0.1"), - } - assert.True(t, searchAllIP(ipList, ips)) - - // search multiple existing ips - ips = []net.IP{ - net.ParseIP("10.0.0.1"), - net.ParseIP("10.0.0.2"), - } - assert.True(t, searchAllIP(ipList, ips)) - - // search multiple existing ips with one missing ip - ips = []net.IP{ - net.ParseIP("10.0.0.3"), - } - assert.False(t, searchAllIP(ipList, ips)) - - // search multiple existing ips with multiple missing ips - ips = []net.IP{ - net.ParseIP("10.0.0.1"), - net.ParseIP("10.0.0.4"), - net.ParseIP("10.0.0.3"), - net.ParseIP("10.0.0.2"), - } - assert.False(t, searchAllIP(ipList, ips)) -} diff --git a/pkg/util/ip/ip.go b/pkg/util/ip/ip.go index 66530725904..df2cd7dde24 100644 --- a/pkg/util/ip/ip.go +++ b/pkg/util/ip/ip.go @@ -88,3 +88,23 @@ func ParseIPList(ips []string) []net.IP { } return results } + +// searchIP returns true if ip is in ipList +func SearchIP(ipList []net.IP, ip net.IP) bool { + for _, ipItem := range ipList { + if ipItem.Equal(ip) { + return true + } + } + return false +} + +// searchAllIP returns true if all ips are in ipList +func SearchAllIP(ipList []net.IP, ips []net.IP) bool { + for _, ip := range ips { + if !SearchIP(ipList, ip) { + return false + } + } + return true +} diff --git a/pkg/util/ip/ip_test.go b/pkg/util/ip/ip_test.go index 67251a102e5..389cabf039d 100644 --- a/pkg/util/ip/ip_test.go +++ b/pkg/util/ip/ip_test.go @@ -20,6 +20,8 @@ import ( "net" "reflect" "testing" + + "github.com/stretchr/testify/assert" ) func TestGetLoopbackIP(t *testing.T) { @@ -135,3 +137,47 @@ func TestParseIPList(t *testing.T) { }) } } + +func TestSearchIPs(t *testing.T) { + + // empty ip list + ipList := []net.IP{} + assert.False(t, SearchIP(ipList, net.ParseIP("10.0.0.1"))) + + // ip list with multiple ips + ipList = []net.IP{ + net.ParseIP("10.0.0.1"), + net.ParseIP("10.0.0.2"), + } + assert.True(t, SearchIP(ipList, net.ParseIP("10.0.0.1"))) + assert.True(t, SearchIP(ipList, net.ParseIP("10.0.0.2"))) + assert.False(t, SearchIP(ipList, net.ParseIP("10.0.0.3"))) + + // search one exiting ip + ips := []net.IP{ + net.ParseIP("10.0.0.1"), + } + assert.True(t, SearchAllIP(ipList, ips)) + + // search multiple existing ips + ips = []net.IP{ + net.ParseIP("10.0.0.1"), + net.ParseIP("10.0.0.2"), + } + assert.True(t, SearchAllIP(ipList, ips)) + + // search multiple existing ips with one missing ip + ips = []net.IP{ + net.ParseIP("10.0.0.3"), + } + assert.False(t, SearchAllIP(ipList, ips)) + + // search multiple existing ips with multiple missing ips + ips = []net.IP{ + net.ParseIP("10.0.0.1"), + net.ParseIP("10.0.0.4"), + net.ParseIP("10.0.0.3"), + net.ParseIP("10.0.0.2"), + } + assert.False(t, SearchAllIP(ipList, ips)) +}