Skip to content

WIP refactor: switch to universal addon #29

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 17 commits into
base: main
Choose a base branch
from
Open

Conversation

matejhasul
Copy link

@matejhasul matejhasul commented Apr 14, 2025

Description

Replace template addon with universal addon

Type of change

  • A bug fix (PR prefix fix)
  • A new feature (PR prefix feat)
  • A code change that neither fixes a bug nor adds a feature (PR prefix refactor)
  • Adding missing tests or correcting existing tests (PR prefix test)
  • Changes that do not affect the meaning of the code like white-spaces, formatting, missing semi-colons, etc. (PR prefix style)
  • Changes to our CI configuration files and scripts (PR prefix ci)
  • Documentation only changes (PR prefix docs)

How Has This Been Tested?

TBD

Tasks:

  • Impove examples
  • Add irsa and pod identity to examples
  • Doc updates
  • README update
  • Support pod identity
  • Testing
  • Separate crds installation? - not possible
  • Modify variables.tf? - not needed
  • Modify .templatesyncignore
  • Modify main.tf
  • add support for pod identity to universal addon - skipped in the end
  • decide how to load iam policy (json file, aws_iam_document, ...)

@matejhasul matejhasul self-assigned this Apr 14, 2025
@matejhasul matejhasul requested a review from jaygridley April 25, 2025 07:43
main.tf Outdated
*/
locals {
addon = {
# TODO: Is the name correct?
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this should be aws-load-balancer-controller according to

default = "aws-load-balancer-controller"

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

main.tf Outdated
addon_irsa = {
(local.addon.name) = {
irsa_policy_enabled = var.irsa_policy_enabled != null ? var.irsa_policy_enabled : true
irsa_policy = var.irsa_policy != null ? var.irsa_policy : file("${path.module}/default_irsa_policy.json")
Copy link
Member

Choose a reason for hiding this comment

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

Regarding using plain JSON file for the policy, I will come to this later. Need to evaluate pros and cons.

Copy link
Author

Choose a reason for hiding this comment

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

As agreed, I'm using policy from aws_iam_policy_document datasource. Defined in default_policy.tf file. Is it ok to use it that way? Or should I put it somewhere else?

Copy link
Member

@jaygridley jaygridley left a comment

Choose a reason for hiding this comment

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

As we discussed we would like to retain EKS Pod Identity support.

@matejhasul matejhasul requested a review from jaygridley April 28, 2025 11:58
Copy link
Member

Choose a reason for hiding this comment

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

I would name the file iam.tf as per our convention.

Copy link
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,290 @@
data "aws_iam_policy_document" "default_policy" {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data "aws_iam_policy_document" "default_policy" {
data "aws_iam_policy_document" "this" { # or `controller`

We tend to name TF resources that are used only once using this. If there are multiple instances then name it accordingly. In this case this should work.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we should add a condition using count = var.enabled && var.irsa_policy == null to prevent unnecessary creation of this when it wont be used after all.

Copy link
Author

Choose a reason for hiding this comment

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

Using this with condition as suggested.

@sassdavid
Copy link
Contributor

sassdavid commented Apr 29, 2025

Hi,

sorry for jumping in here — I just noticed that you’ve recently started using asdf.

Have you heard about mise?
It’s more than just a tool manager; it’s a complete solution for managing your development environment smoothly.

While it primarily helps you manage different versions of various tools — like asdf — it also allows you to manage environment variables and run tasks, making it much more than just a tool installer.
Additionally, mise addresses some of the supply chain risks present in asdf. With asdf, you're often relying on third-party plugins that may execute untrusted code on your system. mise improves on this by using trusted "backends" like the aqua registry and ubi to install tools. When it does need to fall back to the traditional asdf plugin system, it uses forks of those plugins maintained by the mise team, aiming to ensure they are handled more strictly and securely.
It also supports using precompiled binaries for most tools — for example, if your system supports it, you can install a precompiled Python version, saving time and avoiding unnecessary builds.

Please take a look and consider using mise instead of asdf.
You can find more details here and here.

You can also find a more detailed comparison here.

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