From a4a3aff6adf80fd8a48dcdc73e90492b765ef945 Mon Sep 17 00:00:00 2001 From: Petr Benas Date: Thu, 16 Jan 2025 10:47:19 +0100 Subject: [PATCH] NLB display name to OCID map Cache the mapping of the display name of a LB to an OCID to avoid looking it up again in each reconciliation. Signed-off-by: Petr Benas --- pkg/cloudprovider/providers/oci/ccm.go | 2 +- pkg/oci/client/client.go | 19 +-- pkg/oci/client/client_factory.go | 2 +- pkg/oci/client/load_balancer.go | 28 ++++ pkg/oci/client/network_load_balancer.go | 76 +++++++++++ pkg/oci/client/network_load_balancer_test.go | 122 +++++++++++++++++- pkg/oci/client/utils.go | 46 +++++++ pkg/volume/provisioner/core/provisioner.go | 2 +- .../e2e/framework/cloud_provider_framework.go | 2 +- 9 files changed, 280 insertions(+), 19 deletions(-) diff --git a/pkg/cloudprovider/providers/oci/ccm.go b/pkg/cloudprovider/providers/oci/ccm.go index 6af041abe3..fe13510e19 100644 --- a/pkg/cloudprovider/providers/oci/ccm.go +++ b/pkg/cloudprovider/providers/oci/ccm.go @@ -100,7 +100,7 @@ func NewCloudProvider(config *providercfg.Config) (cloudprovider.Interface, erro rateLimiter := client.NewRateLimiter(logger.Sugar(), config.RateLimiter) - c, err := client.New(logger.Sugar(), cp, &rateLimiter, config.Auth.TenancyID) + c, err := client.New(logger.Sugar(), cp, &rateLimiter, config) if err != nil { return nil, err } diff --git a/pkg/oci/client/client.go b/pkg/oci/client/client.go index 405f96ed1b..a385d6b3b7 100644 --- a/pkg/oci/client/client.go +++ b/pkg/oci/client/client.go @@ -18,6 +18,7 @@ import ( "context" "time" + providercfg "github.com/oracle/oci-cloud-controller-manager/pkg/cloudprovider/providers/oci/config" "github.com/oracle/oci-go-sdk/v65/common" "github.com/oracle/oci-go-sdk/v65/common/auth" "github.com/oracle/oci-go-sdk/v65/core" @@ -194,7 +195,7 @@ type client struct { } // New constructs an OCI API client. -func New(logger *zap.SugaredLogger, cp common.ConfigurationProvider, opRateLimiter *RateLimiter, targetTenancyID string) (Interface, error) { +func New(logger *zap.SugaredLogger, cp common.ConfigurationProvider, opRateLimiter *RateLimiter, cloudProviderConfig *providercfg.Config) (Interface, error) { compute, err := core.NewComputeClientWithConfigurationProvider(cp) if err != nil { @@ -283,23 +284,15 @@ func New(logger *zap.SugaredLogger, cp common.ConfigurationProvider, opRateLimit RetryPolicy: newRetryPolicy(), } - loadbalancer := loadbalancerClientStruct{ - loadbalancer: lb, - requestMetadata: requestMetadata, - rateLimiter: *opRateLimiter, - } - networkloadbalancer := networkLoadbalancer{ - networkloadbalancer: nlb, - requestMetadata: requestMetadata, - rateLimiter: *opRateLimiter, - } + loadbalancer := NewLBClient(lb, requestMetadata, opRateLimiter).WithEmptyNameCache() + networkloadbalancer := NewNLBClient(nlb, requestMetadata, opRateLimiter).WithEmptyNameCache() c := &client{ compute: &compute, network: &network, identity: &identity, - loadbalancer: &loadbalancer, - networkloadbalancer: &networkloadbalancer, + loadbalancer: loadbalancer, + networkloadbalancer: networkloadbalancer, bs: &bs, filestorage: &fss, //compartment: &compartment, diff --git a/pkg/oci/client/client_factory.go b/pkg/oci/client/client_factory.go index 1d7410e7aa..765fca2480 100644 --- a/pkg/oci/client/client_factory.go +++ b/pkg/oci/client/client_factory.go @@ -29,6 +29,6 @@ func GetClient(logger *zap.SugaredLogger, cfg *config.Config) (Interface, error) rateLimiter := NewRateLimiter(logger, cfg.RateLimiter) - c, err := New(logger, cp, &rateLimiter, cfg.Auth.TenancyID) + c, err := New(logger, cp, &rateLimiter, cfg) return c, err } diff --git a/pkg/oci/client/load_balancer.go b/pkg/oci/client/load_balancer.go index f72337b197..81e1f07f40 100644 --- a/pkg/oci/client/load_balancer.go +++ b/pkg/oci/client/load_balancer.go @@ -33,6 +33,7 @@ const ( ) type loadbalancerClientStruct struct { + nameToOcid LBNameOcidCache loadbalancer loadBalancerClient requestMetadata common.RequestMetadata rateLimiter RateLimiter @@ -64,6 +65,21 @@ type GenericLoadBalancerInterface interface { UpdateLoadBalancer(ctx context.Context, lbID string, details *GenericUpdateLoadBalancerDetails) (string, error) } +func NewLBClient(lb loadBalancerClient, rm common.RequestMetadata, lim *RateLimiter) *loadbalancerClientStruct { + l := loadbalancerClientStruct{ + loadbalancer: lb, + requestMetadata: rm, + rateLimiter: *lim, + } + return &l +} + +func (c *loadbalancerClientStruct) WithEmptyNameCache() *loadbalancerClientStruct { + c.nameToOcid.Initialize() + c.nameToOcid.SetEnabled(true) + return c +} + func (c *loadbalancerClientStruct) GetLoadBalancer(ctx context.Context, id string) (*GenericLoadBalancer, error) { if !c.rateLimiter.Reader.TryAccept() { return nil, RateLimitError(false, "GetLoadBalancer") @@ -83,6 +99,17 @@ func (c *loadbalancerClientStruct) GetLoadBalancer(ctx context.Context, id strin } func (c *loadbalancerClientStruct) GetLoadBalancerByName(ctx context.Context, compartmentID, name string) (*GenericLoadBalancer, error) { + if ocid, ok := c.nameToOcid.Get(name); ok { + lb, err := c.GetLoadBalancer(ctx, ocid) + if err == nil { + return lb, err + } + + if IsNotFound(err) { // Only remove the cached value on 404, not on a 5XX + c.nameToOcid.Delete(name) + } + } + var page *string for { if !c.rateLimiter.Reader.TryAccept() { @@ -101,6 +128,7 @@ func (c *loadbalancerClientStruct) GetLoadBalancerByName(ctx context.Context, co } for _, lb := range resp.Items { if *lb.DisplayName == name { + c.nameToOcid.Set(name, *lb.Id) return c.loadbalancerToGenericLoadbalancer(&lb), nil } } diff --git a/pkg/oci/client/network_load_balancer.go b/pkg/oci/client/network_load_balancer.go index e95dab3bf4..639a58686a 100644 --- a/pkg/oci/client/network_load_balancer.go +++ b/pkg/oci/client/network_load_balancer.go @@ -16,6 +16,7 @@ package client import ( "context" + "regexp" "go.uber.org/zap" "k8s.io/apimachinery/pkg/util/wait" @@ -26,6 +27,7 @@ import ( ) type networkLoadbalancer struct { + nameToOcid LBNameOcidCache networkloadbalancer networkLoadBalancerClient requestMetadata common.RequestMetadata rateLimiter RateLimiter @@ -33,8 +35,70 @@ type networkLoadbalancer struct { const ( NetworkLoadBalancerEntityType = "NetworkLoadBalancer" + // TODO move to utils? + dns1123LabelFmt = "[a-z0-9]([-a-z0-9]*[a-z0-9])?" + uuidFmt = "[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}" + // // + LBNameRegex = "^" + dns1123LabelFmt + "/" + dns1123LabelFmt + "/" + uuidFmt + "$" ) +func NewNLBClient(nlb networkLoadBalancerClient, rm common.RequestMetadata, lim *RateLimiter) *networkLoadbalancer { + n := networkLoadbalancer{ + networkloadbalancer: nlb, + requestMetadata: rm, + rateLimiter: *lim, + } + return &n +} + +// WithEmptyNameCache initializes the NLB Display Name -> OCID cache. Without this, names are not cached. +func (c *networkLoadbalancer) WithEmptyNameCache() *networkLoadbalancer { + c.nameToOcid.Initialize() + c.nameToOcid.SetEnabled(true) + return c +} + +// WithPrepopulatedNameCache initializes the NLB Display Name -> OCID cache and pre-fills it with ocids of all NLBs +// in a given compartment matching the expected display name format. In case of error, the cache is left on, but unpopulated +// or partially populated. +func (c *networkLoadbalancer) WithPrepopulatedNameCache(ctx context.Context, compartmentID string) *networkLoadbalancer { + c.nameToOcid.Initialize() + c.nameToOcid.SetEnabled(true) + + nameRegex, err := regexp.Compile(LBNameRegex) + if err != nil { + return c + } + + var page *string + for { + if !c.rateLimiter.Reader.TryAccept() { + break + } + + resp, err := c.networkloadbalancer.ListNetworkLoadBalancers(ctx, networkloadbalancer.ListNetworkLoadBalancersRequest{ + CompartmentId: &compartmentID, + Page: page, + RequestMetadata: c.requestMetadata, + }) + incRequestCounter(err, listVerb, networkLoadBalancerResource) + if err != nil { + break + } + + for _, lb := range resp.Items { + if nameRegex.MatchString(*lb.DisplayName) { + c.nameToOcid.Set(*lb.DisplayName, *lb.Id) + } + } + if page = resp.OpcNextPage; page == nil { + break + } + } + + return c +} + func (c *networkLoadbalancer) GetLoadBalancer(ctx context.Context, id string) (*GenericLoadBalancer, error) { if !c.rateLimiter.Reader.TryAccept() { return nil, RateLimitError(false, "GetLoadBalancer") @@ -54,6 +118,17 @@ func (c *networkLoadbalancer) GetLoadBalancer(ctx context.Context, id string) (* } func (c *networkLoadbalancer) GetLoadBalancerByName(ctx context.Context, compartmentID string, name string) (*GenericLoadBalancer, error) { + if ocid, ok := c.nameToOcid.Get(name); ok { + lb, err := c.GetLoadBalancer(ctx, ocid) + if err == nil { + return lb, err + } + + if IsNotFound(err) { // Only remove the cached value on 404, not on a 5XX + c.nameToOcid.Delete(name) + } + } + var page *string for { if !c.rateLimiter.Reader.TryAccept() { @@ -72,6 +147,7 @@ func (c *networkLoadbalancer) GetLoadBalancerByName(ctx context.Context, compart } for _, lb := range resp.Items { if *lb.DisplayName == name { + c.nameToOcid.Set(name, *lb.Id) return c.networkLoadbalancerSummaryToGenericLoadbalancer(&lb), nil } } diff --git a/pkg/oci/client/network_load_balancer_test.go b/pkg/oci/client/network_load_balancer_test.go index 711d33d434..56c237a3be 100644 --- a/pkg/oci/client/network_load_balancer_test.go +++ b/pkg/oci/client/network_load_balancer_test.go @@ -17,6 +17,7 @@ package client import ( "context" "errors" + "fmt" errors2 "github.com/pkg/errors" "log" "strings" @@ -98,10 +99,112 @@ func TestNLB_AwaitWorkRequest(t *testing.T) { } } +var ( + fakeNlbOcid1 = "ocid.nlb.fake1" + fakeNlbName1 = "fake display name 1" + fakeNlbOcid2 = "ocid.nlb.fake2" + fakeNlbName2 = "fake display name 2" + fakeSubnetOcid = "ocid.subnet.fake" + + NLBMap = map[string]networkloadbalancer.NetworkLoadBalancer{ + "ocid.nlb.fake1": networkloadbalancer.NetworkLoadBalancer{ + Id: &fakeNlbOcid1, + DisplayName: &fakeNlbName1, + SubnetId: &fakeSubnetOcid, + }, + "ocid.nlb.fake2": networkloadbalancer.NetworkLoadBalancer{ + Id: &fakeNlbOcid2, + DisplayName: &fakeNlbName2, + SubnetId: &fakeSubnetOcid, + }, + } +) + +func TestGetLoadBalancerByName(t *testing.T) { + var totalListCalls int + var loadbalancer = NewNLBClient( + &MockNetworkLoadBalancerClient{debug: true, listCalls: &totalListCalls}, + common.RequestMetadata{}, + &RateLimiter{ + Reader: flowcontrol.NewFakeAlwaysRateLimiter(), + Writer: flowcontrol.NewFakeAlwaysRateLimiter(), + }).WithEmptyNameCache() + + var tests = []struct { + skip bool // set true to skip a test-case + compartment, name, testname string + want string + wantErr error + wantListCalls int + }{ + { + testname: "getFirstNLBFirstTime", + compartment: "ocid.compartment.fake", + name: fakeNlbName1, + want: fakeNlbOcid1, + wantErr: nil, + wantListCalls: 1, + }, + { + testname: "getFirstNLBSecondTime", + compartment: "ocid.compartment.fake", + name: fakeNlbName1, + want: fakeNlbOcid1, + wantErr: nil, + wantListCalls: 1, // totals, no new list should be performed + }, + { + testname: "getSecondNLBTime", + compartment: "ocid.compartment.fake", + name: fakeNlbName2, + want: fakeNlbOcid2, + wantErr: nil, + wantListCalls: 2, + }, + { + testname: "getFirstNLBThirdTime", + compartment: "ocid.compartment.fake", + name: fakeNlbName1, + want: fakeNlbOcid1, + wantErr: nil, + wantListCalls: 2, + }, + { + testname: "getSecondNLBSecondTime", + compartment: "ocid.compartment.fake", + name: fakeNlbName2, + want: fakeNlbOcid2, + wantErr: nil, + wantListCalls: 2, + }, + } + + for _, tt := range tests { + if tt.skip { + continue + } + + t.Run(tt.testname, func(t *testing.T) { + log.Println("running test ", tt.testname) + got, err := loadbalancer.GetLoadBalancerByName(context.Background(), tt.compartment, tt.name) + if got == nil || *got.Id != tt.want { + t.Errorf("Expected %v, but got %v", tt.want, got) + } + if !errors.Is(err, tt.wantErr) { + t.Errorf("Expected error = %v, but got %v", tt.wantErr, err) + } + if totalListCalls != tt.wantListCalls { + t.Errorf("Expected the total number of NLB list calls %d, but got %d", tt.wantListCalls, totalListCalls) + } + }) + } +} + type MockNetworkLoadBalancerClient struct { // MockLoadBalancerClient mocks LoadBalancer client implementation. - counter int - debug bool // set true to run tests with debug logs + counter int + debug bool // set true to run tests with debug logs + listCalls *int // number of list operations performed } type getNetworkLoadBalancerWorkRequestResponse struct { @@ -173,12 +276,27 @@ func (c *MockNetworkLoadBalancerClient) GetWorkRequest(ctx context.Context, requ } func (c *MockNetworkLoadBalancerClient) GetNetworkLoadBalancer(ctx context.Context, request networkloadbalancer.GetNetworkLoadBalancerRequest) (response networkloadbalancer.GetNetworkLoadBalancerResponse, err error) { + if c.debug { + log.Println(fmt.Sprintf("Getting NLB %v", *request.NetworkLoadBalancerId)) + } + + response = networkloadbalancer.GetNetworkLoadBalancerResponse{ + NetworkLoadBalancer: NLBMap[*request.NetworkLoadBalancerId], + } return } func (c *MockNetworkLoadBalancerClient) ListWorkRequests(ctx context.Context, request networkloadbalancer.ListWorkRequestsRequest) (response networkloadbalancer.ListWorkRequestsResponse, err error) { return } func (c *MockNetworkLoadBalancerClient) ListNetworkLoadBalancers(ctx context.Context, request networkloadbalancer.ListNetworkLoadBalancersRequest) (response networkloadbalancer.ListNetworkLoadBalancersResponse, err error) { + if c.debug { + log.Println(fmt.Sprintf("Lising NLBs in compartment %v", *request.CompartmentId)) + } + + for _, nlb := range NLBMap { + response.NetworkLoadBalancerCollection.Items = append(response.NetworkLoadBalancerCollection.Items, networkloadbalancer.NetworkLoadBalancerSummary(nlb)) + } + *c.listCalls += 1 return } func (c *MockNetworkLoadBalancerClient) CreateNetworkLoadBalancer(ctx context.Context, request networkloadbalancer.CreateNetworkLoadBalancerRequest) (response networkloadbalancer.CreateNetworkLoadBalancerResponse, err error) { diff --git a/pkg/oci/client/utils.go b/pkg/oci/client/utils.go index 7459fb0732..4f08c3afcc 100644 --- a/pkg/oci/client/utils.go +++ b/pkg/oci/client/utils.go @@ -18,6 +18,7 @@ import ( "net/http" "os" "strings" + "sync" providercfg "github.com/oracle/oci-cloud-controller-manager/pkg/cloudprovider/providers/oci/config" "github.com/oracle/oci-go-sdk/v65/common" @@ -110,3 +111,48 @@ func IsIpv6SingleStackCluster() bool { } return false } + +type LBNameOcidCache struct { + enabled bool + values map[string]string + lock sync.RWMutex +} + +func (c *LBNameOcidCache) Initialize() { + c.values = make(map[string]string) +} + +func (c *LBNameOcidCache) Get(name string) (ocid string, ok bool) { + c.lock.RLock() + defer c.lock.RUnlock() + + if !c.enabled { + return "", false + } + + ocid, ok = c.values[name] + return +} + +func (c *LBNameOcidCache) Set(name, ocid string) { + c.lock.Lock() + defer c.lock.Unlock() + + if c.enabled && c.values != nil { // nil check prevents panic in case of attempt to write to uninitialized map + c.values[name] = ocid + } +} + +func (c *LBNameOcidCache) SetEnabled(enabled bool) { + c.lock.Lock() + defer c.lock.Unlock() + + c.enabled = enabled +} + +func (c *LBNameOcidCache) Delete(name string) { + c.lock.Lock() + defer c.lock.Unlock() + + delete(c.values, name) +} diff --git a/pkg/volume/provisioner/core/provisioner.go b/pkg/volume/provisioner/core/provisioner.go index e1bb67872e..b92e5d5829 100644 --- a/pkg/volume/provisioner/core/provisioner.go +++ b/pkg/volume/provisioner/core/provisioner.go @@ -124,7 +124,7 @@ func NewOCIProvisioner(logger *zap.SugaredLogger, kubeClient kubernetes.Interfac rateLimiter := client.NewRateLimiter(logger, cfg.RateLimiter) - client, err := client.New(logger, cp, &rateLimiter, cfg.Auth.TenancyID) + client, err := client.New(logger, cp, &rateLimiter, cfg) if err != nil { return nil, errors.Wrapf(err, "unable to construct OCI client") } diff --git a/test/e2e/framework/cloud_provider_framework.go b/test/e2e/framework/cloud_provider_framework.go index 1d12f5215f..31eb3adc59 100644 --- a/test/e2e/framework/cloud_provider_framework.go +++ b/test/e2e/framework/cloud_provider_framework.go @@ -370,7 +370,7 @@ func createOCIClient(cloudProviderConfig *providercfg.Config) (client.Interface, ociClientConfig := common.NewRawConfigurationProvider(cpc.TenancyID, cpc.UserID, cpc.Region, cpc.Fingerprint, cpc.PrivateKey, &cpc.PrivateKeyPassphrase) logger := zap.L() rateLimiter := client.NewRateLimiter(logger.Sugar(), cloudProviderConfig.RateLimiter) - ociClient, err := client.New(logger.Sugar(), ociClientConfig, &rateLimiter, cloudProviderConfig.Auth.TenancyID) + ociClient, err := client.New(logger.Sugar(), ociClientConfig, &rateLimiter, cloudProviderConfig) if err != nil { return nil, errors.Wrapf(err, "Couldn't create oci client from configuration: %s.", cloudConfigFile) }