Added support for EKS control plane logging

[MarcusNoble]

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8204

Changes proposed in this pull request:

• Add the ability to enable or disable the various logging for an EKS control plane

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSEksCluster_Logging'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSEksCluster_Logging -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSEksCluster_Logging
=== PAUSE TestAccAWSEksCluster_Logging
=== CONT  TestAccAWSEksCluster_Logging
--- PASS: TestAccAWSEksCluster_Logging (1314.83s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       1315.972s

[bflad]

Hi @MarcusNoble 👋 Thanks for submitting this, it is off to a good start. Please see some initial feedback below and let us know if you have any questions or do not have time to complete these items.

[bflad]

To make this check more meaningful, it should check len(l.([]interface{})) > 0 as d.GetOk() should always return ok for root level list attributes. It'd be simpler if the logging was always set in the struct above and the expand function handled any special logic though, e.g.

	input := &eks.CreateClusterInput{
		Name:               aws.String(name),
		Logging:            expandEksLoggingTypes(d.Get("logging").([]interface{})),
		RoleArn:            aws.String(d.Get("role_arn").(string)),
		ResourcesVpcConfig: expandEksVpcConfigRequest(d.Get("vpc_config").([]interface{})),
	}
// ...
func expandEksLoggingTypes(logTypes []interface{}) *eks.Logging {
	if len(logTypes) == 0 {
		return nil
	}
	// ...

[bflad]

Before accessing anything inside cluster.Logging we should first perform a nil check to prevent potential panics. 👍

It also doesn't seem like we should always depend on a single LogSetup object that is Enabled here, even if that is how the API behaves today. We could miss results by not looping through the objects and especially if the API's first result is disabled while some of the rest may be enabled. It seems like we should check each LogSetup element then check Enabled. We then would typically suggest moving this into its own flatten function to simplify things a little.

e.g.

enabledClusterLogTypes := []string{}

if cluster.Logging != nil {
  enabledClusterLogTypes = flattenEksEnabledLogTypes(cluster.Logging.ClusterLogging)
}
// ...
func flattenEksEnabledLogTypes(logSetups []*eks.LogSetup) []interface{} {
	enabledLogTypes := []string{}

	for _, logSetup := range logSetups {
		if logSetup == nil || !aws.BoolValue(logSetup.Enabled) {
			continue
		}

		for _, logType := logSetup.Types {
			enabledLogTypes = append(enabledLogTypes, aws.BoolValue(logType))
		}
	}

	return enabledLogTypes
}

[bflad]

When using d.Set() with aggregate types (TypeList, TypeSet, TypeMap), we should perform error checking to prevent issues where the code is not properly able to set the Terraform state. e.g.

Suggested change

	d.Set("logging", types)
	if err := d.Set("logging", enabledClusterLoggingTypes); err != nil {
	return fmt.Errorf("error setting logging: %s", err)
	}

[bflad]

We should consider renaming this attribute cluster_log_types or even enabled_cluster_log_types to show the designation that this handling is specifically for the Types within the ClusterLogging of the API. It also needs documentation added in website/docs/r/eks_cluster.html.markdown 👍

[bflad]

Nit: This can be simplified to

Suggested change

	Enabled: &enabled,
	Enabled: aws.Bool(true),

[bflad]

Nit: I believe this could be simplified with expandStringList(logTypes)

[MarcusNoble]

@bflad Thank you so much for your feedback. I'm still fairly new to Go so was very helpful. I believe I've addressed all your comments now. :)

[bflad]

Looks good, @MarcusNoble! Thanks so much. 🚀

Output from acceptance testing:

--- PASS: TestAccAWSEksCluster_Logging (1210.36s)
--- PASS: TestAccAWSEksCluster_basic (1229.58s)
--- PASS: TestAccAWSEksClusterDataSource_basic (1249.35s)
--- PASS: TestAccAWSEksCluster_VpcConfig_SecurityGroupIds (1371.63s)
--- PASS: TestAccAWSEksCluster_VpcConfig_EndpointPrivateAccess (1418.73s)
--- PASS: TestAccAWSEksCluster_VpcConfig_EndpointPublicAccess (1553.32s)
--- PASS: TestAccAWSEksCluster_Version (2674.15s)

[bflad]

Nit: Should be alphabetically sorted to match rest of documentation

[bflad]

This has been released in version 2.6.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!