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

Changes to support ipv6 in ncproxy #1452

Merged
merged 2 commits into from
Sep 29, 2022

Conversation

katiewasnothere
Copy link
Contributor

@katiewasnothere katiewasnothere commented Jul 11, 2022

  • Update proto file and dependencies
  • Update hcn code paths to return dual stack info
  • Update hcn ncproxy tests with dual stack scenarios

Signed-off-by: Kathryn Baldauf kabaldau@microsoft.com

@katiewasnothere katiewasnothere requested a review from a team as a code owner July 11, 2022 19:56
@jterry75
Copy link
Contributor

First off, super cool! Second, could you add an assert test if you dont already that the IPv6DualStack feature check support works as expected as well.

@helsaawy helsaawy self-assigned this Jul 11, 2022
@katiewasnothere katiewasnothere force-pushed the ipv6_ncproxy branch 2 times, most recently from 1442d98 to 83e4af3 Compare July 18, 2022 22:26
@katiewasnothere
Copy link
Contributor Author

IPv6DualStack

😊 Which feature check specifically are you referring to?

@jterry75
Copy link
Contributor

I mean the dual stack capability is returned as true in the opts. So that if we want to check we are sure it works. Does that make senes?

@jterry75
Copy link
Contributor

This one:

features.IPv6DualStack = isFeatureSupported(globals.Version, IPv6DualStackVersion)

@dcantah
Copy link
Contributor

dcantah commented Aug 19, 2022

Yea does make me wonder if we end up getting both a v4 and v6 addr if we should short circuit if DualStack isn't supported on the machine by checking hcn.IPv6DualStackSupported()


func TestModifyNIC_HCN_Error_InvalidArgument(t *testing.T) {
// support for setting IOV policy was added in 21H1
if osversion.Build() < osversion.V21H1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to some other tests in this file, but we don't have a syso file with the manifest info in it in this directory so I think we'd pretty much permanently skip this test as we'd spit out windows 8's build number

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you rebase for the functional test PR, you can _ "github.com/Microsoft/hcsshim/test/internal/manifest"
or, itll be imported automatically if you use "github.com/Microsoft/hcsshim/test/internal/require".Build(osversion.V21H1), itll manifest the exe automatically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should plop a syso in here as ncproxy should probably be manifested anyways

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used go-winres to add the syso files. @dcantah could you verify if it looks valid?

@dcantah
Copy link
Contributor

dcantah commented Aug 19, 2022

Merge conflict on our pal go.sum

Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general questions

cmd/ncproxy/hcn.go Show resolved Hide resolved
cmd/ncproxy/hcn.go Show resolved Hide resolved
cmd/ncproxy/hcn.go Outdated Show resolved Hide resolved
cmd/ncproxy/hcn.go Show resolved Hide resolved
cmd/ncproxy/hcn.go Show resolved Hide resolved
Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (minus go.sum issue conflict)

@dcantah
Copy link
Contributor

dcantah commented Aug 30, 2022

The supportedOS bits list Windows 7 and 8 which this definitely won't run on but not the end of the world, as long as osversion returns the right build I'm fine with this

Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go.sum conflict but lgtm after

@katiewasnothere katiewasnothere force-pushed the ipv6_ncproxy branch 2 times, most recently from d6f2743 to ba416d5 Compare August 30, 2022 19:55
@dcantah
Copy link
Contributor

dcantah commented Sep 14, 2022

@katiewasnothere Can we fix the conflicts and get this in?

 * Update proto file and dependencies
 * Update hcn code paths to return dual stack info
 * Update hcn ncproxy tests with dual stack scenarios

Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>
Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>
@katiewasnothere katiewasnothere merged commit 045973d into microsoft:main Sep 29, 2022
@katiewasnothere katiewasnothere deleted the ipv6_ncproxy branch September 29, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants