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.

Sign up for GitHub

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

Jump to bottom

enable the use of an external control plane #4611

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+339 −159
Open

enable the use of an external control plane #4611

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
558454c
enable the use off an external control plane
rpahli Mar 1, 2024
845077e
cleanup
rpahli Apr 8, 2024
18009a3
fix tests
rpahli Apr 8, 2024
0883d45
fix tests
rpahli Apr 9, 2024
125c187
PR comments
rpahli Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 33 additions & 15 deletions api/v1beta1/azurecluster_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ limitations under the License.
package v1beta1

import (
"context"
"encoding/json"
"fmt"
"sigs.k8s.io/controller-runtime/pkg/log"

"k8s.io/utils/ptr"
)
Expand Down Expand Up @@ -54,13 +57,26 @@ func (c *AzureCluster) setDefaults() {
}

func (c *AzureCluster) setNetworkSpecDefaults() {
logger := log.FromContext(context.TODO())
logger.Info(fmt.Sprintf("Was ist hier los %v", c.Spec.ControlPlaneEnabled))
c.setVnetDefaults()
c.setBastionDefaults()
c.setSubnetDefaults()
c.setVnetPeeringDefaults()
c.setAPIServerLBDefaults()
if c.Spec.ControlPlaneEnabled {
logger.Info("dont call me setAPIServerLBDefaults")
c.setAPIServerLBDefaults()
}
c.SetNodeOutboundLBDefaults()
c.SetControlPlaneOutboundLBDefaults()
if c.Spec.ControlPlaneEnabled {
logger.Info("dont call me SetControlPlaneOutboundLBDefaults")
c.SetControlPlaneOutboundLBDefaults()
}
if c.Spec.ControlPlaneEnabled {
c.Spec.NetworkSpec.APIServerLB = nil
}
bolB, _ := json.Marshal(c.Spec)
logger.Info(fmt.Sprintf("Was ist hier los %s", string(bolB)))
}

func (c *AzureCluster) setResourceGroupDefault() {
Expand Down Expand Up @@ -93,16 +109,18 @@ func (c *AzureCluster) setSubnetDefaults() {
c.Spec.NetworkSpec.UpdateSubnet(clusterSubnet, SubnetCluster)
}

/* if there is a cp subnet set defaults
if no cp subnet and cluster subnet create a default cp subnet */
cpSubnet, errcp := c.Spec.NetworkSpec.GetSubnet(SubnetControlPlane)
if errcp == nil {
cpSubnet.setControlPlaneSubnetDefaults(c.ObjectMeta.Name)
c.Spec.NetworkSpec.UpdateSubnet(cpSubnet, SubnetControlPlane)
} else if !clusterSubnetExists {
cpSubnet = SubnetSpec{SubnetClassSpec: SubnetClassSpec{Role: SubnetControlPlane}}
cpSubnet.setControlPlaneSubnetDefaults(c.ObjectMeta.Name)
c.Spec.NetworkSpec.Subnets = append(c.Spec.NetworkSpec.Subnets, cpSubnet)
if c.Spec.ControlPlaneEnabled {
/* if there is a cp subnet set defaults
if no cp subnet and cluster subnet create a default cp subnet */
cpSubnet, errcp := c.Spec.NetworkSpec.GetSubnet(SubnetControlPlane)
if errcp == nil {
cpSubnet.setControlPlaneSubnetDefaults(c.ObjectMeta.Name)
c.Spec.NetworkSpec.UpdateSubnet(cpSubnet, SubnetControlPlane)
} else if !clusterSubnetExists {
cpSubnet = SubnetSpec{SubnetClassSpec: SubnetClassSpec{Role: SubnetControlPlane}}
cpSubnet.setControlPlaneSubnetDefaults(c.ObjectMeta.Name)
c.Spec.NetworkSpec.Subnets = append(c.Spec.NetworkSpec.Subnets, cpSubnet)
}
}

var nodeSubnetFound bool
Expand Down Expand Up @@ -210,7 +228,7 @@ func (c *AzureCluster) setVnetPeeringDefaults() {
}

func (c *AzureCluster) setAPIServerLBDefaults() {
lb := &c.Spec.NetworkSpec.APIServerLB
lb := c.Spec.NetworkSpec.APIServerLB

lb.LoadBalancerClassSpec.setAPIServerLBDefaults()

Expand Down Expand Up @@ -249,7 +267,7 @@ func (c *AzureCluster) setAPIServerLBDefaults() {
// SetNodeOutboundLBDefaults sets the default values for the NodeOutboundLB.
func (c *AzureCluster) SetNodeOutboundLBDefaults() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do worker nodes use the node outbound LB too?

if c.Spec.NetworkSpec.NodeOutboundLB == nil {
if c.Spec.NetworkSpec.APIServerLB.Type == Internal {
if !c.Spec.ControlPlaneEnabled || c.Spec.NetworkSpec.APIServerLB.Type == Internal {
return
}

Expand Down Expand Up @@ -314,7 +332,7 @@ func (c *AzureCluster) SetBackendPoolNameDefault() {

// SetAPIServerLBBackendPoolNameDefault defaults the name of the backend pool for apiserver LB.
func (c *AzureCluster) SetAPIServerLBBackendPoolNameDefault() {
apiServerLB := &c.Spec.NetworkSpec.APIServerLB
apiServerLB := c.Spec.NetworkSpec.APIServerLB
if apiServerLB.BackendPool.Name == "" {
apiServerLB.BackendPool.Name = generateBackendAddressPoolName(apiServerLB.Name)
}
Expand Down
5 changes: 5 additions & 0 deletions api/v1beta1/azurecluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ type AzureClusterSpec struct {
// +optional
BastionSpec BastionSpec `json:"bastionSpec,omitempty"`

// ControlPlaneEnabled enable control plane cluster components.
// +kubebuilder:default=true
// +optional
ControlPlaneEnabled bool `json:"controlPlaneEnabled"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ControlPlaneEnabled bool `json:"controlPlaneEnabled"`
ControlPlaneEnabled bool `json:"controlPlaneEnabled,omitempty"`


// ControlPlaneEndpoint represents the endpoint used to communicate with the control plane. It is not recommended to set
// this when creating an AzureCluster as CAPZ will set this for you. However, if it is set, CAPZ will not change it.
// +optional
Expand Down
53 changes: 32 additions & 21 deletions api/v1beta1/azurecluster_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ limitations under the License.
package v1beta1

import (
"context"
"fmt"
"net"
"reflect"
"regexp"
"sigs.k8s.io/controller-runtime/pkg/log"

valid "github.com/asaskevich/govalidator"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -85,12 +87,16 @@ func (c *AzureCluster) validateCluster(old *AzureCluster) (admission.Warnings, e

// validateClusterSpec validates a ClusterSpec.
func (c *AzureCluster) validateClusterSpec(old *AzureCluster) field.ErrorList {
logger := log.FromContext(context.TODO())
var allErrs field.ErrorList
var oldNetworkSpec NetworkSpec
if old != nil {
oldNetworkSpec = old.Spec.NetworkSpec
}
allErrs = append(allErrs, validateNetworkSpec(c.Spec.NetworkSpec, oldNetworkSpec, field.NewPath("spec").Child("networkSpec"))...)

logger.Info(fmt.Sprintf("ControlPlaneEnabled: %v", c.Spec.ControlPlaneEnabled))

allErrs = append(allErrs, validateNetworkSpec(c.Spec.ControlPlaneEnabled, c.Spec.NetworkSpec, oldNetworkSpec, field.NewPath("spec").Child("networkSpec"))...)

var oldCloudProviderConfigOverrides *CloudProviderConfigOverrides
if old != nil {
Expand Down Expand Up @@ -154,7 +160,7 @@ func validateIdentityRef(identityRef *corev1.ObjectReference, fldPath *field.Pat
}

// validateNetworkSpec validates a NetworkSpec.
func validateNetworkSpec(networkSpec NetworkSpec, old NetworkSpec, fldPath *field.Path) field.ErrorList {
func validateNetworkSpec(controlPlaneEnabled bool, networkSpec NetworkSpec, old NetworkSpec, fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList
// If the user specifies a resourceGroup for vnet, it means
// that they intend to use a pre-existing vnet. In this case,
Expand All @@ -167,20 +173,21 @@ func validateNetworkSpec(networkSpec NetworkSpec, old NetworkSpec, fldPath *fiel

allErrs = append(allErrs, validateVnetCIDR(networkSpec.Vnet.CIDRBlocks, fldPath.Child("cidrBlocks"))...)

allErrs = append(allErrs, validateSubnets(networkSpec.Subnets, networkSpec.Vnet, fldPath.Child("subnets"))...)
allErrs = append(allErrs, validateSubnets(controlPlaneEnabled, networkSpec.Subnets, networkSpec.Vnet, fldPath.Child("subnets"))...)

allErrs = append(allErrs, validateVnetPeerings(networkSpec.Vnet.Peerings, fldPath.Child("peerings"))...)
}

var cidrBlocks []string
controlPlaneSubnet, err := networkSpec.GetControlPlaneSubnet()
if err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("subnets"), networkSpec.Subnets, "ControlPlaneSubnet invalid"))
}

cidrBlocks = controlPlaneSubnet.CIDRBlocks
if controlPlaneEnabled {
controlPlaneSubnet, err := networkSpec.GetControlPlaneSubnet()
if err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("subnets"), networkSpec.Subnets, "ControlPlaneSubnet invalid"))
}

allErrs = append(allErrs, validateAPIServerLB(networkSpec.APIServerLB, old.APIServerLB, cidrBlocks, fldPath.Child("apiServerLB"))...)
cidrBlocks = controlPlaneSubnet.CIDRBlocks
allErrs = append(allErrs, validateAPIServerLB(networkSpec.APIServerLB, old.APIServerLB, cidrBlocks, fldPath.Child("apiServerLB"))...)
}

var needOutboundLB bool
for _, subnet := range networkSpec.Subnets {
Expand All @@ -192,10 +199,14 @@ func validateNetworkSpec(networkSpec NetworkSpec, old NetworkSpec, fldPath *fiel
if needOutboundLB {
allErrs = append(allErrs, validateNodeOutboundLB(networkSpec.NodeOutboundLB, old.NodeOutboundLB, networkSpec.APIServerLB, fldPath.Child("nodeOutboundLB"))...)
}

allErrs = append(allErrs, validateControlPlaneOutboundLB(networkSpec.ControlPlaneOutboundLB, networkSpec.APIServerLB, fldPath.Child("controlPlaneOutboundLB"))...)

allErrs = append(allErrs, validatePrivateDNSZoneName(networkSpec.PrivateDNSZoneName, networkSpec.APIServerLB.Type, fldPath.Child("privateDNSZoneName"))...)
if controlPlaneEnabled {
allErrs = append(allErrs, validateControlPlaneOutboundLB(networkSpec.ControlPlaneOutboundLB, networkSpec.APIServerLB, fldPath.Child("controlPlaneOutboundLB"))...)
}
var lbType LBType = Internal
if networkSpec.APIServerLB != nil {
lbType = networkSpec.APIServerLB.Type
}
allErrs = append(allErrs, validatePrivateDNSZoneName(networkSpec.PrivateDNSZoneName, controlPlaneEnabled, lbType, fldPath.Child("privateDNSZoneName"))...)

if len(allErrs) == 0 {
return nil
Expand All @@ -214,11 +225,11 @@ func validateResourceGroup(resourceGroup string, fldPath *field.Path) *field.Err

// validateSubnets validates a list of Subnets.
// When configuring a cluster, it is essential to include either a control-plane subnet and a node subnet, or a user can configure a cluster subnet which will be used as a control-plane subnet and a node subnet.
func validateSubnets(subnets Subnets, vnet VnetSpec, fldPath *field.Path) field.ErrorList {
func validateSubnets(controlPlaneEnabled bool, subnets Subnets, vnet VnetSpec, fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList
subnetNames := make(map[string]bool, len(subnets))
requiredSubnetRoles := map[string]bool{
"control-plane": false,
"control-plane": !controlPlaneEnabled,
"node": false,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be refactored this way:

requiredSubnetRoles := map[string]bool{
		"node": false,
	}
	
	if controlPlaneEnabled {
		requiredSubnetRoles["control-plane"] = false
	}

The key control-plane must not be present when the Control Plane is externally managed.

clusterSubnet := false
Expand Down Expand Up @@ -383,7 +394,7 @@ func validateSecurityRule(rule SecurityRule, fldPath *field.Path) (allErrs field
return allErrs
}

func validateAPIServerLB(lb LoadBalancerSpec, old LoadBalancerSpec, cidrs []string, fldPath *field.Path) field.ErrorList {
func validateAPIServerLB(lb *LoadBalancerSpec, old *LoadBalancerSpec, cidrs []string, fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList

lbClassSpec := lb.LoadBalancerClassSpec
Expand Down Expand Up @@ -433,7 +444,7 @@ func validateAPIServerLB(lb LoadBalancerSpec, old LoadBalancerSpec, cidrs []stri
return allErrs
}

func validateNodeOutboundLB(lb *LoadBalancerSpec, old *LoadBalancerSpec, apiserverLB LoadBalancerSpec, fldPath *field.Path) field.ErrorList {
func validateNodeOutboundLB(lb *LoadBalancerSpec, old *LoadBalancerSpec, apiserverLB *LoadBalancerSpec, fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList

var lbClassSpec, oldClassSpec *LoadBalancerClassSpec
Expand Down Expand Up @@ -483,7 +494,7 @@ func validateNodeOutboundLB(lb *LoadBalancerSpec, old *LoadBalancerSpec, apiserv
return allErrs
}

func validateControlPlaneOutboundLB(lb *LoadBalancerSpec, apiserverLB LoadBalancerSpec, fldPath *field.Path) field.ErrorList {
func validateControlPlaneOutboundLB(lb *LoadBalancerSpec, apiserverLB *LoadBalancerSpec, fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList

var lbClassSpec *LoadBalancerClassSpec
Expand All @@ -505,11 +516,11 @@ func validateControlPlaneOutboundLB(lb *LoadBalancerSpec, apiserverLB LoadBalanc
}

// validatePrivateDNSZoneName validates the PrivateDNSZoneName.
func validatePrivateDNSZoneName(privateDNSZoneName string, apiserverLBType LBType, fldPath *field.Path) field.ErrorList {
func validatePrivateDNSZoneName(privateDNSZoneName string, controlPlaneEnabled bool, apiserverLBType LBType, fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList

if len(privateDNSZoneName) > 0 {
if apiserverLBType != Internal {
if controlPlaneEnabled && apiserverLBType != Internal {
allErrs = append(allErrs, field.Invalid(fldPath, apiserverLBType,
"PrivateDNSZoneName is available only if APIServerLB.Type is Internal"))
}
Expand Down
8 changes: 4 additions & 4 deletions api/v1beta1/azurecluster_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ func TestNetworkSpecWithPreexistingVnetValid(t *testing.T) {
for _, test := range testCase {
t.Run(test.name, func(t *testing.T) {
g := NewWithT(t)
errs := validateNetworkSpec(test.networkSpec, NetworkSpec{}, field.NewPath("spec").Child("networkSpec"))
errs := validateNetworkSpec(true, test.networkSpec, NetworkSpec{}, field.NewPath("spec").Child("networkSpec"))
g.Expect(errs).To(BeNil())
})
}
Expand All @@ -338,7 +338,7 @@ func TestNetworkSpecWithPreexistingVnetLackRequiredSubnets(t *testing.T) {

t.Run(testCase.name, func(t *testing.T) {
g := NewWithT(t)
errs := validateNetworkSpec(testCase.networkSpec, NetworkSpec{}, field.NewPath("spec").Child("networkSpec"))
errs := validateNetworkSpec(true, testCase.networkSpec, NetworkSpec{}, field.NewPath("spec").Child("networkSpec"))
g.Expect(errs).To(HaveLen(1))
g.Expect(errs[0].Type).To(Equal(field.ErrorTypeRequired))
g.Expect(errs[0].Field).To(Equal("spec.networkSpec.subnets"))
Expand All @@ -361,7 +361,7 @@ func TestNetworkSpecWithPreexistingVnetInvalidResourceGroup(t *testing.T) {

t.Run(testCase.name, func(t *testing.T) {
g := NewWithT(t)
errs := validateNetworkSpec(testCase.networkSpec, NetworkSpec{}, field.NewPath("spec").Child("networkSpec"))
errs := validateNetworkSpec(true, testCase.networkSpec, NetworkSpec{}, field.NewPath("spec").Child("networkSpec"))
g.Expect(errs).To(HaveLen(1))
g.Expect(errs[0].Type).To(Equal(field.ErrorTypeInvalid))
g.Expect(errs[0].Field).To(Equal("spec.networkSpec.vnet.resourceGroup"))
Expand All @@ -384,7 +384,7 @@ func TestNetworkSpecWithoutPreexistingVnetValid(t *testing.T) {

t.Run(testCase.name, func(t *testing.T) {
g := NewWithT(t)
errs := validateNetworkSpec(testCase.networkSpec, NetworkSpec{}, field.NewPath("spec").Child("networkSpec"))
errs := validateNetworkSpec(true, testCase.networkSpec, NetworkSpec{}, field.NewPath("spec").Child("networkSpec"))
g.Expect(errs).To(BeNil())
})
}
Expand Down
6 changes: 6 additions & 0 deletions api/v1beta1/azurecluster_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ limitations under the License.
package v1beta1

import (
"context"
"reflect"
"sigs.k8s.io/controller-runtime/pkg/log"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -43,11 +45,15 @@ var _ webhook.Defaulter = &AzureCluster{}

// Default implements webhook.Defaulter so a webhook will be registered for the type.
func (c *AzureCluster) Default() {
logger := log.FromContext(context.TODO())
logger.Info("Defaulter")
c.setDefaults()
}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type.
func (c *AzureCluster) ValidateCreate() (admission.Warnings, error) {
logger := log.FromContext(context.TODO())
logger.Info("ValidateCreate")
return c.validateCluster(nil)
}

Expand Down
1 change: 1 addition & 0 deletions api/v1beta1/azureclustertemplate_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ func (c *AzureClusterTemplate) validatePrivateDNSZoneName() field.ErrorList {

allErrs = append(allErrs, validatePrivateDNSZoneName(
networkSpec.PrivateDNSZoneName,
true,
networkSpec.APIServerLB.Type,
fldPath,
)...)
Expand Down
2 changes: 1 addition & 1 deletion api/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ type NetworkSpec struct {

// APIServerLB is the configuration for the control-plane load balancer.
// +optional
APIServerLB LoadBalancerSpec `json:"apiServerLB,omitempty"`
APIServerLB *LoadBalancerSpec `json:"apiServerLB,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using a pointer, what about moving the additional field you introduced (ControlPlaneEnabled) here?

With such approach we could introduce the feature smoothly with no breaking changes, and maintain the current state of the pointers.

Copy link
Author

Choose a reason for hiding this comment

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

The "problem" is that der is a mutation inside the webhook and if this is no pointer then some fields will be filled which makes problems later. To me the question is omitempty means it can be null so the pointer also makes sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, it makes sense.


// NodeOutboundLB is the configuration for the node outbound load balancer.
// +optional
Expand Down
6 changes: 5 additions & 1 deletion api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading