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

Auto update ipv6 router id based on nodename #215

Merged
merged 5 commits into from Mar 7, 2019

Conversation

Projects
None yet
3 participants
@roshanirathi
Copy link

commented Mar 6, 2019

This code allows to set router id for ipv6 only systems based on the node name.
Earlier either the ip address of the node or if the env variable for the router id was set, then that used to be set as router id.
This piece of code allows us to hash the node name and create a new ip to be used as router id if the env variable for router id is set to "HASH"

roshanirathi added some commits Mar 6, 2019

@fasaxc
Copy link
Member

left a comment

Looks like it'll work, just a few minor comments.

@@ -6,7 +6,9 @@ include "bird6_ipam.cfg";
{{- $node_ip6_key := printf "/host/%s/ip_addr_v6" (getenv "NODENAME")}}{{$node_ip6 := getv $node_ip6_key}}
{{- $router_id := getenv "CALICO_ROUTER_ID" ""}}

router id {{if ne "" ($router_id)}}{{$router_id}}{{else}}{{$node_ip}}{{end}}; # Use IPv4 address since router id is 4 octets, even in MP-BGP
{{- $node_name := getenv "NODENAME"}}
router id {{if eq "HASH" ($router_id)}}{{hash $node_name}}{{else}}{{if ne "" ($router_id)}}{{$router_id}}{{else}}{{$node_ip}}{{end}}{{end}}; # Use IPv4 address since router id is 4 octets, even in MP-BGP

This comment has been minimized.

Copy link
@fasaxc

fasaxc Mar 6, 2019

Member
  • I think our other env var special values are lower case so please use "hash" instead of "HASH".
  • Let's make the comment depend on the mode so it's easier to diagnose where it came from.

Tip: if you use this type of paren: {{- or -}} then whitespace will be suppresses on that side of the embedded template block. So, you can do this to make it easier to see the template control flow without affecting the output:

router id {{ if ... -}}
    {{hash $node_name}} # new comment about hash
{{- else -}}
    ...
{{ end }}

This comment has been minimized.

Copy link
@roshanirathi
@@ -33,6 +35,7 @@ func newFuncMap() map[string]interface{} {
m["fileExists"] = isFileExist
m["base64Encode"] = Base64Encode
m["base64Decode"] = Base64Decode
m["hash"] = hashNodeName

This comment has been minimized.

Copy link
@fasaxc

fasaxc Mar 6, 2019

Member

Since the output format of this hash is a bit odd (an IP address), how about making the function name hashToIPv4?

This comment has been minimized.

Copy link
@roshanirathi
@@ -42,6 +45,20 @@ func addFuncs(out, in map[string]interface{}) {
}
}

// hashNodeName hashes the node name for ipv6 only systems.
// This hash is then converted to ipv4 IP to be used as router id.
func hashNodeName(nodeName string) string {

This comment has been minimized.

Copy link
@fasaxc

fasaxc Mar 6, 2019

Member

Nit: this function is more generic than the comment suggests. It hashes any string and converts the result to an IPv4 address. I'd call the function hashToIPv4 as above and remove the mention of nodeName.

This comment has been minimized.

Copy link
@roshanirathi
func hashNodeName(nodeName string) string {
hash := sha256.New()
hash.Write([]byte(nodeName))
ipv6 := hash.Sum(nil)

This comment has been minimized.

Copy link
@fasaxc

fasaxc Mar 6, 2019

Member

Nit: The result of Sum() is the value of the hash, suggest you rename ipv6 to hashBytes

This comment has been minimized.

Copy link
@roshanirathi

This comment has been minimized.

Copy link
@fasaxc

fasaxc Mar 7, 2019

Member

Think you missed this change?

@fasaxc
Copy link
Member

left a comment

Looking good, just a couple of nits.

@@ -33,6 +35,7 @@ func newFuncMap() map[string]interface{} {
m["fileExists"] = isFileExist
m["base64Encode"] = Base64Encode
m["base64Decode"] = Base64Decode
m["hash"] = hashToIPv4

This comment has been minimized.

Copy link
@fasaxc

fasaxc Mar 7, 2019

Member

Nit: please can you use the same name for the template function as you use for the golang function. I.e. hash["hashToIPv4"] = hashToIPv4 (you'll need to change the template too).

This comment has been minimized.

Copy link
@roshanirathi
@@ -42,6 +45,20 @@ func addFuncs(out, in map[string]interface{}) {
}
}

// hashToIPv4 hashes a string for ipv6 only systems.

This comment has been minimized.

Copy link
@fasaxc

fasaxc Mar 7, 2019

Member

Comment slightly out of date after the change. Suggest // hashToIPv4 hashes the given string and formats the resulting 4 bytes as an IPv4 address.

This comment has been minimized.

Copy link
@roshanirathi

roshanirathi added some commits Mar 7, 2019

@fasaxc

fasaxc approved these changes Mar 7, 2019

@fasaxc fasaxc merged commit dd0e5ec into projectcalico:master Mar 7, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

roshanirathi added a commit to roshanirathi/confd that referenced this pull request Mar 7, 2019

Merge pull request projectcalico#215 from roshanirathi/ipv6-router-id…
…-autoupdate

Auto update ipv6 router id based on nodename

@caseydavenport caseydavenport added this to the Calico v3.7.0 milestone Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.