Skip to content

Commit 732599d

Browse files
rauthsujrauthsuj
committed
refactor: find or default instead of lookup and only one subscribe to avoid multiple restarts
1 parent 6cd26a4 commit 732599d

File tree

2 files changed

+56
-93
lines changed

2 files changed

+56
-93
lines changed

‎src/main/java/com/aws/greengrass/tes/TokenExchangeService.java‎

Lines changed: 38 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -65,70 +65,62 @@ public TokenExchangeService(Topics topics,
6565
AuthorizationHandler authZHandler, DeviceConfiguration deviceConfiguration) {
6666
super(topics);
6767
port = Coerce.toInt(config.lookup(CONFIGURATION_CONFIG_KEY, PORT_TOPIC).dflt(DEFAULT_PORT));
68-
config.subscribe((why, node) -> {
69-
if (node != null && node.childOf(PORT_TOPIC)) {
70-
logger.atDebug("tes-config-change").kv("node", node).kv("why", why).log();
71-
port = Coerce.toInt(node);
72-
Topic activePortTopic = config.lookup(CONFIGURATION_CONFIG_KEY, ACTIVE_PORT_TOPIC);
73-
if (port != Coerce.toInt(activePortTopic)) {
74-
logger.atInfo("tes-config-change").kv(PORT_TOPIC, port).kv("node", node).kv("why", why)
75-
.log("Restarting TES server due to port config change");
76-
requestRestart();
77-
}
78-
}
79-
});
68+
8069
deviceConfiguration.getIotRoleAlias().subscribe((why, newv) -> {
8170
iotRoleAlias = Coerce.toString(newv);
8271
});
8372

8473
this.authZHandler = authZHandler;
8574
this.credentialRequestHandler = credentialRequestHandler;
8675

87-
cloud4xxErrorCache = validateCacheConfig(Coerce.toInt(config.lookup(
88-
CONFIGURATION_CONFIG_KEY, CLOUD_4XX_ERROR_CACHE_TOPIC).dflt(
89-
CredentialRequestHandler.CLOUD_4XX_ERROR_CACHE_IN_SEC)),
90-
CredentialRequestHandler.CLOUD_4XX_ERROR_CACHE_IN_SEC);
91-
cloud5xxErrorCache = validateCacheConfig(Coerce.toInt(config.lookup(
92-
CONFIGURATION_CONFIG_KEY, CLOUD_5XX_ERROR_CACHE_TOPIC).dflt(
93-
CredentialRequestHandler.CLOUD_5XX_ERROR_CACHE_IN_SEC)),
94-
CredentialRequestHandler.CLOUD_5XX_ERROR_CACHE_IN_SEC);
95-
unknownErrorCache = validateCacheConfig(Coerce.toInt(config.lookup(
96-
CONFIGURATION_CONFIG_KEY, UNKNOWN_ERROR_CACHE_TOPIC).dflt(
97-
CredentialRequestHandler.UNKNOWN_ERROR_CACHE_IN_SEC)),
98-
CredentialRequestHandler.UNKNOWN_ERROR_CACHE_IN_SEC);
76+
cloud4xxErrorCache = validateCacheConfig(Coerce.toInt(config.findOrDefault(
77+
CredentialRequestHandler.CLOUD_4XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY,
78+
CLOUD_4XX_ERROR_CACHE_TOPIC)), CLOUD_4XX_ERROR_CACHE_TOPIC);
79+
cloud5xxErrorCache = validateCacheConfig(Coerce.toInt(config.findOrDefault(
80+
CredentialRequestHandler.CLOUD_5XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY,
81+
CLOUD_5XX_ERROR_CACHE_TOPIC)), CLOUD_5XX_ERROR_CACHE_TOPIC);
82+
unknownErrorCache = validateCacheConfig(Coerce.toInt(config.findOrDefault(
83+
CredentialRequestHandler.UNKNOWN_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY,
84+
UNKNOWN_ERROR_CACHE_TOPIC)), UNKNOWN_ERROR_CACHE_TOPIC);
9985

10086
credentialRequestHandler.configureCacheSettings(cloud4xxErrorCache, cloud5xxErrorCache, unknownErrorCache);
10187

10288
// Subscribe to cache configuration changes
10389
config.subscribe((why, node) -> {
104-
if (node != null && (node.childOf(CLOUD_4XX_ERROR_CACHE_TOPIC)
90+
if (node != null && (node.childOf(PORT_TOPIC)
91+
|| node.childOf(CLOUD_4XX_ERROR_CACHE_TOPIC)
10592
|| node.childOf(CLOUD_5XX_ERROR_CACHE_TOPIC)
10693
|| node.childOf(UNKNOWN_ERROR_CACHE_TOPIC))) {
107-
logger.atDebug("tes-cache-config-change").kv("node", node).kv("why", why).log();
108-
109-
int newCloud4xxErrorCache = validateCacheConfig(Coerce.toInt(config.lookup(
110-
CONFIGURATION_CONFIG_KEY, CLOUD_4XX_ERROR_CACHE_TOPIC).dflt(
111-
CredentialRequestHandler.CLOUD_4XX_ERROR_CACHE_IN_SEC)), cloud4xxErrorCache);
112-
int newCloud5xxErrorCache = validateCacheConfig(Coerce.toInt(config.lookup(
113-
CONFIGURATION_CONFIG_KEY, CLOUD_5XX_ERROR_CACHE_TOPIC).dflt(
114-
CredentialRequestHandler.CLOUD_5XX_ERROR_CACHE_IN_SEC)), cloud5xxErrorCache);
115-
int newUnknownErrorCache = validateCacheConfig(Coerce.toInt(config.lookup(
116-
CONFIGURATION_CONFIG_KEY, UNKNOWN_ERROR_CACHE_TOPIC).dflt(
117-
CredentialRequestHandler.UNKNOWN_ERROR_CACHE_IN_SEC)), unknownErrorCache);
118-
119-
if (cloud4xxErrorCache != newCloud4xxErrorCache
94+
logger.atDebug("tes-config-change").kv("node", node).kv("why", why).log();
95+
96+
port = Coerce.toInt(node);
97+
Topic activePortTopic = config.lookup(CONFIGURATION_CONFIG_KEY, ACTIVE_PORT_TOPIC);
98+
99+
int newCloud4xxErrorCache = validateCacheConfig(Coerce.toInt(config.findOrDefault(
100+
CredentialRequestHandler.CLOUD_4XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY,
101+
CLOUD_4XX_ERROR_CACHE_TOPIC)), CLOUD_4XX_ERROR_CACHE_TOPIC);
102+
int newCloud5xxErrorCache = validateCacheConfig(Coerce.toInt(config.findOrDefault(
103+
CredentialRequestHandler.CLOUD_5XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY,
104+
CLOUD_5XX_ERROR_CACHE_TOPIC)), CLOUD_5XX_ERROR_CACHE_TOPIC);
105+
int newUnknownErrorCache = validateCacheConfig(Coerce.toInt(config.findOrDefault(
106+
CredentialRequestHandler.UNKNOWN_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY,
107+
UNKNOWN_ERROR_CACHE_TOPIC)), UNKNOWN_ERROR_CACHE_TOPIC);
108+
109+
if (port != Coerce.toInt(activePortTopic)
110+
|| cloud4xxErrorCache != newCloud4xxErrorCache
120111
|| cloud5xxErrorCache != newCloud5xxErrorCache
121112
|| unknownErrorCache != newUnknownErrorCache) {
113+
122114
cloud4xxErrorCache = newCloud4xxErrorCache;
123115
cloud5xxErrorCache = newCloud5xxErrorCache;
124116
unknownErrorCache = newUnknownErrorCache;
117+
125118
credentialRequestHandler.configureCacheSettings(
126119
newCloud4xxErrorCache, newCloud5xxErrorCache, newUnknownErrorCache);
127120

128-
logger.atInfo("tes-cache-config-change").kv("unknownErrorCache", newUnknownErrorCache)
129-
.kv("cloud4xxErrorCache", newCloud4xxErrorCache)
130-
.kv("cloud5xxErrorCache", newCloud5xxErrorCache)
131-
.log("Restarting TES server due to cache config change");
121+
logger.atInfo("tes-config-change")
122+
.kv("node", node).kv("why", why)
123+
.log("Restarting TES server due to config change");
132124
requestRestart();
133125
}
134126
}
@@ -188,10 +180,11 @@ private void validateConfig() {
188180
}
189181
}
190182

191-
private int validateCacheConfig(int newCacheValue, int oldCacheValue) {
183+
private int validateCacheConfig(int newCacheValue, String topic) {
192184
if (newCacheValue < MINIMUM_ERROR_CACHE_IN_SEC) {
193-
logger.atError().log("Error cache value must be at least {}", MINIMUM_ERROR_CACHE_IN_SEC);
194-
return oldCacheValue;
185+
logger.atError().log("Error cache value must be at least {} seconds, setting {} to minimum value {}",
186+
MINIMUM_ERROR_CACHE_IN_SEC, topic, MINIMUM_ERROR_CACHE_IN_SEC);
187+
return MINIMUM_ERROR_CACHE_IN_SEC;
195188
}
196189
return newCacheValue;
197190
}

‎src/test/java/com/aws/greengrass/tes/TokenExchangeServiceTest.java‎

Lines changed: 18 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -162,24 +162,14 @@ void GIVEN_token_exchange_service_WHEN_started_THEN_correct_env_set(int port) th
162162
return null;
163163
});
164164

165-
Topic cloud4xxCacheTopic = mock(Topic.class);
166-
when(cloud4xxCacheTopic.dflt(CLOUD_4XX_ERROR_CACHE_IN_SEC))
167-
.thenReturn(cloud4xxCacheTopic);
165+
when(config.findOrDefault(CLOUD_4XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, CLOUD_4XX_ERROR_CACHE_TOPIC))
166+
.thenReturn(CLOUD_4XX_ERROR_CACHE_IN_SEC);
168167

169-
Topic cloud5xxCacheTopic = mock(Topic.class);
170-
when(cloud5xxCacheTopic.dflt(CLOUD_5XX_ERROR_CACHE_IN_SEC))
171-
.thenReturn(cloud5xxCacheTopic);
168+
when(config.findOrDefault(CLOUD_5XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, CLOUD_5XX_ERROR_CACHE_TOPIC))
169+
.thenReturn(CLOUD_5XX_ERROR_CACHE_IN_SEC);
172170

173-
Topic unknownCacheTopic = mock(Topic.class);
174-
when(unknownCacheTopic.dflt(UNKNOWN_ERROR_CACHE_IN_SEC))
175-
.thenReturn(unknownCacheTopic);
176-
177-
when(config.lookup(CONFIGURATION_CONFIG_KEY, CLOUD_4XX_ERROR_CACHE_TOPIC))
178-
.thenReturn(cloud4xxCacheTopic);
179-
when(config.lookup(CONFIGURATION_CONFIG_KEY, CLOUD_5XX_ERROR_CACHE_TOPIC))
180-
.thenReturn(cloud5xxCacheTopic);
181-
when(config.lookup(CONFIGURATION_CONFIG_KEY, UNKNOWN_ERROR_CACHE_TOPIC))
182-
.thenReturn(unknownCacheTopic);
171+
when(config.findOrDefault(UNKNOWN_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, UNKNOWN_ERROR_CACHE_TOPIC))
172+
.thenReturn(UNKNOWN_ERROR_CACHE_IN_SEC);
183173

184174
TokenExchangeService tes = new TokenExchangeService(config,
185175
mockCredentialHandler,
@@ -230,24 +220,14 @@ void GIVEN_token_exchange_service_WHEN_started_with_empty_role_alias_THEN_server
230220
when(configuration.lookup(SERVICES_NAMESPACE_TOPIC, DEFAULT_NUCLEUS_COMPONENT_NAME, CONFIGURATION_CONFIG_KEY,
231221
IOT_ROLE_ALIAS_TOPIC)).thenReturn(roleTopic);
232222

233-
Topic cloud4xxCacheTopic = mock(Topic.class);
234-
when(cloud4xxCacheTopic.dflt(CLOUD_4XX_ERROR_CACHE_IN_SEC))
235-
.thenReturn(cloud4xxCacheTopic);
236-
237-
Topic cloud5xxCacheTopic = mock(Topic.class);
238-
when(cloud5xxCacheTopic.dflt(CLOUD_5XX_ERROR_CACHE_IN_SEC))
239-
.thenReturn(cloud5xxCacheTopic);
223+
when(config.findOrDefault(CLOUD_4XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, CLOUD_4XX_ERROR_CACHE_TOPIC))
224+
.thenReturn(CLOUD_4XX_ERROR_CACHE_IN_SEC);
240225

241-
Topic unknownCacheTopic = mock(Topic.class);
242-
when(unknownCacheTopic.dflt(UNKNOWN_ERROR_CACHE_IN_SEC))
243-
.thenReturn(unknownCacheTopic);
226+
when(config.findOrDefault(CLOUD_5XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, CLOUD_5XX_ERROR_CACHE_TOPIC))
227+
.thenReturn(CLOUD_5XX_ERROR_CACHE_IN_SEC);
244228

245-
when(config.lookup(CONFIGURATION_CONFIG_KEY, CLOUD_4XX_ERROR_CACHE_TOPIC))
246-
.thenReturn(cloud4xxCacheTopic);
247-
when(config.lookup(CONFIGURATION_CONFIG_KEY, CLOUD_5XX_ERROR_CACHE_TOPIC))
248-
.thenReturn(cloud5xxCacheTopic);
249-
when(config.lookup(CONFIGURATION_CONFIG_KEY, UNKNOWN_ERROR_CACHE_TOPIC))
250-
.thenReturn(unknownCacheTopic);
229+
when(config.findOrDefault(UNKNOWN_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, UNKNOWN_ERROR_CACHE_TOPIC))
230+
.thenReturn(UNKNOWN_ERROR_CACHE_IN_SEC);
251231

252232
TokenExchangeService tes = spy(new TokenExchangeService(config,
253233
mockCredentialHandler,
@@ -280,24 +260,14 @@ void GIVEN_token_exchange_service_WHEN_auth_errors_THEN_server_errors_out(Extens
280260
when(configuration.lookup(SERVICES_NAMESPACE_TOPIC, DEFAULT_NUCLEUS_COMPONENT_NAME, CONFIGURATION_CONFIG_KEY,
281261
IOT_ROLE_ALIAS_TOPIC)).thenReturn(roleTopic);
282262

283-
Topic cloud4xxCacheTopic = mock(Topic.class);
284-
when(cloud4xxCacheTopic.dflt(CLOUD_4XX_ERROR_CACHE_IN_SEC))
285-
.thenReturn(cloud4xxCacheTopic);
286-
287-
Topic cloud5xxCacheTopic = mock(Topic.class);
288-
when(cloud5xxCacheTopic.dflt(CLOUD_5XX_ERROR_CACHE_IN_SEC))
289-
.thenReturn(cloud5xxCacheTopic);
263+
when(config.findOrDefault(CLOUD_4XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, CLOUD_4XX_ERROR_CACHE_TOPIC))
264+
.thenReturn(CLOUD_4XX_ERROR_CACHE_IN_SEC);
290265

291-
Topic unknownCacheTopic = mock(Topic.class);
292-
when(unknownCacheTopic.dflt(UNKNOWN_ERROR_CACHE_IN_SEC))
293-
.thenReturn(unknownCacheTopic);
266+
when(config.findOrDefault(CLOUD_5XX_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, CLOUD_5XX_ERROR_CACHE_TOPIC))
267+
.thenReturn(CLOUD_5XX_ERROR_CACHE_IN_SEC);
294268

295-
when(config.lookup(CONFIGURATION_CONFIG_KEY, CLOUD_4XX_ERROR_CACHE_TOPIC))
296-
.thenReturn(cloud4xxCacheTopic);
297-
when(config.lookup(CONFIGURATION_CONFIG_KEY, CLOUD_5XX_ERROR_CACHE_TOPIC))
298-
.thenReturn(cloud5xxCacheTopic);
299-
when(config.lookup(CONFIGURATION_CONFIG_KEY, UNKNOWN_ERROR_CACHE_TOPIC))
300-
.thenReturn(unknownCacheTopic);
269+
when(config.findOrDefault(UNKNOWN_ERROR_CACHE_IN_SEC, CONFIGURATION_CONFIG_KEY, UNKNOWN_ERROR_CACHE_TOPIC))
270+
.thenReturn(UNKNOWN_ERROR_CACHE_IN_SEC);
301271

302272
TokenExchangeService tes = spy(new TokenExchangeService(config,
303273
mockCredentialHandler,

0 commit comments

Comments
 (0)