-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Application gateway attribues are now sorted to avoid recreation. #19963
Conversation
There was a problem hiding this 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.
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. |
@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 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. |
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. |
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. |
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! |
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. |
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)