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

Fix bug where clusterctl move does not work, and add correct default … #74

Merged
merged 1 commit into from
Apr 27, 2022
Merged

Fix bug where clusterctl move does not work, and add correct default … #74

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions api/v1beta1/ocicluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,17 @@ const (

// OCIClusterSpec defines the desired state of OciCluster
type OCIClusterSpec struct {

// The unique ID which will be used to tag all the resources created by this Cluster.
// The tag will be used to identify resources belonging to this cluster.
// this will be auto-generated and should not be set by the user.
// +optional
OCIResourceIdentifier string `json:"ociResourceIdentifier,omitempty"`

// NetworkSpec encapsulates all things related to OCI network.
// +optional
NetworkSpec NetworkSpec `json:"networkSpec,omitempty"`

// Free-form tags for this resource.
// +optional
FreeformTags map[string]string `json:"freeformTags,omitempty"`
Expand Down Expand Up @@ -101,6 +109,11 @@ func (c *OCICluster) SetConditions(conditions clusterv1.Conditions) {
c.Status.Conditions = conditions
}

// GetOCIResourceIdentifier will return the OCI resource identifier.
func (c *OCICluster) GetOCIResourceIdentifier() string {
return c.Spec.OCIResourceIdentifier
}

func init() {
SchemeBuilder.Register(&OCICluster{}, &OCIClusterList{})
}
20 changes: 19 additions & 1 deletion api/v1beta1/ocicluster_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,27 @@ import (

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/uuid"
"k8s.io/apimachinery/pkg/util/validation/field"

ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/webhook"
)

var clusterlogger = ctrl.Log.WithName("ocicluster-resource")

var (
_ webhook.Defaulter = &OCICluster{}
_ webhook.Validator = &OCICluster{}
)

// +kubebuilder:webhook:verbs=create;update,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-ocicluster,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=ociclusters,versions=v1beta1,name=validation.ocicluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1beta1
// +kubebuilder:webhook:verbs=create;update,path=/mutate-infrastructure-cluster-x-k8s-io-v1beta1-ocicluster,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=infrastructure.cluster.x-k8s.io,resources=ociclusters,versions=v1beta1,name=default.ocicluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1

func (c *OCICluster) Default() {
if c.Spec.OCIResourceIdentifier == "" {
c.Spec.OCIResourceIdentifier = string(uuid.NewUUID())
}
}
shyamradhakrishnan marked this conversation as resolved.
Show resolved Hide resolved

func (c *OCICluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
return ctrl.NewWebhookManagedBy(mgr).
Expand Down Expand Up @@ -81,6 +89,10 @@ func (c *OCICluster) ValidateUpdate(old runtime.Object) error {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "region"), c.Spec.Region, "field is immutable"))
}

if c.Spec.OCIResourceIdentifier != oldCluster.Spec.OCIResourceIdentifier {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "ociResourceIdentifier"), c.Spec.OCIResourceIdentifier, "field is immutable"))
}

allErrs = append(allErrs, c.validate()...)

if len(allErrs) == 0 {
Expand All @@ -107,6 +119,12 @@ func (c *OCICluster) validate() field.ErrorList {
field.Invalid(field.NewPath("spec", "compartmentId"), c.Spec.CompartmentId, "field is invalid"))
}

if len(c.Spec.OCIResourceIdentifier) <= 0 {
allErrs = append(
allErrs,
field.Invalid(field.NewPath("spec", "ociResourceIdentifier"), c.Spec.OCIResourceIdentifier, "field is required"))
}

if len(allErrs) == 0 {
return nil
}
Expand Down
69 changes: 65 additions & 4 deletions api/v1beta1/ocicluster_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"testing"

"github.com/onsi/gomega"
. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -52,13 +53,24 @@ func TestOCICluster_ValidateCreate(t *testing.T) {
expectErr: true,
},
{
name: "should succeed",
name: "shouldn't allow blank OCIResourceIdentifier",
c: &OCICluster{
ObjectMeta: metav1.ObjectMeta{},
Spec: OCIClusterSpec{
CompartmentId: "ocid",
},
},
expectErr: true,
},
{
name: "should succeed",
c: &OCICluster{
ObjectMeta: metav1.ObjectMeta{},
Spec: OCIClusterSpec{
CompartmentId: "ocid",
OCIResourceIdentifier: "uuid",
},
},
expectErr: false,
},
}
Expand Down Expand Up @@ -98,19 +110,40 @@ func TestOCICluster_ValidateUpdate(t *testing.T) {
},
expectErr: true,
},
{
name: "shouldn't change OCIResourceIdentifier",
c: &OCICluster{
ObjectMeta: metav1.ObjectMeta{},
Spec: OCIClusterSpec{
CompartmentId: "ocid",
Region: "old-region",
OCIResourceIdentifier: "uuid-1",
},
},
old: &OCICluster{
ObjectMeta: metav1.ObjectMeta{},
Spec: OCIClusterSpec{
Region: "old-region",
OCIResourceIdentifier: "uuid-2",
},
},
expectErr: true,
},
{
name: "should succeed",
c: &OCICluster{
ObjectMeta: metav1.ObjectMeta{},
Spec: OCIClusterSpec{
CompartmentId: "ocid",
Region: "old-region",
CompartmentId: "ocid",
Region: "old-region",
OCIResourceIdentifier: "uuid",
},
},
old: &OCICluster{
ObjectMeta: metav1.ObjectMeta{},
Spec: OCIClusterSpec{
Region: "old-region",
Region: "old-region",
OCIResourceIdentifier: "uuid",
},
},
expectErr: false,
Expand All @@ -128,5 +161,33 @@ func TestOCICluster_ValidateUpdate(t *testing.T) {
}
})
}
}

func TestOCICluster_CreateDefault(t *testing.T) {

tests := []struct {
name string
c *OCICluster
expect func(g *gomega.WithT, c *OCICluster)
}{
{
name: "should set default OCIResourceIdentifier",
c: &OCICluster{
ObjectMeta: metav1.ObjectMeta{},
Spec: OCIClusterSpec{
CompartmentId: "badocid",
},
},
expect: func(g *gomega.WithT, c *OCICluster) {
g.Expect(c.Spec.OCIResourceIdentifier).To(Not(BeNil()))
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
g := gomega.NewWithT(t)
test.c.Default()
test.expect(g, test.c)
})
}
}
20 changes: 12 additions & 8 deletions cloud/ociutil/ociutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ package ociutil
import (
"context"
"fmt"
nlb "github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer"
"net/http"
"time"

nlb "github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer"

"github.com/oracle/oci-go-sdk/v63/common"
"github.com/oracle/oci-go-sdk/v63/core"
"github.com/oracle/oci-go-sdk/v63/networkloadbalancer"
Expand All @@ -31,9 +32,12 @@ import (
)

const (
WorkRequestPollInterval = 5 * time.Second
WorkRequestTimeout = 2 * time.Minute
MaxOPCRetryTokenBytes = 64
WorkRequestPollInterval = 5 * time.Second
WorkRequestTimeout = 2 * time.Minute
MaxOPCRetryTokenBytes = 64
CreatedBy = "CreatedBy"
OCIClusterAPIProvider = "OCIClusterAPIProvider"
ClusterResourceIdentifier = "ClusterResourceIdentifier"
)

// ErrNotFound is for simulation during testing, OCI SDK does not have a way
Expand Down Expand Up @@ -102,13 +106,13 @@ func GetBaseLineOcpuOptimizationEnum(baseLineOcpuOptmimizationString string) (co
// GetDefaultClusterTags creates and returns a map of the default tags for all clusters
func GetDefaultClusterTags() map[string]string {
tags := make(map[string]string)
tags["CreatedBy"] = "OCIClusterAPIProvider"
tags[CreatedBy] = OCIClusterAPIProvider
return tags
}

// BuildClusterTags uses the default tags and adds the ClusterUUID tag
func BuildClusterTags(clusterUUID string) map[string]string {
// BuildClusterTags uses the default tags and adds the ClusterResourceUID tag
func BuildClusterTags(ClusterResourceUID string) map[string]string {
tags := GetDefaultClusterTags()
tags["ClusterUUID"] = clusterUUID
tags[ClusterResourceIdentifier] = ClusterResourceUID
return tags
}
4 changes: 2 additions & 2 deletions cloud/ociutil/ociutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ func TestGetCloudProviderConfig(t *testing.T) {
func TestAddToDefaultClusterTags(t *testing.T) {
testUUID := "UUIDTEST"
tags := BuildClusterTags(testUUID)
if tags["ClusterUUID"] != testUUID {
t.Errorf("Tags don't match Expected: %s, Actual: %s", testUUID, tags["ClusterUUID"])
if tags[ClusterResourceIdentifier] != testUUID {
t.Errorf("Tags don't match Expected: %s, Actual: %s", testUUID, tags[ClusterResourceIdentifier])
}

// should also contain default tags
Expand Down
4 changes: 2 additions & 2 deletions cloud/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func (s *ClusterScope) ReconcileFailureDomains(ctx context.Context) error {
}

func (s *ClusterScope) IsResourceCreatedByClusterAPI(resourceFreeFormTags map[string]string) bool {
tagsAddedByClusterAPI := ociutil.BuildClusterTags(string(s.OCICluster.UID))
tagsAddedByClusterAPI := ociutil.BuildClusterTags(s.OCICluster.GetOCIResourceIdentifier())
for k, v := range tagsAddedByClusterAPI {
if resourceFreeFormTags[k] != v {
return false
Expand Down Expand Up @@ -251,7 +251,7 @@ func (s *ClusterScope) GetFreeFormTags() map[string]string {
if tags == nil {
tags = make(map[string]string)
}
tagsAddedByClusterAPI := ociutil.BuildClusterTags(string(s.OCICluster.UID))
tagsAddedByClusterAPI := ociutil.BuildClusterTags(string(s.OCICluster.GetOCIResourceIdentifier()))
for k, v := range tagsAddedByClusterAPI {
tags[k] = v
}
Expand Down
2 changes: 1 addition & 1 deletion cloud/scope/drg_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ func (s *ClusterScope) createDRG(ctx context.Context) (*core.Drg, error) {
DefinedTags: s.GetDefinedTags(),
DisplayName: common.String(s.GetDRGName()),
},
OpcRetryToken: ociutil.GetOPCRetryToken("%s-%s", "create-drg", string(s.OCICluster.UID)),
OpcRetryToken: ociutil.GetOPCRetryToken("%s-%s", "create-drg", string(s.OCICluster.GetOCIResourceIdentifier())),
})
if err != nil {
return nil, err
Expand Down
24 changes: 13 additions & 11 deletions cloud/scope/drg_reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,12 @@ func TestDRGReconciliation(t *testing.T) {
client := fake.NewClientBuilder().Build()
ociCluster = infrastructurev1beta1.OCICluster{
ObjectMeta: metav1.ObjectMeta{
UID: "a",
UID: "cluster_uid",
Name: "cluster",
},
Spec: infrastructurev1beta1.OCIClusterSpec{
CompartmentId: "compartment-id",
CompartmentId: "compartment-id",
OCIResourceIdentifier: "resource_uid",
},
}
ociCluster.Spec.ControlPlaneEndpoint.Port = 6443
Expand All @@ -66,8 +67,8 @@ func TestDRGReconciliation(t *testing.T) {
Client: client,
})
tags = make(map[string]string)
tags["CreatedBy"] = "OCIClusterAPIProvider"
tags["ClusterUUID"] = "a"
tags[ociutil.CreatedBy] = ociutil.OCIClusterAPIProvider
tags[ociutil.ClusterResourceIdentifier] = "resource_uid"
vcnPeering = infrastructurev1beta1.VCNPeering{}
g.Expect(err).To(BeNil())
}
Expand Down Expand Up @@ -167,8 +168,8 @@ func TestDRGReconciliation(t *testing.T) {
vcnPeering.DRG.Manage = true
vcnPeering.DRG.ID = common.String("drg-id")
existingTags := make(map[string]string)
existingTags["CreatedBy"] = "OCIClusterAPIProvider"
existingTags["ClusterUUID"] = "a"
existingTags[ociutil.CreatedBy] = ociutil.OCIClusterAPIProvider
existingTags[ociutil.ClusterResourceIdentifier] = "resource_uid"
existingTags["test"] = "test"
clusterScope.OCICluster.Spec.NetworkSpec.VCNPeering = &vcnPeering
vcnClient.EXPECT().GetDrg(gomock.Any(), gomock.Eq(core.GetDrgRequest{
Expand Down Expand Up @@ -209,7 +210,7 @@ func TestDRGReconciliation(t *testing.T) {
DefinedTags: make(map[string]map[string]interface{}),
DisplayName: common.String("cluster"),
},
OpcRetryToken: ociutil.GetOPCRetryToken("%s-%s", "create-drg", "a"),
OpcRetryToken: ociutil.GetOPCRetryToken("%s-%s", "create-drg", "resource_uid"),
})).
Return(core.CreateDrgResponse{
Drg: core.Drg{
Expand Down Expand Up @@ -258,11 +259,12 @@ func TestDRGDeletion(t *testing.T) {
client := fake.NewClientBuilder().Build()
ociCluster = infrastructurev1beta1.OCICluster{
ObjectMeta: metav1.ObjectMeta{
UID: "a",
UID: "cluster_uid",
Name: "cluster",
},
Spec: infrastructurev1beta1.OCIClusterSpec{
CompartmentId: "compartment-id",
CompartmentId: "compartment-id",
OCIResourceIdentifier: "resource_uid",
},
}
ociCluster.Spec.ControlPlaneEndpoint.Port = 6443
Expand All @@ -273,8 +275,8 @@ func TestDRGDeletion(t *testing.T) {
Client: client,
})
tags = make(map[string]string)
tags["CreatedBy"] = "OCIClusterAPIProvider"
tags["ClusterUUID"] = "a"
tags[ociutil.CreatedBy] = ociutil.OCIClusterAPIProvider
tags[ociutil.ClusterResourceIdentifier] = "resource_uid"
vcnPeering = infrastructurev1beta1.VCNPeering{}
g.Expect(err).To(BeNil())
}
Expand Down