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

protocol parameter case mismatch azure_rm_securitygroup #1358

Closed
Mohammad-Atif-Khan opened this issue Dec 5, 2023 · 8 comments · Fixed by #1381
Closed

protocol parameter case mismatch azure_rm_securitygroup #1358

Mohammad-Atif-Khan opened this issue Dec 5, 2023 · 8 comments · Fixed by #1381
Labels
has_pr PR fixes have been made medium_priority Medium priority work in In trying to solve, or in working with contributors

Comments

@Mohammad-Atif-Khan
Copy link

SUMMARY

I'm trying to extract NSG rules from an NSG in Azure and then apply those to another NSG, however, the data returned by azure_rm_securitygroup_info module has the protocol attribute in all upper case while the azure_rm_securitygroup module expects it with only the first letter capitalized

ISSUE TYPE
  • Bug Report
COMPONENT NAME

azure.azcollection.azure_rm_securitygroup_info
azure.azcollection.azure_rm_securitygroup

ANSIBLE VERSION
---
ansible:
  collections:
    details: {}
    errors:
    - |-
      ERROR! Unexpected Exception, this is probably a bug: module 'lib' has no attribute 'X509_V_FLAG_CB_ISSUER_CHECK'
  version:
    details: ansible [core 2.12.5.post0]
COLLECTION VERSION
# /usr/share/ansible/collections/ansible_collections
Collection         Version
------------------ -------
azure.azcollection 1.16.0 
CONFIGURATION
CONFIG_FILE() = /etc/ansible/ansible.cfg
OS / ENVIRONMENT

Execution Environment via ansible-navigator

STEPS TO REPRODUCE
#### get NSG info using:
- name: Get NSG Info
  azure.azcollection.azure_rm_securitygroup_info:
    subscription_id: "{{ subscription_id }}" 
    resource_group: "{{ rg_name }}"
    name: "{{ nsg_name }}"
  register: nsg_info

you get output like:

TASK [Print NSG rules] ********************************************************************************************************************************
ok: [localhost] => {
    "msg": [
        {
            "access": "Allow",
            "description": "CIDR blocks",
            "destination_address_prefix": "*",
            "destination_address_prefixes": [],
            "destination_application_security_groups": null,
            "destination_port_range": null,
            "destination_port_ranges": [
                "22",
                "3389"
            ],
            "direction": "Inbound",
            "etag": "W/\"0a1b2237-3af4-477a-d3afa99e3594\"",
            "id": "/subscriptions/xxxxxxxxxxx-xxxx-xxxx-xxxxx-xxxxxxxxx/resourceGroups/RG-xxxxxxxxx-01/providers/Microsoft.Network/networkSecurityGroups/DCCLDA00DEV01-nsg/securityRules/rule1",
            "name": "rule1",
            "priority": 320,
            "protocol": "TCP",
            "provisioning_state": "Succeeded",
            "source_address_prefix": null,
            "source_address_prefixes": [
                "185.46.212.0/22"
  
            ],
            "source_application_security_groups": null,
            "source_port_range": "*",
            "source_port_ranges": []
        }
    ]
}

then try to apply it via azure.azcollection.azure_rm_securitygroup :

- name: Create NSG
  azure.azcollection.azure_rm_securitygroup:
    subscription_id: "{{ subscription_id }}"
    resource_group: "{{ rg_name }}"
    name: "{{ nsg_name }}"
    location: "{{ azure_region }}"
    rules: "{{ nsg_info.securitygroups.0.rules }}"

you get the error:

fatal: [localhost]: FAILED! => {"changed": false, "msg": "value of protocol must be one of: Udp, Tcp, Icmp, *, got: TCP found in rules"}

EXPECTED RESULTS

Either the azure.azcollection.azure_rm_securitygroup_info should return output that is compatible with input for azure.azcollection.azure_rm_securitygroup
OR
azure.azcollection.azure_rm_securitygroup should not care about the case of the protocol parameter value supplied

ACTUAL RESULTS
as explained above
@Fred-sun
Copy link
Collaborator

Fred-sun commented Dec 5, 2023

@Mohammad-Atif-Khan You are welcome to submit any questions you encounter, The protocol option is based on the Python SDK (described in the SDK with first capitalize letters). The information obtained in azure_rm_securitygroup_info.py is returned directly by the server, and we just print it out. Thank you!

@Mohammad-Atif-Khan
Copy link
Author

Mohammad-Atif-Khan commented Dec 5, 2023

hi @Fred-sun ,
Thanks for clarifying why that is the case currently.
However, I'd have to argue that the security rules exported by the info module for the same resource must be aligned with the input for its create/update module. It just makes sense.

Otherwise we will have to update every single list item returned so the case is 'appropriate'.

I suppose the best way to address this at the ansible module level is by setting the 'right' case before calling the Python SDK to create/update, thereby making it more robust and impervious to case mismatch typos.

Here are similar issues:
#726 - this one is seems to be closely related but it was addressing the schema of the returned value as per the description but may be not the case?

#1096 - this one also support my case it seems.

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors labels Dec 6, 2023
@Mohammad-Atif-Khan
Copy link
Author

Hi @Fred-sun ,
azure.azcollection.azure_rm_keyvault_info and azure.azcollection.azure_rm_keyvault are another pair that have differing schema that I just found.

the info module returns access_policies in the following format:

{
                "object_id": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx",
                "permissions": {
                    "certificates": [
                        "get",
                        "list",
                        "set",
                        "delete",
                        "backup",
                        "restore",
                        "recover",
                        "purge",
                        "update",
                        "create",
                        "import",
                        "restore"
                    ],
                    "keys": null,
                    "secrets": null,
                    "storage": null
                },
                "tenant_id": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx"
            }

while the create/update module asks for the input for access_policies on this format:

{
                "application_id": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx",
                "certificates": [
                    "get",
                    "list",
                    "set",
                    "delete",
                    "backup",
                    "restore",
                    "recover",
                    "purge",
                    "update",
                    "create",
                    "import",
                    "restore"
                ],
                "object_id": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx",
                "tenant_id": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx"
            }

can this be included in the same issue please?

@Fred-sun
Copy link
Collaborator

@Mohammad-Atif-Khan Sorry to reply you so late, #726 is because msrest will be abandoned, we manually processed the return value ourselves! #1096 is handled this way, and this is also handled to avoid the problem of character capitalization! Now our future design patterns are designed according to the SDK standards. Thank you!

@Fred-sun Fred-sun added the not a bug Not a bug label Dec 12, 2023
@Mohammad-Atif-Khan
Copy link
Author

Hi @Fred-sun ,
What I’ve asked will benefit the project, make the collection more useable. I understand your prerogative is to simply follow the SDK but I’m sure you’d agree there is room for improvement. Ansible is that additional layer of abstraction that can make the experience better than the Python SDK for Azure.

If you’re going to ignore feedback on improvement and close this issue then should this be logged as a feature request instead?

@Fred-sun
Copy link
Collaborator

@Mohammad-Atif-Khan Yes, it really helps to improve the experience, I will discuss with other developers how to solve this problem, thanks!

@Fred-sun Fred-sun reopened this Dec 14, 2023
@Fred-sun
Copy link
Collaborator

@Mohammad-Atif-Khan The SDK also returns uppercase letters, like the following!

                {
                    "access": "Allow",
                    "description": null,
                    "destination_address_prefix": "*",
                    "destination_address_prefixes": [],
                    "destination_application_security_groups": null,
                    "destination_port_range": "22",
                    "destination_port_ranges": [],
                    "direction": "Inbound",
                    "etag": "W/\"86b14ac0-1043-4d9e-8b38-24e61e3e7ad7\"",
                    "id": "/subscriptions/xxxxxxxx/resourceGroups/v-xisuRG/providers/Microsoft.Network/networkSecurityGroups/sgb57dc95226/securityRules/AllowSSH",
                    "name": "AllowSSH",
                    "priority": 101,
                    "protocol": "Tcp",
                    "provisioning_state": "Succeeded",
                    "source_address_prefix": "174.109.158.0/24",
                    "source_address_prefixes": [],
                    "source_application_security_groups": null,
                    "source_port_range": "*",
                    "source_port_ranges": []
                }

@Fred-sun
Copy link
Collaborator

Add upper letter protocol in #1381

@Fred-sun Fred-sun added has_pr PR fixes have been made and removed not a bug Not a bug labels Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has_pr PR fixes have been made medium_priority Medium priority work in In trying to solve, or in working with contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants