Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix client caching #54

Merged
merged 4 commits into from
Jun 20, 2024
Merged

fix client caching #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5fc5a8f
fix client caching
RalfHammer Jun 19, 2024
e1755ed
support passord changes in client caches
RalfHammer Jun 20, 2024
b4b4ac5
re-activate test inside docker build
RalfHammer Jun 20, 2024
590c3ac
same indentation as before
RalfHammer Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,28 @@
]
},
{
"name": "Run integration test",
"type": "go",
"request": "launch",
"mode": "test",
"program": "${workspaceFolder}/internal/controllers",
"console": "internalConsole",
"env": {
"KUBEBUILDER_ASSETS": "${workspaceFolder}/bin/k8s/current",
"TEST_TIMEOUT": "1200",
},
"args": [],
"showLog": true
"name": "Run client test",
"type": "go",
"request": "launch",
"mode": "test",
"program": "${workspaceFolder}/internal/cf",
"console": "internalConsole",
"args": [],
"showLog": true
},
{
"name": "Run integration test",
"type": "go",
"request": "launch",
"mode": "test",
"program": "${workspaceFolder}/internal/controllers",
"console": "internalConsole",
"env": {
"KUBEBUILDER_ASSETS": "${workspaceFolder}/bin/k8s/current",
"TEST_TIMEOUT": "1200",
},
"args": [],
"showLog": true
},
]
}
134 changes: 85 additions & 49 deletions internal/cf/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,32 @@ const (
)

type organizationClient struct {
url string
username string
password string
organizationName string
client cfclient.Client
}

type clientIdentifier struct {
URL string
Username string
}

type spaceClient struct {
url string
username string
password string
spaceGuid string
client cfclient.Client
}

type clientIdentifier struct {
url string
username string
}

type clientCacheEntry struct {
url string
username string
password string
client cfclient.Client
}

var (
cacheMutex = &sync.Mutex{}
clientCache = make(map[clientIdentifier]*clientCacheEntry)
)

func newOrganizationClient(organizationName string, url string, username string, password string) (*organizationClient, error) {
if organizationName == "" {
return nil, fmt.Errorf("missing or empty organization name")
Expand All @@ -68,7 +74,7 @@ func newOrganizationClient(organizationName string, url string, username string,
if err != nil {
return nil, err
}
return &organizationClient{url: url, username: username, password: password, organizationName: organizationName, client: *c}, nil
return &organizationClient{organizationName: organizationName, client: *c}, nil
}

func newSpaceClient(spaceGuid string, url string, username string, password string) (*spaceClient, error) {
Expand All @@ -92,71 +98,101 @@ func newSpaceClient(spaceGuid string, url string, username string, password stri
if err != nil {
return nil, err
}
return &spaceClient{url: url, username: username, password: password, spaceGuid: spaceGuid, client: *c}, nil
return &spaceClient{spaceGuid: spaceGuid, client: *c}, nil
}

var (
spaceClientCache = make(map[clientIdentifier]*spaceClient)
orgClientCache = make(map[clientIdentifier]*organizationClient)
cacheMutex = &sync.Mutex{}
)

func NewOrganizationClient(organizationName string, url string, username string, password string) (facade.OrganizationClient, error) {
cacheMutex.Lock()
defer cacheMutex.Unlock()
identifier := clientIdentifier{URL: url, Username: username}
client, cached := orgClientCache[identifier]
var err error
if !cached {

// look up CF client in cache
identifier := clientIdentifier{url: url, username: username}
cacheEntry, isInCache := clientCache[identifier]

var err error = nil
var client *organizationClient = nil
if isInCache {
// re-use CF client and wrap it as organizationClient
client = &organizationClient{organizationName: organizationName, client: cacheEntry.client}
if cacheEntry.password != password {
// password was rotated => delete client from cache and create a new one below
delete(clientCache, identifier)
isInCache = false
}
}

if !isInCache {
// create new CF client and wrap it as organizationClient
client, err = newOrganizationClient(organizationName, url, username, password)
if err == nil {
orgClientCache[identifier] = client
}
} else {
// If the password has changed since we cached the client, we want to update it to the new one
if client.password != password {
client.password = password
// add CF client to cache
clientCache[identifier] = &clientCacheEntry{url: url, username: username, password: password, client: client.client}
Comment on lines +108 to +129
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a little bit of an odd way of doing it, but it should work.

}
}

return client, err
}

func NewSpaceClient(spaceGuid string, url string, username string, password string) (facade.SpaceClient, error) {
cacheMutex.Lock()
defer cacheMutex.Unlock()
identifier := clientIdentifier{URL: url, Username: username}
client, cached := spaceClientCache[identifier]
var err error
if !cached {

// look up CF client in cache
identifier := clientIdentifier{url: url, username: username}
cacheEntry, isInCache := clientCache[identifier]

var err error = nil
var client *spaceClient = nil
if isInCache {
// re-use CF client from cache and wrap it as spaceClient
client = &spaceClient{spaceGuid: spaceGuid, client: cacheEntry.client}
if cacheEntry.password != password {
// password was rotated => delete client from cache and create a new one below
delete(clientCache, identifier)
isInCache = false
}
}

if !isInCache {
// create new CF client and wrap it as spaceClient
client, err = newSpaceClient(spaceGuid, url, username, password)
if err == nil {
spaceClientCache[identifier] = client
}
} else {
// If the password has changed since we cached the client, we want to update it to the new one
if client.password != password {
client.password = password
// add CF client to cache
clientCache[identifier] = &clientCacheEntry{url: url, username: username, password: password, client: client.client}
Comment on lines +140 to +161
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I think we should merge this, but later think about factoring out this part.

}
}

return client, err
}

func NewSpaceHealthChecker(spaceGuid string, url string, username string, password string) (facade.SpaceHealthChecker, error) {
cacheMutex.Lock()
defer cacheMutex.Unlock()
identifier := clientIdentifier{URL: url, Username: username}
client, cached := spaceClientCache[identifier]
var err error
if !cached {

// look up CF client in cache
identifier := clientIdentifier{url: url, username: username}
cacheEntry, isInCache := clientCache[identifier]

var err error = nil
var client *spaceClient = nil
if isInCache {
// re-use CF client from cache and wrap it as spaceClient
client = &spaceClient{spaceGuid: spaceGuid, client: cacheEntry.client}
if cacheEntry.password != password {
// password was rotated => delete client from cache and create a new one below
delete(clientCache, identifier)
isInCache = false
}
}

if !isInCache {
// create new CF client and wrap it as spaceClient
client, err = newSpaceClient(spaceGuid, url, username, password)
if err == nil {
spaceClientCache[identifier] = client
}
} else {
// If the password has changed since we cached the client, we want to update it to the new one
if client.password != password {
client.password = password
// add CF client to cache
clientCache[identifier] = &clientCacheEntry{url: url, username: username, password: password, client: client.client}
}
}

return client, err
}
68 changes: 62 additions & 6 deletions internal/cf/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,14 @@ import (
// - use separate resource names to prevent collisions between tests

const (
OrgName = "test-org"
Username = "testUser"
Password = "testPass"
Owner = "testOwner"
OrgName = "test-org"
OrgName2 = "test-org-2"
SpaceName = "test-space"
SpaceName2 = "test-space-2"
Username = "testUser"
Password = "testPass"
Owner = "testOwner"
Owner2 = "testOwner2"

spacesURI = "/v3/spaces"
serviceInstancesURI = "/v3/service_instances"
Expand Down Expand Up @@ -87,7 +91,7 @@ var _ = Describe("CF Client tests", Ordered, func() {
Describe("NewOrganizationClient", func() {
BeforeEach(func() {
// Reset the cache so tests can be run independently
orgClientCache = make(map[clientIdentifier]*organizationClient)
clientCache = make(map[clientIdentifier]*clientCacheEntry)
// Reset server call counts
server.Reset()
// Register handlers
Expand Down Expand Up @@ -156,12 +160,38 @@ var _ = Describe("CF Client tests", Ordered, func() {

Expect(server.ReceivedRequests()).To(HaveLen(4))
})

It("should be able to query two different orgs", func() {
// test org 1
orgClient1, err1 := NewOrganizationClient(OrgName, url, Username, Password)
Expect(err1).To(BeNil())
orgClient1.GetSpace(ctx, Owner)
// Discover UAA endpoint
Expect(server.ReceivedRequests()[0].Method).To(Equal("GET"))
Expect(server.ReceivedRequests()[0].URL.Path).To(Equal("/"))
// Get new oAuth token
Expect(server.ReceivedRequests()[1].Method).To(Equal("POST"))
Expect(server.ReceivedRequests()[1].URL.Path).To(Equal(uaaURI))
// Get instance
Expect(server.ReceivedRequests()[2].Method).To(Equal("GET"))
Expect(server.ReceivedRequests()[2].RequestURI).To(ContainSubstring(Owner))

// test org 2
orgClient2, err2 := NewOrganizationClient(OrgName2, url, Username, Password)
Expect(err2).To(BeNil())
orgClient2.GetSpace(ctx, Owner2)
// no discovery of UAA endpoint or oAuth token here due to caching
// Get instance
Expect(server.ReceivedRequests()[3].Method).To(Equal("GET"))
Expect(server.ReceivedRequests()[3].RequestURI).To(ContainSubstring(Owner2))
})

})

Describe("NewSpaceClient", func() {
BeforeEach(func() {
// Reset the cache so tests can be run independently
spaceClientCache = make(map[clientIdentifier]*spaceClient)
clientCache = make(map[clientIdentifier]*clientCacheEntry)
// Reset server call counts
server.Reset()
// Register handlers
Expand Down Expand Up @@ -230,5 +260,31 @@ var _ = Describe("CF Client tests", Ordered, func() {

Expect(server.ReceivedRequests()).To(HaveLen(4))
})

It("should be able to query two different spaces", func() {
// test space 1
spaceClient1, err1 := NewSpaceClient(SpaceName, url, Username, Password)
Expect(err1).To(BeNil())
spaceClient1.GetInstance(ctx, Owner)
// Discover UAA endpoint
Expect(server.ReceivedRequests()[0].Method).To(Equal("GET"))
Expect(server.ReceivedRequests()[0].URL.Path).To(Equal("/"))
// Get new oAuth token
Expect(server.ReceivedRequests()[1].Method).To(Equal("POST"))
Expect(server.ReceivedRequests()[1].URL.Path).To(Equal(uaaURI))
// Get instance
Expect(server.ReceivedRequests()[2].Method).To(Equal("GET"))
Expect(server.ReceivedRequests()[2].RequestURI).To(ContainSubstring(Owner))

// test space 2
spaceClient2, err2 := NewSpaceClient(SpaceName2, url, Username, Password)
Expect(err2).To(BeNil())
spaceClient2.GetInstance(ctx, Owner2)
// no discovery of UAA endpoint or oAuth token here due to caching
// Get instance
Expect(server.ReceivedRequests()[3].Method).To(Equal("GET"))
Expect(server.ReceivedRequests()[3].RequestURI).To(ContainSubstring(Owner2))
})

})
})