Skip to content

Commit

Permalink
Merge pull request #5295 from kubernetes-sigs/revert-5274-make_privat…
Browse files Browse the repository at this point in the history 
…e_lb_ip_configurable

Revert "make Private IP of the internal LB of the API Server configurable"
  • Loading branch information
@jackfrancis
jackfrancis authored Nov 21, 2024
2 parents be37836 + d3d5147 commit a9ab6eb
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 248 deletions.
30 changes: 8 additions & 22 deletions api/v1beta1/azurecluster_default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,31 +232,17 @@ func (c *AzureCluster) setAPIServerLBDefaults() {
if lb.Name == "" {
lb.Name = generateInternalLBName(c.ObjectMeta.Name)
}
}

// create default private IP if not set
privateIPFound := false
for i := range lb.FrontendIPs {
if lb.FrontendIPs[i].FrontendIPClass.PrivateIPAddress != "" {
if lb.FrontendIPs[i].Name == "" {
lb.FrontendIPs[i].Name = generateFrontendIPConfigName(lb.Name) + "-internal-ip"
if len(lb.FrontendIPs) == 0 {
lb.FrontendIPs = []FrontendIP{
{
Name: generateFrontendIPConfigName(lb.Name),
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: DefaultInternalLBIPAddress,
},
},
}
privateIPFound = true
break
}
}

// if no private IP found, create a default one
if !privateIPFound {
privateIP := FrontendIP{
Name: generateFrontendIPConfigName(lb.Name) + "-internal-ip",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: DefaultInternalLBIPAddress,
},
}
lb.FrontendIPs = append(lb.FrontendIPs, privateIP)
}

c.SetAPIServerLBBackendPoolNameDefault()
}

Expand Down
21 changes: 4 additions & 17 deletions api/v1beta1/azurecluster_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,8 @@ func TestVnetDefaults(t *testing.T) {
Subnets: Subnets{
{
SubnetClassSpec: SubnetClassSpec{
Role: SubnetControlPlane,
Name: "control-plane-subnet",
CIDRBlocks: []string{DefaultControlPlaneSubnetCIDR},
Role: SubnetControlPlane,
Name: "control-plane-subnet",
},

SecurityGroup: SecurityGroup{},
Expand All @@ -133,12 +132,6 @@ func TestVnetDefaults(t *testing.T) {
DNSName: "myfqdn.azure.com",
},
},
{
Name: "ip-config-internal-ip",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: DefaultInternalLBIPAddress,
},
},
},
LoadBalancerClassSpec: LoadBalancerClassSpec{
SKU: SKUStandard,
Expand Down Expand Up @@ -1244,12 +1237,6 @@ func TestAPIServerLBDefaults(t *testing.T) {
DNSName: "",
},
},
{
Name: "cluster-test-public-lb-frontEnd-internal-ip",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: DefaultInternalLBIPAddress,
},
},
},
BackendPool: BackendPool{
Name: "cluster-test-public-lb-backendPool",
Expand Down Expand Up @@ -1289,7 +1276,7 @@ func TestAPIServerLBDefaults(t *testing.T) {
APIServerLB: LoadBalancerSpec{
FrontendIPs: []FrontendIP{
{
Name: "cluster-test-internal-lb-frontEnd-internal-ip",
Name: "cluster-test-internal-lb-frontEnd",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: DefaultInternalLBIPAddress,
},
Expand Down Expand Up @@ -1337,7 +1324,7 @@ func TestAPIServerLBDefaults(t *testing.T) {
APIServerLB: LoadBalancerSpec{
FrontendIPs: []FrontendIP{
{
Name: "cluster-test-internal-lb-frontEnd-internal-ip",
Name: "cluster-test-internal-lb-frontEnd",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: DefaultInternalLBIPAddress,
},
Expand Down
63 changes: 19 additions & 44 deletions api/v1beta1/azurecluster_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,58 +400,33 @@ func validateAPIServerLB(lb LoadBalancerSpec, old LoadBalancerSpec, cidrs []stri
allErrs = append(allErrs, field.Forbidden(fldPath.Child("name"), "API Server load balancer name should not be modified after AzureCluster creation."))
}

publicIPCount := 0
privateIPCount := 0
newPrivateIP := ""
for i := range lb.FrontendIPs {
if lb.FrontendIPs[i].PublicIP != nil {
publicIPCount++
}
if lb.FrontendIPs[i].PrivateIPAddress != "" {
privateIPCount++
newPrivateIP = lb.FrontendIPs[i].PrivateIPAddress
}
}

if lb.Type == Public {
// public IP count should be 1 for public LB.
if publicIPCount != 1 || ptr.Deref[int32](lb.FrontendIPsCount, 1) != 1 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("frontendIPConfigs"), lb.FrontendIPs,
"API Server Load balancer should have 1 Frontend IP"))
}
}

// if Internal, IP config should not have a public IP.
if lb.Type == Internal {
if publicIPCount != 0 {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("frontendIPConfigs").Index(0).Child("publicIP"),
"API Server's associated internal load balancer cannot have a Public IP"))
}
}

// private IP count should be 1 for public LB.
if privateIPCount != 1 {
// There should only be one IP config.
if len(lb.FrontendIPs) != 1 || ptr.Deref[int32](lb.FrontendIPsCount, 1) != 1 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("frontendIPConfigs"), lb.FrontendIPs,
"API Server Load balancer should have 1 private IP"))
"API Server Load balancer should have 1 Frontend IP"))
} else {
for i := range lb.FrontendIPs {
if lb.FrontendIPs[i].PrivateIPAddress != "" {
if err := validateInternalLBIPAddress(lb.FrontendIPs[i].PrivateIPAddress, cidrs,
// if Internal, IP config should not have a public IP.
if lb.Type == Internal {
if lb.FrontendIPs[0].PublicIP != nil {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("frontendIPConfigs").Index(0).Child("publicIP"),
"Internal Load Balancers cannot have a Public IP"))
}
if lb.FrontendIPs[0].PrivateIPAddress != "" {
if err := validateInternalLBIPAddress(lb.FrontendIPs[0].PrivateIPAddress, cidrs,
fldPath.Child("frontendIPConfigs").Index(0).Child("privateIP")); err != nil {
allErrs = append(allErrs, err)
}
if len(old.FrontendIPs) != 0 && old.FrontendIPs[0].PrivateIPAddress != lb.FrontendIPs[0].PrivateIPAddress {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("name"), "API Server load balancer private IP should not be modified after AzureCluster creation."))
}
}
}

if len(old.FrontendIPs) != 0 {
oldPrivateIP := ""
for i := range old.FrontendIPs {
if old.FrontendIPs[i].PrivateIPAddress != "" {
oldPrivateIP = old.FrontendIPs[i].PrivateIPAddress
}
}
if newPrivateIP != oldPrivateIP {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("name"), "API Server load balancer private IP should not be modified after AzureCluster creation."))
// if Public, IP config should not have a private IP.
if lb.Type == Public {
if lb.FrontendIPs[0].PrivateIPAddress != "" {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("frontendIPConfigs").Index(0).Child("privateIP"),
"Public Load Balancers cannot have a Private IP"))
}
}
}
Expand Down
95 changes: 6 additions & 89 deletions api/v1beta1/azurecluster_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -891,7 +891,6 @@ func TestValidateAPIServerLB(t *testing.T) {
{
name: "too many IP configs",
lb: LoadBalancerSpec{
Name: "my-valid-lb",
FrontendIPs: []FrontendIP{
{
Name: "ip-1",
Expand All @@ -900,10 +899,6 @@ func TestValidateAPIServerLB(t *testing.T) {
Name: "ip-2",
},
},
LoadBalancerClassSpec: LoadBalancerClassSpec{
Type: Public,
SKU: SKUStandard,
},
},
wantErr: true,
expectedErr: field.Error{
Expand All @@ -921,80 +916,26 @@ func TestValidateAPIServerLB(t *testing.T) {
},
},
{
name: "too many private IP configs",
name: "public LB with private IP",
lb: LoadBalancerSpec{
Name: "my-valid-lb",
FrontendIPs: []FrontendIP{
{
Name: "ip-1",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: "10.0.0.100",
},
},
{
Name: "ip-2",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: "10.0.0.200",
PrivateIPAddress: "10.0.0.4",
},
},
{
Name: "ip-3",
},
},
LoadBalancerClassSpec: LoadBalancerClassSpec{
Type: Public,
SKU: SKUStandard,
},
},
wantErr: true,
expectedErr: field.Error{
Type: "FieldValueInvalid",
Field: "apiServerLB.frontendIPConfigs",
BadValue: []FrontendIP{
{
Name: "ip-1",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: "10.0.0.100",
},
},
{
Name: "ip-2",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: "10.0.0.200",
},
},
{
Name: "ip-3",
},
},
Detail: "API Server Load balancer should have 1 private IP",
},
},
{
name: "public LB with private IP",
cpCIDRS: []string{"10.0.0.0/24"},
lb: LoadBalancerSpec{
Name: "my-valid-lb",
FrontendIPs: []FrontendIP{
{
Name: "ip-1",
PublicIP: &PublicIPSpec{
Name: "my-valid-ip-name",
},
},
{
Name: "ip-1",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: "10.0.0.4",
},
},
},
LoadBalancerClassSpec: LoadBalancerClassSpec{
Type: Public,
SKU: SKUStandard,
},
Type: "FieldValueForbidden",
Field: "apiServerLB.frontendIPConfigs[0].privateIP",
Detail: "Public Load Balancers cannot have a Private IP",
},
wantErr: false,
},
{
name: "internal LB with public IP",
Expand All @@ -1015,7 +956,7 @@ func TestValidateAPIServerLB(t *testing.T) {
expectedErr: field.Error{
Type: "FieldValueForbidden",
Field: "apiServerLB.frontendIPConfigs[0].publicIP",
Detail: "API Server's associated internal load balancer cannot have a Public IP",
Detail: "Internal Load Balancers cannot have a Public IP",
},
},
{
Expand Down Expand Up @@ -1542,18 +1483,12 @@ func createClusterNetworkSpec() NetworkSpec {
Vnet: VnetSpec{
ResourceGroup: "custom-vnet",
Name: "my-vnet",
VnetClassSpec: VnetClassSpec{
CIDRBlocks: []string{DefaultVnetCIDR},
},
},
Subnets: Subnets{
{
SubnetClassSpec: SubnetClassSpec{
Role: "cluster",
Name: "cluster-subnet",
CIDRBlocks: []string{
DefaultClusterSubnetCIDR,
},
},
},
},
Expand All @@ -1567,18 +1502,12 @@ func createValidNetworkSpecWithClusterSubnet() NetworkSpec {
Vnet: VnetSpec{
ResourceGroup: "custom-vnet",
Name: "my-vnet",
VnetClassSpec: VnetClassSpec{
CIDRBlocks: []string{DefaultVnetCIDR},
},
},
Subnets: Subnets{
{
SubnetClassSpec: SubnetClassSpec{
Role: "cluster",
Name: "cluster-subnet",
CIDRBlocks: []string{
DefaultClusterSubnetCIDR,
},
},
},
},
Expand All @@ -1592,9 +1521,6 @@ func createValidNetworkSpec() NetworkSpec {
Vnet: VnetSpec{
ResourceGroup: "custom-vnet",
Name: "my-vnet",
VnetClassSpec: VnetClassSpec{
CIDRBlocks: []string{DefaultVnetCIDR},
},
},
Subnets: createValidSubnets(),
APIServerLB: createValidAPIServerLB(),
Expand All @@ -1608,9 +1534,6 @@ func createValidSubnets() Subnets {
SubnetClassSpec: SubnetClassSpec{
Role: "control-plane",
Name: "control-plane-subnet",
CIDRBlocks: []string{
DefaultControlPlaneSubnetCIDR,
},
},
},
{
Expand Down Expand Up @@ -1643,12 +1566,6 @@ func createValidAPIServerLB() LoadBalancerSpec {
DNSName: "myfqdn.azure.com",
},
},
{
Name: "ip-config-internal-ip",
FrontendIPClass: FrontendIPClass{
PrivateIPAddress: DefaultInternalLBIPAddress,
},
},
},
LoadBalancerClassSpec: LoadBalancerClassSpec{
SKU: SKUStandard,
Expand Down
Loading

0 comments on commit a9ab6eb

Please sign in to comment.