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.

Sign up for GitHub

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

Jump to bottom

helm: Update the example value for EKS load balancer internal annotation #31329

Merged
merged 1 commit into from Mar 13, 2024
Merged

helm: Update the example value for EKS load balancer internal annotation #31329

merged 1 commit into from Mar 13, 2024

Conversation

oshangalwaduge
Copy link
Contributor

Fix Cilium default values for EKS when Cilium clustermesh-apiserver LoadBalancer fails to create with the below error.

Annotation "service.beta.kubernetes.io/aws-load-balancer-internal: 0.0.0.0/0" is not supported and returns an error 'Failed build model due to failed to parse bool annotation, service.beta.kubernetes.io/aws-load-balancer-internal: 0.0.0.0/0: strconv.ParseBool: parsing "0.0.0.0/0": invalid syntax'.

Annotation value for service.beta.kubernetes.io/aws-load-balancer-internal should be described as in the AWS Load Balancer Controller documentation. https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.5/guide/service/annotations/#lb-internal

Fix Cilium default values for EKS when Cilium clustermesh-apiserver LoadBalancer fails to create NLB with AWS Load Balancer Controller with syntax error.

@oshangalwaduge oshangalwaduge requested review from a team as code owners March 12, 2024 01:21
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 12, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 12, 2024
sayboras
sayboras reviewed Mar 12, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for your PR, the Readme.md is actually generated from values.yaml.tmpl, so can you make the change here, and run below commands to generate all related files.

cilium/install/kubernetes/cilium/values.yaml.tmpl

Line 2675 in 5c2ed45

# For EKS LoadBalancer, use annotation service.beta.kubernetes.io/aws-load-balancer-internal: 0.0.0.0/0 

make -C install/kubernetes
make -C Documentation update-helm-values

@sayboras sayboras changed the title Fix Cilium readme default values for EKS - annotation service.beta.kubernetes.io/aws-load-balancer-internal helm: Update the example value for EKS load balancer internal annotation Mar 12, 2024
@sayboras sayboras added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/helm Impacts helm charts and user deployment experience labels Mar 12, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 12, 2024
@maintainer-s-little-helper
Copy link

Commit 213e124 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 12, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 12, 2024
@oshangalwaduge
Copy link
Contributor Author

Hi @sayboras

Thanks for the comment! I have updated the PR as you have instructed. Can you please have a look?

Thanks

@sayboras
Copy link
Member

The changes look good to me. Can you help to squash two commits into a single one? Thanks.

sayboras
sayboras approved these changes Mar 12, 2024
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Apart from merging two commits into a single one, the changes look good to me. Thanks a lot for your contribution.

@oshangalwaduge
Copy link
Contributor Author

oshangalwaduge commented Mar 12, 2024 •
edited

Apart from merging two commits into a single one, the changes look good to me. Thanks a lot for your contribution.

Thanks @sayboras! Just squashed the two commits.

Happy to contribute more! :)

@gandro
Copy link
Member

gandro commented Mar 12, 2024

/test

@gandro gandro removed the request for review from youngnick March 12, 2024 11:40
@gandro
Copy link
Member

gandro commented Mar 13, 2024

/test

@gandro
Copy link
Member

gandro commented Mar 13, 2024

@oshangalwaduge I've started a full CI run. Unless you want/need to do any changes, please avoid rebasing the branch as that invalidates the test results (and thus blocking the PR from being merged). Thanks!

@oshangalwaduge
helm: Update the example value for EKS load balancer internal annotation
8fd0c4d 
Signed-off-by: Oshan Galwaduge <oshan304@gmail.com>
@oshangalwaduge
Copy link
Contributor Author

@oshangalwaduge I've started a full CI run. Unless you want/need to do any changes, please avoid rebasing the branch as that invalidates the test results (and thus blocking the PR from being merged). Thanks!

Hi @gandro

My apologies... Just noticed this. Will avoid rebasing the branch from now on. Sorry for the inconveniences caused 🙏

@gandro
Copy link
Member

gandro commented Mar 13, 2024

No worries!

@gandro
Copy link
Member

gandro commented Mar 13, 2024

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 13, 2024
@gandro gandro added this pull request to the merge queue Mar 13, 2024
Merged via the queue into cilium:main with commit 9fbe73c Mar 13, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@oshangalwaduge @sayboras @gandro