Skip to content
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

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

ajpaws
Copy link
Contributor

@ajpaws ajpaws commented May 15, 2024

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.

Copy link
Contributor

@elamaran11 elamaran11 left a 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 :

  1. 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.
  2. 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.
  3. So basically most of the work you have in bin folder should move to lib folder refactored.
  4. See if you have any opportunities to minimize the number of custom addons.
  5. Also fix all GH Warnings and errors.
    Overall great progress, we have some work to do. Lets connect if you have questions.

Copy link
Contributor

@elamaran11 elamaran11 left a 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.

bin/crossplane-argocd-gitops.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@shapirov103 shapirov103 left a 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/common/construct-utils.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,89 @@
import 'source-map-support/register';
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not addressed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ajpaws This comment?

Copy link
Contributor Author

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/management-cluster-builder.ts Outdated Show resolved Hide resolved
lib/crossplane-argocd-gitops/multi-cluster-pipeline.ts Outdated Show resolved Hide resolved
lib/crossplane-argocd-gitops/multi-cluster-pipeline.ts Outdated Show resolved Hide resolved
lib/crossplane-argocd-gitops/multi-cluster-pipeline.ts Outdated Show resolved Hide resolved
docs/patterns/crosplane-argocd-gitops.md Outdated Show resolved Hide resolved
docs/patterns/crosplane-argocd-gitops.md Outdated Show resolved Hide resolved
Copy link
Contributor

@elamaran11 elamaran11 left a 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.

lib/crossplane-argocd-gitops/multi-cluster-pipeline.ts Outdated Show resolved Hide resolved
@elamaran11
Copy link
Contributor

@shapirov103 Could you please check again and let me know if this is good to merge.

Copy link
Contributor

@shapirov103 shapirov103 left a 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 ^^

docs/patterns/crossplane-argocd-gitops.md Outdated Show resolved Hide resolved
docs/patterns/crossplane-argocd-gitops.md Outdated Show resolved Hide resolved
docs/patterns/crossplane-argocd-gitops.md Outdated Show resolved Hide resolved
docs/patterns/crossplane-argocd-gitops.md Outdated Show resolved Hide resolved
@@ -0,0 +1,89 @@
import 'source-map-support/register';
Copy link
Contributor

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.

lib/crossplane-argocd-gitops/multi-cluster-options.ts Outdated Show resolved Hide resolved
@ajpaws
Copy link
Contributor Author

ajpaws commented Aug 30, 2024

@shapirov103 Could you please check again and let me know if this is good to merge.

@ajpaws Thankyou for working on this and quick PR. There are few alignments to the approach we have to make :

  1. 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.
  2. 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.
  3. So basically most of the work you have in bin folder should move to lib folder refactored.
  4. See if you have any opportunities to minimize the number of custom addons.
  5. Also fix all GH Warnings and errors.
    Overall great progress, we have some work to do. Lets connect if you have questions.

done. made all changes

Copy link
Contributor

@shapirov103 shapirov103 left a 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

docs/patterns/crossplane-argocd-gitops.md Outdated Show resolved Hide resolved
```
make build
make list
make pattern aws-addon-clusters deploy
Copy link
Contributor

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?

Copy link
Contributor Author

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';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not addressed?

@elamaran11
Copy link
Contributor

@ajpaws Please pull from main to get past the Markdown error,.

@ajpaws
Copy link
Contributor Author

ajpaws commented Sep 4, 2024

@ajpaws Please pull from main to get past the Markdown error,.

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')
Copy link
Contributor

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.

Copy link
Contributor Author

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',
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@elamaran11
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

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
Copy link
Contributor

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```

Copy link
Contributor

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
Copy link
Contributor

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

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.

3 participants