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

Incorrect parsing of media type in OpenBSD #49128

Closed
wants to merge 1 commit into from

Conversation

@jpdasma
Copy link
Contributor

@jpdasma jpdasma commented Nov 26, 2018

SUMMARY

Fixes #49051

Based on the issue, there is a possibility that the output of ifconfig for OpenBSD is similar to below which would cause to show a closing parenthesis for media_type.

$ ifconfig -Aa 
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 32768
        index 3 priority 0 llprio 3
        groups: lo
        inet6 ::1 prefixlen 128
        inet6 fe80::1%lo0 prefixlen 64 scopeid 0x3
        inet 127.0.0.1 netmask 0xff000000
vmx0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
        lladdr 00:50:56:ba:d4:2e
        index 1 priority 0 llprio 3
        groups: egress
        media: Ethernet autoselect (10GbaseT)
        status: active
        inet 192.168.XXX.YYY netmask 0xffffff00 broadcast 192.168.XXX.YYY
enc0: flags=0<>
        index 2 priority 0 llprio 3
        groups: enc
        status: active
pflog0: flags=141<UP,RUNNING,PROMISC> mtu 33136
        index 4 priority 0 llprio 3
        groups: pflog
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

generic_bsd.py

@ansibot
Copy link
Contributor

@ansibot ansibot commented Nov 26, 2018

Hi @jpdasma, thank you for submitting this pull-request!

click here for bot help

@gundalow
Copy link
Contributor

@gundalow gundalow commented Nov 27, 2018

Could you please include some before/after output?

@bcoca bcoca self-assigned this Nov 27, 2018
@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Nov 28, 2018

hi, sorry, will add it later

@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Nov 28, 2018

I'll have to wait for @karlism to test. Based on his output:

$ ifconfig -Aa 
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 32768
        index 3 priority 0 llprio 3
        groups: lo
        inet6 ::1 prefixlen 128
        inet6 fe80::1%lo0 prefixlen 64 scopeid 0x3
        inet 127.0.0.1 netmask 0xff000000
vmx0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
        lladdr 00:50:56:ba:d4:2e
        index 1 priority 0 llprio 3
        groups: egress
        media: Ethernet autoselect (10GbaseT)
        status: active
        inet 192.168.XXX.YYY netmask 0xffffff00 broadcast 192.168.XXX.YYY
enc0: flags=0<>
        index 2 priority 0 llprio 3
        groups: enc
        status: active
pflog0: flags=141<UP,RUNNING,PROMISC> mtu 33136
        index 4 priority 0 llprio 3
        groups: pflog

He is using vmx which makes the media_type to show (10GbaseT) as opposed to my server which uses em and shows (1000baseT full-duplex).

Actually I noticed another bug. If we manually set the media type, e.g.

# ifconfig em0 media 10baseT mediaopts full-duplex
# ifconfig 
<snip>
em0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
        lladdr 52:54:00:a1:86:46
        index 1 priority 0 llprio 3
        groups: egress
        media: Ethernet 10baseT full-duplex (1000baseT full-duplex)
        status: active
        inet 192.168.122.222 netmask 0xffffff00 broadcast 192.168.122.255
<\snip>

ansible would show this

<snip>
        "ansible_em0": {
            "device": "em0",
            "flags": [
                "UP",
                "BROADCAST",
                "RUNNING",
                "SIMPLEX",
                "MULTICAST"
            ],
            "ipv4": [
                {
                    "address": "192.168.122.222",
                    "broadcast": "192.168.122.255",
                    "netmask": "255.255.255.0",
                    "network": "192.168.122.0"
                }
            ],
            "ipv6": [],
            "macaddress": "52:54:00:a1:86:46",
            "media": "Ethernet",
            "media_options": [],
            "media_select": "10baseT",
            "media_type": "ull-duplex",
            "mtu": "1500",
            "status": "active",
            "type": "ether"
        },
<\snip>

Notice media_opts, media_select and media_type is wrong. Ansible would only be right if it is using autoselect as media type.

@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Nov 28, 2018

@gundalow should I try to fix it here? or should I open another pull request?

@gundalow
Copy link
Contributor

@gundalow gundalow commented Nov 28, 2018

@jpdasma fix in this PR would be good

@gundalow gundalow changed the title Remove possible parenthesis for media type in OpenBSD [WIP] Remove possible parenthesis for media type in OpenBSD Nov 29, 2018
@gundalow
Copy link
Contributor

@gundalow gundalow commented Nov 29, 2018

I've updated the title to be [WIP] (Work In Progress), please update that once you've added the other fix

@ansibot ansibot added the WIP label Nov 29, 2018
@jpdasma jpdasma force-pushed the jpdasma:bsd-mediatype branch Nov 29, 2018
@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Nov 29, 2018

I have pushed a new commit.

openbsd$ ifconfig -aA 
em0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
        lladdr 52:54:00:a1:86:46
        index 1 priority 0 llprio 3
        groups: egress
        media: Ethernet 10baseT full-duplex (1000baseT full-duplex)
        status: active
        inet 192.168.122.222 netmask 0xffffff00 broadcast 192.168.122.255

Ansible will now return the following:

            "media": "Ethernet",
            "media_options": [
                "full-duplex"
            ],
            "media_select": "10baseT full-duplex",
            "media_type": "1000baseT",

For media_select, instead of autoselect, I processed it as what I "selected" via ifconfig em0 media 10baseT mediaopts full-duplex . I'm not really sure if that is the intended behavior. For the media_type and media_opts, I parsed the one inside the parenthesis.

I also need the original reporter to test this change if it fixed his issue.

@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Nov 29, 2018

Also, I changed it so that only OpenBSD is affected. I'm not sure if this bug also affects NetBSD. I'm gonna need to spinup a NetBSD image to test.

I have updated the title to reflect the other issue.

@jpdasma jpdasma changed the title [WIP] Remove possible parenthesis for media type in OpenBSD [WIP] Incorrect parsing of media type in OpenBSD Nov 29, 2018
@ansibot
Copy link
Contributor

@ansibot ansibot commented Nov 29, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/module_utils/facts/network/openbsd.py:59:37: bad-whitespace Exactly one space required after assignment             current_if['media_type'] =  media_type_opts_list[0]                                      ^

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/module_utils/facts/network/openbsd.py:59:39: E222 multiple spaces after operator

click here for bot help

@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Nov 30, 2018

I checked with NetBSD, and it seems that parse_media_line is also broken (this time using default settings, it would seem to work if i manually configure ifconfig wm0 media type):

$ fconfig -a
localhost# ifconfig -a
wm0: flags=0x8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
        capabilities=2bf80<TSO4,IP4CSUM_Rx,IP4CSUM_Tx,TCP4CSUM_Rx>
        capabilities=2bf80<TCP4CSUM_Tx,UDP4CSUM_Rx,UDP4CSUM_Tx,TCP6CSUM_Tx>
        capabilities=2bf80<UDP6CSUM_Tx>
        enabled=0
        ec_capabilities=7<VLAN_MTU,VLAN_HWTAGGING,JUMBO_MTU>
        ec_enabled=0
        address: 52:54:00:03:f6:b1
        media: Ethernet autoselect (1000baseT full-duplex)
        status: active
        inet 192.168.122.81/24 broadcast 192.168.122.255 flags 0x0
        inet6 fe80::cce4:8674:9130:5946%wm0/64 flags 0x0 scopeid 0x1

Now using ansible

            "ipv4": [
                {
                    "address": "192.168.122.81",
                    "broadcast": "192.168.122.255",
                    "netmask": "255.255.255.0",
                    "network": "192.168.122.0"
                }
            ],
            "ipv6": [
                {
                    "address": "fe80::cce4:8674:9130:5946%wm0",
                    "prefix": "64",
                    "scope": "0x1"
                }
            ],
            "macaddress": "unknown",
            "media": "Ethernet",
            "media_options": [
                "(1000baseT"
            ],
            "media_type": "autoselect",

This is using ansible version

ansible 2.7.2
  config file = None
  configured module search path = ['/home/jpdasma/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.6/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.6.7 (default, Nov  3 2018, 21:32:46) [GCC 8.2.0]

This behavior is unrelated to the OpenBSD one.

@jpdasma jpdasma force-pushed the jpdasma:bsd-mediatype branch Nov 30, 2018
@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Nov 30, 2018

I could try to fix the NetBSD issue here. Although I have to clarify which would be identified as:

  • media_select
  • media_type
  • media_options

For OpenBSD, should I disregard the one's inside the parenthesis when setting manually? and just use that when the configuration is ifconfig em0 media autoselect? If that is the case, what is the variable media_select for?

@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Nov 30, 2018

It seems FreeBSD network parsing is also slightly broken:

$ ifconfig -a
vtnet0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
	options=6c07bb<RXCSUM,TXCSUM,VLAN_MTU,VLAN_HWTAGGING,JUMBO_MTU,VLAN_HWCSUM,TSO4,TSO6,LRO,VLAN_HWTSO,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
	ether 16:36:48:74:fb:00
	hwaddr 16:36:48:74:fb:00
	inet6 fe80::1436:48ff:fe74:fb00%vtnet0 prefixlen 64 scopeid 0x1 
	inet <snip> netmask 0xfffff000 broadcast <snip>
	inet 10.10.0.5 netmask 0xffff0000 broadcast 10.10.255.255 
	nd6 options=21<PERFORMNUD,AUTO_LINKLOCAL>
	media: Ethernet 10Gbase-T <full-duplex>
	status: active

Ansible returns:

            "media": "Ethernet",
            "media_select": "10Gbase-T",
            "media_type": "full-duplex>",

Notice the '>' at the end of type. EDIT: Also media_type should be "10Gbase-T"

Is there any documentation on what is defined as media_select, media_type, and media_options?

@jpdasma
Copy link
Contributor Author

@jpdasma jpdasma commented Jan 24, 2019

I'm still planning on continuing this, although I'm still confused on what must be media_type, and media_select, it seems this is selected differently across the BSDs

@ansibot
Copy link
Contributor

@ansibot ansibot commented Feb 1, 2019

@ansibot
Copy link
Contributor

@ansibot ansibot commented Feb 17, 2019

@ansibot
Copy link
Contributor

@ansibot ansibot commented Jun 1, 2019

@grayed
Copy link

@grayed grayed commented Dec 14, 2019

It looks like the parser was broken from the start (see the e2643cb55f0 revision). I think it should be re-done from scratch, considering the following actual format:

media: TYPE MEDIA [mediaopt OPTIONS] [mode MODE] (SELECTEDMEDIA [mediaopt SELECTEDOPTIONS] [mode SELECTEDMODE])

TYPE is e.g. 'Ethernet', 'IEEE802.11'.

MEDIA is either 'autoselect', or thing like '10BaseT'

OPTIONS could be something like 'full-duplex', but missing when it's default for corresponding MEDIA. This means, in particular, that mediaopt full-duplex will be missing for '1000BaseT'.

MODE is applicable to e.g. 802.11 devices, where it may be 'auto', or '11b', '11n' etc.

All SELECTED* represent actual state, so no 'autoselect', 'auto' etc. here. SELECTEDMEDIA would be 'none' in case of missing link.

I don't think that fine-grained is really needed here, so we can simplify the structure above to the following:

media: TYPE MEDIA (SELECTEDMEDIA)

and be done with it. If this is considered the way to go, I'll cook a patch for generic_bsd.py; no OS-specific handling will be needed, ever.

@Akasurde
Copy link
Member

@Akasurde Akasurde commented Jul 22, 2020

@grayed @jpdasma Are you working on this? Thanks.

@ansibot ansibot added pre_azp and removed stale_ci labels Dec 5, 2020
@bcoca bcoca changed the title [WIP] Incorrect parsing of media type in OpenBSD Incorrect parsing of media type in OpenBSD Mar 12, 2021
@bcoca bcoca removed the WIP label Mar 12, 2021
@samdoran
Copy link
Member

@samdoran samdoran commented Mar 15, 2021

/azp run

@azure-pipelines
Copy link

@azure-pipelines azure-pipelines bot commented Mar 15, 2021

Azure Pipelines successfully started running 1 pipeline(s).
@samdoran
Copy link
Member

@samdoran samdoran commented Mar 17, 2021

The PR looks abandoned at this point, is lacking tests, and a changelog fragment so I'm going to close it.

@samdoran samdoran closed this Mar 17, 2021
@ansible ansible locked and limited conversation to collaborators Apr 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants