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

Application gateway attribues are now sorted to avoid recreation. #19963

Conversation

michal-malec
Copy link
Contributor

@michal-malec michal-malec commented Jan 11, 2023

The problem
In Application Gateway order of listeners, routing rules, ssl settings, etc. changes between applies. This causes part of the configuration to be recreated.

Current behaviour
Current implementation in Terraform sorts configurations based on hash that is generated from most of the properties. It means that in most of the cases after adding new configuration block it is very likely that it will be added in the middle of the list and will cause removal and addition of settings.

Proposed solution
There is no ideal solution for this issue as in Azure API all AppGw configurations are kept in lists which don't have any argument that could be easily used to sort them (e.g. id). I propose to sort the lists by name (or priority in one case). This results in keeping old configuration while adding new settings if you name resources in alphabetical order.

The PR resolves #16136, #18422 (but only if names are set in alphabetical order)

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hi @michal-malec, I appreciate the effort put into this but as you mentioned, this will break people who don't have their configuration in alphabetical order. I know application gateways are already broken and needs fixing but that fix should be one that requires any work on the users part.

I'm not sure what the best solution is but I don't think it involves List or Sets. There is a new terraform framework that has some additional tools like a Map that we could use. It'll require some investigation on our part when we do move to it and I'm not sure when that will even be.

Unfortunately, I'm going to close this PR down and it's something we'll have to come back to at a later date when we've moved over to the new framework.

@EAaronHarris
Copy link

Extremely disappointing that this has been closed when a workable fix had been proposed.

We have a large/complex app gateway configuration, and we can re-sort our code to alphabetical order relatively easily - at this point, we would be thrilled to do that, in order to have a module that functions.

I do not understand the reasoning behind the decision to close this proposal. There is surely a large number of customers who would be happy with the proposed solution.

Asking everyone to wait for an indeterminate/indefinite period of time - when a solution is possible - does not seem like the right answer here.

@katbyte
Copy link
Collaborator

katbyte commented Feb 1, 2023

@EAaronHarris - unfortunately changing this to a sorted list would be a a breaking change and is not something we should really ship without a major version change. As moving to framework is part of our plans for 4.0 later this year we will likely look to leveraging some of the new functionality it has to solve this (and other situations where neither Sets or Lists work) in v4.0.

@katbyte katbyte added this to the v4.0.0 milestone Feb 1, 2023
@EAaronHarris
Copy link

@katbyte It's possible that I am misunderstanding the scope of the issue and the fix.

If there are any customers for whom the module works already, then wouldn't that mean they are not using these unordered lists, and so wouldn't be impacted by any changes to the way they are handled?

On the flip side of the coin, for customers who are attempting to use the unordered lists (such as we are), changing the way/order in which they are handled will, in the worst case scenario, simply continue producing the exact same behavior we are already seeing each time we run a plan/apply today (i.e. re-ordering the elements) - but at the same time it would actually provide a path to resolving them.

This is the part that I'm not sure I understand. If it is truly a breaking change, then I agree. I just don't understand how it could break anything that isn't already... broken (for lack of a better word).

Would the change impact areas outside the unordered property/attribute lists?

Thank you for the response.

@mbfrahry
Copy link
Member

mbfrahry commented Feb 7, 2023

Our big issue here is that we can't guarantee ordering and we're relying on users to get that ordering correct. Getting that ordering correct can be confusing too if you're not entirely sure why you're getting perpetual diffs. It's putting a lot of onus on users when we need a better solution that doesn't require constant upkeep on the config file.

With all that said, I have thought of a solution that won't require waiting for the framework transition. It involves splitting out each of the block attributes into their own resources. This will still involve a config file rewrite but it should be a one time thing and we won't have to worry about ordering these settings in any particular way going forward.

I'm going to start taking a crack at getting those resources written starting today.

@krknopp
Copy link

krknopp commented Feb 7, 2023

We were actually hoping this is where it would end up. We love the way the new Front Door CDN stuff was rewritten, which sounds very similar to what you're proposing.

@EAaronHarris
Copy link

Yes, that would be fantastic. This would not only address the issue (if it works as desired), but would also be a net overall improvement to the app gateway module as a whole. Thank you for considering this!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application Gateway request_routing_rule order change even with Azurerm 3.0.2
6 participants