Skip to content

Commit 15ae5f1

Browse files
FedosinFedosin
authored
Add validation for GC tasks annotation (#4485)
* Move GC annotation constants from exp/api/v1beta2 to api/v1beta2 To prevent a breaking change we keep the constants in exp/api/v1beta2, and they will have the same values as ones in api/v1beta2 * Add validation for GC tasks annotation
1 parent 627ec24 commit 15ae5f1

File tree

8 files changed

+124
-36
lines changed

8 files changed

+124
-36
lines changed

‎api/v1beta2/awscluster_webhook.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package v1beta2
1818

1919
import (
2020
"fmt"
21+
"strings"
2122

2223
"github.com/google/go-cmp/cmp"
2324
apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -70,6 +71,8 @@ func (r *AWSCluster) ValidateDelete() error {
7071
func (r *AWSCluster) ValidateUpdate(old runtime.Object) error {
7172
var allErrs field.ErrorList
7273

74+
allErrs = append(allErrs, r.validateGCTasksAnnotation()...)
75+
7376
oldC, ok := old.(*AWSCluster)
7477
if !ok {
7578
return apierrors.NewBadRequest(fmt.Sprintf("expected an AWSCluster but got a %T", old))
@@ -175,6 +178,42 @@ func (r *AWSCluster) Default() {
175178
SetObjectDefaults_AWSCluster(r)
176179
}
177180

181+
func (r *AWSCluster) validateGCTasksAnnotation() field.ErrorList {
182+
var allErrs field.ErrorList
183+
184+
annotations := r.GetAnnotations()
185+
if annotations == nil {
186+
return nil
187+
}
188+
189+
if gcTasksAnnotationValue := annotations[ExternalResourceGCTasksAnnotation]; gcTasksAnnotationValue != "" {
190+
gcTasks := strings.Split(gcTasksAnnotationValue, ",")
191+
192+
supportedGCTasks := []GCTask{GCTaskLoadBalancer, GCTaskTargetGroup, GCTaskSecurityGroup}
193+
194+
for _, gcTask := range gcTasks {
195+
found := false
196+
197+
for _, supportedGCTask := range supportedGCTasks {
198+
if gcTask == string(supportedGCTask) {
199+
found = true
200+
break
201+
}
202+
}
203+
204+
if !found {
205+
allErrs = append(allErrs,
206+
field.Invalid(field.NewPath("metadata", "annotations"),
207+
r.Annotations,
208+
fmt.Sprintf("annotation %s contains unsupported GC task %s", ExternalResourceGCTasksAnnotation, gcTask)),
209+
)
210+
}
211+
}
212+
}
213+
214+
return allErrs
215+
}
216+
178217
func (r *AWSCluster) validateSSHKeyName() field.ErrorList {
179218
return validateSSHKeyName(r.Spec.SSHKeyName)
180219
}

‎api/v1beta2/awscluster_webhook_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -655,6 +655,48 @@ func TestAWSClusterValidateUpdate(t *testing.T) {
655655
},
656656
wantErr: true,
657657
},
658+
{
659+
name: "correct GC tasks annotation",
660+
oldCluster: &AWSCluster{
661+
Spec: AWSClusterSpec{},
662+
},
663+
newCluster: &AWSCluster{
664+
ObjectMeta: metav1.ObjectMeta{
665+
Annotations: map[string]string{
666+
ExternalResourceGCTasksAnnotation: "load-balancer,target-group,security-group",
667+
},
668+
},
669+
},
670+
wantErr: false,
671+
},
672+
{
673+
name: "empty GC tasks annotation",
674+
oldCluster: &AWSCluster{
675+
Spec: AWSClusterSpec{},
676+
},
677+
newCluster: &AWSCluster{
678+
ObjectMeta: metav1.ObjectMeta{
679+
Annotations: map[string]string{
680+
ExternalResourceGCTasksAnnotation: "",
681+
},
682+
},
683+
},
684+
wantErr: false,
685+
},
686+
{
687+
name: "incorrect GC tasks annotation",
688+
oldCluster: &AWSCluster{
689+
Spec: AWSClusterSpec{},
690+
},
691+
newCluster: &AWSCluster{
692+
ObjectMeta: metav1.ObjectMeta{
693+
Annotations: map[string]string{
694+
ExternalResourceGCTasksAnnotation: "load-balancer,INVALID,security-group",
695+
},
696+
},
697+
},
698+
wantErr: true,
699+
},
658700
}
659701
for _, tt := range tests {
660702
t.Run(tt.name, func(t *testing.T) {

‎api/v1beta2/types.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,29 @@ const (
7070
MachineCreated AWSMachineProviderConditionType = "MachineCreated"
7171
)
7272

73+
const (
74+
// ExternalResourceGCAnnotation is the name of an annotation that indicates if
75+
// external resources should be garbage collected for the cluster.
76+
ExternalResourceGCAnnotation = "aws.cluster.x-k8s.io/external-resource-gc"
77+
78+
// ExternalResourceGCTasksAnnotation is the name of an annotation that indicates what
79+
// external resources tasks should be executed by garbage collector for the cluster.
80+
ExternalResourceGCTasksAnnotation = "aws.cluster.x-k8s.io/external-resource-tasks-gc"
81+
)
82+
83+
type GCTask string
84+
85+
var (
86+
// GCTaskLoadBalancer defines a task to cleaning up resources for AWS load balancers.
87+
GCTaskLoadBalancer = GCTask("load-balancer")
88+
89+
// GCTaskTargetGroup defines a task to cleaning up resources for AWS target groups.
90+
GCTaskTargetGroup = GCTask("target-group")
91+
92+
// GCTaskSecurityGroup defines a task to cleaning up resources for AWS security groups.
93+
GCTaskSecurityGroup = GCTask("security-group")
94+
)
95+
7396
// AZSelectionScheme defines the scheme of selecting AZs.
7497
type AZSelectionScheme string
7598

‎cmd/clusterawsadm/gc/gc.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929

3030
infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
3131
ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta2"
32-
expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2"
3332
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/annotations"
3433
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3534
"sigs.k8s.io/cluster-api/controllers/external"
@@ -132,7 +131,7 @@ func (c *CmdProcessor) setAnnotationAndPatch(ctx context.Context, annotationValu
132131
return fmt.Errorf("creating patch helper: %w", err)
133132
}
134133

135-
annotations.Set(infraObj, expinfrav1.ExternalResourceGCAnnotation, annotationValue)
134+
annotations.Set(infraObj, infrav1.ExternalResourceGCAnnotation, annotationValue)
136135

137136
if err := patchHelper.Patch(ctx, infraObj); err != nil {
138137
return fmt.Errorf("patching infra cluster with gc annotation: %w", err)

‎cmd/clusterawsadm/gc/gc_test.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929

3030
infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
3131
ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta2"
32-
expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2"
3332
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/annotations"
3433
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
3534
"sigs.k8s.io/cluster-api/controllers/external"
@@ -73,7 +72,7 @@ func TestEnableGC(t *testing.T) {
7372
{
7473
name: "with managed control plane and existing annotation",
7574
clusterName: testClusterName,
76-
existingObjs: newManagedClusterWithAnnotations(testClusterName, map[string]string{expinfrav1.ExternalResourceGCAnnotation: "false"}),
75+
existingObjs: newManagedClusterWithAnnotations(testClusterName, map[string]string{infrav1.ExternalResourceGCAnnotation: "false"}),
7776
expectError: false,
7877
},
7978
}
@@ -107,7 +106,7 @@ func TestEnableGC(t *testing.T) {
107106
g.Expect(err).NotTo(HaveOccurred())
108107
g.Expect(obj).NotTo(BeNil())
109108

110-
annotationVal, hasAnnotation := annotations.Get(obj, expinfrav1.ExternalResourceGCAnnotation)
109+
annotationVal, hasAnnotation := annotations.Get(obj, infrav1.ExternalResourceGCAnnotation)
111110
g.Expect(hasAnnotation).To(BeTrue())
112111
g.Expect(annotationVal).To(Equal("true"))
113112
})
@@ -140,13 +139,13 @@ func TestDisableGC(t *testing.T) {
140139
{
141140
name: "with managed control plane and with annotation",
142141
clusterName: testClusterName,
143-
existingObjs: newManagedClusterWithAnnotations(testClusterName, map[string]string{expinfrav1.ExternalResourceGCAnnotation: "true"}),
142+
existingObjs: newManagedClusterWithAnnotations(testClusterName, map[string]string{infrav1.ExternalResourceGCAnnotation: "true"}),
144143
expectError: false,
145144
},
146145
{
147146
name: "with awscluster and with annotation",
148147
clusterName: testClusterName,
149-
existingObjs: newUnManagedClusterWithAnnotations(testClusterName, map[string]string{expinfrav1.ExternalResourceGCAnnotation: "true"}),
148+
existingObjs: newUnManagedClusterWithAnnotations(testClusterName, map[string]string{infrav1.ExternalResourceGCAnnotation: "true"}),
150149
expectError: false,
151150
},
152151
}
@@ -180,7 +179,7 @@ func TestDisableGC(t *testing.T) {
180179
g.Expect(err).NotTo(HaveOccurred())
181180
g.Expect(obj).NotTo(BeNil())
182181

183-
annotationVal, hasAnnotation := annotations.Get(obj, expinfrav1.ExternalResourceGCAnnotation)
182+
annotationVal, hasAnnotation := annotations.Get(obj, infrav1.ExternalResourceGCAnnotation)
184183
g.Expect(hasAnnotation).To(BeTrue())
185184
g.Expect(annotationVal).To(Equal("false"))
186185
})

‎exp/api/v1beta2/types.go

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,16 +22,6 @@ import (
2222
infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
2323
)
2424

25-
const (
26-
// ExternalResourceGCAnnotation is the name of an annotation that indicates if
27-
// external resources should be garbage collected for the cluster.
28-
ExternalResourceGCAnnotation = "aws.cluster.x-k8s.io/external-resource-gc"
29-
30-
// ExternalResourceGCTasksAnnotation is the name of an annotation that indicates what
31-
// external resources tasks should be executed by garbage collector for the cluster.
32-
ExternalResourceGCTasksAnnotation = "aws.cluster.x-k8s.io/external-resource-tasks-gc"
33-
)
34-
3525
// EBS can be used to automatically set up EBS volumes when an instance is launched.
3626
type EBS struct {
3727
// Encrypted is whether the volume should be encrypted or not.

‎pkg/cloud/services/gc/cleanup.go

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import (
2727
rgapi "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi"
2828

2929
infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
30-
expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2"
3130
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/annotations"
3231
)
3332

@@ -42,14 +41,14 @@ const (
4241
func (s *Service) ReconcileDelete(ctx context.Context) error {
4342
s.scope.Info("reconciling deletion for garbage collection", "cluster", s.scope.InfraClusterName())
4443

45-
val, found := annotations.Get(s.scope.InfraCluster(), expinfrav1.ExternalResourceGCAnnotation)
44+
val, found := annotations.Get(s.scope.InfraCluster(), infrav1.ExternalResourceGCAnnotation)
4645
if !found {
4746
val = "true"
4847
}
4948

5049
shouldGC, err := strconv.ParseBool(val)
5150
if err != nil {
52-
return fmt.Errorf("converting value %s of annotation %s to bool: %w", val, expinfrav1.ExternalResourceGCAnnotation, err)
51+
return fmt.Errorf("converting value %s of annotation %s to bool: %w", val, infrav1.ExternalResourceGCAnnotation, err)
5352
}
5453

5554
if !shouldGC {
@@ -71,21 +70,19 @@ func (s *Service) deleteResources(ctx context.Context) error {
7170

7271
cleanupFuncs := s.cleanupFuncs
7372

74-
if val, found := annotations.Get(s.scope.InfraCluster(), expinfrav1.ExternalResourceGCTasksAnnotation); found {
75-
var gcTaskToFunc = map[string]ResourceCleanupFunc{
76-
"load-balancer": s.deleteLoadBalancers,
77-
"target-group": s.deleteTargetGroups,
78-
"security-group": s.deleteSecurityGroups,
73+
if val, found := annotations.Get(s.scope.InfraCluster(), infrav1.ExternalResourceGCTasksAnnotation); found {
74+
var gcTaskToFunc = map[infrav1.GCTask]ResourceCleanupFunc{
75+
infrav1.GCTaskLoadBalancer: s.deleteLoadBalancers,
76+
infrav1.GCTaskTargetGroup: s.deleteTargetGroups,
77+
infrav1.GCTaskSecurityGroup: s.deleteSecurityGroups,
7978
}
8079

8180
cleanupFuncs = ResourceCleanupFuncs{}
8281

8382
tasks := strings.Split(val, ",")
8483

85-
// TODO: add some validation here.
86-
8784
for _, task := range tasks {
88-
cleanupFuncs = append(cleanupFuncs, gcTaskToFunc[task])
85+
cleanupFuncs = append(cleanupFuncs, gcTaskToFunc[infrav1.GCTask(task)])
8986
}
9087
}
9188

‎pkg/cloud/services/gc/cleanup_test.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ import (
3636

3737
infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
3838
ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta2"
39-
expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2"
4039
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud"
4140
"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope"
4241
"sigs.k8s.io/cluster-api-provider-aws/v2/test/mocks"
@@ -924,16 +923,16 @@ func createManagedControlPlane(gcAnnotationValue, gcTasksAnnotationValue string)
924923

925924
if gcAnnotationValue != "" {
926925
cp.ObjectMeta.Annotations = map[string]string{
927-
expinfrav1.ExternalResourceGCAnnotation: gcAnnotationValue,
926+
infrav1.ExternalResourceGCAnnotation: gcAnnotationValue,
928927
}
929928
}
930929

931930
if gcTasksAnnotationValue != "" {
932931
if cp.ObjectMeta.Annotations != nil {
933-
cp.ObjectMeta.Annotations[expinfrav1.ExternalResourceGCTasksAnnotation] = gcTasksAnnotationValue
932+
cp.ObjectMeta.Annotations[infrav1.ExternalResourceGCTasksAnnotation] = gcTasksAnnotationValue
934933
} else {
935934
cp.ObjectMeta.Annotations = map[string]string{
936-
expinfrav1.ExternalResourceGCTasksAnnotation: gcTasksAnnotationValue,
935+
infrav1.ExternalResourceGCTasksAnnotation: gcTasksAnnotationValue,
937936
}
938937
}
939938
}
@@ -956,16 +955,16 @@ func createAWSCluser(gcAnnotationValue, gcTasksAnnotationValue string) *infrav1.
956955

957956
if gcAnnotationValue != "" {
958957
awsc.ObjectMeta.Annotations = map[string]string{
959-
expinfrav1.ExternalResourceGCAnnotation: gcAnnotationValue,
958+
infrav1.ExternalResourceGCAnnotation: gcAnnotationValue,
960959
}
961960
}
962961

963962
if gcTasksAnnotationValue != "" {
964963
if awsc.ObjectMeta.Annotations != nil {
965-
awsc.ObjectMeta.Annotations[expinfrav1.ExternalResourceGCTasksAnnotation] = gcTasksAnnotationValue
964+
awsc.ObjectMeta.Annotations[infrav1.ExternalResourceGCTasksAnnotation] = gcTasksAnnotationValue
966965
} else {
967966
awsc.ObjectMeta.Annotations = map[string]string{
968-
expinfrav1.ExternalResourceGCTasksAnnotation: gcTasksAnnotationValue,
967+
infrav1.ExternalResourceGCTasksAnnotation: gcTasksAnnotationValue,
969968
}
970969
}
971970
}

0 commit comments

Comments
 (0)