Skip to content

Commit 1e3e36a

Browse files
google-oss-prow[bot]google-oss-prow[bot]
authored
Merge pull request GoogleCloudPlatform#1149 from maqiuyujoyce/202401-move-defaulter-no-bc
Move the state-into-spec defaulter from webhook to controllers
2 parents 320b1b8 + c2dd959 commit 1e3e36a

10 files changed

+125
-69
lines changed

‎pkg/controller/dcl/controller.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (res
197197
if err != nil {
198198
return reconcile.Result{}, fmt.Errorf("could not parse resource %s: %w", req.NamespacedName.String(), err)
199199
}
200+
if err := r.handleDefaultStateIntoSpecValue(resource); err != nil {
201+
return reconcile.Result{}, fmt.Errorf("error handling default values for resource '%v': %w", k8s.GetNamespacedName(resource), err)
202+
}
200203
if err := r.applyChangesForBackwardsCompatibility(ctx, resource); err != nil {
201204
return reconcile.Result{}, fmt.Errorf("error applying changes to resource '%v' for backwards compatibility: %v", k8s.GetNamespacedName(resource), err)
202205
}
@@ -414,6 +417,15 @@ func (r *Reconciler) obtainResourceLeaseIfNecessary(ctx context.Context, resourc
414417
return nil
415418
}
416419

420+
func (r *Reconciler) handleDefaultStateIntoSpecValue(resource *dcl.Resource) error {
421+
// Validate or set the default value (cluster-level or namespace-level) for
422+
// the 'state-into-spec' annotation.
423+
if err := k8s.ValidateOrDefaultStateIntoSpecAnnotation(&resource.Resource, k8s.StateMergeIntoSpec); err != nil {
424+
return fmt.Errorf("error validating or defaulting the '%v' annotation for resource '%v': %w", k8s.StateIntoSpecAnnotation, k8s.GetNamespacedName(resource), err)
425+
}
426+
return nil
427+
}
428+
417429
func (r *Reconciler) applyChangesForBackwardsCompatibility(ctx context.Context, resource *dcl.Resource) error {
418430
// Ensure the resource has a hierarchical reference. This is done to be
419431
// backwards compatible with resources created before the webhook for
@@ -430,13 +442,6 @@ func (r *Reconciler) applyChangesForBackwardsCompatibility(ctx context.Context,
430442
if err := k8s.EnsureHierarchicalReference(ctx, &resource.Resource, hierarchicalRefs, containers, r.Client); err != nil {
431443
return fmt.Errorf("error ensuring resource '%v' has a hierarchical reference: %v", k8s.GetNamespacedName(resource), err)
432444
}
433-
434-
// Ensure the resource has a state-into-spec annotation.
435-
// This is done to be backwards compatible with resources
436-
// created before the webhook for defaulting the annotation was added.
437-
if err := k8s.EnsureSpecIntoSateAnnotation(&resource.Resource); err != nil {
438-
return fmt.Errorf("error ensuring resource '%v' has a '%v' annotation: %v", k8s.GetNamespacedName(resource), k8s.StateIntoSpecAnnotation, err)
439-
}
440445
return nil
441446
}
442447

‎pkg/controller/iam/auditconfig/iamauditconfig_controller.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
139139
}
140140
return reconcile.Result{}, err
141141
}
142+
if err := r.handleDefaultStateIntoSpecValue(&auditConfig); err != nil {
143+
return reconcile.Result{}, fmt.Errorf("error handling default values for IAM policy '%v': %w", k8s.GetNamespacedName(&auditConfig), err)
144+
}
142145
reconcileContext := &reconcileContext{
143146
Reconciler: r,
144147
Ctx: ctx,
@@ -159,6 +162,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
159162
return reconcile.Result{RequeueAfter: jitteredPeriod}, nil
160163
}
161164

165+
func (r *Reconciler) handleDefaultStateIntoSpecValue(auditConfig *iamv1beta1.IAMAuditConfig) error {
166+
// Validate or set the default value (cluster-level or namespace-level) for
167+
// the 'state-into-spec' annotation.
168+
if err := k8s.ValidateOrDefaultStateIntoSpecAnnotation(auditConfig, k8s.StateMergeIntoSpec); err != nil {
169+
return fmt.Errorf("error validating or defaulting the '%v' annotation for resource '%v': %w", k8s.StateIntoSpecAnnotation, k8s.GetNamespacedName(auditConfig), err)
170+
}
171+
return nil
172+
}
173+
162174
func (r *reconcileContext) doReconcile(auditConfig *iamv1beta1.IAMAuditConfig) (requeue bool, err error) {
163175
defer execution.RecoverWithInternalError(&err)
164176
if !auditConfig.DeletionTimestamp.IsZero() {

‎pkg/controller/iam/partialpolicy/iampartialpolicy_controller.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,9 @@ func (r *ReconcileIAMPartialPolicy) Reconcile(ctx context.Context, request recon
151151
// Error reading the object - requeue the request.
152152
return reconcile.Result{}, err
153153
}
154+
if err := r.handleDefaultStateIntoSpecValue(policy); err != nil {
155+
return reconcile.Result{}, fmt.Errorf("error handling default values for IAM partial policy '%v': %w", k8s.GetNamespacedName(policy), err)
156+
}
154157
runCtx := &reconcileContext{
155158
Reconciler: r,
156159
Ctx: ctx,
@@ -171,6 +174,15 @@ func (r *ReconcileIAMPartialPolicy) Reconcile(ctx context.Context, request recon
171174
return reconcile.Result{RequeueAfter: jitteredPeriod}, nil
172175
}
173176

177+
func (r *ReconcileIAMPartialPolicy) handleDefaultStateIntoSpecValue(pp *iamv1beta1.IAMPartialPolicy) error {
178+
// Validate or set the default value (cluster-level or namespace-level) for
179+
// the 'state-into-spec' annotation.
180+
if err := k8s.ValidateOrDefaultStateIntoSpecAnnotation(pp, k8s.StateMergeIntoSpec); err != nil {
181+
return fmt.Errorf("error validating or defaulting the '%v' annotation for resource '%v': %w", k8s.StateIntoSpecAnnotation, k8s.GetNamespacedName(pp), err)
182+
}
183+
return nil
184+
}
185+
174186
func (r *reconcileContext) doReconcile(pp *iamv1beta1.IAMPartialPolicy) (requeue bool, err error) {
175187
defer execution.RecoverWithInternalError(&err)
176188
if !pp.DeletionTimestamp.IsZero() {

‎pkg/controller/iam/policy/iampolicy_controller.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,9 @@ func (r *ReconcileIAMPolicy) Reconcile(ctx context.Context, request reconcile.Re
150150
// Error reading the object - requeue the request.
151151
return reconcile.Result{}, err
152152
}
153+
if err := r.handleDefaultStateIntoSpecValue(policy); err != nil {
154+
return reconcile.Result{}, fmt.Errorf("error handling default values for IAM policy '%v': %w", k8s.GetNamespacedName(policy), err)
155+
}
153156
runCtx := &reconcileContext{
154157
Reconciler: r,
155158
Ctx: ctx,
@@ -170,6 +173,15 @@ func (r *ReconcileIAMPolicy) Reconcile(ctx context.Context, request reconcile.Re
170173
return reconcile.Result{RequeueAfter: jitteredPeriod}, nil
171174
}
172175

176+
func (r *ReconcileIAMPolicy) handleDefaultStateIntoSpecValue(policy *iamv1beta1.IAMPolicy) error {
177+
// Validate or set the default value (cluster-level or namespace-level) for
178+
// the 'state-into-spec' annotation.
179+
if err := k8s.ValidateOrDefaultStateIntoSpecAnnotation(policy, k8s.StateMergeIntoSpec); err != nil {
180+
return fmt.Errorf("error validating or defaulting the '%v' annotation for resource '%v': %w", k8s.StateIntoSpecAnnotation, k8s.GetNamespacedName(policy), err)
181+
}
182+
return nil
183+
}
184+
173185
func (r *reconcileContext) doReconcile(policy *iamv1beta1.IAMPolicy) (requeue bool, err error) {
174186
defer execution.RecoverWithInternalError(&err)
175187
if !policy.DeletionTimestamp.IsZero() {

‎pkg/controller/iam/policymember/iampolicymember_controller.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
149149
}
150150
return reconcile.Result{}, err
151151
}
152+
if err := r.handleDefaultStateIntoSpecValue(&memberPolicy); err != nil {
153+
return reconcile.Result{}, fmt.Errorf("error handling default values for IAM policy member '%v': %w", k8s.GetNamespacedName(&memberPolicy), err)
154+
}
152155
reconcileContext := &reconcileContext{
153156
Reconciler: r,
154157
Ctx: ctx,
@@ -172,6 +175,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
172175
return reconcile.Result{RequeueAfter: requeueAfter}, nil
173176
}
174177

178+
func (r *Reconciler) handleDefaultStateIntoSpecValue(policyMember *iamv1beta1.IAMPolicyMember) error {
179+
// Validate or set the default value (cluster-level or namespace-level) for
180+
// the 'state-into-spec' annotation.
181+
if err := k8s.ValidateOrDefaultStateIntoSpecAnnotation(policyMember, k8s.StateMergeIntoSpec); err != nil {
182+
return fmt.Errorf("error validating or defaulting the '%v' annotation for resource '%v': %w", k8s.StateIntoSpecAnnotation, k8s.GetNamespacedName(policyMember), err)
183+
}
184+
return nil
185+
}
186+
175187
func (r *reconcileContext) doReconcile(policyMember *iamv1beta1.IAMPolicyMember) (requeue bool, err error) {
176188
defer execution.RecoverWithInternalError(&err)
177189
if !policyMember.DeletionTimestamp.IsZero() {

‎pkg/controller/lifecyclehandler/handler.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -197,20 +197,22 @@ func reasonForUnresolvableDeps(err error) (string, error) {
197197

198198
func (r *LifecycleHandler) EnsureFinalizers(ctx context.Context, original, resource *k8s.Resource, finalizers ...string) error {
199199
if !k8s.EnsureFinalizers(resource, finalizers...) {
200-
u, err := original.MarshalAsUnstructured()
200+
uo, err := original.MarshalAsUnstructured()
201201
if err != nil {
202202
return err
203203
}
204-
copy, err := k8s.NewResource(u)
204+
originalCopy, err := k8s.NewResource(uo)
205205
if err != nil {
206206
return err
207207
}
208-
if !k8s.EnsureFinalizers(copy, finalizers...) {
209-
if err := r.updateAPIServer(ctx, copy); err != nil {
208+
if !k8s.EnsureFinalizers(originalCopy, finalizers...) {
209+
if err := r.updateAPIServer(ctx, originalCopy); err != nil {
210210
return err
211211
}
212-
// sync the resource up with the updated metadata.
213-
resource.ObjectMeta = copy.ObjectMeta
212+
// Sync the resource up with the updated metadata except for the
213+
// defaulted / pre-processed annotations.
214+
originalCopy.ObjectMeta.Annotations = resource.ObjectMeta.Annotations
215+
resource.ObjectMeta = originalCopy.ObjectMeta
214216
}
215217
}
216218
return nil

‎pkg/controller/tf/controller.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (res
174174
if err != nil {
175175
return reconcile.Result{}, fmt.Errorf("could not parse resource %s: %v", req.NamespacedName.String(), err)
176176
}
177+
if err := r.handleDefaultStateIntoSpecValue(resource); err != nil {
178+
return reconcile.Result{}, fmt.Errorf("error handling default values for resource '%v': %w", k8s.GetNamespacedName(resource), err)
179+
}
177180
if err := r.applyChangesForBackwardsCompatibility(ctx, resource); err != nil {
178181
return reconcile.Result{}, fmt.Errorf("error applying changes to resource '%v' for backwards compatibility: %v", k8s.GetNamespacedName(resource), err)
179182
}
@@ -398,6 +401,15 @@ func (r *Reconciler) enqueueForImmediateReconciliation(resourceNN types.Namespac
398401
r.immediateReconcileRequests <- genEvent
399402
}
400403

404+
func (r *Reconciler) handleDefaultStateIntoSpecValue(resource *krmtotf.Resource) error {
405+
// Validate or set the default value (cluster-level or namespace-level) for
406+
// the 'state-into-spec' annotation.
407+
if err := k8s.ValidateOrDefaultStateIntoSpecAnnotation(&resource.Resource, k8s.StateMergeIntoSpec); err != nil {
408+
return fmt.Errorf("error validating or defaulting the '%v' annotation for resource '%v': %w", k8s.StateIntoSpecAnnotation, k8s.GetNamespacedName(resource), err)
409+
}
410+
return nil
411+
}
412+
401413
func (r *Reconciler) applyChangesForBackwardsCompatibility(ctx context.Context, resource *krmtotf.Resource) error {
402414
rc := resource.ResourceConfig
403415

@@ -414,13 +426,6 @@ func (r *Reconciler) applyChangesForBackwardsCompatibility(ctx context.Context,
414426
if err := k8s.EnsureHierarchicalReference(ctx, &resource.Resource, rc.HierarchicalReferences, rc.Containers, r.Client); err != nil {
415427
return fmt.Errorf("error ensuring resource '%v' has a hierarchical reference: %v", k8s.GetNamespacedName(resource), err)
416428
}
417-
418-
// Ensure the resource has a state-into-spec annotation.
419-
// This is done to be backwards compatible with resources
420-
// created before the webhook for defaulting the annotation was added.
421-
if err := k8s.EnsureSpecIntoSateAnnotation(&resource.Resource); err != nil {
422-
return fmt.Errorf("error ensuring resource '%v' has a '%v' annotation: %v", k8s.GetNamespacedName(resource), k8s.StateIntoSpecAnnotation, err)
423-
}
424429
return nil
425430
}
426431

‎pkg/k8s/state_into_spec_annotation.go

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,21 +19,16 @@ import (
1919
"strings"
2020

2121
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
22-
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2322
)
2423

25-
func ValidateOrDefaultStateIntoSpecAnnotation(obj *unstructured.Unstructured) error {
24+
// ValidateOrDefaultStateIntoSpecAnnotation validates the value of the
25+
// 'state-into-spec' annotation if it is set and defaults the annotation to the
26+
// passed in defaultValue if it is unset.
27+
func ValidateOrDefaultStateIntoSpecAnnotation(obj metav1.Object, defaultValue string) error {
2628
_, found := GetAnnotation(StateIntoSpecAnnotation, obj)
2729
if !found {
28-
SetAnnotation(StateIntoSpecAnnotation, StateMergeIntoSpec, obj)
29-
}
30-
return validateStateIntoSpecAnnotation(obj)
31-
}
32-
33-
func EnsureSpecIntoSateAnnotation(obj *Resource) error {
34-
_, found := GetAnnotation(StateIntoSpecAnnotation, obj)
35-
if !found {
36-
SetAnnotation(StateIntoSpecAnnotation, StateMergeIntoSpec, obj)
30+
// TODO(b/322836859): Ensure ComputeAddress doesn't need special handling.
31+
SetAnnotation(StateIntoSpecAnnotation, defaultValue, obj)
3732
}
3833
return validateStateIntoSpecAnnotation(obj)
3934
}
@@ -44,14 +39,14 @@ func validateStateIntoSpecAnnotation(obj metav1.Object) error {
4439
return fmt.Errorf("couldn't find the value for '%v' annotation", StateIntoSpecAnnotation)
4540
}
4641

47-
if !isAcceptedValue(val) {
42+
if !isAcceptedValue(val, StateIntoSpecAnnotationValues) {
4843
return fmt.Errorf("invalid value '%v' for '%v' annotation, can be one of {%v}", val, StateIntoSpecAnnotation, strings.Join(StateIntoSpecAnnotationValues, ", "))
4944
}
5045
return nil
5146
}
5247

53-
func isAcceptedValue(val string) bool {
54-
for _, v := range StateIntoSpecAnnotationValues {
48+
func isAcceptedValue(val string, acceptedValues []string) bool {
49+
for _, v := range acceptedValues {
5550
if val == v {
5651
return true
5752
}

‎pkg/k8s/state_into_spec_annotation_test.go

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,14 @@ import (
2222

2323
func TestValidateOrDefaultStateIntoSpecAnnotation(t *testing.T) {
2424
tests := []struct {
25-
name string
26-
obj *unstructured.Unstructured
27-
hasError bool
28-
expectedVal string
25+
name string
26+
obj *unstructured.Unstructured
27+
defaultValue string
28+
hasError bool
29+
expectedVal string
2930
}{
3031
{
31-
name: "user specifies an accepted value",
32+
name: "valid 'state-into-spec' value",
3233
obj: &unstructured.Unstructured{
3334
Object: map[string]interface{}{
3435
"apiVersion": "test1.cnrm.cloud.google.com/v1alpha1",
@@ -40,10 +41,11 @@ func TestValidateOrDefaultStateIntoSpecAnnotation(t *testing.T) {
4041
},
4142
},
4243
},
43-
expectedVal: "merge",
44+
defaultValue: "merge",
45+
expectedVal: "merge",
4446
},
4547
{
46-
name: "user specifies an unacceptable value",
48+
name: "invalid 'state-into-spec' value",
4749
obj: &unstructured.Unstructured{
4850
Object: map[string]interface{}{
4951
"apiVersion": "test1.cnrm.cloud.google.com/v1alpha1",
@@ -55,10 +57,11 @@ func TestValidateOrDefaultStateIntoSpecAnnotation(t *testing.T) {
5557
},
5658
},
5759
},
58-
hasError: true,
60+
defaultValue: "merge",
61+
hasError: true,
5962
},
6063
{
61-
name: "user specifies an empty string",
64+
name: "'state-into-spec' value is an empty string",
6265
obj: &unstructured.Unstructured{
6366
Object: map[string]interface{}{
6467
"apiVersion": "test1.cnrm.cloud.google.com/v1alpha1",
@@ -70,20 +73,22 @@ func TestValidateOrDefaultStateIntoSpecAnnotation(t *testing.T) {
7073
},
7174
},
7275
},
73-
hasError: true,
76+
defaultValue: "merge",
77+
hasError: true,
7478
},
7579
{
76-
name: "defaulting if absent",
80+
name: "'state-into-spec' annotation defaulted to defaultValue if unset",
7781
obj: &unstructured.Unstructured{
7882
Object: map[string]interface{}{
7983
"apiVersion": "test1.cnrm.cloud.google.com/v1alpha1",
8084
"kind": "Test1Bar",
8185
},
8286
},
83-
expectedVal: "merge",
87+
defaultValue: "absent",
88+
expectedVal: "absent",
8489
},
8590
{
86-
name: "BigQueryDataset kind can use 'absent' value",
91+
name: "BigQueryDataset kind can use 'absent' as the 'state-into-spec' value",
8792
obj: &unstructured.Unstructured{
8893
Object: map[string]interface{}{
8994
"apiVersion": "bigquery.cnrm.cloud.google.com/v1beta1",
@@ -95,10 +100,11 @@ func TestValidateOrDefaultStateIntoSpecAnnotation(t *testing.T) {
95100
},
96101
},
97102
},
98-
expectedVal: "absent",
103+
defaultValue: "absent",
104+
expectedVal: "absent",
99105
},
100106
{
101-
name: "BigQueryDataset kind can use 'merge' value",
107+
name: "BigQueryDataset kind can use 'merge' as the 'state-into-spec' value",
102108
obj: &unstructured.Unstructured{
103109
Object: map[string]interface{}{
104110
"apiVersion": "bigquery.cnrm.cloud.google.com/v1beta1",
@@ -110,14 +116,26 @@ func TestValidateOrDefaultStateIntoSpecAnnotation(t *testing.T) {
110116
},
111117
},
112118
},
113-
expectedVal: "merge",
119+
defaultValue: "merge",
120+
expectedVal: "merge",
121+
},
122+
{
123+
name: "error out if defaultValue is invalid",
124+
obj: &unstructured.Unstructured{
125+
Object: map[string]interface{}{
126+
"apiVersion": "test1.cnrm.cloud.google.com/v1alpha1",
127+
"kind": "Test1Bar",
128+
},
129+
},
130+
defaultValue: "invalid",
131+
hasError: true,
114132
},
115133
}
116134

117135
for _, tc := range tests {
118136
tc := tc
119137
t.Run(tc.name, func(t *testing.T) {
120-
err := ValidateOrDefaultStateIntoSpecAnnotation(tc.obj)
138+
err := ValidateOrDefaultStateIntoSpecAnnotation(tc.obj, tc.defaultValue)
121139
if tc.hasError {
122140
if err == nil {
123141
t.Fatalf("got nil, but expect an error")

0 commit comments

Comments
 (0)