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

adds optional loopbackDSR argument to cni conf #60

Merged
merged 2 commits into from
May 10, 2021

Conversation

daschott
Copy link
Contributor

This is intended to address kubernetes-sigs/sig-windows-tools#119

@nagiesek
Copy link
Contributor

nagiesek commented Mar 3, 2021

Tests pass okay?

@daschott
Copy link
Contributor Author

I validated the changes on an active Kubernetes cluster, but haven't run the tests yet. Will let you know when I have a successful test pass.

cni/cni.go Show resolved Hide resolved
network/policy.go Outdated Show resolved Hide resolved
@daschott
Copy link
Contributor Author

Updated with suggestions and tested the latest changes -- please review

@vikas-bh
Copy link
Contributor

vikas-bh commented May 2, 2021

LGTM

func (config *NetworkConfig) GetNetworkInfo(podNamespace string) *network.NetworkInfo {
func (config *NetworkConfig) GetNetworkInfo(podNamespace string) (ninfo *network.NetworkInfo, err error) {
if config.OptionalFlags.LoopbackDSR {
if err := hcn.DSRSupported(); err != nil {

Choose a reason for hiding this comment

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

(I am not well versed in golang, so bear with me) Why is err := hcn.DSRSupported(); included in the if condition? does this if condition basically come down to err != nil and have you tested that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I actually encountered this condition on RS5 by using older hcsshim version that did not include microsoft/hcsshim#848 (it was using the old DSR version check, hence it failed on RS5).

This inline assignment and error check in the If statement is a common idiom in Golang, e.g. see: https://golang.org/doc/effective_go#if

Choose a reason for hiding this comment

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

awesome, thanks David! maybe I will use this in the future :)

@daschott daschott force-pushed the daschott/loopbackDSR branch 2 times, most recently from f972fe1 to 6cdcf01 Compare May 8, 2021 00:19
@daschott daschott force-pushed the daschott/loopbackDSR branch from 6cdcf01 to b9911ee Compare May 8, 2021 00:32
@elweb9858
Copy link

lgtm

@daschott daschott merged commit 340c5fe into microsoft:master May 10, 2021
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.

6 participants