Skip to content

Commit

Permalink
fix(amazon): Don't overwrite the LB target group attribute values dur…
Browse files Browse the repository at this point in the history 
…ing upsert operation (#4666)

* fix(amazon): Don't overwrite the ppv2 value if its already set

* fixup based on comments

* more fixups

* refactor and add ut

* cleanup log msg

* rename const

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
@aravindmd @mergify
aravindmd and mergify[bot] committed Jun 12, 2020
1 parent e1372cc commit 05d2b68
Show file tree
Hide file tree
Showing 3 changed files with 239 additions and 22 deletions.
12 changes: 6 additions & 6 deletions ...x/spinnaker/clouddriver/aws/deploy/description/UpsertAmazonLoadBalancerV2Description.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,13 +329,13 @@ public void setRedirectActionConfig(RedirectActionConfig redirectActionConfig) {
}

public static class Attributes {
private Integer deregistrationDelay = 300;
private Boolean stickinessEnabled = false;
private String stickinessType = "lb_cookie";
private Integer stickinessDuration = 86400;
private Boolean proxyProtocolV2 = false;
private Integer deregistrationDelay;
private Boolean stickinessEnabled;
private String stickinessType;
private Integer stickinessDuration;
private Boolean proxyProtocolV2;
/** The following attribute is supported only if the target is a Lambda function. */
private Boolean multiValueHeadersEnabled = false;
private Boolean multiValueHeadersEnabled;

public Integer getDeregistrationDelay() {
return deregistrationDelay;
Expand Down
79 changes: 63 additions & 16 deletions .../com/netflix/spinnaker/clouddriver/aws/deploy/handlers/LoadBalancerV2UpsertHandler.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,61 @@ class LoadBalancerV2UpsertHandler {
private static final String ATTRIBUTE_IDLE_TIMEOUT = "idle_timeout.timeout_seconds"
private static final String ATTRIBUTE_DELETION_PROTECTION = "deletion_protection.enabled"

//Defaults for Target Group Attributes
private static final String DEREGISTRATION_DELAY = "300"
private static final Boolean STICKINESS_ENABLED = false
private static final String STICKINESS_TYPE = "lb_cookie"
private static final String STICKINESS_DURATION = "86400"
private static final Boolean PROXY_PROTOCOL_V2 = false
/** The following attribute is supported only if the target is a Lambda function. */
private static final Boolean MULTI_VALUE_HEADERS_ENABLED = false

private static Task getTask() {
TaskRepository.threadLocalTask.get()
}

private static String modifyTargetGroupAttributes(AmazonElasticLoadBalancing loadBalancing, LoadBalancer loadBalancer, TargetGroup targetGroup, UpsertAmazonLoadBalancerV2Description.Attributes attributes) {
return modifyTargetGroupAttributes(loadBalancing, loadBalancer, targetGroup, attributes, null)
//Create Target Group Attributes with values provided in description, set to defaults other wise
static String createTargetGroupAttributes(AmazonElasticLoadBalancing loadBalancing, LoadBalancer loadBalancer, TargetGroup targetGroup, UpsertAmazonLoadBalancerV2Description.Attributes attributes, DeployDefaults deployDefaults) {
def targetGroupAttributes = []
log.info("Creating target group attributes for targetGroup {}", targetGroup.targetGroupName)
if (attributes) {
if (TargetTypeEnum.Lambda.toString().equalsIgnoreCase(targetGroup.getTargetType())) {
def multiValueHeaderAttribute = attributes.multiValueHeadersEnabled ?: MULTI_VALUE_HEADERS_ENABLED
targetGroupAttributes.add(new TargetGroupAttribute(key: "lambda.multi_value_headers.enabled", value: multiValueHeaderAttribute))

} else {
Integer deregistrationDelay = [attributes.deregistrationDelay, deployDefaults?.loadBalancing?.deregistrationDelayDefault].findResult(Closure.IDENTITY)

def deregistrationDealyAttribute = deregistrationDelay?.toString() ?: DEREGISTRATION_DELAY
targetGroupAttributes.add(new TargetGroupAttribute(key: "deregistration_delay.timeout_seconds", value: deregistrationDealyAttribute))
}
if (loadBalancer.type == 'application') {
def stickinessEnabledAttribute = attributes.stickinessEnabled?.toString() ?: STICKINESS_ENABLED
targetGroupAttributes.add(new TargetGroupAttribute(key: "stickiness.enabled", value: stickinessEnabledAttribute))

def stickinessTypeAttribute = attributes.stickinessType ?: STICKINESS_TYPE
targetGroupAttributes.add(new TargetGroupAttribute(key: "stickiness.type", value: stickinessTypeAttribute))

def stickinessDurationAttribute = attributes.stickinessDuration?.toString() ?: STICKINESS_DURATION
targetGroupAttributes.add(new TargetGroupAttribute(key: "stickiness.lb_cookie.duration_seconds", value: stickinessDurationAttribute))

}
if (loadBalancer.type == 'network') {
def proxyProtocolV2Attribute = attributes.proxyProtocolV2 ?: PROXY_PROTOCOL_V2
targetGroupAttributes.add(new TargetGroupAttribute(key: "proxy_protocol_v2.enabled", value: proxyProtocolV2Attribute))

}
}
return updateTargetGroupAttributes(loadBalancing, targetGroup, targetGroupAttributes)
}

// Modify target group attributes with attributes that are set in the description , do not update attributes that are not set
private static String modifyTargetGroupAttributes(AmazonElasticLoadBalancing loadBalancing, LoadBalancer loadBalancer, TargetGroup targetGroup, UpsertAmazonLoadBalancerV2Description.Attributes attributes, DeployDefaults deployDefaults) {

log.info("Update target group attributes for targetGroup {}", targetGroup.targetGroupName)
def targetGroupAttributes = []
if (attributes) {
if (TargetTypeEnum.Lambda.toString().equalsIgnoreCase(targetGroup.getTargetType()))
{
if (TargetTypeEnum.Lambda.toString().equalsIgnoreCase(targetGroup.getTargetType())) {
if (attributes.multiValueHeadersEnabled != null) {
targetGroupAttributes.add(new TargetGroupAttribute(key: "lambda.multi_value_headers.enabled", value: attributes.multiValueHeadersEnabled))
}
Expand All @@ -65,23 +108,27 @@ class LoadBalancerV2UpsertHandler {
targetGroupAttributes.add(new TargetGroupAttribute(key: "stickiness.lb_cookie.duration_seconds", value: attributes.stickinessDuration.toString()))
}
}
if (loadBalancer.type == 'network' ) {
if(attributes.proxyProtocolV2 != null) {
if (loadBalancer.type == 'network') {
if (attributes.proxyProtocolV2 != null) {
targetGroupAttributes.add(new TargetGroupAttribute(key: "proxy_protocol_v2.enabled", value: attributes.proxyProtocolV2))
}
}
}
}
return updateTargetGroupAttributes(loadBalancing, targetGroup, targetGroupAttributes)
}

try {
loadBalancing.modifyTargetGroupAttributes(new ModifyTargetGroupAttributesRequest()
.withTargetGroupArn(targetGroup.targetGroupArn)
.withAttributes(targetGroupAttributes))
task.updateStatus BASE_PHASE, "Modified target group ${targetGroup.targetGroupName} attributes."
} catch (AmazonServiceException e) {
return handleError("Failed to modify attributes for target group ${targetGroup.targetGroupName} - reason: ${e.toString()}.", e)
static String updateTargetGroupAttributes(AmazonElasticLoadBalancing loadBalancing, TargetGroup targetGroup, List<TargetGroupAttribute> targetGroupAttributes) {
if (!targetGroupAttributes.isEmpty()) {
try {
loadBalancing.modifyTargetGroupAttributes(new ModifyTargetGroupAttributesRequest()
.withTargetGroupArn(targetGroup.targetGroupArn)
.withAttributes(targetGroupAttributes))
task.updateStatus BASE_PHASE, "Modified target group ${targetGroup.targetGroupName} attributes."
} catch (AmazonServiceException e) {
return handleError("Failed to modify attributes for target group ${targetGroup.targetGroupName} - reason: ${e.toString()}.", e)
}
}

return null
}

Expand Down Expand Up @@ -145,7 +192,7 @@ class LoadBalancerV2UpsertHandler {
createdTargetGroups.add(createdTargetGroup)

// Add attributes
String exceptionMessage = modifyTargetGroupAttributes(loadBalancing, loadBalancer, createdTargetGroup, targetGroup.attributes, deployDefaults)
String exceptionMessage = createTargetGroupAttributes(loadBalancing, loadBalancer, createdTargetGroup, targetGroup.attributes, deployDefaults)
if (exceptionMessage) {
amazonErrors << exceptionMessage
}
Expand Down Expand Up @@ -198,7 +245,7 @@ class LoadBalancerV2UpsertHandler {
task.updateStatus BASE_PHASE, "Target group updated in ${loadBalancer.loadBalancerName} (${awsTargetGroup.targetGroupName}:${awsTargetGroup.port}:${awsTargetGroup.protocol})."

// Update attributes
String exceptionMessage = modifyTargetGroupAttributes(loadBalancing, loadBalancer, awsTargetGroup, targetGroup.attributes)
String exceptionMessage = modifyTargetGroupAttributes(loadBalancing, loadBalancer, awsTargetGroup, targetGroup.attributes, null)
if (exceptionMessage) {
amazonErrors << exceptionMessage
}
Expand Down
170 changes: 170 additions & 0 deletions ...uddriver/aws/deploy/ops/loadbalancer/UpsertAmazonLoadBalancerV2AtomicOperationSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,72 @@ class UpsertAmazonLoadBalancerV2AtomicOperationSpec extends Specification {
idleTimeout: 60,
deletionProtection: true
)
UpsertAmazonLoadBalancerV2Description updateDescription = new UpsertAmazonLoadBalancerV2Description(
loadBalancerType: AmazonLoadBalancerType.APPLICATION,
name: "foo-main-frontend",
availabilityZones: ["us-east-1": ["us-east-1a"]],
listeners: [
new UpsertAmazonLoadBalancerV2Description.Listener(
port: 80,
protocol: ProtocolEnum.HTTP,
defaultActions: [
new UpsertAmazonLoadBalancerV2Description.Action(
targetGroupName: targetGroupName
)
]
)
],
securityGroups: ["foo"],
credentials: TestCredential.named('bar'),
targetGroups: [
new UpsertAmazonLoadBalancerV2Description.TargetGroup(
name: "target-group-foo",
protocol: ProtocolEnum.HTTP,
port: 80,
healthCheckProtocol: ProtocolEnum.HTTP,
healthCheckPort: 8080,
attributes: [
deregistrationDelay: 300,
]
)
],
subnetType: "internal",
idleTimeout: 60,
deletionProtection: true
)
UpsertAmazonLoadBalancerV2Description descriptionWithNoAttributes = new UpsertAmazonLoadBalancerV2Description(
loadBalancerType: AmazonLoadBalancerType.APPLICATION,
name: "foo-main-frontend",
availabilityZones: ["us-east-1": ["us-east-1a"]],
listeners: [
new UpsertAmazonLoadBalancerV2Description.Listener(
port: 80,
protocol: ProtocolEnum.HTTP,
defaultActions: [
new UpsertAmazonLoadBalancerV2Description.Action(
targetGroupName: targetGroupName
)
]
)
],
securityGroups: ["foo"],
credentials: TestCredential.named('bar'),
targetGroups: [
new UpsertAmazonLoadBalancerV2Description.TargetGroup(
name: "target-group-foo",
protocol: ProtocolEnum.HTTP,
port: 80,
healthCheckProtocol: ProtocolEnum.HTTP,
healthCheckPort: 8080,
attributes: [
]
)
],
subnetType: "internal",
idleTimeout: 60,
deletionProtection: true
)

def loadBalancerArn = "test:arn"
def targetGroupArn = "test:target:group:arn"
def targetGroup = new TargetGroup(targetGroupArn: targetGroupArn, targetGroupName: targetGroupName, port: 80, protocol: ProtocolEnum.HTTP)
Expand Down Expand Up @@ -184,6 +250,74 @@ class UpsertAmazonLoadBalancerV2AtomicOperationSpec extends Specification {
0 * _
}

void "should create target group attributes passed for existing load balancer"() {
setup:
def existingLoadBalancers = [ loadBalancerOld ]
def existingTargetGroups = []
def existingListeners = []

when:
operation.operate([])

then:
1 * loadBalancing.describeLoadBalancers(new DescribeLoadBalancersRequest(names: ["foo-main-frontend"])) >>
new DescribeLoadBalancersResult(loadBalancers: existingLoadBalancers)
1 * loadBalancing.setSecurityGroups(new SetSecurityGroupsRequest(
loadBalancerArn: loadBalancerArn,
securityGroups: ["sg-1234"]
))
1 * loadBalancing.describeTargetGroups(new DescribeTargetGroupsRequest(loadBalancerArn: loadBalancerArn)) >> new DescribeTargetGroupsResult(targetGroups: existingTargetGroups)
1 * loadBalancing.createTargetGroup(_ as CreateTargetGroupRequest) >> new CreateTargetGroupResult(targetGroups: [targetGroup])
1 * loadBalancing.describeListeners(new DescribeListenersRequest(loadBalancerArn: loadBalancerArn)) >> new DescribeListenersResult(listeners: existingListeners)
1 * loadBalancing.createListener(new CreateListenerRequest(loadBalancerArn: loadBalancerArn, port: 80, protocol: "HTTP", defaultActions: [new Action(targetGroupArn: targetGroupArn, type: ActionTypeEnum.Forward, order: 1)]))
1 * loadBalancing.describeLoadBalancerAttributes(_) >> [attributes: loadBalancerAttributes]
1 * loadBalancing.modifyTargetGroupAttributes(_) >> { ModifyTargetGroupAttributesRequest request ->
assert request.attributes.find { it.key == 'deregistration_delay.timeout_seconds'}.value == "300"
assert request.attributes.find { it.key == 'stickiness.enabled'}.value == "false"
assert request.attributes.find { it.key == 'stickiness.type'}.value == "lb_cookie"
assert request.attributes.find { it.key == 'stickiness.lb_cookie.duration_seconds'}.value == "86400"
assert request.targetGroupArn == "test:target:group:arn"
return new ModifyTargetGroupAttributesResult()
}
0 * _
}

void "should create target group attributes with defaults for existing load balancer"() {
@Subject createOperation = new UpsertAmazonLoadBalancerV2AtomicOperation(descriptionWithNoAttributes)
setup:
def existingLoadBalancers = [ loadBalancerOld ]
def existingTargetGroups = []
def existingListeners = []
createOperation.amazonClientProvider = mockAmazonClientProvider
createOperation.regionScopedProviderFactory = regionScopedProviderFactory
createOperation.deployDefaults = new AwsConfiguration.DeployDefaults(addAppGroupToServerGroup: true, createLoadBalancerIngressPermissions: true)
createOperation.ingressLoadBalancerBuilder = ingressLoadBalancerBuilder
when:
createOperation.operate([])

then:
1 * loadBalancing.describeLoadBalancers(new DescribeLoadBalancersRequest(names: ["foo-main-frontend"])) >>
new DescribeLoadBalancersResult(loadBalancers: existingLoadBalancers)
1 * loadBalancing.setSecurityGroups(new SetSecurityGroupsRequest(
loadBalancerArn: loadBalancerArn,
securityGroups: ["sg-1234"]
))
1 * loadBalancing.describeTargetGroups(new DescribeTargetGroupsRequest(loadBalancerArn: loadBalancerArn)) >> new DescribeTargetGroupsResult(targetGroups: existingTargetGroups)
1 * loadBalancing.createTargetGroup(_ as CreateTargetGroupRequest) >> new CreateTargetGroupResult(targetGroups: [targetGroup])
1 * loadBalancing.describeListeners(new DescribeListenersRequest(loadBalancerArn: loadBalancerArn)) >> new DescribeListenersResult(listeners: existingListeners)
1 * loadBalancing.createListener(new CreateListenerRequest(loadBalancerArn: loadBalancerArn, port: 80, protocol: "HTTP", defaultActions: [new Action(targetGroupArn: targetGroupArn, type: ActionTypeEnum.Forward, order: 1)]))
1 * loadBalancing.describeLoadBalancerAttributes(_) >> [attributes: loadBalancerAttributes]
1 * loadBalancing.modifyTargetGroupAttributes(_) >> { ModifyTargetGroupAttributesRequest request ->
assert request.attributes.find { it.key == 'deregistration_delay.timeout_seconds'}.value == "300"
assert request.attributes.find { it.key == 'stickiness.enabled'}.value == "false"
assert request.attributes.find { it.key == 'stickiness.type'}.value == "lb_cookie"
assert request.attributes.find { it.key == 'stickiness.lb_cookie.duration_seconds'}.value == "86400"
assert request.targetGroupArn == "test:target:group:arn"
return new ModifyTargetGroupAttributesResult()
}
0 * _
}

void "should modify target group of existing load balancer"() {
setup:
def existingLoadBalancers = [ loadBalancerOld ]
Expand All @@ -209,6 +343,42 @@ class UpsertAmazonLoadBalancerV2AtomicOperationSpec extends Specification {
0 * _
}

void "should modify only target group attributes that are passed of existing load balancer"() {
@Subject updateOperation = new UpsertAmazonLoadBalancerV2AtomicOperation(updateDescription)

setup:
def existingLoadBalancers = [ loadBalancerOld ]
def existingTargetGroups = [ targetGroup ]
def existingListeners = []
updateOperation.amazonClientProvider = mockAmazonClientProvider
updateOperation.regionScopedProviderFactory = regionScopedProviderFactory
updateOperation.deployDefaults = new AwsConfiguration.DeployDefaults(addAppGroupToServerGroup: true, createLoadBalancerIngressPermissions: true)
updateOperation.ingressLoadBalancerBuilder = ingressLoadBalancerBuilder

when:
updateOperation.operate([])

then:
1 * loadBalancing.describeLoadBalancers(new DescribeLoadBalancersRequest(names: ["foo-main-frontend"])) >>
new DescribeLoadBalancersResult(loadBalancers: existingLoadBalancers)
1 * loadBalancing.setSecurityGroups(new SetSecurityGroupsRequest(
loadBalancerArn: loadBalancerArn,
securityGroups: ["sg-1234"]
))
1 * loadBalancing.describeTargetGroups(new DescribeTargetGroupsRequest(loadBalancerArn: loadBalancerArn)) >> new DescribeTargetGroupsResult(targetGroups: existingTargetGroups)
1 * loadBalancing.modifyTargetGroup(_ as ModifyTargetGroupRequest)
1 * loadBalancing.modifyTargetGroupAttributes(_) >> { ModifyTargetGroupAttributesRequest request ->
assert request.attributes.find { it.key == 'deregistration_delay.timeout_seconds'}.value == "300"
assert request.attributes.find { it.key == 'stickiness.enabled'} == null
assert request.targetGroupArn == "test:target:group:arn"
return new ModifyTargetGroupAttributesResult()
}
1 * loadBalancing.describeListeners(new DescribeListenersRequest(loadBalancerArn: loadBalancerArn)) >> new DescribeListenersResult(listeners: existingListeners)
1 * loadBalancing.createListener(new CreateListenerRequest(loadBalancerArn: loadBalancerArn, port: 80, protocol: "HTTP", defaultActions: [new Action(targetGroupArn: targetGroupArn, type: ActionTypeEnum.Forward, order: 1)]))
1 * loadBalancing.describeLoadBalancerAttributes(_) >> [attributes: loadBalancerAttributes]
0 * _
}

void "should remove missing target group of existing load balancer"() {
setup:
def existingLoadBalancers = [ loadBalancerOld ]
Expand Down

0 comments on commit 05d2b68

Please sign in to comment.