Skip to content

Commit

Permalink
Implements first code review modifications
Browse files Browse the repository at this point in the history
  • Loading branch information
@CDellaGiusta
CDellaGiusta committed Jan 23, 2025
1 parent b640370 commit 965eca6
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 57 deletions.
22 changes: 12 additions & 10 deletions java/code/src/com/redhat/rhn/taskomatic/TaskomaticApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -836,24 +836,26 @@ public void scheduleSingleRootCaCertUpdate(String fileName, String rootCaCertCon

/**
* Schedule multiple root ca certificates update.
* Filters out null certificates or the ones that have no content
*
* @param filenameToRootCaCertMap maps filename to root ca certificate actual content
* @throws TaskomaticApiException if there was an error
*/
public void scheduleSingleRootCaCertUpdate(Map<String, String> filenameToRootCaCertMap)
throws TaskomaticApiException {

//filters out meaningless certificates
Map<String, String> realFilenameToRootCaCertMap = filenameToRootCaCertMap.entrySet()
if ((null == filenameToRootCaCertMap) || filenameToRootCaCertMap.isEmpty()) {
return; // nothing to do: avoid invoke call, to spare a potential exception
}

//sanitise map keys and values: XmlRpc actual call does not like null strings
//(exception: Cannot invoke "Object.toString()" because "key" is null)
Map<String, String> sanitisedFilenameToRootCaCertMap = filenameToRootCaCertMap.entrySet()
.stream()
.filter(e -> StringUtils.isNotEmpty(e.getValue()))
.collect(Collectors.toMap(p -> p.getKey(), p -> p.getValue()));
.collect(Collectors.toMap(p -> Objects.toString(p.getKey(), ""),
p -> Objects.toString(p.getValue(), "")));

if (!realFilenameToRootCaCertMap.isEmpty()) {
Map<String, Object> paramList = new HashMap<>();
paramList.put("filename_to_root_ca_cert_map", realFilenameToRootCaCertMap);
invoke("tasko.scheduleSingleSatBunchRun", "root-ca-cert-update-bunch", paramList);
}
Map<String, Object> paramList = new HashMap<>();
paramList.put("filename_to_root_ca_cert_map", sanitisedFilenameToRootCaCertMap);
invoke("tasko.scheduleSingleSatBunchRun", "root-ca-cert-update-bunch", paramList);
}
}
56 changes: 46 additions & 10 deletions java/code/src/com/redhat/rhn/taskomatic/task/RootCaCertUpdateTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import java.io.FileWriter;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.HashMap;
import java.util.Map;

Expand Down Expand Up @@ -57,27 +59,61 @@ public void execute(JobExecutionContext context) throws JobExecutionException {
final JobDataMap jobDataMap = context.getJobDetail().getJobDataMap();
Map<String, String> filenameToRootCaCertMap = getFilenameToRootCaCertMap(jobDataMap);

saveAndUpdateCaCertificates(filenameToRootCaCertMap);
}

/**
* Saves multiple root ca certificates, then updates trusted directory
* This is a public entry point for any other taskomatic task
* that needs to deal with ca certificates in the local filesystem
*
* @param filenameToRootCaCertMap maps filename to root ca certificate actual content
* @throws JobExecutionException if there was an error
*/
public void saveAndUpdateCaCertificates(Map<String, String> filenameToRootCaCertMap) throws JobExecutionException {
if ((null == filenameToRootCaCertMap) || filenameToRootCaCertMap.isEmpty()) {
return; // nothing to do
}

for (Map.Entry<String, String> pair : filenameToRootCaCertMap.entrySet()) {
String fileName = pair.getKey();
String rootCaCertContent = pair.getValue();

try {
saveCertificate(fileName, rootCaCertContent);
log.info("CA certificate file: {} successfully written", fileName);
if (fileName.isEmpty()) {
continue;
}

if (rootCaCertContent.isEmpty()) {
try {
removeCertificate(fileName);
log.info("CA certificate file: {} successfully removed", fileName);
}
catch (IOException e) {
log.error("error when removing CA certificate file {}: {}", fileName, e);
}
}
catch (IOException e) {
log.error("error when writing CA certificate file {}: {}", fileName, e);
else {
try {
saveCertificate(fileName, rootCaCertContent);
log.info("CA certificate file: {} successfully written", fileName);
}
catch (IOException e) {
log.error("error when writing CA certificate file {}: {}", fileName, e);
}
}
}

if (!filenameToRootCaCertMap.isEmpty()) {
updateCaCertificates();
}
updateCaCertificates();
}

private void removeCertificate(String fileName) throws IOException {
String fullPathName = CertificateUtils.CERTS_PATH.resolve(fileName).toString();
Files.delete(Path.of(fullPathName));
}

private void saveCertificate(String fileName, String rootCaCertContent) throws IOException {
String fullFileName = CertificateUtils.CERTS_PATH.resolve(fileName).toString();
try (FileWriter fw = new FileWriter(fullFileName, false)) {
String fullPathName = CertificateUtils.CERTS_PATH.resolve(fileName).toString();
try (FileWriter fw = new FileWriter(fullPathName, false)) {
fw.write(rootCaCertContent);
}
}
Expand Down
13 changes: 3 additions & 10 deletions java/code/src/com/redhat/rhn/taskomatic/task/payg/PaygUpdateHostsTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@

import com.redhat.rhn.domain.cloudpayg.CloudRmtHost;
import com.redhat.rhn.domain.cloudpayg.CloudRmtHostFactory;
import com.redhat.rhn.taskomatic.TaskomaticApi;
import com.redhat.rhn.taskomatic.TaskomaticApiException;
import com.redhat.rhn.taskomatic.task.RhnJavaJob;
import com.redhat.rhn.taskomatic.task.RootCaCertUpdateTask;

import org.quartz.JobExecutionContext;
import org.quartz.JobExecutionException;
Expand Down Expand Up @@ -64,14 +63,8 @@ private void loadHttpsCertificates(List<CloudRmtHost> hostToUpdate) throws JobEx
filenameToRootCaCertMap.put(caFileName, host.getSslCert());
}

TaskomaticApi taskomaticApi = new TaskomaticApi();
try {
taskomaticApi.scheduleSingleRootCaCertUpdate(filenameToRootCaCertMap);
}
catch (TaskomaticApiException e) {
log.error(e.getMessage(), e);
throw new JobExecutionException(e);
}
RootCaCertUpdateTask a = new RootCaCertUpdateTask();
a.saveAndUpdateCaCertificates(filenameToRootCaCertMap);
}

private void updateHost(List<CloudRmtHost> hostToUpdate) {
Expand Down
24 changes: 5 additions & 19 deletions java/code/src/com/suse/manager/hub/HubManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,42 +55,29 @@ public class HubManager {

private final IssClientFactory clientFactory;

private TaskomaticApi taskomaticApi = new TaskomaticApi();
private TaskomaticApi taskomaticApi;

private static final String ROOT_CA_FILENAME_TEMPLATE = "%s_%s_root_ca.pem";

/**
* Default constructor
*/
public HubManager() {
this(new HubFactory(), new IssClientFactory(), new MirrorCredentialsManager());
this(new HubFactory(), new IssClientFactory(), new MirrorCredentialsManager(), new TaskomaticApi());
}

/**
* Builds an instance with the given dependencies
* @param hubFactoryIn the hub factory
* @param clientFactoryIn the ISS client factory
* @param mirrorCredentialsManagerIn the mirror credentials manager
* @param taskomaticApiIn the TaskomaticApi object
*/
public HubManager(HubFactory hubFactoryIn, IssClientFactory clientFactoryIn,
MirrorCredentialsManager mirrorCredentialsManagerIn) {
MirrorCredentialsManager mirrorCredentialsManagerIn, TaskomaticApi taskomaticApiIn) {
this.hubFactory = hubFactoryIn;
this.clientFactory = clientFactoryIn;
this.mirrorCredentialsManager = mirrorCredentialsManagerIn;
}

/**
* Builds an instance with the given dependencies
* used to inject TaskomaticApi object for testing purposes
*
* @param hubFactoryIn the hub factory
* @param clientFactoryIn the ISS client factory
* @param mirrorCredentialsManagerIn the mirror credentials manager
* @param taskomaticApiIn the TaskomaticApi object
*/
public HubManager(HubFactory hubFactoryIn, IssClientFactory clientFactoryIn,
MirrorCredentialsManager mirrorCredentialsManagerIn, TaskomaticApi taskomaticApiIn) {
this(hubFactoryIn, clientFactoryIn, mirrorCredentialsManagerIn);
this.taskomaticApi = taskomaticApiIn;
}

Expand Down Expand Up @@ -369,17 +356,16 @@ private String computeRootCaFileName(IssRole role, String serverFqdn) {
}

private IssServer createServer(IssRole role, String serverFqdn, String rootCA) throws TaskomaticApiException {
taskomaticApi.scheduleSingleRootCaCertUpdate(computeRootCaFileName(role, serverFqdn), rootCA);
return switch (role) {
case HUB -> {
IssHub hub = new IssHub(serverFqdn, rootCA);
hubFactory.save(hub);
taskomaticApi.scheduleSingleRootCaCertUpdate(computeRootCaFileName(role, serverFqdn), rootCA);
yield hub;
}
case PERIPHERAL -> {
IssPeripheral peripheral = new IssPeripheral(serverFqdn, rootCA);
hubFactory.save(peripheral);
taskomaticApi.scheduleSingleRootCaCertUpdate(computeRootCaFileName(role, serverFqdn), rootCA);
yield peripheral;
}
};
Expand Down
18 changes: 10 additions & 8 deletions java/code/src/com/suse/manager/hub/test/HubManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ public void resetTaskomaticCall() {
invokeCalled = false;
invokeName = "";
invokeBunch = "";
invokeRootCaFilename = "";
invokeRootCaContent = "";
invokeRootCaFilename = null;
invokeRootCaContent = null;
}

public void testTaskoRootCaCertUpdateCall(String expectedRootCaFilename,
Expand Down Expand Up @@ -142,8 +142,8 @@ protected Object invoke(String name, Object... args) throws TaskomaticApiExcepti
invokeRootCaContent = firstKeyVal.get().getValue();
}
else {
invokeRootCaFilename = "";
invokeRootCaContent = "";
invokeRootCaFilename = null;
invokeRootCaContent = null;
}
return null;
}
Expand Down Expand Up @@ -441,11 +441,11 @@ public void canSaveRootCa() throws TaskomaticApiException {

mockTaskomaticApi.resetTaskomaticCall();
hubManager.saveNewServer(getValidToken("dummy2.hub.fqdn"), IssRole.HUB, "");
mockTaskomaticApi.testTaskoNoRootCaCertUpdateCall();
mockTaskomaticApi.testTaskoRootCaCertUpdateCall("hub_dummy2.hub.fqdn_root_ca.pem", "");

mockTaskomaticApi.resetTaskomaticCall();
hubManager.saveNewServer(getValidToken("dummy3.hub.fqdn"), IssRole.HUB, null);
mockTaskomaticApi.testTaskoNoRootCaCertUpdateCall();
mockTaskomaticApi.testTaskoRootCaCertUpdateCall("hub_dummy3.hub.fqdn_root_ca.pem", "");

mockTaskomaticApi.resetTaskomaticCall();
hubManager.saveNewServer(getValidToken("dummy.periph.fqdn"), IssRole.PERIPHERAL,
Expand All @@ -455,11 +455,13 @@ public void canSaveRootCa() throws TaskomaticApiException {

mockTaskomaticApi.resetTaskomaticCall();
hubManager.saveNewServer(getValidToken("dummy2.periph.fqdn"), IssRole.PERIPHERAL, "");
mockTaskomaticApi.testTaskoNoRootCaCertUpdateCall();
mockTaskomaticApi.testTaskoRootCaCertUpdateCall("peripheral_dummy2.periph.fqdn_root_ca.pem",
"");

mockTaskomaticApi.resetTaskomaticCall();
hubManager.saveNewServer(getValidToken("dummy3.periph.fqdn"), IssRole.PERIPHERAL, null);
mockTaskomaticApi.testTaskoNoRootCaCertUpdateCall();
mockTaskomaticApi.testTaskoRootCaCertUpdateCall("peripheral_dummy3.periph.fqdn_root_ca.pem",
"");
}

@Test
Expand Down

0 comments on commit 965eca6

Please sign in to comment.