-
Notifications
You must be signed in to change notification settings - Fork 205
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
feat: support argocd addon management #588
Conversation
Feature/argo config management
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@starchx Looks very good! I added a few mostly minor comments/questions on this revision.
P.S.
It is weird that GitOps mode won't apply to core EKS add-on like vpc-cni, ebs-csi and such, e.g customers won't be able to change versions of such add-ons through GitOps.
lib/addons/cert-manager/index.ts
Outdated
@@ -23,7 +23,7 @@ export interface CertManagerAddOnProps extends HelmAddOnUserProps { | |||
* Default props to be used when creating the Helm chart | |||
*/ | |||
const defaultProps: HelmAddOnProps & CertManagerAddOnProps = { | |||
name: "blueprints-cert-manager-addon", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the change makes sense, however want to understand if it has any impact for customers upgrading the blueprints version. We will need to document and/provide path forward for any changes that may be incompatible with existing deployments through EKS blueprints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I am testing this at the moment:
- creating a cluster with old name
blueprints-cert-manager-addon
- changing name from
blueprints-cert-manager-addon
tocert-manager
- update the cluster again
I will update here once the test is completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per my testing, the name is only used as a logical name in the CloudFormation. Notice the props.name
in the code below.
public static applyHelmDeployment = function (clusterInfo: ClusterInfo, props: HelmChartDeployment): Construct {
return clusterInfo.cluster.addHelmChart(props.name, {
repository: props.repository,
HOWEVER - This causes all the deployed cert-manager in cluster resources got DELETED.
BEFORE NAME CHANGES:
AFTER NAME CHANGES:
Looks like I will have to revert this name change. @shapirov103 Let me know if you have any other ideas please :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is going on with cert-manager, unsure if it is related. We started experiencing failures of cert-manager yesterday. I am curious if provisioning of cert-manager works at all at the moment with blueprints. I understand that name change can cause the previous deployment to be deleted, but new resources should be provisioned. One workaround is to change the namespace of the cert-manager deployment.
There is a chance that namespace termination happens after the add-on with the new name is added.
@starchx since we are quite close to finishing this, do you mind taking a look at the above comments and bring it to the finish line? |
@shapirov103 , GitOps mode only applies to those addons extending from
Core addons are not applied:
|
@shapirov103 , I added a small change to enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@starchx almost there, looks great. I added a few minor comments, we can kick off tests after that. Targeting the next release.
nameValues.push({ name: key, value: `${flatValues[key]}`}); | ||
for (let key in flatValues) { | ||
// Avoid passing the undefined values and empty objects | ||
if (flatValues[key] !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement is a little confusing. You do the same action in both cases, why are you using else if
? Why not just if(<all conditions>)
?
Also if you are checking for undefined, what is the purpose of testing for Object.getPrototypeOf
? Why not just flatValues[key] === undefined
(or !== undefined
? Are you checking for empty arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am filtering out undefined
and empty objects
.
After flatten using dot.dot(deployment.values)
, we will have in flatValues
:
'appmeshController.serviceAccount.create': false,
'appmeshController.serviceAccount.name': 'appmesh-controller',
'appmeshController.tracing.enabled': false,
'appmeshController.tracing.provider': 'x-ray',
'appmeshController.tracing.address': undefined, <<=== This will be filtered out
'appmeshController.tracing.port': undefined, <<=== This will be filtered out
'appmeshController.enable': true,
'something': {} <<=== This will be filtered out, although I cannot recall how this was generated in the first place..
The logic of If else
is:
- If value is not defined, skip.
- otherwise:
- If the value is NOT an object, include it
- If the value is an object, and it is not empty, include it as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added a comment to the code, hopefully it can help.
@@ -89,6 +89,8 @@ function populateValues(helmOptions: EfsCsiDriverProps, clusterName: string, | |||
setPath(values, "clusterName", clusterName); | |||
setPath(values, "controller.serviceAccount.create", false); | |||
setPath(values, "controller.serviceAccount.name", serviceAccountName); | |||
setPath(values, "node.serviceAccount.create", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What prompted this change? I recall vaguely there was an issue with the EFS SA, seems like the right change, just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EFS CSI Driver addon provisions two kinds of pods in the cluster: controller
and node
. Both of them requires IAM permissions to AWS resources. This line passes the same k8s service account
to the node
pods, which was missing, otherwise the node
pods fail to stabilize.
'awsEfsCsiDriver.controller.serviceAccount.create': false,
'awsEfsCsiDriver.controller.serviceAccount.name': 'efs-csi-controller-sa',
'awsEfsCsiDriver.node.serviceAccount.create': false, <<=== new
'awsEfsCsiDriver.node.serviceAccount.name': 'efs-csi-controller-sa', <<=== new
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests passed
Issue #, if available:
#299
Description of changes:
A factory class is introduced to make it easy for users to enable or disable GitOps mode via cluster builder:
Two patterns have been added to show how to use this feature: aws-samples/cdk-eks-blueprints-patterns#59
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.