aws-quickstart / cdk-eks-blueprints

AWS Quick Start Team
Apache License 2.0
460 stars 207 forks source link

(addons/aws-loadbalancer-controller): AWS Load Balancer Controller does not support Helm configuration option `values` #968

Open Booligoosh opened 8 months ago

Booligoosh commented 8 months ago

Describe the bug

According to the docs, the AWS Load Balancer Controller supports the standard helm configuration options.

However, the source code does not use the values option at all, and does not pass it to the created helm chart.

Expected Behavior

Setting the values option on AwsLoadBalancerControllerAddOn passes values to the HelmChart construct (unless they are overridden by the AddOn itself).

Current Behavior

Setting the values option on AwsLoadBalancerControllerAddOn has no effect.

Reproduction Steps

Pass values to the AddOn:

const addOn = new blueprints.addons.AwsLoadBalancerControllerAddOn({
  values: {
    tolerations: [{ key: "node-role.kubernetes.io/master", effect: "NoSchedule"}],
  },
});

The values will not get added to the Helm Chart in the Cloudformation template.

Possible Solution

To fix the issue, options.values should be injected here: https://github.com/aws-quickstart/cdk-eks-blueprints/blob/main/lib/addons/aws-loadbalancer-controller/index.ts#L105

From a wider perspective, it might be a good idea to add injection of options.values in the addHelmChart of the Helm AddOn itself, that way, all Helm AddOns will automatically have the functionality that their props claim to support (adding the values argument to the Helm chart), whether or not they remember to inject options.values themselves. See the workaround below for a sense of how this would be implemented.

Additional Information/Context

No response

CDK CLI Version

2.115.0

EKS Blueprints Version

1.13.1

Node.js Version

20.11.1

Environment details (OS name and version, etc.)

Windows 10 Enterprise

Other information

For anyone stumbling upon this issue, here is workaround AddOn we have been using to get it working. You can use this class in place of blueprints.addons.AwsLoadBalancerControllerAddOn:

import {
  AwsLoadBalancerControllerAddOn,
  ClusterInfo,
  Values,
} from "@aws-quickstart/eks-blueprints";
import { Duration } from "aws-cdk-lib";
import { Construct } from "constructs";

/**
 * This class is a workaround/override until this Blueprints issue is fixed: https://github.com/aws-quickstart/cdk-eks-blueprints/issues/968
 */
export class AwsLoadBalancerControllerAddOnWithValues extends AwsLoadBalancerControllerAddOn {
  protected addHelmChart(
    clusterInfo: ClusterInfo,
    values?: Values | undefined,
    createNamespace?: boolean | undefined,
    wait?: boolean | undefined,
    timeout?: Duration | undefined,
  ): Construct {
    return super.addHelmChart(
      clusterInfo,
      { ...this.options.values, ...values },
      createNamespace,
      wait,
      timeout,
    );
  }
}
shapirov103 commented 8 months ago

Defect, will be fixed in the next patch.

thpham commented 7 months ago

I'm look forward to see a fix. Would be very much appreciated to help me have defaultTags which can help me to cleanup the LB and sg after stack destroy. Thank you !