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

Crossplane Addon - Part1 #226

Merged
merged 10 commits into from
Feb 5, 2022
Merged

Crossplane Addon - Part1 #226

merged 10 commits into from
Feb 5, 2022

Conversation

vara-bonthu
Copy link
Contributor

@vara-bonthu vara-bonthu commented Feb 3, 2022

What does this PR do?

  • Deploys Crossplane Add-on to EKS Cluster
  • Docs added
  • Example added

Please note that this is a part1 of Crossplane PR. The next PR will include the configuration related to AWS Provider and some sample deployments

Motivation

More

  • Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I have added a new example under examples to support my PR
  • Yes, I have updated the docs for this feature
  • Yes, I ran pre-commit run -a with this PR

Note: Not all the PRs required examples and docs except a new pattern or add-on added.

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

@vara-bonthu vara-bonthu added documentation Improvements or additions to documentation enhancement New feature or request examples labels Feb 3, 2022
Copy link
Contributor

@kcoleman731 kcoleman731 left a comment

Choose a reason for hiding this comment

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

Awesome work.

crossplane_irsa_policies = [] # Optional to add additional policies to Crossplane IRSA
```

Here is a full example to consume `kubernetes-addons` module and deploy Crossplane
Copy link
Contributor

Choose a reason for hiding this comment

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

To be concise, might be easiest to just link to the example here?

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

@@ -0,0 +1,39 @@
locals {
name = "crossplane-system"
Copy link
Contributor

Choose a reason for hiding this comment

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

should we call this var namespace? Since it is used for namespace values? Maybe need a name and a namespace var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Changed it now

@@ -0,0 +1,39 @@
locals {
name = "crossplane-system"
service_account_name = "crossplane"
Copy link
Contributor

Choose a reason for hiding this comment

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

service account name is the same as values on line 5 and 6. Prob can reword vars. Also convention in the codebase is to use the -sa postfix for namespaces yea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crossplane doesn't provide option to create our own SA. I just removed the reference. SA will be created by Helm chart.

@@ -0,0 +1,6 @@
module "helm_addon" {
Copy link
Contributor

Choose a reason for hiding this comment

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

amazing

@@ -0,0 +1,6 @@
module "helm_addon" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to add the "short license" file header to these files. @jomcy-amzn ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No I don't think we need them. Repos like https://github.com/aws-ia/terraform-aws-control_tower_account_factory have not included license files in the source files. Root level is good enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok great!

@@ -1,6 +1,6 @@
locals {
namespace = "karpenter"
service_account_name = "karpenter-sa"
Copy link
Contributor

Choose a reason for hiding this comment

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

why changing?

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 know we did discuss this but it may be worth revisiting again. we don't need to tell namespace as -ns and service account as -sa. We are tagging the resources with SSP so we should be able to identify the resources created by SSP.

Also, we may avoid creating Service Accounts where possible and let the Helm chart handle that to avoid any possible issues. This also helps for ArgoCD sync.


enable_crossplane = true

crossplane_helm_config = {
Copy link
Contributor

Choose a reason for hiding this comment

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

these are default values right? Why include in the example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the example to enable_crossplane = true and commented the optional one.

I think this is one common question that many asked on how to pass their own values.yaml using our repo for the add-ons. Showing this in the example itself will be useful for users. They don't need to go through all the docs.

@@ -0,0 +1,5 @@
nodeSelector:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as default yaml right? Why include in example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above response

Copy link
Contributor

@askulkarni2 askulkarni2 left a comment

Choose a reason for hiding this comment

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

Looking good, need a PR in the addons repo as well.

# Example to consume aws-eks-accelerator-for-terraform module
#---------------------------------------------------------------
module "aws-eks-accelerator-for-terraform" {
source = "github.com/aws-samples/aws-eks-accelerator-for-terraform"
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been doing relative paths for modules, might want to keep doing that for consistency.

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

@@ -1,7 +1,7 @@
module "helm_addon" {
source = "../helm_addon"
source = "../helm-addon"
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌🏽

@@ -0,0 +1,6 @@
module "helm_addon" {
Copy link
Contributor

Choose a reason for hiding this comment

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

No I don't think we need them. Repos like https://github.com/aws-ia/terraform-aws-control_tower_account_factory have not included license files in the source files. Root level is good enough.

count = var.enable_argo_rollouts ? 1 : 0
source = "./argo-rollouts"
eks_cluster_id = var.eks_cluster_id
helm_config = var.argo_rollouts_helm_config
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please pass tags here? I forgot to add them in my PR.

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

Copy link
Contributor

@kcoleman731 kcoleman731 left a comment

Choose a reason for hiding this comment

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

Awesome work.

@kcoleman731 kcoleman731 merged commit eb51417 into main Feb 5, 2022
@vara-bonthu vara-bonthu deleted the crossplane branch February 5, 2022 11:20
alidonmez pushed a commit to alidonmez/terraform-aws-eks-blueprints-1 that referenced this pull request Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants