Skip to content

Commit 2d15731

Browse files
alai8alai8
committed
implement review suggestions
1 parent 00932c1 commit 2d15731

File tree

7 files changed

+115
-169
lines changed

7 files changed

+115
-169
lines changed

‎operator/src/main/java/oracle/kubernetes/operator/DomainProcessorImpl.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -317,9 +317,9 @@ public void run() {
317317
@Override
318318
public void onCompletion(Packet packet) {
319319
if (Boolean.TRUE.equals(packet.get(ProcessingConstants.SERVER_HEALTH_READ))) {
320-
loggingFilter.setFilteringOff(true);
320+
loggingFilter.setFiltering(false).resetLogHistory();
321321
} else {
322-
loggingFilter.setFilteringOn();
322+
loggingFilter.setFiltering(true);
323323
}
324324
Boolean isStatusUnchanged =
325325
(Boolean) packet.get(ProcessingConstants.STATUS_UNCHANGED);
@@ -353,7 +353,7 @@ public void onCompletion(Packet packet) {
353353
@Override
354354
public void onThrowable(Packet packet, Throwable throwable) {
355355
LOGGER.severe(MessageKeys.EXCEPTION, throwable);
356-
loggingFilter.setFilteringOn();
356+
loggingFilter.setFiltering(true);
357357
// retry to trying after shorter delay because of exception
358358
unchangedCount.set(0);
359359
registerStatusUpdater(

‎operator/src/main/java/oracle/kubernetes/operator/http/HttpClient.java

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -198,17 +198,21 @@ public NextAction apply(Packet packet) {
198198
password = secretData.get(SecretHelper.ADMIN_SERVER_CREDENTIALS_PASSWORD);
199199
packet.put(KEY, createAuthenticatedClient(username, password));
200200

201-
if (username != null) {
202-
Arrays.fill(username, (byte) 0);
203-
}
204-
if (password != null) {
205-
Arrays.fill(password, (byte) 0);
206-
}
201+
clearCredential(username);
202+
clearCredential(password);
207203
}
208204
return doNext(packet);
209205
}
210206
}
211207

208+
/**
209+
* Erase authentication credential so that it is not sitting in memory where a rogue program can
210+
* find it.
211+
*/
212+
private static void clearCredential(byte[] credential) {
213+
if (credential != null) Arrays.fill(credential, (byte) 0);
214+
}
215+
212216
/**
213217
* Create authenticated client specifically targeted at an admin server.
214218
*

‎operator/src/main/java/oracle/kubernetes/operator/logging/LoggingFilter.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,6 @@ public interface LoggingFilter {
2727
* true if loggingFilter is null
2828
*/
2929
static boolean canLog(LoggingFilter loggingFilter, String msg) {
30-
if (loggingFilter == null) {
31-
return true;
32-
}
33-
return loggingFilter.canLog(msg);
30+
return loggingFilter == null || loggingFilter.canLog(msg);
3431
}
3532
}

‎operator/src/main/java/oracle/kubernetes/operator/logging/OncePerMessageLoggingFilter.java

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,42 +4,34 @@
44

55
package oracle.kubernetes.operator.logging;
66

7-
import java.util.ArrayList;
8-
import java.util.List;
7+
import java.util.HashSet;
8+
import java.util.Set;
99

1010
/** A LoggingFilter that logs each log message, which are typically message keys, at most once */
1111
public class OncePerMessageLoggingFilter implements LoggingFilter {
1212

1313
// allow all messages to be logged when filtering is off
1414
volatile boolean filtering = false;
1515

16-
List messagesLogged = new ArrayList();
16+
Set messagesLogged = new HashSet();
1717

1818
/**
19-
* Turn on filtering of log messages and skip logging of messages that have already been logged
20-
*/
21-
public void setFilteringOn() {
22-
filtering = true;
23-
}
24-
25-
/**
26-
* Turn off filtering of log messages and allow all log messages to be logged
19+
* Turn on or off the filtering of log messages and skip logging of messages that have already
20+
* been logged
2721
*
28-
* @param resetLogHistory Whether to also clear the history of messages logged so those messages
29-
* will be logged again next time filtering is turned on
22+
* @param value true if filtering should be on, false if filtering should be off
3023
*/
31-
public void setFilteringOff(boolean resetLogHistory) {
32-
filtering = false;
33-
if (resetLogHistory) {
34-
resetLogHistory();
35-
}
24+
public OncePerMessageLoggingFilter setFiltering(boolean value) {
25+
filtering = value;
26+
return this;
3627
}
3728

3829
/** Clears the list of history of messages logged and turn off filtering */
39-
public void resetLogHistory() {
30+
public OncePerMessageLoggingFilter resetLogHistory() {
4031
synchronized (messagesLogged) {
4132
messagesLogged.clear();
4233
}
34+
return this;
4335
}
4436

4537
@Override

‎operator/src/main/java/oracle/kubernetes/operator/steps/ReadHealthStep.java

Lines changed: 69 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import com.fasterxml.jackson.databind.ObjectMapper;
99
import io.kubernetes.client.models.V1ObjectMeta;
1010
import io.kubernetes.client.models.V1Service;
11+
import java.io.IOException;
1112
import java.util.ArrayList;
1213
import java.util.Iterator;
1314
import java.util.List;
@@ -98,67 +99,28 @@ public NextAction apply(Packet packet) {
9899
(LoggingFilter) packet.get(LoggingFilter.LOGGING_FILTER_PACKET_KEY),
99100
MessageKeys.WLS_HEALTH_READ_FAILED_NO_HTTPCLIENT,
100101
packet.get(ProcessingConstants.SERVER_NAME));
101-
return doNext(packet);
102-
}
103-
104-
String serviceURL = HttpClient.getServiceURL(service);
105-
if (serviceURL != null) {
106-
String jsonResult =
107-
httpClient
108-
.executePostUrlOnServiceClusterIP(
109-
getRetrieveHealthSearchUrl(),
110-
serviceURL,
111-
getRetrieveHealthSearchPayload(),
112-
true)
113-
.getResponse();
114-
115-
ObjectMapper mapper = new ObjectMapper();
116-
JsonNode root = mapper.readTree(jsonResult);
117-
118-
JsonNode state = null;
119-
JsonNode subsystemName = null;
120-
JsonNode symptoms = null;
121-
JsonNode overallHealthState = root.path("overallHealthState");
122-
if (overallHealthState != null) {
123-
state = overallHealthState.path("state");
124-
subsystemName = overallHealthState.path("subsystemName");
125-
symptoms = overallHealthState.path("symptoms");
126-
}
127-
JsonNode activationTime = root.path("activationTime");
128-
129-
List<String> sym = new ArrayList<>();
130-
if (symptoms != null) {
131-
Iterator<JsonNode> it = symptoms.elements();
132-
while (it.hasNext()) {
133-
sym.add(it.next().asText());
134-
}
102+
} else {
103+
104+
String serviceURL = HttpClient.getServiceURL(service);
105+
if (serviceURL != null) {
106+
String jsonResult =
107+
httpClient
108+
.executePostUrlOnServiceClusterIP(
109+
getRetrieveHealthSearchUrl(),
110+
serviceURL,
111+
getRetrieveHealthSearchPayload(),
112+
true)
113+
.getResponse();
114+
115+
ServerHealth health = parseServerHealthJson(jsonResult);
116+
117+
@SuppressWarnings("unchecked")
118+
ConcurrentMap<String, ServerHealth> serverHealthMap =
119+
(ConcurrentMap<String, ServerHealth>)
120+
packet.get(ProcessingConstants.SERVER_HEALTH_MAP);
121+
serverHealthMap.put((String) packet.get(ProcessingConstants.SERVER_NAME), health);
122+
packet.put(ProcessingConstants.SERVER_HEALTH_READ, Boolean.TRUE);
135123
}
136-
137-
String subName = null;
138-
if (subsystemName != null) {
139-
String s = subsystemName.asText();
140-
if (s != null && !"null".equals(s)) {
141-
subName = s;
142-
}
143-
}
144-
145-
ServerHealth health =
146-
new ServerHealth()
147-
.withOverallHealth(state != null ? state.asText() : null)
148-
.withActivationTime(
149-
activationTime != null ? new DateTime(activationTime.asLong()) : null);
150-
if (subName != null) {
151-
health
152-
.getSubsystems()
153-
.add(new SubsystemHealth().withSubsystemName(subName).withSymptoms(sym));
154-
}
155-
156-
@SuppressWarnings("unchecked")
157-
ConcurrentMap<String, ServerHealth> serverHealthMap =
158-
(ConcurrentMap<String, ServerHealth>)
159-
packet.get(ProcessingConstants.SERVER_HEALTH_MAP);
160-
serverHealthMap.put((String) packet.get(ProcessingConstants.SERVER_NAME), health);
161-
packet.put(ProcessingConstants.SERVER_HEALTH_READ, Boolean.TRUE);
162124
}
163125
return doNext(packet);
164126
} catch (Throwable t) {
@@ -171,5 +133,52 @@ public NextAction apply(Packet packet) {
171133
return doNext(packet);
172134
}
173135
}
136+
137+
private ServerHealth parseServerHealthJson(String jsonResult) throws IOException {
138+
if (jsonResult == null) return null;
139+
140+
ObjectMapper mapper = new ObjectMapper();
141+
JsonNode root = mapper.readTree(jsonResult);
142+
143+
JsonNode state = null;
144+
JsonNode subsystemName = null;
145+
JsonNode symptoms = null;
146+
JsonNode overallHealthState = root.path("overallHealthState");
147+
if (overallHealthState != null) {
148+
state = overallHealthState.path("state");
149+
subsystemName = overallHealthState.path("subsystemName");
150+
symptoms = overallHealthState.path("symptoms");
151+
}
152+
JsonNode activationTime = root.path("activationTime");
153+
154+
List<String> sym = new ArrayList<>();
155+
if (symptoms != null) {
156+
Iterator<JsonNode> it = symptoms.elements();
157+
while (it.hasNext()) {
158+
sym.add(it.next().asText());
159+
}
160+
}
161+
162+
String subName = null;
163+
if (subsystemName != null) {
164+
String s = subsystemName.asText();
165+
if (s != null && !"null".equals(s)) {
166+
subName = s;
167+
}
168+
}
169+
170+
ServerHealth health =
171+
new ServerHealth()
172+
.withOverallHealth(state != null ? state.asText() : null)
173+
.withActivationTime(
174+
activationTime != null ? new DateTime(activationTime.asLong()) : null);
175+
if (subName != null) {
176+
health
177+
.getSubsystems()
178+
.add(new SubsystemHealth().withSubsystemName(subName).withSymptoms(sym));
179+
}
180+
181+
return health;
182+
}
174183
}
175184
}

‎operator/src/test/java/oracle/kubernetes/operator/logging/LoggingFacadeTest.java

Lines changed: 10 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,22 @@
99

1010
import java.util.logging.Level;
1111
import java.util.logging.Logger;
12+
import org.junit.Before;
1213
import org.junit.Test;
1314

1415
public class LoggingFacadeTest {
1516

17+
MockLogger mockLogger;
18+
LoggingFacade loggingFacade;
19+
20+
@Before
21+
public void setup() {
22+
mockLogger = new MockLogger();
23+
loggingFacade = new LoggingFacade(mockLogger);
24+
}
25+
1626
@Test
1727
public void verifyInfoMessageLoggedIfLoggingFilterIsNull() {
18-
MockLogger mockLogger = new MockLogger();
19-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
20-
2128
loggingFacade.info((LoggingFilter) null, "msg");
2229

2330
assertThat(mockLogger.isLogpCalled(), is(true));
@@ -26,9 +33,6 @@ public void verifyInfoMessageLoggedIfLoggingFilterIsNull() {
2633
@Test
2734
public void verifyInfoMessageLoggedIfLoggingFilterAllows() {
2835
final String MESSAGE = "info message";
29-
MockLogger mockLogger = new MockLogger();
30-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
31-
3236
loggingFacade.info(MockLoggingFilter.createWithReturnValue(true), MESSAGE);
3337

3438
assertThat(mockLogger.isLogpCalled(), is(true));
@@ -38,19 +42,13 @@ public void verifyInfoMessageLoggedIfLoggingFilterAllows() {
3842

3943
@Test
4044
public void verifyInfoMessageNotLoggedIfLoggingFilterDenies() {
41-
MockLogger mockLogger = new MockLogger();
42-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
43-
4445
loggingFacade.info(MockLoggingFilter.createWithReturnValue(false), "msg");
4546

4647
assertThat(mockLogger.isLogpCalled(), is(false));
4748
}
4849

4950
@Test
5051
public void verifyWarningMessageLoggedIfLoggingFilterIsNull() {
51-
MockLogger mockLogger = new MockLogger();
52-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
53-
5452
loggingFacade.warning((LoggingFilter) null, "msg");
5553

5654
assertThat(mockLogger.isLogpCalled(), is(true));
@@ -59,9 +57,6 @@ public void verifyWarningMessageLoggedIfLoggingFilterIsNull() {
5957
@Test
6058
public void verifyWarningMessageLoggedIfLoggingFilterAllows() {
6159
final String MESSAGE = "warning message";
62-
MockLogger mockLogger = new MockLogger();
63-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
64-
6560
loggingFacade.warning(MockLoggingFilter.createWithReturnValue(true), MESSAGE);
6661

6762
assertThat(mockLogger.isLogpCalled(), is(true));
@@ -71,19 +66,13 @@ public void verifyWarningMessageLoggedIfLoggingFilterAllows() {
7166

7267
@Test
7368
public void verifyWarningMessageNotLoggedIfLoggingFilterDenies() {
74-
MockLogger mockLogger = new MockLogger();
75-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
76-
7769
loggingFacade.warning(MockLoggingFilter.createWithReturnValue(false), "msg");
7870

7971
assertThat(mockLogger.isLogpCalled(), is(false));
8072
}
8173

8274
@Test
8375
public void verifySevereMessageLoggedIfLoggingFilterIsNull() {
84-
MockLogger mockLogger = new MockLogger();
85-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
86-
8776
loggingFacade.severe((LoggingFilter) null, "msg");
8877

8978
assertThat(mockLogger.isLogpCalled(), is(true));
@@ -92,9 +81,6 @@ public void verifySevereMessageLoggedIfLoggingFilterIsNull() {
9281
@Test
9382
public void verifySevereMessageLoggedIfLoggingFilterAllows() {
9483
final String MESSAGE = "severe message";
95-
MockLogger mockLogger = new MockLogger();
96-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
97-
9884
loggingFacade.severe(MockLoggingFilter.createWithReturnValue(true), MESSAGE);
9985

10086
assertThat(mockLogger.isLogpCalled(), is(true));
@@ -104,19 +90,13 @@ public void verifySevereMessageLoggedIfLoggingFilterAllows() {
10490

10591
@Test
10692
public void verifySevereMessageNotLoggedIfLoggingFilterDenies() {
107-
MockLogger mockLogger = new MockLogger();
108-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
109-
11093
loggingFacade.severe(MockLoggingFilter.createWithReturnValue(false), "msg");
11194

11295
assertThat(mockLogger.isLogpCalled(), is(false));
11396
}
11497

11598
@Test
11699
public void verifySevereMessageWithThrowableLoggedIfLoggingFilterIsNull() {
117-
MockLogger mockLogger = new MockLogger();
118-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
119-
120100
loggingFacade.severe((LoggingFilter) null, "msg", new Throwable());
121101

122102
assertThat(mockLogger.isLogpCalled(), is(true));
@@ -126,9 +106,6 @@ public void verifySevereMessageWithThrowableLoggedIfLoggingFilterIsNull() {
126106
public void verifySevereMessageWithThrowableLoggedIfLoggingFilterAllows() {
127107
final String MESSAGE = "severe message";
128108
final Throwable THROWABLE = new Throwable("throwable");
129-
MockLogger mockLogger = new MockLogger();
130-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
131-
132109
loggingFacade.severe(MockLoggingFilter.createWithReturnValue(true), MESSAGE, THROWABLE);
133110

134111
assertThat(mockLogger.isLogpCalled(), is(true));
@@ -139,9 +116,6 @@ public void verifySevereMessageWithThrowableLoggedIfLoggingFilterAllows() {
139116

140117
@Test
141118
public void verifySevereMessageWithThrowableNotLoggedIfLoggingFilterDenies() {
142-
MockLogger mockLogger = new MockLogger();
143-
LoggingFacade loggingFacade = new LoggingFacade(mockLogger);
144-
145119
loggingFacade.severe(MockLoggingFilter.createWithReturnValue(false), "msg", new Throwable());
146120

147121
assertThat(mockLogger.isLogpCalled(), is(false));

0 commit comments

Comments
 (0)