Add update operations for VolumeSize, InstanceType, and BrokerCount by michaelhtm · Pull Request #55 · aws-controllers-k8s/kafka-controller

michaelhtm · 2025-02-26T22:02:18Z

Description of changes:
Also added reconcile before deletion attempt if cluster is
not active. Before, if we tried deleting a cluster that was
not active, the controller would set it to terminal, and does
not reconcile.

Another change being added is a field in the Cluster status
called ClusterVersion. This field is returned by the sdkFind
operation, and is crutial to make updates to VolumeSize,
InstanceType, and BrokerCount.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Also added reconcile before deletion attempt if cluster is not active. Before, if we tried deleting a cluster that was not active, the controller would set it to terminal, and does not reconcile.

ack-prow · 2025-03-04T23:59:40Z

@michaelhtm: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
kafka-verify-attribution	`3857374`	link	false	`/test kafka-verify-attribution`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

rushmash91

Amazing!!!

rushmash91 · 2025-03-05T00:27:39Z

/lgtm

ack-prow · 2025-03-05T00:27:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michaelhtm, rushmash91

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [michaelhtm,rushmash91]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

michaelhtm · 2025-03-05T00:27:47Z

/hold

michaelhtm · 2025-03-05T00:27:55Z

/unhold

a-hilaly

w00t!

a-hilaly · 2025-03-05T00:44:57Z

      BootstrapBrokerStringVpcConnectivityTls:
        type: string
        is_read_only: true
+      CurrentVersion:


Why do you need both spec.KafkaVersion and `CurrentVersion? do they serve the same purpose?

CurrentVersion is just the version of the cluster..it's different values from KafkaVersion...also ClusterVersion comes from the Describe output smh

a-hilaly · 2025-03-05T00:45:44Z

+		input := &svcsdk.UpdateSecurityInput{}
+		if desired.ko.Status.CurrentVersion != nil {
+			input.CurrentVersion = desired.ko.Status.CurrentVersion
+		}
+		if desired.ko.Status.ACKResourceMetadata.ARN != nil {
+			input.ClusterArn = (*string)(desired.ko.Status.ACKResourceMetadata.ARN)
+		}
+		if desired.ko.Spec.ClientAuthentication != nil {
+			f0 := &svcsdktypes.ClientAuthentication{}
+			if desired.ko.Spec.ClientAuthentication.SASL != nil {
+				f0f0 := &svcsdktypes.Sasl{}
+				if desired.ko.Spec.ClientAuthentication.SASL.IAM != nil &&
+					desired.ko.Spec.ClientAuthentication.SASL.IAM.Enabled != nil {
+					f0f0f0 := &svcsdktypes.Iam{
+						Enabled: desired.ko.Spec.ClientAuthentication.SASL.IAM.Enabled,
+					}
+					f0f0.Iam = f0f0f0
+				}
+				if desired.ko.Spec.ClientAuthentication.SASL.SCRAM != nil &&
+					desired.ko.Spec.ClientAuthentication.SASL.SCRAM.Enabled != nil {
+					f0f0f1 := &svcsdktypes.Scram{
+						Enabled: desired.ko.Spec.ClientAuthentication.SASL.SCRAM.Enabled,
+					}
+					f0f0.Scram = f0f0f1
+				}
+				f0.Sasl = f0f0
+			}
+			if desired.ko.Spec.ClientAuthentication.TLS != nil {
+				f0f1 := &svcsdktypes.Tls{}
+				if desired.ko.Spec.ClientAuthentication.TLS.CertificateAuthorityARNList != nil {
+					f0f1.CertificateAuthorityArnList = aws.ToStringSlice(desired.ko.Spec.ClientAuthentication.TLS.CertificateAuthorityARNList)
+				}
+				if desired.ko.Spec.ClientAuthentication.TLS.Enabled != nil {
+					f0f1.Enabled = desired.ko.Spec.ClientAuthentication.TLS.Enabled
+				}
+				f0.Tls = f0f1
+			}
+			if desired.ko.Spec.ClientAuthentication.Unauthenticated != nil &&
+				desired.ko.Spec.ClientAuthentication.Unauthenticated.Enabled != nil {
+				f0.Unauthenticated = &svcsdktypes.Unauthenticated{
+					Enabled: desired.ko.Spec.ClientAuthentication.Unauthenticated.Enabled,
+				}
+			}
+			input.ClientAuthentication = f0
+		}
+
+		_, err = rm.sdkapi.UpdateSecurity(ctx, input)
+		rm.metrics.RecordAPICall("UPDATE", "UpdateSecurity", err)
+		if err != nil {
+			return nil, err
+		}
+		ackcondition.SetSynced(updatedRes, corev1.ConditionFalse, nil, nil)
+		err = requeueAfterAsyncUpdate()
+


Can we use a function to hide this logic?

a-hilaly · 2025-03-05T00:46:06Z

+		_, err := rm.sdkapi.UpdateBrokerStorage(ctx, &svcsdk.UpdateBrokerStorageInput{
+			ClusterArn:     (*string)(latest.ko.Status.ACKResourceMetadata.ARN),
+			CurrentVersion: latest.ko.Status.CurrentVersion,
+			TargetBrokerEBSVolumeInfo: []svcsdktypes.BrokerEBSVolumeInfo{
+				{
+					KafkaBrokerNodeId: aws.String("ALL"),
+					VolumeSizeGB:      aws.Int32(int32(*desired.ko.Spec.BrokerNodeGroupInfo.StorageInfo.EBSStorageInfo.VolumeSize)),
+				},
+			},
+		})
+		rm.metrics.RecordAPICall("UPDATE", "UpdateBrokerStorage", err)
+		if err != nil {
+			return nil, err
+		}
+		message := fmt.Sprintf("kafka is updating broker storage")
+		ackcondition.SetSynced(updatedRes, corev1.ConditionFalse, &message, nil)
+		err = requeueAfterAsyncUpdate()


a-hilaly · 2025-03-05T00:46:15Z

+		_, err := rm.sdkapi.UpdateBrokerType(ctx, &svcsdk.UpdateBrokerTypeInput{
+			ClusterArn:         (*string)(latest.ko.Status.ACKResourceMetadata.ARN),
+			CurrentVersion:     latest.ko.Status.CurrentVersion,
+			TargetInstanceType: desired.ko.Spec.BrokerNodeGroupInfo.InstanceType,
+		})
+		rm.metrics.RecordAPICall("UPDATE", "UpdateBrokerType", err)
+		if err != nil {
+			return nil, err
+		}
+		message := fmt.Sprintf("kafka is updating broker instanceType")
+		ackcondition.SetSynced(updatedRes, corev1.ConditionFalse, &message, nil)


a-hilaly · 2025-03-05T00:49:22Z

+type unprocessedSecret struct {
+	errorCode    string
+	errorMessage string
+	secretArn    string
+}
+
+func (us unprocessedSecret) String() string {
+	return fmt.Sprintf("ErrorCode: %s, ErrorMessage %s, SecretArn: %s", us.errorCode, us.errorMessage, us.secretArn)
+}


how about making an error type, encapsulating all the unprocessed secrets. And implement an Error method that formats it all for you. I guess that's what you're trying to do in L388+

mmm, not sure how we can achieve that since each secret may have its own reason why it can't be attached...i don't think i can return a slice of errors..

unless maybe if i make all three fields a list of strings..after i would just need to fix how I represent it as a string

Issue #55 Description of changes: * wrapped update operations in functions * made `unprocessedSecrets` implement error By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow Bot requested review from jlbutler and rushmash91 February 26, 2025 22:02

ack-prow Bot added the approved label Feb 26, 2025

michaelhtm force-pushed the handle/updates branch 3 times, most recently from 26aab76 to c670340 Compare February 27, 2025 22:42

Add update operations for VolumeSize, InstanceType, and BrokerCount

c40ba8f

Also added reconcile before deletion attempt if cluster is not active. Before, if we tried deleting a cluster that was not active, the controller would set it to terminal, and does not reconcile.

michaelhtm force-pushed the handle/updates branch 7 times, most recently from 5ce92f5 to be660de Compare March 4, 2025 19:58

michaelhtm assigned a-hilaly and rushmash91 Mar 4, 2025

michaelhtm force-pushed the handle/updates branch from be660de to f8009f8 Compare March 4, 2025 21:24

Handle BatchAssociateScramSecret errors

3857374

michaelhtm force-pushed the handle/updates branch from f8009f8 to 3857374 Compare March 4, 2025 23:58

rushmash91 reviewed Mar 5, 2025

View reviewed changes

Comment thread config/controller/kustomization.yaml

Comment thread pkg/resource/cluster/hooks.go

Comment thread pkg/resource/cluster/hooks.go

ack-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2025

ack-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2025

ack-prow Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2025

ack-prow Bot merged commit 1721e84 into aws-controllers-k8s:main Mar 5, 2025

a-hilaly reviewed Mar 5, 2025

View reviewed changes

michaelhtm mentioned this pull request Mar 5, 2025

Address PR comments for Support updates #56

Merged

Conversation

michaelhtm commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow Bot commented Mar 4, 2025

Uh oh!

rushmash91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rushmash91 commented Mar 5, 2025

Uh oh!

ack-prow Bot commented Mar 5, 2025

Uh oh!

michaelhtm commented Mar 5, 2025

Uh oh!

michaelhtm commented Mar 5, 2025

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

a-hilaly Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

a-hilaly Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

a-hilaly Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

michaelhtm Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelhtm commented Feb 26, 2025 •

edited

Loading

michaelhtm Mar 5, 2025 •

edited

Loading