Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Legacy target-access-mode to enable upgrade from pre 0.12.17 version #523

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

AlexanderYastrebov
Copy link
Member

@AlexanderYastrebov AlexanderYastrebov commented Jul 4, 2022

Before version 0.12.17 loadbalancer target group target type was not
specified and defaulted to instance in CloudFormation, see
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-elasticloadbalancingv2-targetgroup.html#cfn-elasticloadbalancingv2-targetgroup-targettype

PR #461 introduced AWS CNI mode and configured target group target type
either to ip or instance.

Changing target type from unset to instance in Cloudformation triggers target
group re-creation which makes it impossible to upgrade from pre 0.12.17 without downtime.

This change:

  • makes target-access-mode flag required to force users to choose proper value
  • introduces a new Legacy option that does not set target type and thus enables upgrade from pre 0.12.17.

Fixes #507

Signed-off-by: Alexander Yastrebov [email protected]

README.md Outdated Show resolved Hide resolved
@AlexanderYastrebov AlexanderYastrebov force-pushed the legacy-target-access-mode branch from 4381b72 to 7a7c527 Compare July 5, 2022 10:01
@jbilliau-rcd
Copy link
Contributor

Are you still working on this @AlexanderYastrebov ?

@AlexanderYastrebov
Copy link
Member Author

@jbilliau-rcd Hi, we need some effort to push it forward, currently I am overwhelmed by the backlog

@jbilliau-rcd
Copy link
Contributor

hey @AlexanderYastrebov , just checking in, I know you prob swamped. This still in limbo?

@szuecs
Copy link
Member

szuecs commented Sep 20, 2022

@jbilliau-rcd soon (~2nd week of October IIRC) we will have more time, because of internal code freeze. Sorry for the delay. :(

@jbilliau-rcd
Copy link
Contributor

@szuecs hey, just checking in; how we looking?

@szuecs
Copy link
Member

szuecs commented Oct 25, 2022

Let's ask @AlexanderYastrebov

@jbilliau-rcd
Copy link
Contributor

hey @AlexanderYastrebov , looks like the only thing holding this up is some code coverage tests?

@szuecs
Copy link
Member

szuecs commented Nov 2, 2022

@jbilliau-rcd this is not required. Can you open the PR as non-draft, please?

@jbilliau-rcd
Copy link
Contributor

@szuecs it's not my PR, I have no access to do that. Also, I'm not sure if @AlexanderYastrebov if fully done or not, maybe he left it in draft for a reason, was still vetting stuff out? Not sure.

@szuecs
Copy link
Member

szuecs commented Nov 3, 2022

@jbilliau-rcd oh, my bad will try to ask him to finish it up.

@AlexanderYastrebov AlexanderYastrebov force-pushed the legacy-target-access-mode branch from 7a7c527 to 8a5eef4 Compare November 3, 2022 13:43
…` version

Before version `0.12.17` loadbalancer target group target type was not
specified and defaulted to `instance` in CloudFormation, see
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-elasticloadbalancingv2-targetgroup.html#cfn-elasticloadbalancingv2-targetgroup-targettype

PR #461 introduced AWS CNI mode and configured target group target type
either to `ip` or `instance`.

Changing target type from unset to `instance` in Cloudformation triggers target
group re-creation which makes it impossible to upgrade from pre `0.12.17` without downtime.

This change:
- makes `target-access-mode` flag required to force users to choose proper value
- introduces a new `Legacy` option that does not set target type and thus enables upgrade from pre `0.12.17`.

Fixes #507

Signed-off-by: Alexander Yastrebov <[email protected]>
@AlexanderYastrebov AlexanderYastrebov force-pushed the legacy-target-access-mode branch from 8a5eef4 to c21ce70 Compare November 3, 2022 13:59
@AlexanderYastrebov AlexanderYastrebov marked this pull request as ready for review November 3, 2022 14:00
@jbilliau-rcd
Copy link
Contributor

Looks like this is all ready to go? @szuecs are you someone who can review and merge? I appreciate the work btw; we have 200 clusters using this controller and so this is a HUGE burden off our shoulders since we have to upgrade to avoid the BoundServiceAccountToken expiration issue.

@szuecs
Copy link
Member

szuecs commented Nov 3, 2022

👍

1 similar comment
@AlexanderYastrebov
Copy link
Member Author

👍

@AlexanderYastrebov AlexanderYastrebov merged commit 9e1ef19 into master Nov 7, 2022
@AlexanderYastrebov AlexanderYastrebov deleted the legacy-target-access-mode branch November 7, 2022 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants