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

Add ip alb ingress target type for EKS values #3343

Conversation

fvde
Copy link
Contributor

@fvde fvde commented Feb 14, 2023

Tracking issue

This change does not close an open Issue per se, but to I had to make this change in my config to get the ALB creation to work at all. See this Slack Thread for additional background:

https://flyte-org.slack.com/archives/C01P3B761A6/p1671215817793359

Describe your changes

Route traffic for EKS deployment directly from ALB to pods. For details: https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.2/guide/ingress/annotations/#target-type

alb.ingress.kubernetes.io/target-type: ip

Check all the applicable boxes

  • I updated the documentation accordingly. Not sure if needed, seems to be referenced directly via eks-values.yaml mostly.
  • All new and existing tests passed.
  • All commits are signed-off.

Screenshots

Note to reviewers

@welcome
Copy link

welcome bot commented Feb 14, 2023

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Signed-off-by: Ferdinand von den Eichen <[email protected]>
@fvde fvde force-pushed the add-alb-ingress-target-ip-for-eks-values branch from 9143fe5 to e5acf64 Compare February 14, 2023 19:11
@eapolinario
Copy link
Contributor

cc: @jeevb

@jeevb
Copy link
Contributor

jeevb commented Feb 14, 2023

Unfortunately, we are unable to test this change right now. But we can test/approve once the Flyte development cluster is reconfigured to use ALB for ingress, by the end of the week.

Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

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

Looks good to me

@davidmirror-ops
Copy link
Contributor

@jeevb Are we ok to merge this?

@jeevb
Copy link
Contributor

jeevb commented Feb 21, 2023

Yes. This should be good! Thanks @fvde!

jeevb
jeevb previously approved these changes Feb 21, 2023
@fvde
Copy link
Contributor Author

fvde commented Mar 1, 2023

The failed test seems legit in the sense that they seem to reference the eks area of the test suite. I'm not too proficient with Go, so I might need some help here...

@jeevb
Copy link
Contributor

jeevb commented Mar 1, 2023

You will need to run the follow make target and commit the changes:

flyte/Makefile

Line 37 in 7f66e47

helm: ## Generate K8s Manifest from Helm Charts.

Signed-off-by: Ferdinand von den Eichen <[email protected]>
@fvde fvde dismissed stale reviews from jeevb and davidmirror-ops via 7d28a8a March 6, 2023 14:13
@fvde
Copy link
Contributor Author

fvde commented Mar 6, 2023

Done, could you retest?

@wild-endeavor wild-endeavor merged commit b7afefa into flyteorg:master Mar 6, 2023
@welcome
Copy link

welcome bot commented Mar 6, 2023

Congrats on merging your first pull request! 🎉

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.

5 participants