moto icon indicating copy to clipboard operation
moto copied to clipboard

Cloudfront: update_distribution issue

Open izipris opened this issue 1 year ago • 1 comments

Hi folks,

I'd like to start with saying that Moto is a great library and it helps me a lot, thanks for your work!

Recently I've started working with the Cloudfront service, and I noticed some unexpected behavior of Moto while using the update_distribution method - the configuration of the distribution is actually not changed. I wrote some test to illustrate it:

@mock_aws
def test_update_distribution():
    cloudfront_service.create_distribution(DistributionConfig=distribution_config)
    distributions = cloudfront_service.list_distributions()['DistributionList']['Items']
    assert len(distributions) == 1
    actual_distribution = distributions[0]
    assert actual_distribution['Enabled']
    get_config_response = cloudfront_service.get_distribution_config(Id=actual_distribution['Id'])
    actual_distribution_config = get_config_response['DistributionConfig']
    etag = get_config_response['ETag']
    actual_distribution_config['Enabled'] = False  # Setup: change the 'Enabled' field to False
    cloudfront_service.update_distribution(DistributionConfig=actual_distribution_config, Id=actual_distribution['Id'],
                                           IfMatch=etag)  # Execution: set the new configuration
    distributions = cloudfront_service.list_distributions()['DistributionList']['Items']
    assert len(distributions) == 1
    actual_distribution = distributions[0]
    assert not actual_distribution['Enabled']  # Verification: make sure 'Enabled' is false FIXME: this assertion fails

I was digging a little bit and found in moto/cloudfront/models.py#L361 that when updating a distribution, the new configuration is stored in the config variable, however the new configuration is not applied, since applying the values of a configuration is happening only in the constructor of DistributionConfig.

Does my thinking makes sense? Would you like to get a PR for this?

izipris avatar Mar 01 '24 12:03 izipris

Hi @izipris! That does make sense - we definitely don't process the configuration correctly there.

A PR would be very welcome!

bblommers avatar Mar 01 '24 21:03 bblommers

Hi @bblommers , I just added a PR. I followed this guide - verified that pylint is happy and all tests passed. Please let me know if I missed anything. Thanks!

izipris avatar Mar 04 '24 21:03 izipris