InterfaceMetric: Configure a static metric for an IPv4 Interface #472

cohdjn · 2020-10-04T08:49:05Z

Pull Request (PR) description

Adds InterfaceMetric to NetIPInterface to statically configure a metric on an IPv4 interface. I need this for multi-homed computers to give preference to one interface over another.

This Pull Request (PR) fixes the following issues

Task list

Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md in resource folder.
Resource parameter descriptions added/updated in schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

PlagueHO · 2020-10-05T07:32:10Z

Thanks @cohdjn - will get onto the review tomorrow

PlagueHO

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cohdjn)

CHANGELOG.md, line 16 at r1 (raw file):

- NetIPInterface
  - Added `InterfaceMetric` parameter.

Can you add link to issue - e.g.

 - Added `InterfaceMetric` parameter - fixes [Issue #473](https://github.com/PowerShell/xNetworking/issues/473).

source/DSCResources/DSC_NetIPInterface/DSC_NetIPInterface.psm1, line 433 at r1 (raw file):

        [Parameter()]
        [System.UInt32]
        $InterfaceMetric

If InterfaceMetric is specified, will AutomaticMetric be automatically set to 'Disabled'? If that is the case, is the reverse true? My thinking here is that if I have an adapter that currently has InterfaceMetric specified and AutomaticMetric is set to 'Disabled' but the desired state should be AutomaticMetric is 'Enabled' then how would this be set?

I'm wondering if we need to expose the AutomaticMetric parameter as well? What do you think?

cohdjn · 2020-10-06T11:42:53Z

I added the issue link to the CHANGELOG.md.

So here's the default state:

Get-NetIPInterface | Select-Object ifIndex,InterfaceAlias,InterfaceMetric,AutomaticMetric

ifIndex InterfaceAlias              InterfaceMetric AutomaticMetric
------- --------------              --------------- ---------------
      4 Ethernet                                 15         Enabled
      1 Loopback Pseudo-Interface 1              75         Enabled
      4 Ethernet                                 15         Enabled
      1 Loopback Pseudo-Interface 1              75         Enabled

If I issue the following nonsense command, this is what happens:

Set-NetIPInterface -InterfaceIndex 4 -AddressFamily IPv4 -AutomaticMetric Enabled -InterfaceMetric 20

Get-NetIPInterface | Select-Object ifIndex,InterfaceAlias,InterfaceMetric,AutomaticMetric

ifIndex InterfaceAlias              InterfaceMetric AutomaticMetric
------- --------------              --------------- ---------------
      4 Ethernet                                 15         Enabled
      1 Loopback Pseudo-Interface 1              75         Enabled
      4 Ethernet                                 20        Disabled
      1 Loopback Pseudo-Interface 1              75         Enabled

Notice I explicitly enabled AutomaticMetric and specified InterfaceMetric which resulted in PowerShell ignoring the AutomaticMetric altogether. Guessing Microsoft gives preference to the interface metric over the automatic metric setting and adjusts accordingly?

I suppose there's 2 ways to go here. We could have the module throw() if there are conflicting settings which strikes me as pretty ugly. Or we could call out something in the documentation saying if you specify a value for InterfaceMetric it implies AutomaticMetric is disabled by default. I think this is the better road to take since that appears to be how PowerShell handles it anyway.

AutomaticMetric is already exposed in the module. Maybe I missed something or didn't understand what you were trying to bring to my atttention?

PlagueHO · 2020-10-06T19:11:34Z

Hi @cohdjn - what I'm thinking though is that how would I set the state to -AutomaticMetric Enabled based on the currently exposed parameters.

The resource could assume that if InterfaceMetric parameter is not specified then AutomaticMetric should be set to Disabled, but that is risky DSC resource behavior- because omitting a parameter usually indicates the value should be ignored - not that a value should be set/unset.

There should always be a way to explicitly set the resource to any valid combination of values. However, only some of these combinations will be valid as you point out. So the questions I have:

Do we expose the AutomaticMetric parameter? My feeling is yes, because otherwise there is no way to set AutomaticMetric to Enabled without making an assumption (that omitting InterfaceMetric means you want to set AutomaticMetric to Enabled)
Do we detect the invalid conditions and throw an error or do we simply document the combination and leave it up to the user's knowledge. I'm OK either way here as we've done both before. Some resources contain a generic function Assert-ResourceProperty that is used to check the resource property combination is valid.

cohdjn · 2020-10-07T06:37:06Z

Hey there @PlagueHO - I think we're missing each other on the AutomaticMetric parameter. AutomaticMetric is already an exposed DSC resource in this module and can be set to either Enabled or Disabled. For example:

        NetIPInterface EthernetMetric
        {
            InterfaceAlias  = 'Ethernet'
            AddressFamily   = 'IPv4'
            AutomaticMetric = 'Disabled'
            InterfaceMetric = 20
        }

        NetIPInterface Ethernet2Metric
        {
            InterfaceAlias  = 'Ethernet 2'
            AddressFamily   = 'IPv4'
            AutomaticMetric = 'Enabled'
            InterfaceMetric = 20
        }

These resource definitions are functionally equivalent because PowerShell will essentially ignore AutomaticMetric in favor of the statically configured a value for InterfaceMetric. I think the "right" answer is to call out the combination in documentation. The behavior I showed before is how PowerShell does it so it would make sense that the DSC resource use the same behavior.

PlagueHO · 2020-10-07T09:09:24Z

Doh!!! Haha I'm sorry- you are right. I should have checked- just assumed I hadn't exposed it. So yep, I'm happy to just note the combination in the README.md for the resource.

cohdjn · 2020-10-07T13:50:43Z

Awesome! Anything else you need from me at this point?

PlagueHO

Nope - once you've added the entry to the README.md I'll merge. Thanks again!

Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

cohdjn · 2020-10-08T06:00:47Z

Which README.md do you want me to make the additions to? The main one at the top of the hierarchy or the one under NetIPInterface?

PlagueHO · 2020-10-08T22:07:37Z

The one under NetIPInterface - just add a section for ## Known Issues. Thanks again

cohdjn · 2020-10-09T06:36:23Z

Just pushed the updated README.md to the PR. This should close #473 and looking around might resolve #391 too?

PlagueHO

Hi @cohdjn - can you check the push occurred? The README.md change hasn't showed up. And yes, I think you're right about #391.

Reviewable status: complete! all files reviewed, all discussions resolved

cohdjn · 2020-10-12T04:46:54Z

That's weird. The push didn't go through on my side. I just pushed it and it looks better now.

PlagueHO

Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

PlagueHO

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cohdjn)

source/DSCResources/DSC_NetIPInterface/README.MD, line 7 at r3 (raw file):

## Known Issues

- If you define a value for `InterfaceMetric`, the `AutomaticMetric` setting is ignored.  PowerShell ignores `AutomaticMetric` when you use both arguments with the   `Set-NetIPInterface` cmdlet.

Minor nit pick: Can you reduce line length to < 80 chars (around end of sentence) and remove double spaces?

cohdjn · 2020-10-12T06:51:49Z

Minor nit pick picked. :)

PlagueHO

Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @cohdjn and @PlagueHO)

source/DSCResources/DSC_NetIPInterface/README.MD, line 7 at r3 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Minor nit pick: Can you reduce line length to < 80 chars (around end of sentence) and remove double spaces?

Doh - seems one of the double spaces turned to tipple and the line is still wrapping 😁

PlagueHO

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cohdjn)

source/DSCResources/DSC_NetIPInterface/README.MD, line 9 at r4 (raw file):

- If you define a value for `InterfaceMetric`, the `AutomaticMetric`
 setting is ignored. PowerShell ignores `AutomaticMetric` when you
 use both arguments with the   `Set-NetIPInterface` cmdlet.

Sorry- correction - it is just this like that is triple spaced :) I know it is such a nitpick 😢

cohdjn · 2020-10-13T05:52:02Z

It's all good! More nits picked! :)

PlagueHO

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

PlagueHO

Reviewable status: complete! all files reviewed, all discussions resolved

cohdjn added 2 commits October 4, 2020 01:30

Added InterfaceMetric

b0c1332

Create 4-NetIPInterface_SetInterfaceMetric.ps1

882366b

cohdjn mentioned this pull request Oct 4, 2020

interfaceMetric: Configure static metric for an IPv4 interface #473

Closed

cohdjn added 2 commits October 4, 2020 02:11

Update DSC_NetIPInterface.data.psd1

48c4ab4

Fixed filenames and changed configuration name for Appveyor tests

bc0a64e

johlju added the needs review The pull request needs a code review. label Oct 5, 2020

johlju requested a review from PlagueHO October 5, 2020 05:36

PlagueHO requested changes Oct 5, 2020

View reviewed changes

Update CHANGELOG.md

38d8784

PlagueHO approved these changes Oct 7, 2020

View reviewed changes

cohdjn added 2 commits October 8, 2020 23:20

Update README.MD

1e7306d

Merge branch 'master' of https://github.com/cohdjn/NetworkingDsc

5bcc02c

PlagueHO approved these changes Oct 9, 2020

View reviewed changes

PlagueHO approved these changes Oct 12, 2020

View reviewed changes

PlagueHO requested changes Oct 12, 2020

View reviewed changes

Update README.MD

6944699

PlagueHO reviewed Oct 13, 2020

View reviewed changes

PlagueHO requested changes Oct 13, 2020

View reviewed changes

Update README.MD

f3a0fe6

Update README.MD

834b2b9

PlagueHO approved these changes Oct 13, 2020

View reviewed changes

PlagueHO merged commit ea0d306 into dsccommunity:master Oct 13, 2020

johlju removed the needs review The pull request needs a code review. label Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InterfaceMetric: Configure a static metric for an IPv4 Interface #472

InterfaceMetric: Configure a static metric for an IPv4 Interface #472

cohdjn commented Oct 4, 2020 •

edited by PlagueHO

Loading

PlagueHO commented Oct 5, 2020

PlagueHO left a comment

cohdjn commented Oct 6, 2020

PlagueHO commented Oct 6, 2020

cohdjn commented Oct 7, 2020

PlagueHO commented Oct 7, 2020

cohdjn commented Oct 7, 2020

PlagueHO left a comment

cohdjn commented Oct 8, 2020

PlagueHO commented Oct 8, 2020

cohdjn commented Oct 9, 2020

PlagueHO left a comment

cohdjn commented Oct 12, 2020

PlagueHO left a comment

PlagueHO left a comment

cohdjn commented Oct 12, 2020

PlagueHO left a comment

PlagueHO left a comment

cohdjn commented Oct 13, 2020

PlagueHO left a comment

PlagueHO left a comment

InterfaceMetric: Configure a static metric for an IPv4 Interface #472

InterfaceMetric: Configure a static metric for an IPv4 Interface #472

Conversation

cohdjn commented Oct 4, 2020 • edited by PlagueHO Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

PlagueHO commented Oct 5, 2020

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Oct 6, 2020

PlagueHO commented Oct 6, 2020

cohdjn commented Oct 7, 2020

PlagueHO commented Oct 7, 2020

cohdjn commented Oct 7, 2020

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Oct 8, 2020

PlagueHO commented Oct 8, 2020

cohdjn commented Oct 9, 2020

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Oct 12, 2020

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Oct 12, 2020

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Oct 13, 2020

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

cohdjn commented Oct 4, 2020 •

edited by PlagueHO

Loading