Add CustomPlugin and WorkerConfiguration resources along with tagging support for all resources #14

divymamg · 2024-04-19T06:42:41Z

Issue #, if available:

Description of changes:

Add AWS::KafkaConnect::CustomPlugin and AWS::KafkaConnect::WorkerConfiguration CFN resources.
Add tagging support to all of the resources.
Upgrade JDK version from 8 to 17.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

… support for all resources

Tvli · 2024-06-06T13:56:17Z

...nect-connector/src/main/java/software/amazon/kafkaconnect/connector/ExceptionTranslator.java

@@ -36,7 +36,7 @@ public BaseHandlerException translateToCfnException(
        }

        if (exception instanceof BadRequestException) {
-            return new CfnInvalidRequestException(ResourceModel.TYPE_NAME, exception);
+            return new CfnInvalidRequestException(exception.getMessage(), exception);


Why only this exception changed from resource type to exception message? I see there are other exceptions that being handled differently

Tvli · 2024-06-06T14:00:27Z

aws-kafkaconnect-connector/src/main/java/software/amazon/kafkaconnect/connector/TagHelper.java

+     */
+    public static Map<String, String> generateTagsToAdd(final Map<String, String> previousTags, final Map<String, String> desiredTags) {
+        return desiredTags.entrySet().stream()
+            .filter(e -> !previousTags.containsKey(e.getKey()) || !Objects.equals(previousTags.get(e.getKey()), e.getValue()))


nit: the e reads like a exception by a glance, consider use a proper name such as desiredTag

Tvli · 2024-06-06T14:01:11Z

aws-kafkaconnect-connector/src/main/java/software/amazon/kafkaconnect/connector/Translator.java

+    }
+
+    /**
+     * Request to add tags to a resource


request to remove tags from a resource

I haven't check all the comments in details, please review the accuracy and correctness of the comments, especially the content in docs

Tvli · 2024-06-06T14:03:51Z

...fkaconnect-connector/src/main/java/software/amazon/kafkaconnect/connector/UpdateHandler.java

@@ -79,6 +83,8 @@ protected ProgressEvent<ResourceModel, CallbackContext> handleRequest(
        return ProgressEvent.progress(request.getDesiredResourceState(), callbackContext)
            .then(progress ->
                verifyUpdatable(proxy, proxyClient, progress, "AWS-KafkaConnect-Connector::PreUpdateCheck"))


does verifyUpdatable also check if tags got changed or not? if customer only changed tags, it should be treated as updatable and move to the next step.

Tvli · 2024-06-06T14:07:52Z

...fkaconnect-connector/src/main/java/software/amazon/kafkaconnect/connector/UpdateHandler.java

+            if (!removedTags.isEmpty()) {
+
+                final UntagResourceRequest untagResourceRequest = Translator.untagResourceRequest(desiredModel, removedTags);
+                try {
+                    proxyClient.injectCredentialsAndInvokeV2(untagResourceRequest, proxyClient.client()::untagResource);
+                    logger.log(String.format("Removed %d tags", removedTags.size()));
+                } catch (final AwsServiceException e) {
+                    throw exceptionTranslator.translateToCfnException(e, identifier);
+                }
+            }
+
+            // calculate tags to update based on Tags (key + value)
+            if (!addedTags.isEmpty()) {
+                final TagResourceRequest tagResourceRequest = Translator.tagResourceRequest(desiredModel, addedTags);
+                try {
+                    proxyClient.injectCredentialsAndInvokeV2(tagResourceRequest, proxyClient.client()::tagResource);
+                    logger.log(String.format("Added %d tags", addedTags.size()));
+                } catch (final AwsServiceException e) {
+                    throw exceptionTranslator.translateToCfnException(e, identifier);
+                }
+            }


Is this logic here idempotent and handle partial failures well?

Let's say when tags being removed successfully but then the add tags failed, what happens?

Tvli · 2024-06-06T14:09:33Z

...fkaconnect-connector/src/main/java/software/amazon/kafkaconnect/connector/UpdateHandler.java


-        return describeConnectorResponse;
+
+    private ProgressEvent<ResourceModel, CallbackContext> verifyNonCreateOnlyFieldsHaveToBeUpdated(


nit: verifyUpdatableFieldsGotChanged? the current name is a bit confusing.

Tvli · 2024-06-10T16:29:32Z

...nect-customplugin/src/main/java/software/amazon/kafkaconnect/customplugin/CreateHandler.java

+            throw exceptionTranslator.translateToCfnException(e, identifier);
+        }
+
+        logger.log(String.format("%s [%s] created successfully.", ResourceModel.TYPE_NAME, identifier));


I wonder how informative is this by using just name as logging identifier, better to include customer account ID as well. Although it's not part of request body, it should be accessible from request context or similar parts.

Tvli · 2024-06-10T16:33:03Z

...nect-customplugin/src/main/java/software/amazon/kafkaconnect/customplugin/CreateHandler.java

+            case DELETING:
+                throw new CfnResourceConflictException(
+                    ResourceModel.TYPE_NAME,
+                    model.getCustomPluginArn(),
+                    String.format("Another process is deleting this %s", ResourceModel.TYPE_NAME));


When got to the stage where checking if resource is stablised or not, it means the create request got a successful response. Given the locking mechanism we have, deleting shouldn't be a possible case to handle, why do we think this case is necessary to be handled?

Tvli · 2024-06-10T16:39:05Z

...nect-customplugin/src/main/java/software/amazon/kafkaconnect/customplugin/CreateHandler.java

+                    successMessagePattern,
+                    ResourceModel.TYPE_NAME,
+                    describeCustomPluginRequest.customPluginArn(),
+                    customPluginState == null ? "unknown" : customPluginState.toString()));


What is the reason to handle the customPluginState to be null case? It seems implying describeCustomPlugin call doesn't handle exception properly and setting state to null, if that's not the case, we can remove this check. Another way to look at this, if pluginArn can't be null then pluginState is the same

Tvli · 2024-06-10T16:45:12Z

...nect-customplugin/src/main/java/software/amazon/kafkaconnect/customplugin/DeleteHandler.java

+        if (describeCustomPluginRequest.customPluginArn() == null) {
+            throw new CfnNotFoundException(ResourceModel.TYPE_NAME, null);
+        }


In which case custom plugin Arn will be null? It looks like to me it's a parameter that customers provid and then we use it to describe the resource. If so, here's is more appropriate to throw a 400 instead of 404.

Tvli · 2024-06-10T16:51:52Z

...nect-customplugin/src/main/java/software/amazon/kafkaconnect/customplugin/DeleteHandler.java

+        } catch (final AwsServiceException e) {
+            throw exceptionTranslator.translateToCfnException(e, identifier);
+        }


Although we are validating the resource state by describing the resource, it's still a best effort which covers most cases. The edge case here is that the the resource can be deleted/updated after passes the validation phase, therefore, additional logics are needed.

If resource is being deleted, it can choose make the operation idempotent and move on. Another option is to fail the request. Better to check how other services are handling this case

If resource is being updated, we need to fail the operation.

One way to tackle this is to move the validation resource after the delete call has been dispatched.

Tvli · 2024-06-10T16:55:54Z

...onnect-customplugin/src/main/java/software/amazon/kafkaconnect/customplugin/ListHandler.java

+
+        return ProgressEvent.<ResourceModel, CallbackContext>builder()
+            .resourceModels(models)
+            .nextToken(nextToken)


Does this progressEvent handles nextToken automatically, and make additional call if token exist and stops when token is null?

Tvli · 2024-06-10T16:59:36Z

...aconnect-customplugin/src/main/java/software/amazon/kafkaconnect/customplugin/TagHelper.java

+            return Collections.emptyMap();
+        }
+        return tags.stream()
+            .filter(tag -> tag.getValue() != null)


Do we not support key only tags? Please test this in API

Tvli · 2024-06-10T17:06:29Z

...nect-customplugin/src/main/java/software/amazon/kafkaconnect/customplugin/UpdateHandler.java

+                        desiredModel,
+                        callbackContext)
+                    .translateToServiceRequest(translator::translateToReadRequest)
+                    .makeServiceCall(this::validateResourceExists)


Same as the delete comment, the validateResource logic is likely need to be expanded and move to after the updateAPI call dispatched and handles the resource to be sure we are verifying against the real status of the resource and avoid any race conditions

Not going to repeat similar comments, but some previous comment on code pattern applies to other resources as well, please verify them even though they don't have a comment

divymamg force-pushed the feature/add-cfn-resources-and-tags-support branch 2 times, most recently from fff467f to 98189da Compare May 30, 2024 16:04

Add CustomPlugin and WorkerConfiguration resources along with tagging…

44b3ed9

… support for all resources

divymamg force-pushed the feature/add-cfn-resources-and-tags-support branch from 98189da to 44b3ed9 Compare May 30, 2024 16:07

kmampent approved these changes Jun 6, 2024

View reviewed changes

Tvli reviewed Jun 6, 2024

View reviewed changes

Tvli reviewed Jun 10, 2024

View reviewed changes

Tvli approved these changes Sep 27, 2024

View reviewed changes

Tvli merged commit 71c22ab into aws-cloudformation:main Sep 27, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CustomPlugin and WorkerConfiguration resources along with tagging support for all resources #14

Add CustomPlugin and WorkerConfiguration resources along with tagging support for all resources #14

divymamg commented Apr 19, 2024 •

edited

Loading

Tvli Jun 6, 2024

Tvli Jun 6, 2024

Tvli Jun 6, 2024

Tvli Jun 6, 2024

Tvli Jun 6, 2024

Tvli Jun 6, 2024

Tvli Jun 6, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024

Tvli Jun 10, 2024


		return describeConnectorResponse;

		private ProgressEvent<ResourceModel, CallbackContext> verifyNonCreateOnlyFieldsHaveToBeUpdated(

Add CustomPlugin and WorkerConfiguration resources along with tagging support for all resources #14

Add CustomPlugin and WorkerConfiguration resources along with tagging support for all resources #14

Conversation

divymamg commented Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divymamg commented Apr 19, 2024 •

edited

Loading