Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,10 @@ public class SupportedParameters {
public static final Set<String> DEPENDENCY_PARAMETERS = Set.of(BINDING_NAME, ENV_VAR_NAME, VISIBILITY, USE_LIVE_ROUTES,
SERVICE_BINDING_CONFIG, DELETE_SERVICE_KEY_AFTER_DEPLOYMENT);

public static final Set<String> MODULE_HOOK_PARAMETERS = Set.of(NAME, COMMAND, MEMORY, DISK_QUOTA, HOOK_REQUIRES);
public static final String HOOK_TARGET_APP = "hook-target-app";
public static final String PHASES_CONFIG = "phases-config";

public static final Set<String> MODULE_HOOK_PARAMETERS = Set.of(NAME, COMMAND, MEMORY, DISK_QUOTA, HOOK_REQUIRES, HOOK_TARGET_APP, PHASES_CONFIG);

public static final Set<String> CONFIGURATION_REFERENCE_PARAMETERS = Set.of(PROVIDER_NID, PROVIDER_ID, TARGET, VERSION,
DEPRECATED_CONFIG_MTA_ID, DEPRECATED_CONFIG_MTA_VERSION,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ public class Messages {
public static final String ERROR_PREPARING_TO_EXECUTE_TASKS_ON_APP = "Error preparing to execute tasks on application \"{0}\"";
public static final String ERROR_PREPARING_TO_RESTART_SERVICE_BROKER_SUBSCRIBERS = "Error preparing to restart service broker subscribers";
public static final String ERROR_EXECUTING_TASK_0_ON_APP_1 = "Execution of task \"{0}\" on application \"{1}\" failed.";
public static final String DUPLICATE_PHASE_IN_PHASES_CONFIG = "Duplicate phase \"{0}\" in \"phases-config\" of hook \"{1}\". Only one target-app per phase is supported.";
public static final String ERROR_DETECTING_COMPONENTS_TO_UNDEPLOY = "Error detecting components to undeploy";
public static final String ERROR_DELETING_IDLE_ROUTES = "Error deleting idle routes";
public static final String ERROR_CREATING_SERVICE_BROKERS = "Error creating service brokers";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
package org.cloudfoundry.multiapps.controller.process.steps;

import java.text.MessageFormat;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

import jakarta.inject.Inject;
import jakarta.inject.Named;
import org.cloudfoundry.multiapps.common.SLException;
import org.cloudfoundry.multiapps.controller.core.cf.CloudHandlerFactory;
import org.cloudfoundry.multiapps.controller.core.model.SupportedParameters;
import org.cloudfoundry.multiapps.controller.core.helpers.MtaDescriptorMerger;
import org.cloudfoundry.multiapps.controller.persistence.dto.BackupDescriptor;
import org.cloudfoundry.multiapps.controller.persistence.dto.ImmutableBackupDescriptor;
Expand All @@ -20,6 +23,8 @@
import org.cloudfoundry.multiapps.controller.process.variables.Variables;
import org.cloudfoundry.multiapps.mta.model.DeploymentDescriptor;
import org.cloudfoundry.multiapps.mta.model.ExtensionDescriptor;
import org.cloudfoundry.multiapps.mta.model.Hook;
import org.cloudfoundry.multiapps.mta.model.Module;
import org.cloudfoundry.multiapps.mta.model.Platform;
import org.cloudfoundry.multiapps.mta.resolvers.ReferenceContainer;
import org.cloudfoundry.multiapps.mta.resolvers.ReferencesFinder;
Expand All @@ -30,6 +35,9 @@
@Scope(BeanDefinition.SCOPE_PROTOTYPE)
public class MergeDescriptorsStep extends SyncFlowableStep {

private static final int HOOKS_MIN_SCHEMA_VERSION = 3;
private static final String PHASES_CONFIG_PHASE_KEY = "phase";

@Inject
private DescriptorBackupService descriptorBackupService;

Expand Down Expand Up @@ -59,13 +67,40 @@ protected StepPhase executeStep(ProcessContext context) {
.toList());
context.setVariable(Variables.DEPLOYMENT_DESCRIPTOR, descriptor);

validatePhasesConfig(context, descriptor);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extract this validation in a new class

warnForUnsupportedParameters(descriptor);

backupDeploymentDescriptor(context, descriptor);
getStepLogger().debug(Messages.DESCRIPTORS_MERGED);
return StepPhase.DONE;
}

private void validatePhasesConfig(ProcessContext context, DeploymentDescriptor descriptor) {
if (context.getVariable(Variables.MTA_MAJOR_SCHEMA_VERSION) < HOOKS_MIN_SCHEMA_VERSION) {
return;
}
descriptor.getModules()
.stream()
.flatMap(module -> module.getHooks().stream())
.forEach(this::validateHookHasNoDuplicatePhaseConfigs);
}

@SuppressWarnings("unchecked")
private void validateHookHasNoDuplicatePhaseConfigs(Hook hook) {
Object phasesConfigValue = hook.getParameters().get(SupportedParameters.PHASES_CONFIG);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Null handling?

if (!(phasesConfigValue instanceof List)) {
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't the deployment fail if phases-config is not a List?

}
List<Map<String, String>> phasesConfig = (List<Map<String, String>>) phasesConfigValue;
Set<String> seenPhases = new HashSet<>();
for (Map<String, String> phaseConfig : phasesConfig) {
String phase = phaseConfig.get(PHASES_CONFIG_PHASE_KEY);
if (phase != null && !seenPhases.add(phase)) {
throw new SLException(MessageFormat.format(Messages.DUPLICATE_PHASE_IN_PHASES_CONFIG, phase, hook.getName()));
}
}
}

private void warnForUnsupportedParameters(DeploymentDescriptor descriptor) {
List<ReferenceContainer> references = new ReferencesFinder().getAllReferences(descriptor);
Map<String, List<String>> unsupportedParameters = unsupportedParameterFinder.findUnsupportedParameters(descriptor,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
package org.cloudfoundry.multiapps.controller.process.steps;

import java.text.MessageFormat;
import java.util.List;
import java.util.Map;

import jakarta.inject.Named;
import org.cloudfoundry.multiapps.controller.client.lib.domain.CloudApplicationExtended;
import org.cloudfoundry.multiapps.controller.core.model.ApplicationColor;
import org.cloudfoundry.multiapps.controller.core.model.BlueGreenApplicationNameSuffix;
import org.cloudfoundry.multiapps.controller.core.model.HookPhaseProcessType;
import org.cloudfoundry.multiapps.controller.core.model.SupportedParameters;
import org.cloudfoundry.multiapps.controller.process.Messages;
import org.cloudfoundry.multiapps.controller.process.variables.Variables;
import org.cloudfoundry.multiapps.mta.model.Hook;
import org.springframework.beans.factory.config.BeanDefinition;
import org.springframework.context.annotation.Scope;

@Named("resolveHookTaskTargetAppStep")
@Scope(BeanDefinition.SCOPE_PROTOTYPE)
public class ResolveHookTaskTargetAppStep extends SyncFlowableStep {

private static final String PHASE_KEY = "phase";
private static final String TARGET_APP_KEY = "target-app";
private static final String APPLICATION_PHASE_SUBSTRING = ".application.";

@Override
protected StepPhase executeStep(ProcessContext context) {
CloudApplicationExtended app = context.getVariable(Variables.APP_TO_PROCESS);
Hook hook = context.getVariable(Variables.HOOK_FOR_EXECUTION);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of passing it as argument, just get it in resolveTargetAppName


String resolvedAppName = resolveTargetAppName(context, app, hook);
if (!resolvedAppName.equals(app.getName())) {
context.setVariable(Variables.APP_TO_PROCESS, buildAppWithName(app, resolvedAppName));
}

return StepPhase.DONE;
}

private String resolveTargetAppName(ProcessContext context, CloudApplicationExtended app, Hook hook) {
if (hook == null) {
return app.getName();
}

List<Map<String, String>> phasesConfig = getPhasesConfig(hook);
if (phasesConfig.isEmpty()) {
return app.getName();
}

String currentPhase = buildCurrentPhaseString(context, hook);
String targetApp = phasesConfig.stream()
.filter(config -> currentPhase.equals(config.get(PHASE_KEY)))
.map(config -> config.get(TARGET_APP_KEY))
.findFirst()
.orElse(null);

if (targetApp == null) {
return app.getName();
}

return resolveAppNameForTarget(context, app, targetApp);
}

@SuppressWarnings("unchecked")
private List<Map<String, String>> getPhasesConfig(Hook hook) {
Object phasesConfigValue = hook.getParameters()
.get(SupportedParameters.PHASES_CONFIG);
if (phasesConfigValue instanceof List) {
return (List<Map<String, String>>) phasesConfigValue;
}
return List.of();
}

private String buildCurrentPhaseString(ProcessContext context, Hook hook) {
String hookExecutionPhase = context.getVariable(Variables.HOOK_EXECUTION_PHASE);
return hook.getPhases()
.stream()
.filter(p -> p.equals(hookExecutionPhase))
.findFirst()
.orElseGet(() -> hook.getPhases()
.stream()
.filter(p -> p.contains(APPLICATION_PHASE_SUBSTRING))
.findFirst()
.orElse(""));
Comment on lines +80 to +84
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we searching for the first application hook?

}

private String resolveAppNameForTarget(ProcessContext context, CloudApplicationExtended app, String targetApp) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What will happen if we using hook:
blue-green.application.before-start.idle
with target-app: live
During initial deployment when live app still does not exist?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With the current implementation we hit 404. What’s the right approach here- should the process fail or just log a warning? Also, should the combination of phase: blue-green.application.before-start.idle and target-app: live even be allowed during an initial deployment?

ApplicationColor idleColor = context.getVariable(Variables.IDLE_MTA_COLOR);
ApplicationColor liveColor = context.getVariable(Variables.LIVE_MTA_COLOR);

if (idleColor == null || liveColor == null) {
return resolveAppNameWithLiveIdleSuffix(app.getName(), targetApp);
}

if (HookPhaseProcessType.HookProcessPhase.IDLE.getType().equals(targetApp)) {
return swapColorSuffix(app.getName(), liveColor, idleColor);
}
if (HookPhaseProcessType.HookProcessPhase.LIVE.getType().equals(targetApp)) {
return swapColorSuffix(app.getName(), idleColor, liveColor);
}
return app.getName();
}

private String resolveAppNameWithLiveIdleSuffix(String appName, String targetApp) {
String liveSuffix = BlueGreenApplicationNameSuffix.LIVE.asSuffix();
String idleSuffix = BlueGreenApplicationNameSuffix.IDLE.asSuffix();

if (HookPhaseProcessType.HookProcessPhase.IDLE.getType().equals(targetApp)) {
if (appName.endsWith(liveSuffix)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When blue-green.application.before-start.idle is used. The apps are still called (for example):
cf-app-with-hooks-idle
and:
cf-app-with-hooks-live
If we remove the suffix we will get a name of application that does not exist.

return appName.substring(0, appName.length() - liveSuffix.length());
}
if (!appName.endsWith(idleSuffix)) {
return appName + idleSuffix;
}
}
if (HookPhaseProcessType.HookProcessPhase.LIVE.getType().equals(targetApp)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we simplify these conditions as they are hard to read?
We have only one hook that is executed before Phase.AFTER_RESUME of the blue-green (blue-green.application.before-start.idle).
When this hook is used apps are called:
cf-app-with-hooks-idle
cf-app-with-hooks-live
So we can just remove the suffix from appName and append idle or live. (more readable) BlueGreenApplicationNameSuffix::removeSuffix

--
All other hooks are executed after Phase.AFTER_RESUME. Then apps are called:
cf-app-with-hooks
cf-app-with-hooks-live
So if idle needed -> remove suffix, if live needed remove suffix append Live. (no conditions at all)

if (appName.endsWith(idleSuffix)) {
return appName.substring(0, appName.length() - idleSuffix.length());
}
if (!appName.endsWith(liveSuffix)) {
return appName + liveSuffix;
}
}
return appName;
}

private String swapColorSuffix(String appName, ApplicationColor fromColor, ApplicationColor toColor) {
String fromSuffix = fromColor.asSuffix();
String toSuffix = toColor.asSuffix();
if (appName.endsWith(fromSuffix)) {
return appName.substring(0, appName.length() - fromSuffix.length()) + toSuffix;
}
if (!appName.endsWith(toSuffix)) {
return appName + toSuffix;
}
Comment on lines +127 to +135
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you just remove suffix BlueGreenApplicationNameSuffix::removeSuffix then append toColor?

return appName;
}

private CloudApplicationExtended buildAppWithName(CloudApplicationExtended app, String resolvedAppName) {
return org.cloudfoundry.multiapps.controller.client.lib.domain.ImmutableCloudApplicationExtended.copyOf(app)
.withName(resolvedAppName);
}

@Override
protected String getStepErrorMessage(ProcessContext context) {
return MessageFormat.format(Messages.ERROR_EXECUTING_HOOK,
context.getVariable(Variables.HOOK_FOR_EXECUTION).getName());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,8 @@ public ProcessType determineDeploymentType(ProcessContext context) {
return processTypeParser.getProcessType(context.getExecution());
}

public ProcessType determineDeploymentTypeSafely(ProcessContext context) {
return processTypeParser.getProcessType(context.getExecution(), false);
}
Comment on lines +23 to +25
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is this?


}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ private List<Hook> executeHooks(StepPhase currentStepPhase) {
moduleToDeploy.getName());
updateExecutedHooksForModule(alreadyExecutedHooksForModule, hooksWithPhases.getHookPhases(), hooksWithPhases.getHooks());
context.setVariable(Variables.HOOKS_FOR_EXECUTION, hooksWithPhases.getHooks());
context.setVariable(Variables.HOOK_EXECUTION_PHASE, hooksWithPhases.getHookPhases()
.stream()
.findFirst()
.map(HookPhase::getValue)
.orElse(null));
Comment on lines +53 to +57
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This variable breaks the idea of having multiple phases for one step. While technically:
getHookPhasesBeforeStep
getHookPhasesAfterStep
always return only one hook phase, in future they can be many, so this variable should comply with one to many relationship (step to hook phases)

return hooksWithPhases.getHooks();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,9 @@ public interface Variables {
.type(Variable.typeReference(Hook.class))
.defaultValue(Collections.emptyList())
.build();
Variable<String> HOOK_EXECUTION_PHASE = ImmutableSimpleVariable.<String> builder()
.name("hookExecutionPhase")
.build();
Variable<List<Module>> MODULES_TO_DEPLOY = ImmutableJsonBinaryListVariable.<Module> builder()
.name("modulesToDeploy")
.type(Variable.typeReference(Module.class))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@
<flowable:in source="isDisposableUserProvidedServiceEnabled" target="isDisposableUserProvidedServiceEnabled"></flowable:in>
<flowable:in source="disposableUserProvidedServiceName" target="disposableUserProvidedServiceName"></flowable:in>
<flowable:in source="mtaId" target="mtaId"></flowable:in>
<flowable:in source="subprocessPhase" target="subprocessPhase"></flowable:in>
<flowable:in source="phase" target="phase"></flowable:in>
<flowable:in source="hookExecutionPhase" target="hookExecutionPhase"></flowable:in>
<flowable:in source="idleMtaColor" target="idleMtaColor"></flowable:in>
<flowable:in source="liveMtaColor" target="liveMtaColor"></flowable:in>
</extensionElements>
<multiInstanceLoopCharacteristics isSequential="false" flowable:collection="hooksForExecution" flowable:elementVariable="hookForExecution">
<extensionElements>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,11 @@
<flowable:in source="isDisposableUserProvidedServiceEnabled" target="isDisposableUserProvidedServiceEnabled"></flowable:in>
<flowable:in source="disposableUserProvidedServiceName" target="disposableUserProvidedServiceName"></flowable:in>
<flowable:in source="mtaId" target="mtaId"></flowable:in>
<flowable:in source="subprocessPhase" target="subprocessPhase"></flowable:in>
<flowable:in source="phase" target="phase"></flowable:in>
<flowable:in source="hookExecutionPhase" target="hookExecutionPhase"></flowable:in>
<flowable:in source="idleMtaColor" target="idleMtaColor"></flowable:in>
<flowable:in source="liveMtaColor" target="liveMtaColor"></flowable:in>
</extensionElements>
<multiInstanceLoopCharacteristics isSequential="false" flowable:collection="hooksForExecution" flowable:elementVariable="hookForExecution"></multiInstanceLoopCharacteristics>
</callActivity>
Expand Down Expand Up @@ -143,6 +148,11 @@
<flowable:in source="isDisposableUserProvidedServiceEnabled" target="isDisposableUserProvidedServiceEnabled"></flowable:in>
<flowable:in source="disposableUserProvidedServiceName" target="disposableUserProvidedServiceName"></flowable:in>
<flowable:in source="mtaId" target="mtaId"></flowable:in>
<flowable:in source="subprocessPhase" target="subprocessPhase"></flowable:in>
<flowable:in source="phase" target="phase"></flowable:in>
<flowable:in source="hookExecutionPhase" target="hookExecutionPhase"></flowable:in>
<flowable:in source="idleMtaColor" target="idleMtaColor"></flowable:in>
<flowable:in source="liveMtaColor" target="liveMtaColor"></flowable:in>
</extensionElements>
<multiInstanceLoopCharacteristics isSequential="false" flowable:collection="hooksForExecution" flowable:elementVariable="hookForExecution"></multiInstanceLoopCharacteristics>
</callActivity>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@
<startEvent id="startEvent1" flowable:formFieldValidation="true"></startEvent>
<callActivity id="executeTasksCallActivity" name="Execute Tasks Call Activity" flowable:async="true" calledElement="executeTasksSubProcess" flowable:calledElementType="key" flowable:inheritVariables="true" flowable:fallbackToDefaultTenant="false"></callActivity>
<serviceTask id="determineTasksFromHookTask" name="Determine Tasks From Hook Step" flowable:async="true" flowable:delegateExpression="${determineTasksFromHookStep}"></serviceTask>
<serviceTask id="resolveHookTaskTargetAppTask" name="Resolve Hook Task Target App Step" flowable:async="true" flowable:delegateExpression="${resolveHookTaskTargetAppStep}"></serviceTask>
<endEvent id="sid-5A2D8943-12FA-4BC2-B6D0-7878EA87FB75">
<extensionElements>
<flowable:executionListener event="end" delegateExpression="${hooksEndProcessListener}"></flowable:executionListener>
</extensionElements>
</endEvent>
<sequenceFlow id="sid-5112772F-7C51-4FC3-BC2B-E4BEE435FC6D" sourceRef="startEvent1" targetRef="determineTasksFromHookTask"></sequenceFlow>
<sequenceFlow id="sid-9A809249-058E-4637-83DC-547314F6144B" sourceRef="determineTasksFromHookTask" targetRef="executeTasksCallActivity"></sequenceFlow>
<sequenceFlow id="sid-9A809249-058E-4637-83DC-547314F6144B" sourceRef="determineTasksFromHookTask" targetRef="resolveHookTaskTargetAppTask"></sequenceFlow>
<sequenceFlow id="sid-B3C1D2E4-1234-5678-ABCD-EF0123456789" sourceRef="resolveHookTaskTargetAppTask" targetRef="executeTasksCallActivity"></sequenceFlow>
<sequenceFlow id="sid-1E5806F9-54D9-4673-BAE3-91C4E13AF3B0" sourceRef="executeTasksCallActivity" targetRef="sid-5A2D8943-12FA-4BC2-B6D0-7878EA87FB75"></sequenceFlow>
</process>
<bpmndi:BPMNDiagram id="BPMNDiagram_executeHookTasksSubProcess">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@
<flowable:in source="isDisposableUserProvidedServiceEnabled" target="isDisposableUserProvidedServiceEnabled"></flowable:in>
<flowable:in source="disposableUserProvidedServiceName" target="disposableUserProvidedServiceName"></flowable:in>
<flowable:in source="mtaId" target="mtaId"></flowable:in>
<flowable:in source="subprocessPhase" target="subprocessPhase"></flowable:in>
<flowable:in source="hookExecutionPhase" target="hookExecutionPhase"></flowable:in>
<flowable:in source="idleMtaColor" target="idleMtaColor"></flowable:in>
<flowable:in source="liveMtaColor" target="liveMtaColor"></flowable:in>
</extensionElements>
<multiInstanceLoopCharacteristics isSequential="false" flowable:collection="hooksForExecution" flowable:elementVariable="hookForExecution"></multiInstanceLoopCharacteristics>
</callActivity>
Expand Down
Loading
Loading