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

azure_rm_storageaccount module not passing https_only true on account creation #457

Closed
arsenicks opened this issue Mar 15, 2021 · 5 comments · Fixed by #458
Closed

azure_rm_storageaccount module not passing https_only true on account creation #457

arsenicks opened this issue Mar 15, 2021 · 5 comments · Fixed by #458
Labels
bug Something isn't working has_pr PR fixes have been made medium_priority Medium priority

Comments

@arsenicks
Copy link

SUMMARY

In the azure_rm_storageaccount module, the 'https_only' flag value always seems to pass 'false' to Azure when run, even when set to true or yes.

ISSUE TYPE
  • Bug Report
COMPONENT NAME

azure_rm_storageaccount

ANSIBLE VERSION
ansible 2.9.6
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/apps/ansible/library']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /bin/ansible
  python version = 2.7.5 (default, Aug  7 2019, 00:51:29) [GCC 4.8.5 20150623 (Red Hat 4.8.5-39)]

And

Ansible 2.10.X

Collection version:

$ ansible-galaxy collection list

# /home/XXXX/.ansible/collections/ansible_collections
Collection         Version
------------------ -------
azure.azcollection 1.4.0  
CONFIGURATION
CACHE_PLUGIN(/etc/ansible/ansible.cfg) = redis
CACHE_PLUGIN_CONNECTION(/etc/ansible/ansible.cfg) = localhost:6379:0
COMMAND_WARNINGS(/etc/ansible/ansible.cfg) = False
DEFAULT_FORKS(/etc/ansible/ansible.cfg) = 50
DEFAULT_GATHERING(/etc/ansible/ansible.cfg) = smart
DEFAULT_HOST_LIST(/etc/ansible/ansible.cfg) = [u'/apps/ansible/inventories/dev/hosts']
DEFAULT_LOG_PATH(/etc/ansible/ansible.cfg) = /apps/ansible/log/ansible.log
DEFAULT_MODULE_PATH(/etc/ansible/ansible.cfg) = [u'/apps/ansible/library']
DEFAULT_MODULE_UTILS_PATH(/etc/ansible/ansible.cfg) = [u'/apps/ansible/module_utils']
DEFAULT_ROLES_PATH(/etc/ansible/ansible.cfg) = [u'/apps/ansible/roles']
OS / ENVIRONMENT

Target is just the Azure cloud - storage accounts in Azure.

STEPS TO REPRODUCE

You use the code below, first pass use create_account function, doesn't push the https_only to azure, if you run a second time the update_account function is used and the option is set on the storage account.

Code sample to reproduce:

---
- name: Create RG and secure storage account
  hosts: localhost
  gather_facts: false
  collections:
    - azure.azcollection

  tasks:
  - name: Create RG
    azure.azcollection.azure_rm_resourcegroup:
      name: "{{ item }}"
      location: canadacentral
    loop:
      - rg01

  - name: Create storage account true
    azure.azcollection.azure_rm_storageaccount:
      resource_group: rg01
      https_only: true
      name: "{{ item }}"
      kind: StorageV2
      type: Standard_LRS
      allow_blob_public_access: no
    loop:
      - pp9901
EXPECTED RESULTS

Expected to have the param set for the module match the correct value being passed to Azure and to be able to create storage accounts with the https_only flag set to true since that feature is available in Azure and the Ansible module documents that it should be working.

ACTUAL RESULTS

Ansible-playbook output using -vvv. You can see the differences between the invocation and the state.

[...]
using module file /home/XXXX/.ansible/collections/ansible_collections/azure/azcollection/plugins/modules/azure_rm_storageaccount.py
[...]
changed: [localhost] => (item=pp9901) => {
    "ansible_loop_var": "item",
    "changed": true,
    "invocation": {
        "module_args": {
            "access_tier": null,
            "account_type": "Standard_LRS",
            "ad_user": null,
            "adfs_authority_url": null,
            "allow_blob_public_access": false,
            "api_profile": "latest",
            "append_tags": true,
            "auth_source": "auto",
            "blob_cors": null,
            "cert_validation_mode": null,
            "client_id": null,
            "cloud_environment": "AzureCloud",
            "custom_domain": null,
            "force_delete_nonempty": false,
            "https_only": true,
            "kind": "StorageV2",
            "location": null,
            "minimum_tls_version": null,
            "name": "pp9901",
            "network_acls": null,
            "password": null,
            "profile": null,
            "resource_group": "rg01",
            "secret": null,
            "state": "present",
            "subscription_id": null,
            "tags": null,
            "tenant": null,
            "type": "Standard_LRS"
        }
    },
    "item": "pp9901",
    "state": {
        "access_tier": "Hot",
        "allow_blob_public_access": false,
        "custom_domain": null,
        "https_only": false,
        "id": "/subscriptions/XXXXXXXXXXXXX/resourceGroups/rg01/providers/Microsoft.Storage/storageAccounts/pp9901",
        "location": "canadacentral",
        "minimum_tls_version": null,
        "name": "pp9901",
        "network_acls": {
            "bypass": "AzureServices",
            "default_action": "Allow",
            "ip_rules": [],
            "virtual_network_rules": []
        },
        "primary_endpoints": {
            "blob": "https://pp9901.blob.core.windows.net/",
            "queue": "https://pp9901.queue.core.windows.net/",
            "table": "https://pp9901.table.core.windows.net/"
        },
        "primary_location": "canadacentral",
        "provisioning_state": "Succeeded",
        "resource_group": "rg01",
        "secondary_endpoints": null,
        "secondary_location": null,
        "sku_name": "Standard_LRS",
        "sku_tier": "Standard",
        "status_of_primary": "available",
        "status_of_secondary": null,
        "tags": null,
        "type": "Microsoft.Storage/storageAccounts"
    }
}

As you can see the module_args show clearly that ""https_only": true" is set and we can see in the state that it's not set ""https_only": false"

Azure activity log output confirm the problem is not on azure side, azure receive supportsHttpsTrafficOnly to false:

    "properties": {
        "requestbody": "{\"sku\":{\"name\":\"Standard_LRS\",\"tier\":\"Standard\"},\"kind\":\"StorageV2\",\"location\":\"canadacentral\",\"properties\":{\"supportsHttpsTrafficOnly\":false,\"allowBlobPublicAccess\":false}}",
        "eventCategory": "Administrative",
        "entity": "/subscriptions/XXXXXXXXXXXXXXXXX/resourceGroups/rg01/providers/Microsoft.Storage/storageAccounts/pp9901",
        "message": "Microsoft.Storage/storageAccounts/write",
    },

The problem is in the logic of the create_account function. I fixed it with a quick and dirty fix here: arsenicks@4ac4b9e

Not to criticize the work of someone but, I don't understand why this exist in the code in the given context:

        if bool(self.https_only):
            self.https_only = False

This force the https_only flag to false if the bool is set right ? I would be more than happy to submit a clean fix here and push it thru but @paultaiton has stated in #325 (comment) in NÉov 2020 he's already working on this module so I don't want to complicate things but this is a real problem for us.

Related issue which has been closed: #184
Related issue which seems to come from the same logic problem: #325

Let me know if you'd like me to contribute or if @paultaiton is still working on this.

Thanks.

@paultaiton
Copy link
Contributor

paultaiton commented Mar 15, 2021

@arsenicks

The cause of this is also different between create and update operations. On create, it fails to set the parameter correctly due to a code bug, but does set the parameter correctly if you run against an existing storage account ( or run the task a second time in the same playbook; the hack I did to make it work in my environment.) It is indeed one of the things I'm correcting in my rewrite of azure_rm_storageaccount (I already did fix this specific issue actually, I just haven't finished the other bits to make the PR complete.)

@arsenicks
Copy link
Author

@paultaiton Thanks for the followup, yes that's exactly what I was saying in the issue(and in the workaround in my fork). If it's already fixed on your side can I suggest to push it through and keep working on the other bits ? I mean it's a major breaking bug with a very simple solution IMO, this collection is now included in the redhat automation hub so in theory they support it and that's a big part of why we want to start using it..

I can't talk for other people, but for us, it can make the difference between management "forcing" the use of terraform vs ansible, and I wouldn't even be mad at them, it's been a year since this specific bug has been identified, there's no fix published and the original issue was closed without even acknowledging a problem. It shouldn't take too long looking at the code to find why this fails but I understand errors/misunderstanding happen. Now I raise the issue again but if you work to complete the whole thing for a few months again how long is it gonna take to push it thru a new version of the collection with the new way of distributing collections ? The last fixes I pushed in a Ansible module was under 2.8, wasn't collection but it was a minimum of 2-3 months before the code was able to reach a release.

I don't want to be rude, sorry if it sound like that, English is not my first language.. I'm just trying to explain "our" situation and the "consequences" of shipping small fixes VS a big one in few months have and I'll be more than happy to help ship it fast if you want to.

Thanks, have a great day!

@paultaiton
Copy link
Contributor

@arsenicks

I have pushed the minimum viable code change in #458 . If this is not acceptable to the Microsoft maintainers of this repo, then the fix will have to wait until my more comprehensive PR since it will be more efficient use of time (that's unfortunately slower for your specific fix.)

@arsenicks
Copy link
Author

Thank you very much for this PR! I'll try to stay around and help with this collection, my python skills need some work so I guess this could be it! Do you guy's still have regular meeting on freenode ?

@paultaiton
Copy link
Contributor

@arsenicks

I'm not aware of any meetings on Freenode, but I'd be open to it. They might have been a thing before I was around (Only been contributing in the last year.

@Fred-sun Fred-sun added bug Something isn't working has_pr PR fixes have been made medium_priority Medium priority labels Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has_pr PR fixes have been made medium_priority Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants