From 40cf974cf3bfb7d0fb6aaf54ab51f8aeff0686ee Mon Sep 17 00:00:00 2001 From: klibr007 <50625221+klibr007@users.noreply.github.com> Date: Fri, 14 Feb 2025 11:26:49 +0100 Subject: [PATCH] Better management of status and conditions for externalIP --- api/v1alpha1/externalip_types.go | 24 +++-- api/v1alpha1/zz_generated.deepcopy.go | 7 ++ .../kubestatic.quortex.io_externalips.yaml | 57 ++++++++++ helm/kubestatic/crds/crds.yaml | 56 ++++++++++ internal/controller/externalip_controller.go | 57 +++++++--- .../provider/aws/converter/error_decoder.go | 3 + internal/provider/aws/provider.go | 101 +++++++++++++++++- internal/provider/errors.go | 10 ++ 8 files changed, 292 insertions(+), 23 deletions(-) diff --git a/api/v1alpha1/externalip_types.go b/api/v1alpha1/externalip_types.go index b0a20f7..06896ae 100644 --- a/api/v1alpha1/externalip_types.go +++ b/api/v1alpha1/externalip_types.go @@ -42,11 +42,24 @@ type ExternalIPState string // All defined ExternalIPStates const ( - ExternalIPStateNone ExternalIPState = "" ExternalIPStateReserved ExternalIPState = "Reserved" + ExternalIPStatePending ExternalIPState = "Pending" ExternalIPStateAssociated ExternalIPState = "Associated" ) +// The list of condition types. +const ( + ExternalIPConditionTypeIPCreated = "IPCreated" + ExternalIPConditionTypeNetworkInterfaceAssociated = "NetworkInterfaceAssociated" +) + +// The list of condition reasons. +const ( + ExternalIPConditionReasonProviderError = "ProviderError" + ExternalIPConditionReasonIPCreated = "IPCreated" + ExternalIPConditionReasonNetworkInterfaceAssociated = "NetworkInterfaceAssociated" +) + // ExternalIPStatus defines the observed state of ExternalIP type ExternalIPStatus struct { // The current state of the ExternalIP @@ -60,6 +73,10 @@ type ExternalIPStatus struct { // The instance identifier InstanceID *string `json:"instanceID,omitempty"` + + // Current conditions of the ExternalIP. + // +kubebuilder:validation:Optional + Conditions []metav1.Condition `json:"conditions,omitempty"` } // +kubebuilder:object:root=true @@ -78,11 +95,6 @@ type ExternalIP struct { Status ExternalIPStatus `json:"status,omitempty"` } -// IsReserved returns if externalIP is reserved -func (e *ExternalIP) IsReserved() bool { - return e.Status.State == ExternalIPStateReserved -} - // IsAssociated returns if externalIP is associated func (e *ExternalIP) IsAssociated() bool { return e.Status.State == ExternalIPStateAssociated diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 7f8c414..63a3fca 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -117,6 +117,13 @@ func (in *ExternalIPStatus) DeepCopyInto(out *ExternalIPStatus) { *out = new(string) **out = **in } + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]v1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ExternalIPStatus. diff --git a/config/crd/bases/kubestatic.quortex.io_externalips.yaml b/config/crd/bases/kubestatic.quortex.io_externalips.yaml index dee0830..dd1468d 100644 --- a/config/crd/bases/kubestatic.quortex.io_externalips.yaml +++ b/config/crd/bases/kubestatic.quortex.io_externalips.yaml @@ -70,6 +70,63 @@ spec: addressID: description: The address identifier type: string + conditions: + description: Current conditions of the ExternalIP. + items: + description: Condition contains details for one aspect of the current + state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array instanceID: description: The instance identifier type: string diff --git a/helm/kubestatic/crds/crds.yaml b/helm/kubestatic/crds/crds.yaml index f9a4b36..b2782c2 100644 --- a/helm/kubestatic/crds/crds.yaml +++ b/helm/kubestatic/crds/crds.yaml @@ -66,6 +66,62 @@ spec: addressID: description: The address identifier type: string + conditions: + description: Current conditions of the ExternalIP. + items: + description: Condition contains details for one aspect of the current state of this API Resource. + properties: + lastTransitionTime: + description: |- + lastTransitionTime is the last time the condition transitioned from one status to another. + This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. + format: date-time + type: string + message: + description: |- + message is a human readable message indicating details about the transition. + This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: |- + observedGeneration represents the .metadata.generation that the condition was set based upon. + For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date + with respect to the current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: |- + reason contains a programmatic identifier indicating the reason for the condition's last transition. + Producers of specific condition types may define expected values and meanings for this field, + and whether the values are considered a guaranteed API. + The value should be a CamelCase string. + This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + type: array instanceID: description: The instance identifier type: string diff --git a/internal/controller/externalip_controller.go b/internal/controller/externalip_controller.go index 6d89c6a..022bd87 100644 --- a/internal/controller/externalip_controller.go +++ b/internal/controller/externalip_controller.go @@ -19,11 +19,12 @@ package controller import ( "context" "encoding/json" + "errors" "fmt" - "reflect" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/strategicpatch" @@ -63,7 +64,7 @@ func (r *ExternalIPReconciler) Reconcile(ctx context.Context, req ctrl.Request) externalIP := &v1alpha1.ExternalIP{} if err := r.Get(ctx, req.NamespacedName, externalIP); err != nil { - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { // Request object not found, could have been deleted after reconcile request. // Return and don't requeue log.Info("ExternalIP resource not found. Ignoring since object must be deleted") @@ -95,7 +96,7 @@ func (r *ExternalIPReconciler) Reconcile(ctx context.Context, req ctrl.Request) var instanceID string if externalIP.Spec.NodeName != "" { if err := r.Get(ctx, types.NamespacedName{Name: externalIP.Spec.NodeName}, &node); err != nil { - if errors.IsNotFound(err) { + if apierrors.IsNotFound(err) { // Invalid nodeName, remove ExternalIP nodeName attribute. externalIP.Spec.NodeName = "" if err != r.Update(ctx, externalIP) { @@ -118,6 +119,10 @@ func (r *ExternalIPReconciler) Reconcile(ctx context.Context, req ctrl.Request) if externalIP.ObjectMeta.DeletionTimestamp.IsZero() { status, err = r.Provider.ReconcileExternalIP(ctx, log, instanceID, externalIP) if err != nil { + if patchErr := patchExternalIPStatus(ctx, r, externalIP, status); patchErr != nil { + log.Error(errors.Join(patchErr, err), "Failed to patch ExternalIP status during error handling") + return ctrl.Result{}, fmt.Errorf("failed to patch ExternalIP status during error handling: %w", errors.Join(patchErr, err)) + } log.Error(err, "Failed to reconcile ExternalIP") return ctrl.Result{}, err } @@ -168,17 +173,9 @@ func (r *ExternalIPReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } - // Copy the existing ExternalIP to avoid mutating the original - existingExternalIP := externalIP.DeepCopy() - - // Patch the Pool status if it differs from the desired status - externalIP.Status = status - if !reflect.DeepEqual(externalIP.Status, existingExternalIP.Status) { - err := r.Status().Patch(ctx, externalIP, client.MergeFrom(existingExternalIP)) - if err != nil { - log.Error(err, "Failed to patch ExternalIP status") - return ctrl.Result{}, fmt.Errorf("failed to patch ExternalIP status: %w", err) - } + if err := patchExternalIPStatus(ctx, r, externalIP, status); err != nil { + log.Error(err, "Failed to patch ExternalIP status") + return ctrl.Result{}, fmt.Errorf("failed to patch ExternalIP status: %w", err) } log.Info("ExternalIP successfully reconciled") @@ -215,3 +212,33 @@ func (r *ExternalIPReconciler) SetupWithManager(mgr ctrl.Manager) error { ). Complete(r) } + +// patchExternalIPStatus updates the status of a ExternalIP resource if there are any changes. +// It patches the status with the new status provided and updates the LastTransitionTime if there are differences. +// +// Parameters: +// +// ctx - The context for the request. +// r - The ExternalIPReconciler responsible for reconciling the ExternalIP resource. +// externalIP - The ExternalIP resource to be updated. +// newStatus - The new status to be applied to the ExternalIP resource. +// +// Returns: +// +// error - An error if the patch operation fails, otherwise nil. +func patchExternalIPStatus( + ctx context.Context, + r *ExternalIPReconciler, + externalIP *v1alpha1.ExternalIP, + newStatus v1alpha1.ExternalIPStatus, +) error { + existingEIP := externalIP.DeepCopy() + externalIP.Status = newStatus + + if !equality.Semantic.DeepEqual(externalIP.Status, existingEIP.Status) { + if err := r.Status().Patch(ctx, externalIP, client.MergeFrom(existingEIP)); err != nil { + return err + } + } + return nil +} diff --git a/internal/provider/aws/converter/error_decoder.go b/internal/provider/aws/converter/error_decoder.go index 9a6eeab..2b824b0 100644 --- a/internal/provider/aws/converter/error_decoder.go +++ b/internal/provider/aws/converter/error_decoder.go @@ -61,6 +61,9 @@ func DecodeCommonError(msg string, err error) error { "MalformedQueryString", "ValidationError": return &provider.Error{Code: provider.BadRequestError, Msg: msg} + case + "AddressLimitExceeded": + return &provider.Error{Code: provider.AddressLimitExceededError, Msg: msg} case "RulesPerSecurityGroupLimitExceeded", "Throttling": diff --git a/internal/provider/aws/provider.go b/internal/provider/aws/provider.go index af497c7..fd370a0 100644 --- a/internal/provider/aws/provider.go +++ b/internal/provider/aws/provider.go @@ -857,23 +857,62 @@ func (p *awsProvider) ReconcileExternalIP( instanceID string, externalIP *v1alpha1.ExternalIP, ) (v1alpha1.ExternalIPStatus, error) { - status := externalIP.Status + status := v1alpha1.ExternalIPStatus{ + State: v1alpha1.ExternalIPStatePending, + Conditions: []kmetav1.Condition{}, + } // Get the address associated with the instance address, err := p.getAddress(ctx, Managed(), WithExternalIPName(externalIP.Name)) if err != nil && err.(*provider.Error).Code != provider.NotFoundError { + status := v1alpha1.ExternalIPStatus{ + State: v1alpha1.ExternalIPStatePending, + Conditions: []kmetav1.Condition{}, + } + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionReasonIPCreated, + Status: kmetav1.ConditionUnknown, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.FirewallRuleConditionReasonProviderError, + Message: fmt.Sprintf("Failed to get address: %s", err), + }) return status, fmt.Errorf("failed to get address: %w", err) } if address == nil { addressID, err := p.createAddress(ctx, externalIP.Name, instanceID) if err != nil { + if provider.IsErrAddressLimitExceeded(err) { + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionReasonIPCreated, + Status: kmetav1.ConditionFalse, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonProviderError, + Message: "Could not create address: The maximum number of addresses has been reached", + }) + } else { + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionReasonIPCreated, + Status: kmetav1.ConditionFalse, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonProviderError, + Message: fmt.Sprintf("Failed to create address: %s", err), + }) + } + return status, fmt.Errorf("failed to create address: %w", err) } log.Info("Address created", "addressID", addressID) address, err = p.getAddress(ctx, Managed(), WithExternalIPName(externalIP.Name), WithAddressID(addressID)) if err != nil { + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionReasonIPCreated, + Status: kmetav1.ConditionTrue, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonProviderError, + Message: fmt.Sprintf("Failed to get address: %s", err), + }) return status, fmt.Errorf("failed to get address: %w", err) } } @@ -881,7 +920,23 @@ func (p *awsProvider) ReconcileExternalIP( status.AddressID = address.AllocationId status.PublicIPAddress = address.PublicIp + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionReasonIPCreated, + Status: kmetav1.ConditionTrue, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonIPCreated, + Message: "The IP has been successfully created", + }) + if instanceID == "" { + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionTypeNetworkInterfaceAssociated, + Status: kmetav1.ConditionFalse, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonProviderError, + Message: "Address has no instanceID", + }) + if address.AssociationId == nil { return status, nil } @@ -906,17 +961,42 @@ func (p *awsProvider) ReconcileExternalIP( // Get the first network interface with a public IP address networkInterface, err := eniWithPublicIP(instance) if err != nil { + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionTypeNetworkInterfaceAssociated, + Status: kmetav1.ConditionUnknown, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonProviderError, + Message: fmt.Sprintf("Failed to get network interface with public IP: %s", err), + }) return status, fmt.Errorf("failed to get network interface with public IP: %w", err) } if address.NetworkInterfaceId != nil { // Address is already associated with the instance if *address.NetworkInterfaceId == *networkInterface.NetworkInterfaceId { + status.State = v1alpha1.ExternalIPStateAssociated + + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionTypeNetworkInterfaceAssociated, + Status: kmetav1.ConditionTrue, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonNetworkInterfaceAssociated, + Message: fmt.Sprintf("Address %s associated with network interface %s", + aws.StringValue(address.AllocationId), aws.StringValue(networkInterface.NetworkInterfaceId)), + }) return status, nil } // Disassociate the address from the current network interface if err := p.disassociateAddress(ctx, *address.AssociationId); err != nil { + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionTypeNetworkInterfaceAssociated, + Status: kmetav1.ConditionFalse, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonProviderError, + Message: fmt.Sprintf("Failed to disassociate address %s with %s", + aws.StringValue(address.AllocationId), aws.StringValue(address.AssociationId)), + }) return status, fmt.Errorf("failed to disassociate address: %w", err) } log.Info("Address disassociated", "addressID", address.AllocationId, "associationID", *address.AssociationId) @@ -927,6 +1007,14 @@ func (p *awsProvider) ReconcileExternalIP( // Associate the address with the network interface if err := p.associateAddress(ctx, aws.StringValue(address.AllocationId), *networkInterface.NetworkInterfaceId); err != nil { + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionTypeNetworkInterfaceAssociated, + Status: kmetav1.ConditionFalse, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonProviderError, + Message: fmt.Sprintf("Failed to associate address: %s", err), + }) + return status, fmt.Errorf("failed to associate address: %w", err) } log.Info("Address associated", "addressID", address.AllocationId, "networkInterfaceID", *networkInterface.NetworkInterfaceId) @@ -934,6 +1022,15 @@ func (p *awsProvider) ReconcileExternalIP( status.InstanceID = &instanceID status.State = v1alpha1.ExternalIPStateAssociated + meta.SetStatusCondition(&status.Conditions, kmetav1.Condition{ + Type: v1alpha1.ExternalIPConditionTypeNetworkInterfaceAssociated, + Status: kmetav1.ConditionTrue, + ObservedGeneration: externalIP.Generation, + Reason: v1alpha1.ExternalIPConditionReasonNetworkInterfaceAssociated, + Message: fmt.Sprintf("Address %s associated with network interface %s", + aws.StringValue(address.AllocationId), aws.StringValue(networkInterface.NetworkInterfaceId)), + }) + return status, nil } @@ -957,7 +1054,7 @@ func (p *awsProvider) ReconcileExternalIPDeletion( address, err := p.getAddress(ctx, Managed(), WithExternalIPName(externalIP.Name)) if err != nil { // The address does not exist, end of reconciliation - if err.(*provider.Error).Code != provider.NotFoundError { + if err.(*provider.Error).Code == provider.NotFoundError { return nil } return fmt.Errorf("failed to get address: %w", err) diff --git a/internal/provider/errors.go b/internal/provider/errors.go index 530a2d9..d9595d3 100644 --- a/internal/provider/errors.go +++ b/internal/provider/errors.go @@ -19,6 +19,8 @@ const ( ConflictError ErrorCode = "ConflictError" // RulesPerSecurityGroupLimitExceededError is when the request could not be processed because of a limit exceeded RulesPerSecurityGroupLimitExceededError ErrorCode = "RulesPerSecurityGroupLimitExceeded" + // AddressLimitExceededError is when the request could not be processed because of a limit exceeded + AddressLimitExceededError ErrorCode = "AddressLimitExceeded" // InternalError is when there was an unexpected error in the server InternalError ErrorCode = "InternalError" ) @@ -80,3 +82,11 @@ func IsErrRulesPerSecurityGroupLimitExceeded(err error) bool { } return false } + +// IsErrAddressLimitExceeded returns if error is kind AddressLimitExceededError +func IsErrAddressLimitExceeded(err error) bool { + if e, ok := err.(*Error); ok { + return e.Code == AddressLimitExceededError + } + return false +}