From 15f9501279b0c31a60564bec0f67366391a47208 Mon Sep 17 00:00:00 2001 From: zetaozhuang Date: Wed, 14 Dec 2022 13:44:27 -0800 Subject: [PATCH 1/2] refactor and move the nmagentConfig code from cns to namgent package. So it can be reused in other components --- cns/configuration/configuration.go | 95 ++++++------------------ cns/configuration/configuration_test.go | 97 ------------------------- cns/service/main.go | 6 +- nmagent/config.go | 51 ++++++++++++- nmagent/config_test.go | 84 +++++++++++++++++++++ 5 files changed, 159 insertions(+), 174 deletions(-) diff --git a/cns/configuration/configuration.go b/cns/configuration/configuration.go index 53da2828e1..b0e5e73fb1 100644 --- a/cns/configuration/configuration.go +++ b/cns/configuration/configuration.go @@ -3,12 +3,8 @@ package configuration import ( "encoding/json" - "net" - "net/url" "os" "path/filepath" - "strconv" - "strings" "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" @@ -23,28 +19,28 @@ const ( ) type CNSConfig struct { - ChannelMode string - EnablePprof bool - EnableSubnetScarcity bool - InitializeFromCNI bool - ManagedSettings ManagedSettings - MetricsBindAddress string - SyncHostNCTimeoutMs int - SyncHostNCVersionIntervalMs int - TLSCertificatePath string - TLSEndpoint string - TLSPort string - TLSSubjectName string - TelemetrySettings TelemetrySettings - UseHTTPS bool - WireserverIP string - KeyVaultSettings KeyVaultSettings - MSISettings MSISettings - ProgramSNATIPTables bool - ManageEndpointState bool - CNIConflistScenario string - EnableCNIConflistGeneration bool - CNIConflistFilepath string + ChannelMode string + EnablePprof bool + EnableSubnetScarcity bool + InitializeFromCNI bool + ManagedSettings ManagedSettings + MetricsBindAddress string + SyncHostNCTimeoutMs int + SyncHostNCVersionIntervalMs int + TLSCertificatePath string + TLSEndpoint string + TLSPort string + TLSSubjectName string + TelemetrySettings TelemetrySettings + UseHTTPS bool + WireserverIP string + KeyVaultSettings KeyVaultSettings + MSISettings MSISettings + ProgramSNATIPTables bool + ManageEndpointState bool + CNIConflistScenario string + EnableCNIConflistGeneration bool + CNIConflistFilepath string PopulateHomeAzCacheRetryIntervalSecs int } @@ -110,53 +106,6 @@ func getConfigFilePath(cmdLineConfigPath string) (string, error) { return configpath, nil } -type NMAgentConfig struct { - Host string - Port uint16 - UseTLS bool -} - -func (c *CNSConfig) NMAgentConfig() (NMAgentConfig, error) { - host := "168.63.129.16" // wireserver's IP - - if c.WireserverIP != "" { - host = c.WireserverIP - } - - if strings.Contains(host, "http") { - parts, err := url.Parse(host) - if err != nil { - return NMAgentConfig{}, errors.Wrap(err, "parsing WireserverIP as URL") - } - host = parts.Host - } - - // create an NMAgent Client based on provided configuration - if strings.Contains(host, ":") { - host, prt, err := net.SplitHostPort(host) //nolint:govet // it's fine to shadow err here - if err != nil { - return NMAgentConfig{}, errors.Wrap(err, "splitting wireserver IP into host port") - } - - port, err := strconv.ParseUint(prt, 10, 16) //nolint:gomnd // obvious from ParseUint docs - if err != nil { - return NMAgentConfig{}, errors.Wrap(err, "parsing wireserver port value as uint16") - } - - return NMAgentConfig{ - Host: host, - Port: uint16(port), - }, nil - } - - return NMAgentConfig{ - Host: host, - - // nolint:gomnd // there's no benefit to constantizing a well-known port - Port: 80, - }, nil -} - // ReadConfig returns a CNS config from file or an error. func ReadConfig(cmdLineConfigPath string) (*CNSConfig, error) { configpath, err := getConfigFilePath(cmdLineConfigPath) diff --git a/cns/configuration/configuration_test.go b/cns/configuration/configuration_test.go index 786deb36c3..00ebd271b6 100644 --- a/cns/configuration/configuration_test.go +++ b/cns/configuration/configuration_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/Azure/azure-container-networking/common" - "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -262,99 +261,3 @@ func TestSetCNSConfigDefaults(t *testing.T) { }) } } - -func TestNMAgentConfig(t *testing.T) { - tests := []struct { - name string - conf CNSConfig - exp NMAgentConfig - shouldErr bool - }{ - { - "empty", - CNSConfig{ - WireserverIP: "", - }, - NMAgentConfig{ - Host: "168.63.129.16", - Port: 80, - }, - false, - }, - { - "ip", - CNSConfig{ - WireserverIP: "127.0.0.1", - }, - NMAgentConfig{ - Host: "127.0.0.1", - Port: 80, - }, - false, - }, - { - "ipport", - CNSConfig{ - WireserverIP: "127.0.0.1:8080", - }, - NMAgentConfig{ - Host: "127.0.0.1", - Port: 8080, - }, - false, - }, - { - "scheme", - CNSConfig{ - WireserverIP: "http://127.0.0.1:8080", - }, - NMAgentConfig{ - Host: "127.0.0.1", - Port: 8080, - }, - false, - }, - { - "invalid URL", - CNSConfig{ - WireserverIP: "a string containing \"http\" with an invalid character: \x7F", - }, - NMAgentConfig{}, - true, - }, - { - "invalid host:port", - CNSConfig{ - WireserverIP: "way:too:many:colons", - }, - NMAgentConfig{}, - true, - }, - { - "too big for a uint16 port", - CNSConfig{ - WireserverIP: "127.0.0.1:4815162342", - }, - NMAgentConfig{}, - true, - }, - } - - for _, test := range tests { - test := test - t.Run(test.name, func(t *testing.T) { - got, err := test.conf.NMAgentConfig() - if err != nil && !test.shouldErr { - t.Fatal("unexpected error fetching nmagent config: err:", err) - } - - if err == nil && test.shouldErr { - t.Fatal("expected error fetching nmagent config but received none") - } - - if !cmp.Equal(got, test.exp) { - t.Error("received config differs from expectation: diff:", cmp.Diff(got, test.exp)) - } - }) - } -} diff --git a/cns/service/main.go b/cns/service/main.go index 645b32ad47..2dc8079072 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -510,13 +510,13 @@ func main() { z, _ := zap.NewProduction() go healthserver.Start(z, cnsconfig.MetricsBindAddress) - nmaConfig, err := cnsconfig.NMAgentConfig() + nmaConfig, err := nmagent.NewConfig(cnsconfig.WireserverIP) if err != nil { - logger.Errorf("[Azure CNS] Failed to produce NMAgent config from supplied configuration: %v", err) + logger.Errorf("[Azure CNS] Failed to produce NMAgent config from the supplied wireserver ip: %v", err) return } - nmaClient, err := nmagent.NewClient(nmagent.Config(nmaConfig)) + nmaClient, err := nmagent.NewClient(nmaConfig) if err != nil { logger.Errorf("[Azure CNS] Failed to start nmagent client due to error: %v", err) return diff --git a/nmagent/config.go b/nmagent/config.go index 25b99c6fc3..7d3f1b31dd 100644 --- a/nmagent/config.go +++ b/nmagent/config.go @@ -1,6 +1,14 @@ package nmagent -import "github.com/Azure/azure-container-networking/nmagent/internal" +import ( + "net" + "net/url" + "strconv" + "strings" + + "github.com/Azure/azure-container-networking/nmagent/internal" + "github.com/pkg/errors" +) // Config is a configuration for an NMAgent Client. type Config struct { @@ -34,3 +42,44 @@ func (c Config) Validate() error { } return err } + +// NewConfig returns a nmagent client config using the provided wireserverIP string +func NewConfig(wireserverIP string) (Config, error) { + host := "168.63.129.16" // wireserver's IP + + if wireserverIP != "" { + host = wireserverIP + } + + if strings.Contains(host, "http") { + parts, err := url.Parse(host) + if err != nil { + return Config{}, errors.Wrap(err, "parsing WireserverIP as URL") + } + host = parts.Host + } + + if strings.Contains(host, ":") { + host, prt, err := net.SplitHostPort(host) //nolint:govet // it's fine to shadow ergit r here + if err != nil { + return Config{}, errors.Wrap(err, "splitting wireserver IP into host port") + } + + port, err := strconv.ParseUint(prt, 10, 16) //nolint:gomnd // obvious from ParseUint docs + if err != nil { + return Config{}, errors.Wrap(err, "parsing wireserver port value as uint16") + } + + return Config{ + Host: host, + Port: uint16(port), + }, nil + } + + return Config{ + Host: host, + + // nolint:gomnd // there's no benefit to constantizing a well-known port + Port: 80, + }, nil +} diff --git a/nmagent/config_test.go b/nmagent/config_test.go index ed9b65f4f9..69ba7d4814 100644 --- a/nmagent/config_test.go +++ b/nmagent/config_test.go @@ -3,6 +3,8 @@ package nmagent_test import ( "testing" + "github.com/google/go-cmp/cmp" + "github.com/Azure/azure-container-networking/nmagent" ) @@ -57,3 +59,85 @@ func TestConfig(t *testing.T) { }) } } + +func TestNMAgentConfig(t *testing.T) { + tests := []struct { + name string + wireserverIp string + exp nmagent.Config + shouldErr bool + }{ + { + "empty", + "", + nmagent.Config{ + Host: "168.63.129.16", + Port: 80, + }, + false, + }, + { + "ip", + "127.0.0.1", + nmagent.Config{ + Host: "127.0.0.1", + Port: 80, + }, + false, + }, + { + "ipport", + "127.0.0.1:8080", + nmagent.Config{ + Host: "127.0.0.1", + Port: 8080, + }, + false, + }, + { + "scheme", + "http://127.0.0.1:8080", + nmagent.Config{ + Host: "127.0.0.1", + Port: 8080, + }, + false, + }, + { + "invalid URL", + "a string containing \"http\" with an invalid character: \x7F", + nmagent.Config{}, + true, + }, + { + "invalid host:port", + "way:too:many:colons", + nmagent.Config{}, + true, + }, + { + "too big for a uint16 port", + "127.0.0.1:4815162342", + nmagent.Config{}, + true, + }, + } + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + got, err := nmagent.NewConfig(test.wireserverIp) + if err != nil && !test.shouldErr { + t.Fatal("unexpected error fetching nmagent config: err:", err) + } + + if err == nil && test.shouldErr { + t.Fatal("expected error fetching nmagent config but received none") + } + + if !cmp.Equal(got, test.exp) { + t.Error("received config differs from expectation: diff:", cmp.Diff(got, test.exp)) + } + }) + } +} From 3a83fec81788509bfd60c535bf9c11e374913ce9 Mon Sep 17 00:00:00 2001 From: zetaozhuang Date: Wed, 14 Dec 2022 13:57:41 -0800 Subject: [PATCH 2/2] fix lint --- nmagent/config_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/nmagent/config_test.go b/nmagent/config_test.go index 69ba7d4814..645513c584 100644 --- a/nmagent/config_test.go +++ b/nmagent/config_test.go @@ -3,9 +3,8 @@ package nmagent_test import ( "testing" - "github.com/google/go-cmp/cmp" - "github.com/Azure/azure-container-networking/nmagent" + "github.com/google/go-cmp/cmp" ) func TestConfig(t *testing.T) { @@ -63,7 +62,7 @@ func TestConfig(t *testing.T) { func TestNMAgentConfig(t *testing.T) { tests := []struct { name string - wireserverIp string + wireserverIP string exp nmagent.Config shouldErr bool }{ @@ -126,7 +125,7 @@ func TestNMAgentConfig(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - got, err := nmagent.NewConfig(test.wireserverIp) + got, err := nmagent.NewConfig(test.wireserverIP) if err != nil && !test.shouldErr { t.Fatal("unexpected error fetching nmagent config: err:", err) }