Updated deprecated service annotation

[tokarev-artem]

Updated deprecated service annotation

https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.6/guide/service/annotations/#:~:text=string-,service.beta.kubernetes.io/aws%2Dload%2Dbalancer%2Dinternal,-boolean

[asauber]

Looking good.

Could you please squash your commits into a single commit, and include a more descriptive summary and description in the commit message?

A good example of our commit message standards: https://github.com/cilium/cilium-cli/pull/1948/commits

[tokarev-artem]

Looking good.

Could you please squash your commits into a single commit, and include a more descriptive summary and description in the commit message?

A good example of our commit message standards: https://github.com/cilium/cilium-cli/pull/1948/commits

Updated, is it acceptable?
Thanks

[asauber]

Thanks for the update. Not quite there with the commit message. Your commit message is:

Updated deprecated annotation which is deprecated
https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.6/guide/service/annotations/#:~:text=string-,service.beta.kubernetes.io/aws%2Dload%2Dbalancer%2Dinternal,-boolean
    
Signed-off-by: [email protected]

One issue is that the title of the commit message is redundant and doesn't describe the change. A better title would be clustermesh: update deprecated AWS annotation. This indicates the area of the codebase being changed, uses the active tense, starts with a verb, and gives an indication of which annotation we are talking about.

A few issues with the body of the commit message as well. If you provide a link to an external resource, you should consider it supplemental material that could disappear at any time. The body of the commit message should clearly describe the change and the reason for the change. There should also be a blank line between the title and the body.

Putting this together your commit message would be along the lines of

clustermesh: update deprecated AWS annotation

[description of the change and why it's necessary]

https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.6/guide/service/annotations/#:~:text=string-,service.beta.kubernetes.io/aws%2Dload%2Dbalancer%2Dinternal
    
Signed-off-by: [email protected]

For more information on commit standards, you can refer to the Cilium "submitting a pull request" documentation.

Some additional guidelines for good commit messages:

• https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
• https://www.git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines
• https://github.com/torvalds/subsurface-for-dirk/blob/master/README.md#contributing

[asauber]

Please see the comment above regarding the commit message.

[tokarev-artem]

Please see the comment above regarding the commit message.

Sorry, GitHub doesn't send any notifications about PRs activity. Updated the commit message. Thanks

[asauber]

Thanks for the update

[ti-mo]

I've kicked the kind job, it timed out reaching cilium.io.

This needs a review from @cilium/sig-clustermesh and it's good to go.

[giorio94]

Thanks @RealArtemiy for the fix and @ti-mo for the ping.

I've left one comment inline about the opportunity of updating the EKS LB configuration to be consistent with that of the other cloud providers.

[giorio94]

Looking at the current annotation, I'm actually puzzled about the intended behavior, given that service.beta.kubernetes.io/aws-load-balancer-internal is expected to take a boolean parameter, while we are setting 0.0.0.0/0. Yet, considering the log message above and the configurations for the other cloud providers, I assume that the goal would be to expose it internally only. For that reason, I'd suggest setting the load balancer scheme to internal.

[tokarev-artem]

@giorio94 but as I understand - we need this LB to get a couple of clusters connected to each other, how can it be reached if LB internal? Am I wrong?

[giorio94]

The assumption is that the two clusters are either on the same VPC, or the VPCs have been peered together, to enable cross-cluster node to node and pod to pod communication.

[ti-mo]

Ping, is this still being worked on? Converting to draft for now.