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

feat(iam): update iam policy to be compatible version 2.12.0 of lb-controller #26

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

Conversation

sassdavid
Copy link
Contributor

Hello,
me again :)

Another policy change is needed to use the v2.12.0 version of the load balancer controller.

Description

  • update IAM policy document according to official docs
  • this is not a breaking change just an option to upgrade to v2.12.0

Here is the release notes: https://github.com/kubernetes-sigs/aws-load-balancer-controller/releases/tag/v2.12.0

In v2.12.0, they have changed the default policy of the LBC webhook from Fail to Ignore in order to improve disaster recovery. See their documentation for how to change the policy back to Fail if you want better guarantees for having readiness gates getting attached to your pods.

They’ve added new fields to both the IngressClassParams and
TargetGroupBinding. Please apply the latest CRD definitions: kubectl apply -k "github.com/aws/eks-charts/stable/aws-load-balancer-controller/crds?ref=master"

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?

deployment of the controller + deployment of testing app

@sassdavid
Copy link
Contributor Author

Hello @jaygridley I kindly ask you to help me move forward with this.

@sassdavid
Copy link
Contributor Author

Other: do you think it would make sense to introduce an option where this policy can be defined by the user and the ARN can be passed to the module as a variable?

@jaygridley jaygridley self-assigned this Apr 10, 2025
@jaygridley jaygridley added the enhancement New feature or request label Apr 10, 2025
@jaygridley jaygridley self-requested a review April 10, 2025 06:49
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.

Hello @sassdavid, thanks for you contribution, much appreciated. I think we should keep the webhook policy to Fail to keep the behaviour the same. Some might rely on this, for example, we do. Please, share your thoughts on this.

@jaygridley
Copy link
Member

Other: do you think it would make sense to introduce an option where this policy can be defined by the user and the ARN can be passed to the module as a variable?

At this point I might reveal to you, that we are internally working on a rector of this addon that will bring this capability. So please, stay tuned.

@sassdavid
Copy link
Contributor Author

Hello @sassdavid, thanks for you contribution, much appreciated. I think we should keep the webhook policy to Fail to keep the behaviour the same. Some might rely on this, for example, we do. Please, share your thoughts on this.

Hi, yeah, that totally makes sense.

I think we can keep the current behavior — for example, by creating a new variable with a default value of "Fail" and passing it to the Helm chart (here).

Users who want to use the new approach will still have the option to opt in.

Would that work for you? If so, I’ll go ahead and make the modification.

@jaygridley
Copy link
Member

Hello @sassdavid, we can just add a default configuration into

and if users want opt-in and change that it can be done using var.values which will be merged with the default set to Fail effectively overriding it.

@sassdavid
Copy link
Contributor Author

sassdavid commented Apr 11, 2025

Okay, it's done @jaygridley.

Should we update the default chart version to 2.12.0, since this value is only available in versions above 2.12.0?
Or is it fine as is?

@jaygridley
Copy link
Member

@sassdavid I would bump the default chart version as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants