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.

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

VPC: Add Custom Image reconciliation #1942

Merged
merged 1 commit into from
Sep 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions api/v1beta2/conditions_consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ const (

// ImageImportFailedReason used when the image import is failed.
ImageImportFailedReason = "ImageImportFailed"

// ImageReconciliationFailedReason used when an error occurs during VPC Custom Image reconciliation.
ImageReconciliationFailedReason = "ImageReconciliationFailed"
)

const (
Expand Down
2 changes: 1 addition & 1 deletion api/v1beta2/ibmvpccluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ type ImageSpec struct {
// name is the name of the desired VPC Custom Image.
// +kubebuilder:validation:MinLength:=1
// +kubebuilder:validation:MaxLength:=63
// +kubebuilder:validation:Pattern='/^([a-z]|[a-z][-a-z0-9]*[a-z0-9])$/'
// +kubebuilder:validation:Pattern=`^([a-z]|[a-z][-a-z0-9]*[a-z0-9])$`
// +optional
Name *string `json:"name,omitempty"`

Expand Down
56 changes: 56 additions & 0 deletions cloud/scope/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"strconv"
"strings"

"sigs.k8s.io/controller-runtime/pkg/client"

Expand Down Expand Up @@ -57,3 +58,58 @@ func CheckCreateInfraAnnotation(cluster infrav1beta2.IBMPowerVSCluster) bool {
}
return createInfra
}

// CRN is a local duplicate of IBM Cloud CRN for parsing and references.
type CRN struct {
Scheme string
Version string
CName string
CType string
ServiceName string
Region string
ScopeType string
Scope string
ServiceInstance string
ResourceType string
Resource string
}

// ParseCRN is a local duplicate of IBM Cloud CRN Parse functionality, to convert a string into a CRN, if it is in the correct format.
func ParseCRN(s string) (*CRN, error) {
if s == "" {
return nil, nil
}

segments := strings.Split(s, ":")
if len(segments) != 10 || segments[0] != "crn" {
return nil, fmt.Errorf("malformed CRN")
}

crn := &CRN{
Scheme: segments[0],
Version: segments[1],
CName: segments[2],
CType: segments[3],
ServiceName: segments[4],
Region: segments[5],
ServiceInstance: segments[7],
ResourceType: segments[8],
Resource: segments[9],
}

// Scope portions require additional parsing.
scopeSegments := segments[6]
if scopeSegments != "" {
if scopeSegments == "global" {
crn.Scope = scopeSegments
} else {
scopeParts := strings.Split(scopeSegments, "/")
if len(scopeParts) != 2 {
return nil, fmt.Errorf("malformed scope in CRN")
}
crn.ScopeType, crn.Scope = scopeParts[0], scopeParts[1]
}
}

return crn, nil
}
192 changes: 192 additions & 0 deletions cloud/scope/vpc_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,16 @@ func (s *VPCClusterScope) SetResourceStatus(resourceType infrav1beta2.ResourceTy
s.IBMVPCCluster.Status.Network.VPC = resource
}
s.NetworkStatus().VPC.Set(*resource)
case infrav1beta2.ResourceTypeCustomImage:
if s.IBMVPCCluster.Status.Image == nil {
s.IBMVPCCluster.Status.Image = &infrav1beta2.ResourceStatus{
ID: resource.ID,
Name: resource.Name,
Ready: resource.Ready,
}
return
}
s.IBMVPCCluster.Status.Image.Set(*resource)
default:
s.V(3).Info("unsupported resource type", "resourceType", resourceType)
}
Expand Down Expand Up @@ -494,3 +504,185 @@ func (s *VPCClusterScope) createVPC() (*vpcv1.VPC, error) {

return vpcDetails, nil
}

// ReconcileVPCCustomImage reconciles the VPC Custom Image.
func (s *VPCClusterScope) ReconcileVPCCustomImage() (bool, error) {
// VPC Custom Image reconciliation is based on the following possibilities.
// 1. Check Status for ID or Name, from previous lookup in reconciliation loop.
// 2. If no Image spec is provided, assume the image is managed externally, thus no reconciliation required.
// 3. If Image name is provided, check if an existing VPC Custom Image exists with that name (unfortunately names may not be unique), checking status of the image, updating appropriately.
// 4. If Image CRN is provided, parse the ID from the CRN to perform lookup. CRN may be for another account, causing lookup to fail (permissions), may require better safechecks based on other CRN details.
// 5. If no Image ID has been identified, assume a VPC Custom Image needs to be created, do so.
var imageID *string
// Attempt to collect VPC Custom Image info from Status.
if s.IBMVPCCluster.Status.Image != nil {
if s.IBMVPCCluster.Status.Image.ID != "" {
imageID = ptr.To(s.IBMVPCCluster.Status.Image.ID)
}
} else if s.IBMVPCCluster.Spec.Image == nil {
// If no Image spec was defined, we expect it is maintained externally and continue without reconciling. For example, using a Catalog Offering Custom Image, which may be in another account, which means it cannot be looked up, but can be used when creating Instances.
s.V(3).Info("No VPC Custom Image defined, skipping reconciliation")
return false, nil
} else if s.IBMVPCCluster.Spec.Image.Name != nil {
// Attempt to retrieve the image details via the name, if it already exists
imageDetails, err := s.VPCClient.GetImageByName(*s.IBMVPCCluster.Spec.Image.Name)
if err != nil {
return false, fmt.Errorf("error checking vpc custom image by name: %w", err)
} else if imageDetails != nil && imageDetails.ID != nil {
// Prevent relookup (API request) of VPC Custom Image if we already have the necessary data
requeue := true
if imageDetails.Status != nil && *imageDetails.Status == string(vpcv1.ImageStatusAvailableConst) {
requeue = false
}
s.SetResourceStatus(infrav1beta2.ResourceTypeCustomImage, &infrav1beta2.ResourceStatus{
ID: *imageDetails.ID,
Name: s.IBMVPCCluster.Spec.Image.Name,
// Ready status will be invert of the need to requeue.
Ready: !requeue,
})
return requeue, nil
}
} else if s.IBMVPCCluster.Spec.Image.CRN != nil {
// Parse the supplied Image CRN for Id, to perform image lookup.
imageCRN, err := ParseCRN(*s.IBMVPCCluster.Spec.Image.CRN)
if err != nil {
return false, fmt.Errorf("error parsing vpc custom image crn: %w", err)
}
// If the value provided isn't a CRN or is missing the Resource ID, raise an error.
if imageCRN == nil || imageCRN.Resource == "" {
return false, fmt.Errorf("error parsing vpc custom image crn, missing resource id")
}
// If we didn't hit an error during parsing, and Resource was set, set that as the Image ID.
imageID = ptr.To(imageCRN.Resource)
}
Comment on lines +516 to +557
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
var imageID *string
// Attempt to collect VPC Custom Image info from Status.
if s.IBMVPCCluster.Status.Image != nil {
if s.IBMVPCCluster.Status.Image.ID != "" {
imageID = ptr.To(s.IBMVPCCluster.Status.Image.ID)
}
} else if s.IBMVPCCluster.Spec.Image == nil {
// If no Image spec was defined, we expect it is maintained externally and continue without reconciling. For example, using a Catalog Offering Custom Image, which may be in another account, which means it cannot be looked up, but can be used when creating Instances.
s.V(3).Info("No VPC Custom Image defined, skipping reconciliation")
return false, nil
} else if s.IBMVPCCluster.Spec.Image.Name != nil {
// Attempt to retrieve the image details via the name, if it already exists
imageDetails, err := s.VPCClient.GetImageByName(*s.IBMVPCCluster.Spec.Image.Name)
if err != nil {
return false, fmt.Errorf("error checking vpc custom image by name: %w", err)
} else if imageDetails != nil && imageDetails.ID != nil {
// Prevent relookup (API request) of VPC Custom Image if we already have the necessary data
requeue := true
if imageDetails.Status != nil && *imageDetails.Status == string(vpcv1.ImageStatusAvailableConst) {
requeue = false
}
s.SetResourceStatus(infrav1beta2.ResourceTypeCustomImage, &infrav1beta2.ResourceStatus{
ID: *imageDetails.ID,
Name: s.IBMVPCCluster.Spec.Image.Name,
// Ready status will be invert of the need to requeue.
Ready: !requeue,
})
return requeue, nil
}
} else if s.IBMVPCCluster.Spec.Image.CRN != nil {
// Parse the supplied Image CRN for Id, to perform image lookup.
imageCRN, err := ParseCRN(*s.IBMVPCCluster.Spec.Image.CRN)
if err != nil {
return false, fmt.Errorf("error parsing vpc custom image crn: %w", err)
}
// If the value provided isn't a CRN or is missing the Resource ID, raise an error.
if imageCRN == nil || imageCRN.Resource == "" {
return false, fmt.Errorf("error parsing vpc custom image crn, missing resource id")
}
// If we didn't hit an error during parsing, and Resource was set, set that as the Image ID.
imageID = ptr.To(imageCRN.Resource)
}
if s.IBMVPCCluster.Spec.Image == nil {
// If no Image spec was defined, we expect it is maintained externally and continue without reconciling. For example, using a Catalog Offering Custom Image, which may be in another account, which means it cannot be looked up, but can be used when creating Instances.
s.V(3).Info("No VPC Custom Image defined, skipping reconciliation")
return false, nil
} else if s.IBMVPCCluster.Spec.Image.Name != nil {
// Attempt to retrieve the image details via the name, if it already exists
imageDetails, err := s.VPCClient.GetImageByName(*s.IBMVPCCluster.Spec.Image.Name)
if err != nil {
return false, fmt.Errorf("error checking vpc custom image by name: %w", err)
} else if imageDetails != nil && imageDetails.ID != nil {
// Prevent relookup (API request) of VPC Custom Image if we already have the necessary data
requeue := true
if imageDetails.Status != nil && *imageDetails.Status == string(vpcv1.ImageStatusAvailableConst) {
requeue = false
}
s.SetResourceStatus(infrav1beta2.ResourceTypeCustomImage, &infrav1beta2.ResourceStatus{
ID: *imageDetails.ID,
Name: s.IBMVPCCluster.Spec.Image.Name,
// Ready status will be invert of the need to requeue.
Ready: !requeue,
})
return requeue, nil
}
}
var imageID *string
// Attempt to collect VPC Custom Image info from Status.
if s.IBMVPCCluster.Status.Image != nil {
if s.IBMVPCCluster.Status.Image.ID != "" {
imageID = ptr.To(s.IBMVPCCluster.Status.Image.ID)
}
} else if s.IBMVPCCluster.Spec.Image.CRN != nil {
// Parse the supplied Image CRN for Id, to perform image lookup.
imageCRN, err := ParseCRN(*s.IBMVPCCluster.Spec.Image.CRN)
if err != nil {
return false, fmt.Errorf("error parsing vpc custom image crn: %w", err)
}
// If the value provided isn't a CRN or is missing the Resource ID, raise an error.
if imageCRN == nil || imageCRN.Resource == "" {
return false, fmt.Errorf("error parsing vpc custom image crn, missing resource id")
}
// If we didn't hit an error during parsing, and Resource was set, set that as the Image ID.
imageID = ptr.To(imageCRN.Resource)
}

Can we please rearrange something like this?
Checking existing image based on name can be a separate block and using id to check the image status in a different block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I do not want to rearrange to check Spec prior to Status in this way.

I could move s.IBMVPCCluster.Spec.Image == nil first, prior to Status check, but that would be the only check I'd want to move prior to Status check.


// Check status of VPC Custom Image.
if imageID != nil {
image, _, err := s.VPCClient.GetImage(&vpcv1.GetImageOptions{
ID: imageID,
})
if err != nil {
return false, fmt.Errorf("error retrieving vpc custom image by id: %w", err)
}
if image == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are passing id won't it return an error if the image is not found?
Please see whether we need this nil check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the image is not found, I thought we got a 404 DetailedResponse back. I am defensively not expecting a non-error (error == nil) to result in image always being set.

return false, fmt.Errorf("error failed to retrieve vpc custom image with id %s", *imageID)
}
s.V(3).Info("Found VPC Custom Image with provided id", "imageID", imageID)

requeue := true
if image.Status != nil && *image.Status == string(vpcv1.ImageStatusAvailableConst) {
requeue = false
}
s.SetResourceStatus(infrav1beta2.ResourceTypeCustomImage, &infrav1beta2.ResourceStatus{
ID: *imageID,
Name: image.Name,
// Ready status will be invert of the need to requeue.
Ready: !requeue,
})
return requeue, nil
}

// No VPC Custom Image exists or was found, so create the Custom Image.
s.V(3).Info("Creating a VPC Custom Image")
err := s.createCustomImage()
if err != nil {
return false, fmt.Errorf("error failure trying to create vpc custom image: %w", err)
}

s.V(3).Info("Successfully created VPC Custom Image")
return true, nil
}

// createCustomImage will create a new VPC Custom Image.
func (s *VPCClusterScope) createCustomImage() error {
// TODO(cjschaef): Remove in favor of webhook validation.
if s.IBMVPCCluster.Spec.Image.OperatingSystem == nil {
return fmt.Errorf("error failed to create vpc custom image due to missing operatingSystem")
}

// Collect the Resource Group ID.
var resourceGroupID *string
// Check Resource Group in Image spec.
if s.IBMVPCCluster.Spec.Image.ResourceGroup != nil {
if s.IBMVPCCluster.Spec.Image.ResourceGroup.ID != "" {
resourceGroupID = ptr.To(s.IBMVPCCluster.Spec.Image.ResourceGroup.ID)
} else if s.IBMVPCCluster.Spec.Image.ResourceGroup.Name != nil {
id, err := s.ResourceManagerClient.GetResourceGroupByName(*s.IBMVPCCluster.Spec.Image.ResourceGroup.Name)
if err != nil {
return fmt.Errorf("error retrieving resource group by name: %w", err)
}
resourceGroupID = id.ID
}
} else {
// Otherwise, we will use the cluster Resource Group ID, as we expect to create all resources in that Resource Group.
id, err := s.GetResourceGroupID()
if err != nil {
return fmt.Errorf("error retrieving resource group id: %w", err)
}
resourceGroupID = ptr.To(id)
}

// Build the COS Object URL using the ImageSpec
fileHRef, err := s.buildCOSObjectHRef()
if err != nil {
return fmt.Errorf("error building vpc custom image file href: %w", err)
}

options := &vpcv1.CreateImageOptions{
ImagePrototype: &vpcv1.ImagePrototype{
Name: s.IBMVPCCluster.Spec.Image.Name,
File: &vpcv1.ImageFilePrototype{
Href: fileHRef,
},
OperatingSystem: &vpcv1.OperatingSystemIdentity{
Name: s.IBMVPCCluster.Spec.Image.OperatingSystem,
},
ResourceGroup: &vpcv1.ResourceGroupIdentity{
ID: resourceGroupID,
},
},
}

imageDetails, _, err := s.VPCClient.CreateImage(options)
if err != nil {
return fmt.Errorf("error unknown failure creating vpc custom image: %w", err)
}
if imageDetails == nil || imageDetails.ID == nil || imageDetails.Name == nil || imageDetails.CRN == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to check all the params separately?
If the error is nil, would imageDetails still be not nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done defensively, to make sure the required Image fields are populated from the API response.

return fmt.Errorf("error failed creating custom image")
}

// Initially populate the Image's status.
s.SetResourceStatus(infrav1beta2.ResourceTypeCustomImage, &infrav1beta2.ResourceStatus{
ID: *imageDetails.ID,
Name: imageDetails.Name,
// We must wait for the image to be ready, on followup reconciliation loops.
Ready: false,
})

// NOTE: This tagging is only attempted once. We may wish to refactor in case this single attempt fails.
if err := s.TagResource(s.Name(), *imageDetails.CRN); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when it fails to tag?
It seems it would again come to createCustomImage func and it will try to create the image again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If tagging fails, after creating the Custom Image (or any resource with this similar logic, like VPC), we return an error. Next loop through could then find the resource that was created the first attempt, and update the resource Status (in the VPC example, a VPC is likely "ready" in the second attempt), with the expectation it is found by the name used during creation. This will monitor and/or update the resource Status accordingly. As we expect to not try to recreate a resource that was already created, we don't expect to try to retag the resource.

Logic could be added to check tags and apply if missing, but that logic will be more cumbersome and time consuming that desired currently, and I only plan to worry about tags during Delete actions, which are a secondary concern currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see you have handled this in ReconcileVPC, but here, you don't have a func like GetVPCID. You are checking either from status or from spec crn. If it's there you are validating the image and set the status based on that. Please take care of this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I am following, but if the recommendation here is to update status (SetResourceStatus) prior to tagging, I can move that for Custom Images.

I can handle VPC in a similar fashion then, in a separate PR I expect.

return fmt.Errorf("error failure tagging vpc custom image: %w", err)
}
return nil
}

// buildCOSObjectHRef will build the HRef path to a COS Object that can be used for VPC Custom Image creation.
func (s *VPCClusterScope) buildCOSObjectHRef() (*string, error) {
// TODO(cjschaef): Remove in favor of webhook validation.
// We need COS details in order to create the Custom Image from.
if s.IBMVPCCluster.Spec.Image.COSInstance == nil || s.IBMVPCCluster.Spec.Image.COSBucket == nil || s.IBMVPCCluster.Spec.Image.COSObject == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets create an issue or make a note to create a validation webhook for this, It will enhance user experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can create an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return nil, fmt.Errorf("error failed to build cos object href, cos details missing")
}

// Get COS Bucket Region, defaulting to cluster Region if not specified.
bucketRegion := s.IBMVPCCluster.Spec.Region
if s.IBMVPCCluster.Spec.Image.COSBucketRegion != nil {
bucketRegion = *s.IBMVPCCluster.Spec.Image.COSBucketRegion
}

// Expected HRef format:
// cos://<bucket_region>/<bucket_name>/<object_name>
href := fmt.Sprintf("cos://%s/%s/%s", bucketRegion, *s.IBMVPCCluster.Spec.Image.COSBucket, *s.IBMVPCCluster.Spec.Image.COSObject)
s.V(3).Info("building image ref", "href", href)
return ptr.To(href), nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ spec:
description: name is the name of the desired VPC Custom Image.
maxLength: 63
minLength: 1
pattern: '''/^([a-z]|[a-z][-a-z0-9]*[a-z0-9])$/'''
pattern: ^([a-z]|[a-z][-a-z0-9]*[a-z0-9])$
type: string
operatingSystem:
description: operatingSystem is the Custom Image's Operating System
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ spec:
Image.
maxLength: 63
minLength: 1
pattern: '''/^([a-z]|[a-z][-a-z0-9]*[a-z0-9])$/'''
pattern: ^([a-z]|[a-z][-a-z0-9]*[a-z0-9])$
type: string
operatingSystem:
description: operatingSystem is the Custom Image's Operating
Expand Down
14 changes: 14 additions & 0 deletions controllers/ibmvpccluster_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,22 @@ func (r *IBMVPCClusterReconciler) reconcileCluster(clusterScope *scope.VPCCluste
clusterScope.Info("VPC creation is pending, requeuing")
return reconcile.Result{RequeueAfter: 15 * time.Second}, nil
}
clusterScope.Info("Reconciliation of VPC complete")
conditions.MarkTrue(clusterScope.IBMVPCCluster, infrav1beta2.VPCReadyCondition)

// Reconcile the cluster's VPC Custom Image.
clusterScope.Info("Reconciling VPC Custom Image")
if requeue, err := clusterScope.ReconcileVPCCustomImage(); err != nil {
clusterScope.Error(err, "failed to reconcile VPC Custom Image")
conditions.MarkFalse(clusterScope.IBMVPCCluster, infrav1beta2.ImageReadyCondition, infrav1beta2.ImageReconciliationFailedReason, capiv1beta1.ConditionSeverityError, err.Error())
return reconcile.Result{}, err
} else if requeue {
clusterScope.Info("VPC Custom Image creation is pending, requeueing")
return reconcile.Result{RequeueAfter: 15 * time.Second}, nil
}
clusterScope.Info("Reconciliation of VPC Custom Image complete")
conditions.MarkTrue(clusterScope.IBMVPCCluster, infrav1beta2.ImageReadyCondition)

// TODO(cjschaef): add remaining resource reconciliation.

// Mark cluster as ready.
Expand Down
47 changes: 47 additions & 0 deletions pkg/cloud/services/vpc/mock/vpc_generated.go

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

Loading