[Enhancement-3191] transport_enabled setting on an auth domain and authorizer may be unnecessary after transport client removal

[prabhask5]

Description

Remove the transport_enabled setting in config.yml and its related code in DynamicConfigModelV6/7, ConfigV6/7, associated tests. Additionally add handlers in Config files to handle when transport_enabled is still used within the config.yml file while alerting the user with a message in startup that this setting can be safely removed from the config.

• Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
  Enhancement

• Why these changes are required?
  transport_enabled setting on an auth domain and authorizer may be unnecessary after transport client removal  #3191 states that since the transport client has been removed, there is no need for the transport_enabled config and its related code to exist in the codebase, so this ticket removes that unnecessary code.

• What is the old behavior before changes and new behavior after changes?
  Nothing.

Issues Resolved

#3191

Is this a backport? If so, please add backport PR # and/or commits #
No

Testing

No tests were made, all tests should run since the behavior should not change.

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (f217fa8) 65.18% compared to head (ba197b7) 65.61%.
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3939      +/-   ##
==========================================
+ Coverage   65.18%   65.61%   +0.43%     
==========================================
  Files         298      298              
  Lines       21216    21246      +30     
  Branches     3457     3457              
==========================================
+ Hits        13829    13940     +111     
+ Misses       5672     5588      -84     
- Partials     1715     1718       +3     

Files	Coverage Δ
...ch/security/securityconf/DynamicConfigModelV7.java	65.51% <100.00%> (-0.79%)	⬇️
...pensearch/security/setting/DeprecatedSettings.java	85.71% <100.00%> (+5.71%)	⬆️
...ch/security/securityconf/DynamicConfigModelV6.java	0.00% <0.00%> (ø)
...search/security/securityconf/impl/v7/ConfigV7.java	78.39% <83.33%> (+1.42%)	⬆️
...search/security/securityconf/impl/v6/ConfigV6.java	62.39% <66.66%> (+2.58%)	⬆️

... and 9 files with indirect coverage changes

[prabhask5]

@peternied @scrawfor99 @cwperks Ready for review!

[peternied]

Thanks, lets reuse DeprecatedSettings and add more test cases, see comments inline.

[stephen-crawford]

Hi @prabhask5 thanks for making these changes. Great job keeping things up to date! Other than Peter's suggestions, things look good to me. Let me know when you address those and I can give a final pass through and approve (assuming nothing else comes up etc.)

[DarshitChanpura]

Changes look good to me. Have we analyzed the blast radius for removing these files? Will there be downstream plugins that will have broken tests?

[prabhask5]

Changes look good to me. Have we analyzed the blast radius for removing these files? Will there be downstream plugins that will have broken tests?

@DarshitChanpura I haven't checked for any other repository tests being impacted by this change, though I can confirm that the only errors that were caused by removing this setting being a parsing error from Jackson which I resolved using the unknownPropertiesHandler. If this error would have occurred in downstream plugins it should not. I'm new to contributing, can you let me know of other plugins to check tests for?

[prabhask5]

@peternied I think I've addressed all your requested changes:

1. I added a new method to the DeprecatedSettings class that allows for the logging of any deprecated setting in which the codebase does not support any more and the utilization of would result in an error, I have "extra information" of where the config that should be removed as a parameter to add extra information for a specific setting.
2. Previously the unknownPropertyHandler didn't account for other unknown properties so I refactored it so it throws the original error if the setting is not transport_enabled (which is currently the only setting that is deprecated here), I also used a switch statement to make it easy to add more deprecated settings in the future.
3. I added some more tests in DeprecatedSettingsTest to account for both of these cases.

This should be ready for review once I fix the test errors!

[stephen-crawford]

Hi @prabhask5, thanks for your work with this! Looks good to me :)

[DarshitChanpura]

@DarshitChanpura I haven't checked for any other repository tests being impacted by this change, though I can confirm that the only errors that were caused by removing this setting being a parsing error from Jackson which I resolved using the unknownPropertiesHandler. If this error would have occurred in downstream plugins it should not. I'm new to contributing, can you let me know of other plugins to check tests for?

I'm not aware of any off the top of my mind. We can merge this and if there are any downstream failures we can analyze and revert of necessary.