Discovery: Add cache argument and embed unmarshal on startup

11 files changed, 93 insertions, 83 deletions

diff --git a/client/client.go b/client/client.go
index 793597a..eb00a16 100644
--- a/client/client.go
+++ b/client/client.go
@@ -192,6 +192,12 @@ func New(name string, version string, directory string, stateCallback func(FSMSt
 
 	disco, release := c.discoMan.Discovery(true)
 	defer release()
+
+	err = disco.Fill()
+	if err != nil {
+		slog.Warn("failed filling discovery cache", "error", err)
+	}
+
 	disco.MarkServersExpired()
 	if !c.cfg.HasSecureInternet() {
 		disco.MarkOrganizationsExpired()
@@ -475,7 +481,7 @@ func (c *Client) GetConfig(ck *cookie.Cookie, identifier string, _type srvtypes.
 	if _type != srvtypes.TypeCustom {
 		disco, release := c.discoMan.Discovery(true)
 		// make sure the servers are fetched fresh
-		_, _, dserverr := disco.Servers(ctx)
+		_, _, dserverr := disco.Servers(ctx, false)
 		if dserverr != nil {
 			slog.Warn("failed to fetch server discovery when getting config", "error", dserverr)
 		}
@@ -489,7 +495,7 @@ func (c *Client) GetConfig(ck *cookie.Cookie, identifier string, _type srvtypes.
 	case srvtypes.TypeSecureInternet:
 		disco, release := c.discoMan.Discovery(true)
 		// make sure the organizations are fetched if they need an update
-		_, _, dorgerr := disco.Organizations(ctx)
+		_, _, dorgerr := disco.Organizations(ctx, false)
 		if dorgerr != nil {
 			slog.Warn("failed to fetch organization discovery when getting config", "error", dorgerr)
 		}
diff --git a/client/discovery.go b/client/discovery.go
index b434025..802fe48 100644
--- a/client/discovery.go
+++ b/client/discovery.go
@@ -19,7 +19,8 @@ func (c *Client) hasDiscovery() bool {
 // If the list cannot be retrieved an error is returned.
 // If this is the case then a previous version of the list is returned if there is any.
 // This takes into account the frequency of updates, see: https://github.com/eduvpn/documentation/blob/v3/SERVER_DISCOVERY.md#organization-list.
-func (c *Client) DiscoOrganizations(ck *cookie.Cookie, search string) (*discotypes.Organizations, error) {
+// cache is set to true if there should be no network call done
+func (c *Client) DiscoOrganizations(ck *cookie.Cookie, cache bool, search string) (*discotypes.Organizations, error) {
 	// Not supported with Let's Connect! & govVPN
 	if !c.hasDiscovery() {
 		return nil, i18nerr.NewInternal("Organization discovery with this client ID is not supported")
@@ -28,7 +29,7 @@ func (c *Client) DiscoOrganizations(ck *cookie.Cookie, search string) (*discotyp
 	disco, release := c.discoMan.Discovery(true)
 	defer release()
 
-	orgs, fresh, err := disco.Organizations(ck.Context())
+	orgs, fresh, err := disco.Organizations(ck.Context(), cache)
 	if fresh {
 		defer c.TrySave()
 	}
@@ -68,7 +69,8 @@ func (c *Client) DiscoOrganizations(ck *cookie.Cookie, search string) (*discotyp
 // If the list cannot be retrieved an error is returned.
 // If this is the case then a previous version of the list is returned if there is any.
 // This takes into account the frequency of updates, see: https://github.com/eduvpn/documentation/blob/v3/SERVER_DISCOVERY.md#server-list.
-func (c *Client) DiscoServers(ck *cookie.Cookie, search string) (*discotypes.Servers, error) {
+// the cache argument is true if no network call should be done
+func (c *Client) DiscoServers(ck *cookie.Cookie, cache bool, search string) (*discotypes.Servers, error) {
 	// Not supported with Let's Connect! & govVPN
 	if !c.hasDiscovery() {
 		return nil, i18nerr.NewInternal("Server discovery with this client ID is not supported")
@@ -76,7 +78,7 @@ func (c *Client) DiscoServers(ck *cookie.Cookie, search string) (*discotypes.Ser
 
 	disco, release := c.discoMan.Discovery(true)
 	defer release()
-	servs, fresh, err := disco.Servers(ck.Context())
+	servs, fresh, err := disco.Servers(ck.Context(), cache)
 	if fresh {
 		defer c.TrySave()
 	}
diff --git a/cmd/eduvpn-cli/main.go b/cmd/eduvpn-cli/main.go
index 92c37f5..8c778a6 100644
--- a/cmd/eduvpn-cli/main.go
+++ b/cmd/eduvpn-cli/main.go
@@ -162,11 +162,11 @@ func printConfig(url string, cc string, srvType srvtypes.Type, prof string, debu
 	_ = c.Register()
 
 	ck := cookie.NewWithContext(context.Background())
-	_, err = c.DiscoOrganizations(ck, "")
+	_, err = c.DiscoOrganizations(ck, false, "")
 	if err != nil {
 		return err
 	}
-	_, err = c.DiscoServers(ck, "")
+	_, err = c.DiscoServers(ck, false, "")
 	if err != nil {
 		return err
 	}
diff --git a/exports/exports.go b/exports/exports.go
index 9be3b66..931bed5 100644
--- a/exports/exports.go
+++ b/exports/exports.go
@@ -634,6 +634,7 @@ func SetSecureLocation(orgID *C.char, cc *C.char) *C.char {
 // DiscoServers gets the servers from discovery, returned as `types/discovery/discovery.go Servers` marshalled as JSON
 //
 //   - `c` is the Cookie that needs to be passed. Create a new Cookie using `CookieNew`
+//   - `cache` indicates whether or not the cache should only be used, meaning no network call
 //   - `search` is the search string for filtering the list.
 //
 // If any of the words in the search query is not contained in any of the display names or keywords, the candidate is filtered.
@@ -677,7 +678,7 @@ func SetSecureLocation(orgID *C.char, cc *C.char) *C.char {
 //		} , null
 //
 //export DiscoServers
-func DiscoServers(c C.uintptr_t, search *C.char) (*C.char, *C.char) {
+func DiscoServers(c C.uintptr_t, cache C.int, search *C.char) (*C.char, *C.char) {
 	state, stateErr := getVPNState()
 	if stateErr != nil {
 		return nil, getCError(stateErr)
@@ -686,7 +687,7 @@ func DiscoServers(c C.uintptr_t, search *C.char) (*C.char, *C.char) {
 	if err != nil {
 		return nil, getCError(err)
 	}
-	servers, err := state.DiscoServers(ck, goString(search))
+	servers, err := state.DiscoServers(ck, cache != 0, goString(search))
 	if servers == nil && err != nil {
 		return nil, getCError(err)
 	}
@@ -700,6 +701,7 @@ func DiscoServers(c C.uintptr_t, search *C.char) (*C.char, *C.char) {
 // DiscoOrganizations gets the organizations from discovery, returned as `types/discovery/discovery.go Organizations` marshalled as JSON.
 //
 //   - `c` is the Cookie that needs to be passed. Create a new Cookie using `CookieNew`
+//   - `cache` indicates whether or not the cache should only be used, meaning no network call
 //   - `search` is the search string for filtering the list.
 //
 // If any of the words in the `search` query is not contained in any of the display names or keywords, the candidate is filtered.
@@ -752,7 +754,7 @@ func DiscoServers(c C.uintptr_t, search *C.char) (*C.char, *C.char) {
 //		}, null
 //
 //export DiscoOrganizations
-func DiscoOrganizations(c C.uintptr_t, search *C.char) (*C.char, *C.char) {
+func DiscoOrganizations(c C.uintptr_t, cache C.int, search *C.char) (*C.char, *C.char) {
 	state, stateErr := getVPNState()
 	if stateErr != nil {
 		return nil, getCError(stateErr)
@@ -761,7 +763,7 @@ func DiscoOrganizations(c C.uintptr_t, search *C.char) (*C.char, *C.char) {
 	if err != nil {
 		return nil, getCError(err)
 	}
-	orgs, err := state.DiscoOrganizations(ck, goString(search))
+	orgs, err := state.DiscoOrganizations(ck, cache != 0, goString(search))
 	if orgs == nil && err != nil {
 		return nil, getCError(err)
 	}
diff --git a/exports/exports_test_wrapper.go b/exports/exports_test_wrapper.go
index 0c9b9dc..96abb74 100644
--- a/exports/exports_test_wrapper.go
+++ b/exports/exports_test_wrapper.go
@@ -510,14 +510,14 @@ func testLetsConnectDiscovery(t *testing.T) {
 		t.Fatalf("failed to add server got a different error: %v, want: %v", addErr, exptErr)
 	}
 
-	_, servErr := DiscoServers(ck, nil)
+	_, servErr := DiscoServers(ck, 0, nil)
 	servErrS := getError(t, servErr)
 	exptErr = "An internal error occurred. The cause of the error is: Server discovery with this client ID is not supported."
 	if servErrS != exptErr {
 		t.Fatalf("discovery servers got a different error: %v, want: %v", servErrS, exptErr)
 	}
 
-	_, orgErr := DiscoOrganizations(ck, nil)
+	_, orgErr := DiscoOrganizations(ck, 0, nil)
 	orgErrS := getError(t, orgErr)
 	exptErr = "An internal error occurred. The cause of the error is: Organization discovery with this client ID is not supported."
 	if orgErrS != exptErr {
diff --git a/internal/discovery/discovery.go b/internal/discovery/discovery.go
index 81163f8..80a69eb 100644
--- a/internal/discovery/discovery.go
+++ b/internal/discovery/discovery.go
@@ -293,44 +293,20 @@ func (discovery *Discovery) DetermineServersUpdate() bool {
 	return !time.Now().Before(upd)
 }
 
-func (discovery *Discovery) previousOrganizations() (*Organizations, error) {
-	// If the version field is not zero then we have a cached struct
-	// We also immediately return this copy if we have no embedded JSON
-	if discovery.OrganizationList.Version != 0 || !HasCache {
-		return &discovery.OrganizationList, nil
+func (discovery *Discovery) cachedOrgs() *Organizations {
+	if discovery.OrganizationList.Version == 0 {
+		return nil
 	}
-
-	// We do not have a cached struct, this we need to get it using the embedded JSON
-	var eo Organizations
-	if err := json.Unmarshal(eOrganizations, &eo); err != nil {
-		return nil, fmt.Errorf("failed parsing discovery organizations from the embedded cache with error: %w", err)
-	}
-	discovery.OrganizationList = eo
-	return &eo, nil
-}
-
-func (discovery *Discovery) previousServers() (*Servers, error) {
-	// If the version field is not zero then we have a cached struct
-	// We also immediately return this copy if we have no embedded JSON
-	if discovery.ServerList.Version != 0 || !HasCache {
-		return &discovery.ServerList, nil
-	}
-
-	// We do not have a cached struct, this we need to get it using the embedded JSON
-	var es Servers
-	if err := json.Unmarshal(eServers, &es); err != nil {
-		return nil, fmt.Errorf("failed parsing discovery servers from the embedded cache with error: %w", err)
-	}
-	discovery.ServerList = es
-	return &es, nil
+	return &discovery.OrganizationList
 }
 
 // Organizations returns the discovery organizations
 // The second return value is a boolean that indicates whether a fresh list was updated internally
 // If there was an error, a cached copy is returned if available.
-func (discovery *Discovery) Organizations(ctx context.Context) (*Organizations, bool, error) {
-	if !discovery.DetermineOrganizationsUpdate() {
-		return &discovery.OrganizationList, false, nil
+// cache is set to true if there should be no network call done
+func (discovery *Discovery) Organizations(ctx context.Context, cache bool) (*Organizations, bool, error) {
+	if cache || !discovery.DetermineOrganizationsUpdate() {
+		return discovery.cachedOrgs(), false, nil
 	}
 	file := "organization_list.json"
 	var jsonDecode Organizations
@@ -347,11 +323,7 @@ func (discovery *Discovery) Organizations(ctx context.Context) (*Organizations,
 			}
 		}
 		// Return previous with an error
-		orgs, perr := discovery.previousOrganizations()
-		if perr != nil {
-			slog.Warn("failed to get previous discovery organizations", "error", perr)
-		}
-		return orgs, false, err
+		return discovery.cachedOrgs(), false, err
 	}
 	if len(jsonDecode.List) == 0 {
 		slog.Warn("fresh organization list is empty")
@@ -365,12 +337,20 @@ func (discovery *Discovery) Organizations(ctx context.Context) (*Organizations,
 	return &discovery.OrganizationList, true, nil
 }
 
+func (discovery *Discovery) cachedServers() *Servers {
+	if discovery.ServerList.Version == 0 {
+		return nil
+	}
+	return &discovery.ServerList
+}
+
 // Servers returns the discovery servers
 // The second return value is a boolean that indicates whether a fresh list was updated internally
 // If there was an error, a cached copy is returned if available.
-func (discovery *Discovery) Servers(ctx context.Context) (*Servers, bool, error) {
-	if !discovery.DetermineServersUpdate() {
-		return &discovery.ServerList, false, nil
+// cache is set to true if there should be no network call done
+func (discovery *Discovery) Servers(ctx context.Context, cache bool) (*Servers, bool, error) {
+	if cache || !discovery.DetermineServersUpdate() {
+		return discovery.cachedServers(), false, nil
 	}
 	file := "server_list.json"
 	var jsonDecode Servers
@@ -387,11 +367,7 @@ func (discovery *Discovery) Servers(ctx context.Context) (*Servers, bool, error)
 			}
 		}
 		// Return previous with an error
-		srvs, perr := discovery.previousServers()
-		if perr != nil {
-			slog.Warn("failed to get previous discovery server", "error", perr)
-		}
-		return srvs, false, err
+		return discovery.cachedServers(), false, err
 	}
 	if len(jsonDecode.List) == 0 {
 		slog.Warn("fresh server list is empty")
@@ -407,9 +383,17 @@ func (discovery *Discovery) Servers(ctx context.Context) (*Servers, bool, error)
 
 // UpdateServers updates the discovery servers to the new version
 // It does this by checking versions
-func (discovery *Discovery) UpdateServers(other Discovery) {
-	if other.ServerList.Version >= discovery.ServerList.Version {
-		discovery.ServerList = other.ServerList
+func (discovery *Discovery) UpdateServers(other Servers) {
+	if other.Version >= discovery.ServerList.Version {
+		discovery.ServerList = other
+	}
+}
+
+// UpdateOrganizations updates the discovery organizations to the new version
+// It does this by checking versions
+func (discovery *Discovery) UpdateOrganizations(other Organizations) {
+	if other.Version >= discovery.OrganizationList.Version {
+		discovery.OrganizationList = other
 	}
 }
 
@@ -429,3 +413,19 @@ func (discovery *Discovery) Copy() (Discovery, error) {
 
 	return dest, nil
 }
+
+// Fill makes sure that the cache is filled with the embedded discovery
+func (discovery *Discovery) Fill() error {
+	if !HasCache {
+		return nil
+	}
+
+	var err error
+	var es Servers
+	err = errors.Join(err, json.Unmarshal(eServers, &es))
+	discovery.UpdateServers(es)
+	var eo Organizations
+	err = errors.Join(err, json.Unmarshal(eOrganizations, &eo))
+	discovery.UpdateOrganizations(eo)
+	return err
+}
diff --git a/internal/discovery/discovery_test.go b/internal/discovery/discovery_test.go
index 08c5ef8..802123a 100644
--- a/internal/discovery/discovery_test.go
+++ b/internal/discovery/discovery_test.go
@@ -23,7 +23,7 @@ func TestServers(t *testing.T) {
 	}
 	d := &Discovery{httpClient: c}
 	// get servers
-	_, fresh, err := d.Servers(context.Background())
+	_, fresh, err := d.Servers(context.Background(), false)
 	if !fresh {
 		t.Fatalf("Did not obtain the server list fresh")
 	}
@@ -51,7 +51,7 @@ func TestServers(t *testing.T) {
 		SupportContact: []string{"mailto:test@example.org"},
 	}
 	// conditional requests: this should not be fetched fresh
-	_, fresh, err = d.Servers(context.Background())
+	_, fresh, err = d.Servers(context.Background(), false)
 	if fresh {
 		t.Fatalf("Obtained the server list fresh with conditional requests")
 	}
@@ -60,7 +60,7 @@ func TestServers(t *testing.T) {
 	}
 	// mock conditional requests
 	d.ServerList.UpdateHeader = time.Time{}
-	s1, fresh, err := d.Servers(context.Background())
+	s1, fresh, err := d.Servers(context.Background(), false)
 	if !fresh {
 		t.Fatalf("Did not obtain the server list fresh with mocked conditional request and mocked entry")
 	}
@@ -74,7 +74,7 @@ func TestServers(t *testing.T) {
 	// Shutdown the server
 	s.Close()
 	// Test if we get the same cached copy
-	s2, fresh, err := d.Servers(context.Background())
+	s2, fresh, err := d.Servers(context.Background(), false)
 	// We should not get an error as the timestamp is not expired
 	if fresh {
 		t.Fatalf("The server list was obtained fresh")
@@ -90,7 +90,7 @@ func TestServers(t *testing.T) {
 	// we should return the previous with an error
 	d.ServerList.Timestamp = time.Now().Add(-1 * time.Hour)
 
-	s3, fresh, err := d.Servers(context.Background())
+	s3, fresh, err := d.Servers(context.Background(), false)
 	if fresh {
 		t.Fatalf("Server list was gotten fresh")
 	}
@@ -115,7 +115,7 @@ func TestOrganizations(t *testing.T) {
 	}
 	d := &Discovery{httpClient: c}
 	// get servers
-	_, fresh, err := d.Organizations(context.Background())
+	_, fresh, err := d.Organizations(context.Background(), false)
 	if !fresh {
 		t.Fatalf("The organization list was not obtained fresh")
 	}
@@ -145,7 +145,7 @@ func TestOrganizations(t *testing.T) {
 		},
 	}
 
-	_, fresh, err = d.Organizations(context.Background())
+	_, fresh, err = d.Organizations(context.Background(), false)
 	if fresh {
 		t.Fatalf("Obtained the organization list fresh with conditional requests")
 	}
@@ -154,7 +154,7 @@ func TestOrganizations(t *testing.T) {
 	}
 	// mock conditional requests
 	d.OrganizationList.UpdateHeader = time.Time{}
-	s1, fresh, err := d.Organizations(context.Background())
+	s1, fresh, err := d.Organizations(context.Background(), false)
 	if !fresh {
 		t.Fatalf("Did not obtain the organization list fresh after inserting a mock entry, faking expiry and mocking conditional request")
 	}
@@ -169,7 +169,7 @@ func TestOrganizations(t *testing.T) {
 	s.Close()
 	// Test if we get the same cached copy
 	// We should not get an error as the timestamp is not zero
-	s2, fresh, err := d.Organizations(context.Background())
+	s2, fresh, err := d.Organizations(context.Background(), false)
 	if fresh {
 		t.Fatalf("The organization list is freshly obtained")
 	}
diff --git a/internal/discovery/manager.go b/internal/discovery/manager.go
index 134525b..4fb4f8e 100644
--- a/internal/discovery/manager.go
+++ b/internal/discovery/manager.go
@@ -72,10 +72,10 @@ func (m *Manager) Startup(ctx context.Context, cb func()) {
 		}
 		m.unlock(false)
 		// we already log the warning
-		discoCopy.Servers(ctx) //nolint:errcheck
+		discoCopy.Servers(ctx, false) //nolint:errcheck
 
 		m.lock(true)
-		m.disco.UpdateServers(discoCopy)
+		m.disco.UpdateServers(discoCopy.ServerList)
 		m.unlock(true)
 		m.wait.Done()
 
diff --git a/internal/server/secureinternet.go b/internal/server/secureinternet.go
index e0d081a..69b1e97 100644
--- a/internal/server/secureinternet.go
+++ b/internal/server/secureinternet.go
@@ -67,8 +67,8 @@ func (s *Servers) AddSecure(ctx context.Context, discom *discovery.Manager, orgI
 			defer release()
 			// the only thing we can do is log warn
 			// this is already done in the functions
-			newd.Servers(ctx)       //nolint:errcheck
-			newd.Organizations(ctx) //nolint:errcheck
+			newd.Servers(ctx, false)       //nolint:errcheck
+			newd.Organizations(ctx, false) //nolint:errcheck
 			updorg, updsrv, err := newd.SecureHomeArgs(orgID)
 			if err != nil {
 				return "", err
@@ -146,9 +146,9 @@ func (s *Servers) GetSecure(ctx context.Context, orgID string, discom *discovery
 			// the only thing we can do is log warn
 			// this is already done in the functions
 			newd.MarkServersExpired()
-			newd.Servers(ctx) //nolint:errcheck
+			newd.Servers(ctx, false) //nolint:errcheck
 			newd.MarkOrganizationsExpired()
-			newd.Organizations(ctx) //nolint:errcheck
+			newd.Organizations(ctx, false) //nolint:errcheck
 			updorg, updsrv, err := newd.SecureHomeArgs(orgID)
 			if err != nil {
 				return "", err
diff --git a/wrappers/python/eduvpn_common/loader.py b/wrappers/python/eduvpn_common/loader.py
index 6957c15..870f0b4 100644
--- a/wrappers/python/eduvpn_common/loader.py
+++ b/wrappers/python/eduvpn_common/loader.py
@@ -49,8 +49,8 @@ def initialize_functions(lib: CDLL) -> None:
     lib.Deregister.argtypes, lib.Deregister.restype = [], None
     lib.ExpiryTimes.argtypes, lib.ExpiryTimes.restype = [], DataError
     lib.FreeString.argtypes, lib.FreeString.restype = [c_void_p], None
-    lib.DiscoOrganizations.argtypes, lib.DiscoOrganizations.restype = [c_int, c_char_p], DataError
-    lib.DiscoServers.argtypes, lib.DiscoServers.restype = [c_int, c_char_p], DataError
+    lib.DiscoOrganizations.argtypes, lib.DiscoOrganizations.restype = [c_int, c_int, c_char_p], DataError
+    lib.DiscoServers.argtypes, lib.DiscoServers.restype = [c_int, c_int, c_char_p], DataError
     lib.GetConfig.argtypes, lib.GetConfig.restype = (
         [
             c_int,
diff --git a/wrappers/python/eduvpn_common/main.py b/wrappers/python/eduvpn_common/main.py
index 613dce2..334c8b8 100644
--- a/wrappers/python/eduvpn_common/main.py
+++ b/wrappers/python/eduvpn_common/main.py
@@ -194,14 +194,14 @@ class EduVPN(object):
             forwardError(server_err)
         return server
 
-    def get_disco_organizations(self, search="") -> str:
-        orgs, _ = self.go_cookie_function(self.lib.DiscoOrganizations, search)
+    def get_disco_organizations(self, cache: bool = False, search="") -> str:
+        orgs, _ = self.go_cookie_function(self.lib.DiscoOrganizations, cache, search)
         # We don't log anything here as we want to return a result and we don't want to throw here
         # we already log for errors in common
         return orgs
 
-    def get_disco_servers(self, search="") -> str:
-        servers, _ = self.go_cookie_function(self.lib.DiscoServers, search)
+    def get_disco_servers(self, cache: bool = False, search="") -> str:
+        servers, _ = self.go_cookie_function(self.lib.DiscoServers, cache, search)
         # We don't log anything here as we want to return a result and we don't want to throw here
         # we already log for errors in common
         return servers