Skip to content

Conversation

@mostolog
Copy link

Proxying is enabled via the network label com.docker.network.bridge.ndp_proxy_interface=

Closes #1159
Overrides #1316

- Proxying is enabled via the network label com.docker.network.bridge.ndp_proxy_interface=<interface>

fixes moby#1159

Signed-off-by: Zvi "CtrlZvi" Effron <[email protected]>
}

// Get current NDP all proxying setup
ndpProxyDataAll, err := ioutil.ReadFile(ndpProxyConfAll)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see in most of bridge code we first check the flag value, and we set it to 1 only if needed.
I know we would do only one write if we set it regardless, but I think it is better to be consistent and check first.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean to change https://github.com/mostolog/libnetwork/blob/907282e5195e7bfaf507da0375bcaaf95a618603/drivers/bridge/setup_ipv6.go#L139 ?:

    if ndpProxyConfPerm[0] != '1' {
        if err := ioutil.WriteFile(ndpProxyConfPath, []byte{'1', '\n'}, ndpProxyConfPerm); err != nil {
            logrus.Warnf("Unable to enable NDP proxying for interface %s: %v", config.NDPProxyInterface,err)
        }
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, to follow same logic we do for the default, for example

https://github.com/mostolog/libnetwork/blob/907282e5195e7bfaf507da0375bcaaf95a618603/drivers/bridge/setup_ipv6.go#L125

First read the value, then set it if not already set.

Copy link
Author

Choose a reason for hiding this comment

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

It may be solved now

@aboch
Copy link
Contributor

aboch commented Mar 10, 2017

Thanks @mostolog for carrying this forward.
Thanks for carrying the original commit.
Regarding the extra 3 commits of yours, please squash them into one, and sign it with your id.

@aboch
Copy link
Contributor

aboch commented Mar 16, 2017

You now also need a rebase

@aboch
Copy link
Contributor

aboch commented Mar 17, 2017

Please fetch latest from upstream and rebase your changes. Otherwise we can't merge this.

@mostolog
Copy link
Author

mostolog commented Mar 20, 2017

Already did git pull --rebase origin master and says it's up to date.

@aboch
Copy link
Contributor

aboch commented Mar 20, 2017

Do this

git fetch <remote>
git rebase <remote>/master

(As remote use the one which points to the github.com/docker/libnetwork repo, check with git remote -v)

@mostolog
Copy link
Author

Current branch patch-1 is up to date.

return fmt.Errorf("Unable to enable NDP proxying for interface %s: %v", config.NDPProxyInterface,err)
}
// Enable NDP proxying only if it is not already enabled
if ndpProxyConfPerm[0] != '1' {
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't build

@aboch
Copy link
Contributor

aboch commented Apr 10, 2017

This has been in non-mergeable state for too long.

I am taking care of the merge conflicts and the modification of setupNDPProxying() to only enable the feature on the user specified host interface in #1714

@mostolog mostolog closed this Aug 24, 2017
@mostolog mostolog deleted the patch-1 branch August 24, 2017 12:49
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