-
Notifications
You must be signed in to change notification settings - Fork 161
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
Adding new pattern for Crossplane Argocd GitOps #173
base: main
Are you sure you want to change the base?
Conversation
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.
@ajpaws Thankyou for working on this and quick PR. There are few alignments to the approach we have to make :
- First the arrangment is not aligning to the theme of the repo. You should have one bin
ts
file for this pattern which invokes the class in the lib file to run the pattern. Please check other patterns like SecureIngress Pattern which we worked for another blog. - I understand you most of the code i shared from my partner work but we cant reuse it as is, we need to refactor it work with patterns repo. We done need this file
aws-addon-clusters-stack.ts
for instance. Plus we need one pipeline which deploys management cluster and all child cluster and those pipeline code should be in lib folder like multi account monitoring pattern. - So basically most of the work you have in bin folder should move to lib folder refactored.
- See if you have any opportunities to minimize the number of custom addons.
- Also fix all GH Warnings and errors.
Overall great progress, we have some work to do. Lets connect if you have questions.
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.
@ajpaws Awesome work. Love the idea of adding Pod Identity Addon via ClusterAuth
We are very near to finish line. One minor feedback that i mentioned on bin file. Also lets add the architecture diagram of the blog optionally to make this complete.
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.
@ajpaws great work, a lot of effort. I went through the first round of review and let's fix the issues, I also posted a couple of questions. I will review again after you are done, and we will validate it as well. I realize there is a dependency on the workloads repo PR.
lib/crossplane-argocd-gitops/custom-addons/crossplane-k8s-provider-addon.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,89 @@ | |||
import 'source-map-support/register'; |
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.
@elamaran11 is there a rationale why this addon is not in the blueprints repo? We have upbound addon there, it may be confusing to the customers how to reconcile this with the one in the blueprints repo. I assume we should deprecate or just replace the upbound addon in the 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.
@shapirov103 The upbound addon in Blueprints is EKS Addon and they are no longer supporting it. I asked the team but no response so the Ubound addon on the bluepeints should be depracated or this should be updated there. I think for now lets depracate upbound addon for upcoming release. I will plan to work to move this later to 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.
It is already in 1.15, let's replace it here, - too much code as it stands for the pattern.
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.
Not addressed?
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.
@ajpaws This comment?
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 believe we are deprecating upbound addon in Blueprints is EKS Addon so no changed made on this. I would recommend go ahead with this for now since anyway we have code for all other upbound addons at one place in custom addons folder.
lib/crossplane-argocd-gitops/custom-addons/upbound-crossplane-addon.ts
Outdated
Show resolved
Hide resolved
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.
@ajpaws Fix this for GH Action to run.
@shapirov103 Could you please check again and let me know if this is good to merge. |
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.
@ajpaws please see my additional comments. Thank you for addressing my previous comments, I see some were deferred and that is ok.
I left a bunch of minor comments, but there a few important. Happy to discuss if needed. I will focus on the workloads PR.
@elamaran11 FYI ^^
lib/crossplane-argocd-gitops/custom-addons/crossplane-k8s-provider-addon.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,89 @@ | |||
import 'source-map-support/register'; |
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.
It is already in 1.15, let's replace it here, - too much code as it stands for the pattern.
done. made all changes |
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.
Overall looks good to me, pending resolution of feedback and functional verification by @elamaran11
``` | ||
make build | ||
make list | ||
make pattern aws-addon-clusters deploy |
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.
Is that the correct name? should not it be crossplace-argocd-gitops?
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.
fixed
@@ -0,0 +1,89 @@ | |||
import 'source-map-support/register'; |
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.
Not addressed?
@ajpaws Please pull from main to get past the Markdown error,. |
merging changes from main to fix Markdown errors
done |
|
||
```shell | ||
export ACCOUNT_ID=$(aws sts get-caller-identity --output text --query Account) | ||
export AWS_REGION=$(curl -s 169.254.169.254/latest/dynamic/instance-identity/document | jq -r '.region') |
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 is not accurate for mac based deployments. May work from Cloud9. I think you should ask the readers to hardcode ther egion.
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.
done
|
||
const gitProps = { | ||
owner :'aws-samples', | ||
secretName : props.gitHubSecret ?? 'cdk_blueprints_gitops_github_secret', |
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.
Secret you are creating is export CDK_REPO_AWS_SECRET_NAME="cdk_blueprints_github_secret" but the code has cdk_blueprints_gitops_github_secret
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.
done
@ajpaws Cleanup Instructions are missing |
The example command looks like below. | ||
|
||
```shell | ||
aws eks update-kubeconfig --name mgmt-cluster --region us-west-2 --role-arn arn:aws:iam::ACCOUNT_ID:role/mgmt-cluster-stage-mgmt-c-managementclusterAccessRo-XYSC5PKL8WnA |
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 is not correct
The example command looks like below. | ||
|
||
```shell | ||
aws eks update-kubeconfig --name workload-amd-1-29-blueprint --region us-west-2 --role-arn arn:aws:iam::ACCOUNT_ID:role/eks-workload-connector-role |
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 is in correct too. You should basically have a command to query outputs from stack and grab or from console.
The example command looks like below. | ||
|
||
```shell | ||
aws eks update-kubeconfig --name workload-arm-1-29-blueprint --region us-west-2 --role-arn arn:aws:iam::$ACCOUNT_ID:role/eks-workload-connector-role |
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 is in correct too. You should basically have a command to query outputs from stack and grab or from console.
The output will look like below. | ||
|
||
```shell | ||
Added new context arn:aws:eks:us-west-2:ACCOUNT_ID:cluster/workload-amd-1-29-blueprint to /Users/jalawala/.kube/config |
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.
Output can be removed.
The output will look like below. | ||
|
||
```shell | ||
Added new context arn:aws:eks:us-west-2:ACCOUNT_ID:cluster/workload-arm-1-29-blueprint to /Users/jalawala/.kube/config |
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.
Output can be removed.
The output will look like below. | ||
|
||
```shell | ||
Updated context arn:aws:eks:us-west-2:ACCOUNT_ID:cluster/mgmt-cluster in /Users/<user_name>/.kube/config |
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.
Output can be removed.
|
||
## Test | ||
|
||
### Install the ArgoCD ALI |
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.
ALI?
Get the ArgoCD Admin password using below command. | ||
|
||
```shell | ||
kubectl --context $MANAGEMENT_CLUSTER_CONTEXT -n argocd get secret argocd-initial-admin-secret -o jsonpath="{.data.password}" | base64 -d; echo |
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 dont see this secret, so i cant get to Argo Server. See what is missing
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.
❯ kubectl -n argocd get secret argocd-initial-admin-secret -o jsonpath="{.data.password}" | base64 -d; echo
─╯
Error from server (NotFound): secrets "argocd-initial-admin-secret" not found
Add EKS Cluster to ArgoCD. | ||
|
||
```shell | ||
argocd cluster add $MANAGEMENT_CLUSTER_CONTEXT |
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.
Fails
─╯
WARNING: This will create a service account `argocd-manager` on the cluster referenced by context `arn:aws:eks:us-west-2:940019131157:cluster/mgmt-cluster` with full cluster level privileges. Do you want to continue [y/N]? y
INFO[0003] ServiceAccount "argocd-manager" created in namespace "kube-system"
INFO[0003] ClusterRole "argocd-manager-role" created
INFO[0003] ClusterRoleBinding "argocd-manager-role-binding" created
INFO[0009] Created bearer token secret for ServiceAccount "argocd-manager"
FATA[0009] Argo CD server address unspecified```
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.
When i describe the pod. i see secret failures. Though i have github-token
in same region.
Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal Scheduled 43m default-scheduler Successfully assigned argocd/blueprints-addon-argocd-server-666866878b-92qgh to ip-10-0-189-12.us-west-2.compute.internal
Warning FailedMount 6s (x29 over 43m) kubelet MountVolume.SetUp failed for volume "blueprints-secret-inline" : rpc error: code = Unknown desc = failed to mount secrets store objects for pod argocd/blueprints-addon-argocd-server-666866878b-92qgh, err: rpc error: code = Unknown desc = Failed to fetch secret from all regions: github-token
Run the below command to get the list of EKS add-on Objects deployed in the Management Cluster. | ||
|
||
```shell | ||
kubectl --context $MANAGEMENT_CLUSTER_CONTEXT get add-ons.eks.aws.upbound.io |
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 command should be kubectl --context $MANAGEMENT_CLUSTER_CONTEXT get addons.eks.aws.upbound.io
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.