-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Replace ippool filters in BIRD template with golang funcs #11759
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 from all commits
9d751de
30a872c
a7bb869
05caa83
5ebcf32
c8c378d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,8 @@ | ||
| # Generated by confd | ||
| function reject_disabled_pools () | ||
| { | ||
| {{range ls "/v1/ipam/v6/pool"}}{{$data := json (getv (printf "/v1/ipam/v6/pool/%s" .))}} | ||
| {{- if $data.disableBGPExport}} | ||
| if ( net ~ {{$data.cidr}} ) then { reject; } | ||
| {{- end}} | ||
| {{- range $line := ippoolsFilterBIRDFunc (gets "/v1/ipam/v6/pool/*") "reject" false "" 6 }} | ||
| {{ $line }} | ||
| {{- end}} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would now consider inlining the above two lines into
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I think we can simplify these templates further, but I think it might sense to leave it to another PR and not complicate this PR more. |
||
| } | ||
|
|
||
|
|
@@ -62,15 +60,9 @@ function calico_export_to_bgp_peers(bool internal_peer) { | |
| {{- end}} | ||
| {{- end}} | ||
| {{- end}} | ||
| {{range ls "/v1/ipam/v6/pool"}}{{$data := json (getv (printf "/v1/ipam/v6/pool/%s" .))}} | ||
| {{- if $data.disableBGPExport}} | ||
| # Skip {{$data.cidr}} as BGP export is disabled for it | ||
| {{- else}} | ||
| if ( net ~ {{$data.cidr}} ) then { | ||
| accept; | ||
| } | ||
| {{- end}} | ||
| {{- end}} | ||
| {{- range $line := ippoolsFilterBIRDFunc (gets "/v1/ipam/v6/pool/*") "accept" false "" 6 }} | ||
| {{ $line }} | ||
| {{- end }} | ||
| } | ||
|
|
||
| filter calico_kernel_programming { | ||
|
|
@@ -85,13 +77,8 @@ filter calico_kernel_programming { | |
| {{- end}} | ||
|
|
||
| {{- end}} | ||
| {{range ls "/v1/ipam/v6/pool"}}{{$data := json (getv (printf "/v1/ipam/v6/pool/%s" .))}} | ||
| {{- if $data.vxlan_mode}} | ||
| if ( net ~ {{$data.cidr}} ) then { | ||
| # Don't program VXLAN routes into the kernel - these are handled by Felix. | ||
| reject; | ||
| } | ||
| {{- end}}{{/* End of '$data.vxlan_mode' */}} | ||
| {{- end}}{{/* End of 'range ls...' */}} | ||
| {{- range $line := ippoolsFilterBIRDFunc (gets "/v1/ipam/v6/pool/*") "reject" true "" 6 }} | ||
| {{ $line }} | ||
| {{- end}} | ||
| accept; {{- /* Destination is not in any ipPool, accept */}} | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -16,6 +16,8 @@ import ( | |||
| v3 "github.com/projectcalico/api/pkg/apis/projectcalico/v3" | ||||
|
|
||||
| "github.com/projectcalico/calico/confd/pkg/backends" | ||||
| "github.com/projectcalico/calico/libcalico-go/lib/backend/encap" | ||||
| "github.com/projectcalico/calico/libcalico-go/lib/backend/model" | ||||
| ) | ||||
|
|
||||
| func newFuncMap() map[string]interface{} { | ||||
|
|
@@ -40,6 +42,7 @@ func newFuncMap() map[string]interface{} { | |||
| m["base64Encode"] = Base64Encode | ||||
| m["base64Decode"] = Base64Decode | ||||
| m["bgpFilterBIRDFuncs"] = BGPFilterBIRDFuncs | ||||
| m["ippoolsFilterBIRDFunc"] = IPPoolsFilterBIRDFunc | ||||
| return m | ||||
| } | ||||
|
|
||||
|
|
@@ -403,6 +406,128 @@ func BGPFilterBIRDFuncs(pairs memkv.KVPairs, version int) ([]string, error) { | |||
| return lines, nil | ||||
| } | ||||
|
|
||||
| // This function generates BIRD statements for IPPool resources to be used as BIRD filters based on the following input: | ||||
| // - pairs: IPPool resources packaged into KVPairs. | ||||
| // - filterAction: specified action to filter generated statements. For exporting pools to BGP peers, we need to | ||||
| // first reject disabled ippools, and then accept the rest at the end after all other filters. Allowed values are | ||||
| // "accept", "reject", and "" (to not filter). | ||||
| // - forProgrammingKernel: Whether the generated statements are intended for programming routes to kernel or exporting to | ||||
| // other BGP Peers. As an example, we need to set "krt_tunnel" for programming IPIP and no-encap IPv4 routes. | ||||
| // - localSubnet: the subnet of local node, which is needed by IPv4 IPIP pool in cross subnet mode. | ||||
| // - version: the statement ip family. | ||||
| // | ||||
| // As an example, For the following sample IPPool resource: | ||||
| // | ||||
| // apiVersion: projectcalico.org/v3 | ||||
| // kind: IPPool | ||||
| // metadata: | ||||
| // | ||||
| // name: my.ippool-1 | ||||
| // | ||||
| // spec: | ||||
| // | ||||
| // cidr: 10.1.0.0/16 | ||||
| // ipipMode: Always | ||||
| // | ||||
| // this function generates the following statement for programming routes to kernel: | ||||
| // | ||||
| // if (net ~ 10.10.0.0/16) then { krt_tunnel="tunl0"; accept; } | ||||
| // | ||||
| // and the following statement for exporting to BGP peers: | ||||
| // | ||||
| // if (net ~ 10.10.0.0/16) then { accept; } | ||||
| func IPPoolsFilterBIRDFunc( | ||||
| pairs memkv.KVPairs, | ||||
| filterAction string, | ||||
| forProgrammingKernel bool, | ||||
| localSubnet string, | ||||
| version int, | ||||
| ) ([]string, error) { | ||||
| if version != 4 && version != 6 { | ||||
| return []string{}, fmt.Errorf("version must be either 4 or 6") | ||||
| } | ||||
|
|
||||
| lines := []string{} | ||||
| for _, kvp := range pairs { | ||||
| var ippool model.IPPool | ||||
| err := json.Unmarshal([]byte(kvp.Value), &ippool) | ||||
| if err != nil { | ||||
| return []string{}, fmt.Errorf("error unmarshalling JSON: %s", err) | ||||
| } | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: longer term we should eliminate the cache from the loop here, and instead store IPPools natively instead of going via JSON.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sense. Our current approach is not efficient. I'll leave this out for another PR. |
||||
|
|
||||
| cidr := ippool.CIDR.String() | ||||
| var action, comment, extraStatement string | ||||
| switch { | ||||
| case ippool.DisableBGPExport && !forProgrammingKernel: | ||||
| // IPPool's BGP export is disabled, and filter is for exporting to other peers. | ||||
| action = "reject" | ||||
| comment = "BGP export is disabled." | ||||
| case ippool.VXLANMode == encap.Always || ippool.VXLANMode == encap.CrossSubnet: | ||||
| // VXLAN encapsulation is always handled by Felix. | ||||
| if forProgrammingKernel { | ||||
| // Felix always handles programming VXLAN IPPools. | ||||
| action = "reject" | ||||
| comment = "VXLAN routes are handled by Felix." | ||||
| } else { | ||||
| action = "accept" | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking my understanding: I think this means that we export VXLAN pool routes, and that it makes sense when exporting to non-cluster peers. Am I right that it wouldn't make sense when exporting to cluster peers, because then we already know that those peers will not program the route? Might be nice to add some commenting about this, if we're sure of the precise logic.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (But it is already nicer to discuss this in code here than in the template!)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your point is correct, we do want to:
However, we have another filter named
In this PR, I've only been trying to replicate the ippools logic in the templates and left other filters out for sake of complexity.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for explaining that. |
||||
| } | ||||
| case ippool.IPIPMode == encap.Always || ippool.IPIPMode == encap.CrossSubnet, // IPIP Encapsulation. | ||||
| ippool.IPIPMode == encap.Undefined || ippool.VXLANMode == encap.Undefined: // No-encapsulation. | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is v1 to libcalico-go/lib/backend/encap/ipip.go, and then use that?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, based on the code it seems that v1
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this change is needed in another place. I'm working on it separately.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Doesn't need to block this PR, if that would delay things.) |
||||
| // IPIP encapsulation or No-Encap. | ||||
| if forProgrammingKernel && version == 4 && len(localSubnet) != 0 { | ||||
| // For IPv4 IPIP and no-encap routes, we need to set `krt_tunnel` variable which is needed by | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems wrong. I'm pretty sure we only use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I'm wrong. We also set it to "" for the no-encap case. Would love to rediscover why I did that! |
||||
| // our fork of BIRD. | ||||
| extraStatement = extraStatementForKernelProgrammingIPIPNoEncap(ippool.IPIPMode, localSubnet) | ||||
| } | ||||
| action = "accept" | ||||
| default: | ||||
| return nil, fmt.Errorf("invalid %s ippool", kvp.Key) | ||||
| } | ||||
|
|
||||
| // Filter statements based on provided filterAction. | ||||
| if len(filterAction) != 0 && filterAction != action { | ||||
| continue | ||||
| } | ||||
| lines = append(lines, emitFilterStatementForIPPools(cidr, extraStatement, action, comment)) | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: IMO it's not helpful to split pieces of the logic into subfunctions |
||||
| } | ||||
| return lines, nil | ||||
| } | ||||
|
|
||||
| func extraStatementForKernelProgrammingIPIPNoEncap(ipipMode encap.Mode, localSubnet string) string { | ||||
| switch v3.EncapMode(ipipMode) { | ||||
| case v3.Always: | ||||
| return `krt_tunnel="tunl0";` | ||||
| case v3.CrossSubnet: | ||||
| format := `if (defined(bgp_next_hop)&&(bgp_next_hop ~ %s)) then krt_tunnel=""; else krt_tunnel="tunl0";` | ||||
| return fmt.Sprintf(format, localSubnet) | ||||
| case v3.Undefined: | ||||
| // No-encap case. | ||||
| return `krt_tunnel="";` | ||||
| default: | ||||
| return `` | ||||
| } | ||||
| } | ||||
|
|
||||
| func emitFilterStatementForIPPools(cidr, extraStatement, action, comment string) (statement string) { | ||||
| // Check mandatory inputs. | ||||
| if len(cidr) == 0 || len(action) == 0 { | ||||
| return | ||||
| } | ||||
| if len(extraStatement) != 0 { | ||||
| statement = fmt.Sprintf(" if (net ~ %s) then { %s %s; }", cidr, extraStatement, action) | ||||
| } else { | ||||
| statement = fmt.Sprintf(" if (net ~ %s) then { %s; }", cidr, action) | ||||
| } | ||||
| if len(comment) != 0 { | ||||
| statement = fmt.Sprintf("%s %s", statement, formatComment(comment)) | ||||
| } | ||||
| return | ||||
| } | ||||
|
|
||||
| func formatComment(comment string) string { | ||||
| return fmt.Sprintf("# %s", comment) | ||||
| } | ||||
|
|
||||
| // Getenv retrieves the value of the environment variable named by the key. | ||||
| // It returns the value, which will the default value if the variable is not present. | ||||
| // If no default value was given - returns "". | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would consider replacing the
"reject", false, ""args (which are a little opaque) with a single enum that describes the context of thisippoolsFilterBIRDFunccall. Think that would be more readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And obviously also in the other cases, but with a different enum value.)