-
Notifications
You must be signed in to change notification settings - Fork 41
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
CORS-2818: Adding ability to render Cloud LB IPs #286
Changes from all commits
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 |
---|---|---|
|
@@ -52,6 +52,11 @@ type Cluster struct { | |
VIPNetmask int | ||
MasterAmount int64 | ||
NodeAddresses []NodeAddress | ||
APILBIPs []string | ||
APIIntLBIPs []string | ||
IngressLBIPs []string | ||
CloudLBRecordType string | ||
CloudLBEmptyType string | ||
} | ||
|
||
type Backend struct { | ||
|
@@ -84,6 +89,12 @@ type Node struct { | |
Configs *[]Node | ||
} | ||
|
||
type ClusterLBConfig struct { | ||
ApiLBIPs []net.IP | ||
ApiIntLBIPs []net.IP | ||
IngressLBIPs []net.IP | ||
} | ||
|
||
func getDNSUpstreams(resolvConfPath string) (upstreams []string, err error) { | ||
dnsFile, err := os.Open(resolvConfPath) | ||
if err != nil { | ||
|
@@ -417,7 +428,13 @@ func getNodeIpForRequestedIpStack(node v1.Node, filterIps []string, machineNetwo | |
// apiPort: The port on which the k8s api listens. Should be 6443. | ||
// lbPort: The port on which haproxy listens. | ||
// statPort: The port on which the haproxy stats endpoint listens. | ||
func GetConfig(kubeconfigPath, clusterConfigPath, resolvConfPath string, apiVips, ingressVips []net.IP, apiPort, lbPort, statPort uint16) (node Node, err error) { | ||
// clusterLBConfig: A struct containing IPs for API, API-Int and Ingress LBs | ||
func GetConfig(kubeconfigPath, clusterConfigPath, resolvConfPath string, apiVips, ingressVips []net.IP, apiPort, lbPort, statPort uint16, clusterLBConfig ClusterLBConfig) (node Node, err error) { | ||
if len(clusterLBConfig.ApiIntLBIPs) > 0 { | ||
// Cloud Platforms with cloud LBs but no Cloud DNS | ||
return getNodeConfigWithCloudLBIPs(kubeconfigPath, clusterConfigPath, resolvConfPath, clusterLBConfig) | ||
} | ||
// On-prem platforms | ||
vipCount := 0 | ||
if len(apiVips) > len(ingressVips) { | ||
vipCount = len(apiVips) | ||
|
@@ -694,3 +711,94 @@ func PopulateNodeAddresses(kubeconfigPath string, node *Node) { | |
} | ||
} | ||
} | ||
|
||
func getNodeConfigWithCloudLBIPs(kubeconfigPath, clusterConfigPath, resolvConfPath string, clusterLBConfig ClusterLBConfig) (node Node, err error) { | ||
var apiLBIP, apiIntLBIP, ingressIP net.IP | ||
nodes := []Node{} | ||
|
||
// The number of LB IPs provided in ClusterLBConfig's ApiIntLBIPs, ApiLBIPs and IngressLBIPs | ||
// could all be different. In private clusters, there would be no API LB IPs provided. So,to | ||
// account for the varying lengths we determine the longest between the list of Ingress LB | ||
// IPs and API-Int LB IPs. When present, len(ApiLBIPs) is equal to len(ApiIntLBIPs). | ||
ipCount := 0 | ||
if len(clusterLBConfig.ApiIntLBIPs) > len(clusterLBConfig.IngressLBIPs) { | ||
ipCount = len(clusterLBConfig.ApiIntLBIPs) | ||
} else { | ||
ipCount = len(clusterLBConfig.IngressLBIPs) | ||
} | ||
|
||
// Iterate through the longest list of LB IPs and provide an API, API-Int and Ingress | ||
// LB IP to each newly created Node. When a list has no more LB IPs, update Node object | ||
// with a nil IP address. | ||
for i := 0; i < ipCount; i++ { | ||
if i < len(clusterLBConfig.ApiIntLBIPs) { | ||
apiIntLBIP = clusterLBConfig.ApiIntLBIPs[i] | ||
} else { | ||
apiIntLBIP = nil | ||
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. Because in L718 you wrote 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. apiIntLBIP and other LB IP values are being set within a loop starting at L726 so its value could have been !nil in the previous iteration. 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. Yeah, I see it now... In that case this deserves at least a comment in Simple English directly in the code or some kind of a simplification... As I read this whole loop now (L726-L730) is that when you finish the loop, you want
Do I get the desired behaviour right? If yes, I'd rather do something like
instead of iteration in which every time we assign a value only to ignore it afterwards and use the last assignment. If my reasoning is wrong and your desired behaviour is different then ignore what I wrote, but that means a comment is more than desired 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 was trying to follow the precedence set by https://github.com/openshift/baremetal-runtimecfg/blob/master/pkg/config/node.go#L421-L447. There are definitely other ways to implement this too. |
||
} | ||
|
||
// For public clusters. Private clusters will not have External | ||
// LBs so apiLBIPs could be empty. | ||
if len(clusterLBConfig.ApiLBIPs) != 0 && i < len(clusterLBConfig.ApiLBIPs) { | ||
apiLBIP = clusterLBConfig.ApiLBIPs[i] | ||
} else { | ||
apiLBIP = nil | ||
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. As above, no need for an explicit nil-assignment |
||
} | ||
if len(clusterLBConfig.IngressLBIPs) != 0 && i < len(clusterLBConfig.IngressLBIPs) { | ||
ingressIP = clusterLBConfig.IngressLBIPs[i] | ||
} else { | ||
ingressIP = nil | ||
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. As above, no need for an explicit nil-assignment |
||
} | ||
newNode, err := getNodeConfig(kubeconfigPath, clusterConfigPath, resolvConfPath, nil, nil, 0, 0, 0) | ||
if err != nil { | ||
return Node{}, err | ||
} | ||
newNode = updateNodewithCloudLBIPs(apiLBIP, apiIntLBIP, ingressIP, newNode) | ||
nodes = append(nodes, newNode) | ||
} | ||
nodes[0].Configs = &nodes | ||
return nodes[0], nil | ||
} | ||
|
||
func updateNodewithCloudLBIPs(apiLBIP, apiIntLBIP, ingressIP net.IP, node Node) Node { | ||
var validLBIP net.IP | ||
if apiIntLBIP != nil { | ||
validLBIP = apiIntLBIP | ||
node.Cluster.APIIntLBIPs = append(node.Cluster.APIIntLBIPs, apiIntLBIP.String()) | ||
} | ||
if apiLBIP != nil { | ||
node.Cluster.APILBIPs = append(node.Cluster.APILBIPs, apiLBIP.String()) | ||
} | ||
if ingressIP != nil { | ||
node.Cluster.IngressLBIPs = append(node.Cluster.IngressLBIPs, ingressIP.String()) | ||
if validLBIP == nil { | ||
validLBIP = ingressIP | ||
} | ||
} | ||
node.Cluster.CloudLBRecordType = "A" | ||
node.Cluster.CloudLBEmptyType = "AAAA" | ||
if validLBIP.To4() == nil { | ||
node.Cluster.CloudLBRecordType = "AAAA" | ||
node.Cluster.CloudLBEmptyType = "A" | ||
} | ||
return node | ||
} | ||
|
||
func PopulateCloudLBIPAddresses(clusterLBConfig ClusterLBConfig, node Node) (updatedNode Node, err error) { | ||
for _, ip := range clusterLBConfig.ApiIntLBIPs { | ||
node.Cluster.APIIntLBIPs = append(node.Cluster.APIIntLBIPs, ip.String()) | ||
} | ||
for _, ip := range clusterLBConfig.ApiLBIPs { | ||
node.Cluster.APILBIPs = append(node.Cluster.APILBIPs, ip.String()) | ||
} | ||
for _, ip := range clusterLBConfig.IngressLBIPs { | ||
node.Cluster.IngressLBIPs = append(node.Cluster.IngressLBIPs, ip.String()) | ||
} | ||
node.Cluster.CloudLBRecordType = "A" | ||
node.Cluster.CloudLBEmptyType = "AAAA" | ||
if len(clusterLBConfig.ApiIntLBIPs) > 0 && clusterLBConfig.ApiIntLBIPs[0].To4() == nil { | ||
node.Cluster.CloudLBRecordType = "AAAA" | ||
node.Cluster.CloudLBEmptyType = "A" | ||
} | ||
return node, nil | ||
} |
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.
When is it possible that we have different number of IPs? This is a honest question, in the IPI loadbalancer we do there is always a requirement that you have the same number of IPs for API and for Ingress
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.
In the cloud IPI case with BYO DNS, there are internal and external Ingress LBs. That is one way to have multiple entries for Ingress. For private clusters, we don't expect to have any IPs for API LB IPs, only API-Int and Ingress LBs are expected to be running. This is a way to future-proof our implementation for all combinations of LB IPs.