Skip to content

Create an Internal Load Balancer if configured #1222

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

Merged
merged 1 commit into from
May 29, 2024
Merged

Create an Internal Load Balancer if configured #1222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions api/v1beta1/gcpcluster_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,13 @@ func (c *GCPCluster) ValidateUpdate(oldRaw runtime.Object) (admission.Warnings,
)
}

if !reflect.DeepEqual(c.Spec.LoadBalancer, old.Spec.LoadBalancer) {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec", "LoadBalancer"),
c.Spec.LoadBalancer, "field is immutable"),
)
}

if len(allErrs) == 0 {
return nil, nil
}
Expand Down
3 changes: 3 additions & 0 deletions api/v1beta1/labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,9 @@ const (

// APIServerRoleTagValue describes the value for the apiserver role.
APIServerRoleTagValue = "apiserver"

// InternalRoleTagValue describes the value for the internal role.
InternalRoleTagValue = "api-internal"
)

// ClusterTagKey generates the key for resources associated with a cluster.
Expand Down
63 changes: 63 additions & 0 deletions api/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,26 @@ type Network struct {
// created for the API Server.
// +optional
APIServerForwardingRule *string `json:"apiServerForwardingRule,omitempty"`

// APIInternalAddress is the IPV4 regional address assigned to the
// internal Load Balancer.
// +optional
APIInternalAddress *string `json:"apiInternalIpAddress,omitempty"`
Copy link
Contributor

@barbacbd barbacbd May 8, 2024

Choose a reason for hiding this comment

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

I understand that this is a breaking change, but should we consider a structure to hold these values? The added values are the same as those for the api server, so a lot of duplicated values appear.

Copy link
Contributor Author

@bfournie bfournie May 8, 2024

Choose a reason for hiding this comment

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

The main reason I didn't do that was because it is a breaking change, I tried to keep the same API for consistency and just add the additional fields.

If the consensus is that its OK to change this API I can make it an array.

Copy link
Member

@cpanato cpanato May 20, 2024

Choose a reason for hiding this comment

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

make sense as we are adding this to add as a struct

wdyt @damdo

Copy link
Contributor Author

@bfournie bfournie May 20, 2024

Choose a reason for hiding this comment

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

OK, how would we handle the API in this case though? Would the existing fields - APIServerAddress, APIServerHealthCheck etc need to be marked as deprecated but still handled for a few releases until the deprecation is complete? It seems like it will add a lot of complexity to this API.

Copy link
Member

@damdo damdo May 24, 2024

Choose a reason for hiding this comment

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

I tend to agree with @bfournie here.
I'd probably stick with a non-breaking change, and then schedule a full API re-review for a new v1beta2 where we can improve the situation and plan the breaking changes we want to make to the API to clean it up.


// APIInternalHealthCheck is the full reference to the health check
// created for the internal Load Balancer.
// +optional
APIInternalHealthCheck *string `json:"apiInternalHealthCheck,omitempty"`

// APIInternalBackendService is the full reference to the backend service
// created for the internal Load Balancer.
// +optional
APIInternalBackendService *string `json:"apiInternalBackendService,omitempty"`

// APIInternalForwardingRule is the full reference to the forwarding rule
// created for the internal Load Balancer.
// +optional
APIInternalForwardingRule *string `json:"apiInternalForwardingRule,omitempty"`
Comment on lines +88 to +107
Copy link
Member

@damdo damdo May 24, 2024

Choose a reason for hiding this comment

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

Are these fields all required when the Internal Load Balancer is set?
If not are they defaulted to something in code?

Copy link
Contributor Author

@bfournie bfournie May 27, 2024

Choose a reason for hiding this comment

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

Similar to the fields for the External Load Balancer (APIServer fields) these will be set to the resources created for the Internal Load Balancer when it is created, including the cluster name.

Copy link
Member

@damdo damdo May 29, 2024

Choose a reason for hiding this comment

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

Got it thanks

}

// NetworkSpec encapsulates all things related to a GCP network.
Expand Down Expand Up @@ -114,6 +134,24 @@ type NetworkSpec struct {
LoadBalancerBackendPort *int32 `json:"loadBalancerBackendPort,omitempty"`
}

// LoadBalancerType defines the Load Balancer that should be created.
type LoadBalancerType string

var (
// External creates a Global External Proxy Load Balancer
// to manage traffic to backends in multiple regions. This is the default Load
// Balancer and will be created if no LoadBalancerType is defined.
External = LoadBalancerType("External")

// Internal creates a Regional Internal Passthrough Load
// Balancer to manage traffic to backends in the configured region.
Internal = LoadBalancerType("Internal")

// InternalExternal creates both External and Internal Load Balancers to provide
// separate endpoints for managing both external and internal traffic.
InternalExternal = LoadBalancerType("InternalExternal")
)

// LoadBalancerSpec contains configuration for one or more LoadBalancers.
type LoadBalancerSpec struct {
// APIServerInstanceGroupTagOverride overrides the default setting for the
Expand All @@ -123,6 +161,15 @@ type LoadBalancerSpec struct {
// +kubebuilder:validation:Pattern=`(^[1-9][0-9]{0,31}$)|(^[a-z][a-z0-9-]{4,28}[a-z0-9]$)`
// +optional
APIServerInstanceGroupTagOverride *string `json:"apiServerInstanceGroupTagOverride,omitempty"`

// LoadBalancerType defines the type of Load Balancer that should be created.
// If not set, a Global External Proxy Load Balancer will be created by default.
// +optional
LoadBalancerType *LoadBalancerType `json:"loadBalancerType,omitempty"`
Copy link
Contributor Author

@bfournie bfournie May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I had thought about but felt that type of API presents too much ambiguity and potential for error. For example with that type of array you could have the following potential combinations, all of which are in error:

  • empty (no LBs defined)
  • INTERNAL/INTERNAL
  • EXTERNAL/EXTERNAL

Explicitly defining the Load Balancer configuration options seems more useful.

Copy link

@nrb nrb May 20, 2024

Choose a reason for hiding this comment

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

I could envision a map[string][]LoadBalancers, with the only keys being external and internal. That way, there's less ambiguity with regards to where the LB will be facing.

However, I'm not sure that such complexity is worth it right now, or if there's near-term a use case for extending to N number of load balancers for internal/external.

In that regard, I personally prefer the solution presented here, since it's avoid ambiguity and doesn't impose too many more validations.

Copy link

@sadasu sadasu May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can achieve those via kubebuilder validations :
// +kubebuilder:validation:UniqueItems= to make sure array entries are unique
and // +kubebuilder:default:="External" will make sure it would not be empty.

I know I had suggested making LoadBalancers of type []LoadBalancerType, with the array containing ExternalLoadBalancerOnly and/or InternalLoadBalancerOnly but looking at the rest of the implementation, I am not sure that is the right path.

Copy link

@sadasu sadasu May 23, 2024

Choose a reason for hiding this comment

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

Are the 4 optional fields added to Network type allowed to be empty when LoadBalancerType is InternalLoadBalancerOnly/DualLoadBalancer ?

Copy link

@sadasu sadasu May 23, 2024

Choose a reason for hiding this comment

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

Also, can the value of LoadBalancerType change once set i.e is it immutable? If so, do we need a validateUpdate() webhook preventing an update for LoadBalancerType?

Copy link
Contributor Author

@bfournie bfournie May 23, 2024

Choose a reason for hiding this comment

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

Are the 4 optional fields added to Network type allowed to be empty when LoadBalancerType is InternalLoadBalancerOnly/DualLoadBalancer ?

Yes, these marked as +optional and are string pointers so its OK to not be set

Copy link
Contributor Author

@bfournie bfournie May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, can the value of LoadBalancerType change once set i.e is it immutable? If so, do we need a validateUpdate() webhook preventing an update for LoadBalancerType?

That's a good point, thanks for catching that. I will add a webhook check for it.

Done,


// InternalLoadBalancer is the configuration for an Internal Passthrough Network Load Balancer.
// +optional
InternalLoadBalancer *LoadBalancer `json:"internalLoadBalancer,omitempty"`
}

// SubnetSpec configures an GCP Subnet.
Expand Down Expand Up @@ -278,3 +325,19 @@ type ObjectReference struct {
// +kubebuilder:validation:Required
Name string `json:"name"`
}

// LoadBalancer specifies the configuration of a LoadBalancer.
type LoadBalancer struct {
// Name is the name of the Load Balancer. If not set a default name
// will be used. For an Internal Load Balancer service the default
// name is "api-internal".
// +kubebuilder:validation:Optional
// +kubebuilder:validation:Pattern=`(^[1-9][0-9]{0,31}$)|(^[a-z][a-z0-9-]{4,28}[a-z0-9]$)`
// +optional
Name *string `json:"name,omitempty"`

// Subnet is the name of the subnet to use for a regional Load Balancer. A subnet is
// required for the Load Balancer, if not defined the first configured subnet will be
// used.
Subnet *string `json:"subnet,omitempty"`
}
55 changes: 55 additions & 0 deletions 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.

16 changes: 8 additions & 8 deletions cloud/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,18 +290,18 @@ func (s *ClusterScope) FirewallRulesSpec() []*compute.Firewall {
// ANCHOR: ClusterControlPlaneSpec

// AddressSpec returns google compute address spec.
func (s *ClusterScope) AddressSpec() *compute.Address {
func (s *ClusterScope) AddressSpec(lbname string) *compute.Address {
return &compute.Address{
Name: fmt.Sprintf("%s-%s", s.Name(), infrav1.APIServerRoleTagValue),
Name: fmt.Sprintf("%s-%s", s.Name(), lbname),
AddressType: "EXTERNAL",
IpVersion: "IPV4",
}
}

// BackendServiceSpec returns google compute backend-service spec.
func (s *ClusterScope) BackendServiceSpec() *compute.BackendService {
func (s *ClusterScope) BackendServiceSpec(lbname string) *compute.BackendService {
return &compute.BackendService{
Name: fmt.Sprintf("%s-%s", s.Name(), infrav1.APIServerRoleTagValue),
Name: fmt.Sprintf("%s-%s", s.Name(), lbname),
LoadBalancingScheme: "EXTERNAL",
PortName: "apiserver",
Protocol: "TCP",
Expand All @@ -310,24 +310,24 @@ func (s *ClusterScope) BackendServiceSpec() *compute.BackendService {
}

// ForwardingRuleSpec returns google compute forwarding-rule spec.
func (s *ClusterScope) ForwardingRuleSpec() *compute.ForwardingRule {
func (s *ClusterScope) ForwardingRuleSpec(lbname string) *compute.ForwardingRule {
port := int32(443)
if c := s.Cluster.Spec.ClusterNetwork; c != nil {
port = ptr.Deref(c.APIServerPort, 443)
}
portRange := fmt.Sprintf("%d-%d", port, port)
return &compute.ForwardingRule{
Name: fmt.Sprintf("%s-%s", s.Name(), infrav1.APIServerRoleTagValue),
Name: fmt.Sprintf("%s-%s", s.Name(), lbname),
IPProtocol: "TCP",
LoadBalancingScheme: "EXTERNAL",
PortRange: portRange,
}
}

// HealthCheckSpec returns google compute health-check spec.
func (s *ClusterScope) HealthCheckSpec() *compute.HealthCheck {
func (s *ClusterScope) HealthCheckSpec(lbname string) *compute.HealthCheck {
return &compute.HealthCheck{
Name: fmt.Sprintf("%s-%s", s.Name(), infrav1.APIServerRoleTagValue),
Name: fmt.Sprintf("%s-%s", s.Name(), lbname),
Type: "HTTPS",
HttpsHealthCheck: &compute.HTTPSHealthCheck{
Port: 6443,
Expand Down
Loading