Issue metadata
Sign in to add a comment
|
shill: setting HostName property is not propagated to DHCP request correctly |
||||||||||||||||||||||
Issue descriptionIn ChromeOS Enterprise we have introduced a policy that allows device admins to set Hostname property so that they can later identify devices on routers. Relevant code calls sets shill manager property kHostNameProperty to relevant value, but this value is not propagated to DHCP requests correctly. It worked in the initial implementation, but it seems that it was broken during some later refactorings: https://chromium-review.googlesource.com/c/chromium/src/+/764168 Also, the DHCPv6Config::GetFlags does not seem to pass the hostname to dhclient, while DHCPv4Config::GetFlags does. Probably the hostname should be passed in common DHCPConfig::GetFlags implementation.
,
Jun 12 2018
Hey guys, I'm concerned that this is now unowned, given that it's a regression that impacts some of our customers. Is this something you can address in the M68 timeframe?
,
Jun 12 2018
It's untriaged, not unowned. I don't have much context of this HostName property. What Chrome OS build did you first observe the regression? Is there any integration test that covers such a feature?
,
Jun 12 2018
Re #1: are you referring to the following refactoring CLs? https://chromium-review.googlesource.com/c/chromiumos/platform2/+/275274 https://chromium-review.googlesource.com/c/chromiumos/platform2/+/276649
,
Jun 12 2018
Looks like that the behavior may have changed since this CL: https://android-review.googlesource.com/c/platform/system/connectivity/shill/+/181040/3/device.cc#735
,
Jun 13 2018
I believe that this is the first time that this feature is actually used (at least from Chrome side), so things might work incorrectly for quite a while. I could not find any shill integration tests that covers such a feature. In the Chrome tests cover part from getting value via policy to setting HostName value via DBUS call.
,
Jun 13 2018
,
Jun 14 2018
,
Jun 14 2018
I don't much of the context about this particular feature. Based on what I've learned from the code history, the "HostName" property was replaced by "DHCPProperties.Hostname" in the following AOSP CL on Nov 05, 2015. I didn't find any related changes on the CrOS side, which explained the issue here. https://android-review.googlesource.com/c/platform/system/connectivity/shill/+/181040/3/doc/manager-api.txt Given that shill is no longer used on AOSP and the original "HostName" property hasn't been really used on the Chrome side according to comment #6, we can simply update the code to use the new property. That should address the issue in DHCPv4Config. The DHCPv6Config code never used either property, but we can add similar code there.
,
Jun 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/system_api/+/557c37af39f5195d80aaad53c3a06b5ec804da95 commit 557c37af39f5195d80aaad53c3a06b5ec804da95 Author: Ben Chan <benchan@chromium.org> Date: Sat Jun 16 01:50:59 2018 shill: add DHCPProperty.Hostname and DHCPProperty.VendorClass property https://android-review.googlesource.com/c/platform/system/connectivity/shill/+/181040 introduced two Manager properties, "DHCPProperty.Hostname" and "DHCPProperty.VendorClass", and removed the "HostName" property. BUG= chromium:852035 TEST=Remote trybot runs. Change-Id: I8ae6c444698cbb6c42472158ca931ecaf19299fe Reviewed-on: https://chromium-review.googlesource.com/1101913 Commit-Ready: Ben Chan <benchan@chromium.org> Tested-by: Ben Chan <benchan@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/557c37af39f5195d80aaad53c3a06b5ec804da95/dbus/shill/dbus-constants.h
,
Jun 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/17b319ed9549b06a07eec17c95ba2d090792829f commit 17b319ed9549b06a07eec17c95ba2d090792829f Author: Ben Chan <benchan@chromium.org> Date: Sat Jun 16 05:16:48 2018 shill: fix passing host name property to DHCP v4 request https://android-review.googlesource.com/c/platform/system/connectivity/shill/+/181040 replaced the "HostName" property in the Manager interface with a new "DHCPProperty.Hostname" property. However, the migration was partially done. The Manager.SetProperty D-Bus API still accepts the deprecated HostName property but not the DHCPProperty.Hostname property. Also, the DHCPProperty.Hostname property is not stored to or loaded from the default profile. When merging the DHCP properties held by Manager and a service, the host name isn't propagated correctly, which results in shill not passing the `-h <hostname>` option to dhcpcd under IPv4. BUG= chromium:852035 CQ-DEPEND=CL:1101913 TEST=Tested the following: 1. Set the DHCPProperty.Hostname property dbus-send --system --print-reply \ --dest=org.chromium.flimflam / org.chromium.flimflam.Manager.SetProperty \ string:DHCPProperty.Hostname string:test-hostname 2. Restart shill 3. Verify that DHCPProperty.HostName=test-hostname is set in the global section of /var/cache/shill/default.profile. 4. Connect the device to Ethernet. 5. Verify that dhcpcd is running with `-h test-hostname` option. Change-Id: Ib94a3b03a871beb03c5061f593fbcbd1f3234788 Reviewed-on: https://chromium-review.googlesource.com/1102152 Commit-Ready: Ben Chan <benchan@chromium.org> Tested-by: Ben Chan <benchan@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> [modify] https://crrev.com/17b319ed9549b06a07eec17c95ba2d090792829f/manager.cc [modify] https://crrev.com/17b319ed9549b06a07eec17c95ba2d090792829f/default_profile.cc
,
Jun 18 2018
,
Jun 18 2018
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 18 2018
Re #12: It's too early to request a M68 merge as this issue hasn't been marked fixed. The chrome code still references the old property. I have two more pending chrome CLs to fix that: https://chromium-review.googlesource.com/c/chromium/src/+/1103708 https://chromium-review.googlesource.com/c/chromium/src/+/1104312 Overall, I'm not sure if the current mechanism for passing down host name is well tested or if it's the right mechanism for your use cases. From looking at the code (again, I don't have any previous context about this feature, so the source code is my only source of truth), the host name property specified through shill's Manager interface is persistent to the default profile of shill and is applied globally to any service, rather than individual service or service of a specific type. That may be undesirable in some cases.
,
Jun 18 2018
We can merge when it all works.
,
Jun 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bfad34f4f4251c94ae82b8b602d293968c9927f9 commit bfad34f4f4251c94ae82b8b602d293968c9927f9 Author: Ben Chan <benchan@chromium.org> Date: Mon Jun 18 18:46:40 2018 Fix passing host name property through shill Manager interface. https://android-review.googlesource.com/c/platform/system/connectivity/shill/+/181040 replaced the "HostName" property in the shill Manager interface with a new "DHCPProperty.Hostname" property. However, the migration was partially done, which has been fixed by CL:1102152. This CL updates the code on the Chrome side accordingly. Bug: 852035 Change-Id: Ib0d7fc7e82f13b9f77cda13324702f610d952f61 Reviewed-on: https://chromium-review.googlesource.com/1104312 Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Commit-Queue: Ben Chan <benchan@chromium.org> Cr-Commit-Position: refs/heads/master@{#568092} [modify] https://crrev.com/bfad34f4f4251c94ae82b8b602d293968c9927f9/chromeos/network/shill_property_handler.cc
,
Jun 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/system_api/+/2159006dfef353443927b2bdad641f39d475f244 commit 2159006dfef353443927b2bdad641f39d475f244 Author: Ben Chan <benchan@chromium.org> Date: Wed Jun 20 17:28:22 2018 shill: remove deprecated kHostNameProperty The kHostNameProperty has been deprecated (CL:1102152, CL:1104312). BUG= chromium:852035 TEST=Run remote trybots. Change-Id: If5baab891bfe65a7d8e5e2ab924cca6cb96f9eaa Reviewed-on: https://chromium-review.googlesource.com/1105484 Commit-Ready: Ben Chan <benchan@chromium.org> Tested-by: Ben Chan <benchan@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/2159006dfef353443927b2bdad641f39d475f244/dbus/shill/dbus-constants.h
,
Jun 25 2018
Hi Ben, is this getting merged into m68? We have acustomer that is looking to test the hostname feature ASAP in order to test out a big use case for Chrome. Thanks!
,
Jun 25 2018
Re #18: Can the customer test with M69 dev channel to confirm the issue is addressed before we merge into M68? Marking this bug as fixed so we can request for merge for M68.
,
Jun 25 2018
,
Jun 25 2018
This bug requires manual review: M68 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 25 2018
Is it available in M69 dev today? I have another customer that can test as well.
,
Jun 25 2018
The Chrome side changed landed in 69.0.3465.0, but unfortunately it hasn't yet reached the dev channel yet. From what I checked this morning, Chrome is still stuck at 69.0.3464.0 in dev channel.
,
Jun 26 2018
We can review for 68 merge after this is verified on 69.
,
Jun 27 2018
Latest canary build - Chrome OS 10822.0.0 (Chrome 69.0.3474.0) should have the fix.
,
Jun 28 2018
I can confirm this worked in my test domain. Value in ASSET_ID now appears as the identifier for the device on the DHCP server.
,
Jun 29 2018
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/system_api/+/bd982946fab9dbe955dc521827aa5c5d34f520bb commit bd982946fab9dbe955dc521827aa5c5d34f520bb Author: Ben Chan <benchan@chromium.org> Date: Fri Jun 29 18:58:20 2018 shill: add DHCPProperty.Hostname and DHCPProperty.VendorClass property https://android-review.googlesource.com/c/platform/system/connectivity/shill/+/181040 introduced two Manager properties, "DHCPProperty.Hostname" and "DHCPProperty.VendorClass", and removed the "HostName" property. BUG= chromium:852035 TEST=Remote trybot runs. Change-Id: I8ae6c444698cbb6c42472158ca931ecaf19299fe Reviewed-on: https://chromium-review.googlesource.com/1101913 Commit-Ready: Ben Chan <benchan@chromium.org> Tested-by: Ben Chan <benchan@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> (cherry picked from commit 557c37af39f5195d80aaad53c3a06b5ec804da95) Reviewed-on: https://chromium-review.googlesource.com/1120757 Reviewed-by: Ben Chan <benchan@chromium.org> Commit-Queue: Ben Chan <benchan@chromium.org> [modify] https://crrev.com/bd982946fab9dbe955dc521827aa5c5d34f520bb/dbus/shill/dbus-constants.h
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/aosp/platform/system/connectivity/shill/+/a95dce9d45c953b1ae9235c12caa49e38184901a commit a95dce9d45c953b1ae9235c12caa49e38184901a Author: Ben Chan <benchan@chromium.org> Date: Fri Jun 29 18:57:36 2018 shill: fix passing host name property to DHCP v4 request https://android-review.googlesource.com/c/platform/system/connectivity/shill/+/181040 replaced the "HostName" property in the Manager interface with a new "DHCPProperty.Hostname" property. However, the migration was partially done. The Manager.SetProperty D-Bus API still accepts the deprecated HostName property but not the DHCPProperty.Hostname property. Also, the DHCPProperty.Hostname property is not stored to or loaded from the default profile. When merging the DHCP properties held by Manager and a service, the host name isn't propagated correctly, which results in shill not passing the `-h <hostname>` option to dhcpcd under IPv4. BUG= chromium:852035 CQ-DEPEND=CL:1101913 TEST=Tested the following: 1. Set the DHCPProperty.Hostname property dbus-send --system --print-reply \ --dest=org.chromium.flimflam / org.chromium.flimflam.Manager.SetProperty \ string:DHCPProperty.Hostname string:test-hostname 2. Restart shill 3. Verify that DHCPProperty.HostName=test-hostname is set in the global section of /var/cache/shill/default.profile. 4. Connect the device to Ethernet. 5. Verify that dhcpcd is running with `-h test-hostname` option. Change-Id: Ib94a3b03a871beb03c5061f593fbcbd1f3234788 Reviewed-on: https://chromium-review.googlesource.com/1102152 Commit-Ready: Ben Chan <benchan@chromium.org> Tested-by: Ben Chan <benchan@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> (cherry picked from commit 17b319ed9549b06a07eec17c95ba2d090792829f) [modify] https://crrev.com/a95dce9d45c953b1ae9235c12caa49e38184901a/manager.cc [modify] https://crrev.com/a95dce9d45c953b1ae9235c12caa49e38184901a/default_profile.cc
,
Jun 29 2018
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e2c6a89054bbc011abbf9ec07578f4adf0dbdbb5 commit e2c6a89054bbc011abbf9ec07578f4adf0dbdbb5 Author: Ben Chan <benchan@chromium.org> Date: Fri Jun 29 19:36:04 2018 Fix passing host name property through shill Manager interface. https://android-review.googlesource.com/c/platform/system/connectivity/shill/+/181040 replaced the "HostName" property in the shill Manager interface with a new "DHCPProperty.Hostname" property. However, the migration was partially done, which has been fixed by CL:1102152. This CL updates the code on the Chrome side accordingly. (back ported from commit bfad34f4f4251c94ae82b8b602d293968c9927f9) Bug: 852035 Change-Id: Ib0d7fc7e82f13b9f77cda13324702f610d952f61 Reviewed-on: https://chromium-review.googlesource.com/1104312 Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Commit-Queue: Ben Chan <benchan@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#568092} Reviewed-on: https://chromium-review.googlesource.com/1121038 Cr-Commit-Position: refs/branch-heads/3440@{#571} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/e2c6a89054bbc011abbf9ec07578f4adf0dbdbb5/chromeos/network/shill_property_handler.cc
,
Jul 12
I can confirm this is tested and working in my lab environment as well. Can we understand the timing of when we can see this merged in?
,
Jul 13
The fixes already merged back to R68 (see comment #31) and should already be available in beta channel (Chrome OS 10718.50.0 / Chrome 68.0.3440.59) |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by benchan@chromium.org
, Jun 12 2018Owner: ----