Skip to content

CSI-1655: added two sample apps #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: integration
Choose a base branch
from
Open

Conversation

kdeng
Copy link

@kdeng kdeng commented May 27, 2025

No description provided.

@kdeng kdeng requested a review from neoakris May 27, 2025 02:57
@kdeng kdeng force-pushed the features/csi-1655 branch 7 times, most recently from 29f47e9 to 8294e49 Compare May 27, 2025 04:01
Copy link
Collaborator

@neoakris neoakris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found 2 things I think should change, left comments.

I'm just now starting to manually test the logic and will test it before commenting on it. (more changes might be requested after I manage to test, but I pulled your code locally so I could test it locally.)

@kdeng kdeng force-pushed the features/csi-1655 branch 2 times, most recently from 2009741 to 202d236 Compare May 28, 2025 09:01
@kdeng
Copy link
Author

kdeng commented May 28, 2025

Thanks for your feedback. I just applied the changes as you said.

@kdeng
Copy link
Author

kdeng commented May 28, 2025

@neoakris I also noticed that the stack cannot be deleted properly since ALB cannot be deleted firstly. Is there a way to solve this problem?

@neoakris
Copy link
Collaborator

I remembered an important thing about the link you shared yesterday
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_eks-readme.html#:~:text=Every%20Kubernetes%20manifest,be%20done%20explicitly

I've discovered that occasionally in theory cdk will have 2-3 options for accomplishing the same end result.

new eks.Cluster(this, 'HelloEKS', {
  version: eks.KubernetesVersion.V1_32,
  albController: {
    version: eks.AlbControllerVersion.V2_8_2,
    additionalHelmChartValues: {
      enableWafv2: false
    }
  },
  kubectlLayer: new KubectlV32Layer(this, 'kubectl'),
});

^-- let's say this = option 1 for deploying AWS LB Controller

Note easy eks's logic doesn't use that (notice optional parameter albController is missing)

this.cluster = new eks.Cluster(this.stack, this.config.id, {
  clusterName: this.config.id,
  version: this.config.kubernetesVersion,
  kubectlLayer: this.config.kubectlLayer,
  vpc: this.config.vpc,
  ipFamily: this.config.ipMode,
  vpcSubnets: [{ subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }],
  defaultCapacity: 0,
  tags: this.config.tags,
  authenticationMode: eks.AuthenticationMode.API_AND_CONFIG_MAP,
  mastersRole: assumableEKSAdminAccessRole, //<-- adds aws eks update-kubeconfig output
  secretsEncryptionKey: kms_key,
});

That's intentional.
Easy EKS, uses a 2nd option. (deploying the AWS LB Controller as a helm chart)

Because I learned that while in theory there's sometimes 2-3 ways to accomplish an end result.
In practice some of the methods are

  • buggy / can't do certain integrations with other components
  • limited in functionality (limited configurability or can only use outdated versions)
  • so while in theory there's 3 ways to do a task in practice there's only 1 good way to do a task, and bad vs good ways are often poorly documented.
  • example: the vast majority of EKS Blueprints for CDK is extremely buggy, can only deploy 3 year old versions of things, and is unmaintained, so using it means certain bugs can't be fixed, which is why I moved away from it.

If I recall correctly I purposefully didn't use the method in the doc you linked, because it could only be used to deploy old versions, and had extremely limited configurability.

The doc you linked mentions a fix involving "cluster.albController"
I wanted to point out that because we use method 2 (ALB deployed via external helm chart)
cluster.albController = null in the Easy EKS code base, so won't work.


Your best bet at solving is probably what we discussed in office hours.

  1. If encounter failures destroying
    Then change default retention policy from destroy to retain
//v-- The following 2 lines help prevent cdk destroy issue
const karpenter_helm_chart_CFR = (stack.node.tryFindChild(config.id)?
  .node.tryFindChild('chart-karpenter')?
  .node.defaultChild as cdk.CfnResource
);
if(karpenter_helm_chart_CFR){ 
  karpenter_helm_chart_CFR.applyRemovalPolicy(cdk.RemovalPolicy.RETAIN); 
}
  1. If encounter failures creating
    Then add dependency
karpenter.node.addDependency(readiness_dependency); //Expected value of awsLoadBalancerController Helm Release

^-- so something like

const AWS_LBC_Cloud_Formation_Resource = (stack.node.tryFindChild(config.id)?
  .node.tryFindChild('chart-AWSLoadBalancerController')?
  .node.defaultChild as cdk.CfnResource
);

const apply_ingress_YAML = new eks.KubernetesManifest(stack, 'ingress_YAML',
  {
    cluster: cluster,
    manifest: ingress_YAML,
    overwrite: true,
    prune: true,
  }
);

apply_ingress_YAML.node.addDependency(AWS_LBC_Cloud_Formation_Resource)

Reminder:

const AWS_LBC_Cloud_Formation_Resource = (stack.node.tryFindChild(config.id)?
  .node.tryFindChild('chart-AWSLoadBalancerController')?
  .node.defaultChild as cdk.CfnResource
);

config.id's expected value is dev1-eks, so the above code snippet
corresponds to the following spot within of Cloud Formation
image

@kdeng kdeng force-pushed the features/csi-1655 branch from 202d236 to b223219 Compare May 30, 2025 12:02
@kdeng
Copy link
Author

kdeng commented May 30, 2025

@neoakris Thanks for this information. Yes, I have tried with your suggestion already. By using tryFindChild to lookup ALB CFR, it would help cdk destroy to delete the ALB resource, but the deletion process will be stuck in the process of deleting target group. Which is the main reason I choose to use RemovalPolicy.RETAIN. I tried to search all resources in the stack, but cannot find a way to locate the TG, which seems to be created by AWS-ALBController.

@kdeng kdeng force-pushed the features/csi-1655 branch from b223219 to 7cebd43 Compare June 2, 2025 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants