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

Feat support external traffic policy #1544

Merged
merged 1 commit into from
Feb 13, 2023
Merged

Feat support external traffic policy #1544

merged 1 commit into from
Feb 13, 2023

Conversation

AhmedGrati
Copy link
Contributor

@AhmedGrati AhmedGrati commented Dec 1, 2022 •
edited
Loading

Description

This pull request solves #1543.
Since the externalTrafficPolicy is only specified when we have a Service of type LoadBalancer or NodePort, we will output a warning message when the user specifies another type of Service.
This feature is implemented for both: Kubernetes and Openshift.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 1, 2022
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 1, 2022
@AhmedGrati AhmedGrati added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2022
@AhmedGrati AhmedGrati changed the title Feat support external traffic policy [WIP] Feat support external traffic policy Dec 1, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2022
@ZakariaNaaija
Copy link

Hi @AhmedGrati,
Thanks for handling the issue I have mentioned.

Since the externalTrafficPolicy is only specified when we have a Service of type LoadBalancer, we will output a warning message when the user specifies another type of Service.

externalTrafficPolicy can be used along with a service of type NodePort.

Local preserves the client source IP and avoids a second hop for LoadBalancer and NodePort type Services, but risks potentially imbalanced traffic spreading.

Source

@AhmedGrati
Copy link
Contributor Author

Hi @AhmedGrati, Thanks for handling the issue I have mentioned.

Since the externalTrafficPolicy is only specified when we have a Service of type LoadBalancer, we will output a warning message when the user specifies another type of Service.

externalTrafficPolicy can be used along with a service of type NodePort.

Local preserves the client source IP and avoids a second hop for LoadBalancer and NodePort type Services, but risks potentially imbalanced traffic spreading.

Source

Thank you for raising it, much appreciated. I will update it.

@AhmedGrati AhmedGrati added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Dec 2, 2022
@AhmedGrati AhmedGrati changed the title Feat support external traffic policy Feat support external traffic policy [WIP] Dec 8, 2022
@AhmedGrati AhmedGrati added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2022
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 9, 2022
@AhmedGrati AhmedGrati removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2022
@AhmedGrati AhmedGrati changed the title Feat support external traffic policy [WIP] Feat support external traffic policy Dec 9, 2022
@kubernetes kubernetes deleted a comment from mrsangjunboon2018 Jan 11, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 21, 2023
@cdrage
Copy link
Member

cdrage commented Feb 8, 2023

This will need a huge rebase, sorry for the work @AhmedGrati !

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2023
@AhmedGrati AhmedGrati added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 8, 2023
@AhmedGrati AhmedGrati force-pushed the feat-support-external-traffic-policy branch from 54098f1 to d7aadea Compare February 9, 2023 21:53
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2023
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 9, 2023
@AhmedGrati AhmedGrati force-pushed the feat-support-external-traffic-policy branch 2 times, most recently from 15fadbf to 3229891 Compare February 9, 2023 22:24
@AhmedGrati AhmedGrati removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2023
@AhmedGrati
Copy link
Contributor Author

@cdrage It's ready to be reviewed.

cdrage
cdrage reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

The code LGTM but can you add some documentation as well for the user guide / under labels as this is a new label?

cdrage
cdrage reviewed Feb 10, 2023
View reviewed changes
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Just sayin', these tests are GREAT and thank you so much for always covering it all 💯 💯 💯

@AhmedGrati AhmedGrati force-pushed the feat-support-external-traffic-policy branch from 3229891 to a10beaf Compare February 11, 2023 11:26
@AhmedGrati
Copy link
Contributor Author

The code LGTM but can you add some documentation as well for the user guide / under labels as this is a new label?

Yes makes sense 👌. Done!

@cdrage
Copy link
Member

cdrage commented Feb 12, 2023

Thank you! Can you show all the options? I think it's cluster, local and blank right?

@AhmedGrati AhmedGrati force-pushed the feat-support-external-traffic-policy branch from a10beaf to 6366876 Compare February 13, 2023 10:44
@AhmedGrati
Copy link
Contributor Author

@cdrage Good point. Done 👍. Please lmk if there are other things that need to be changed.

@AhmedGrati AhmedGrati requested a review from cdrage February 13, 2023 10:45
cdrage
cdrage requested changes Feb 13, 2023
View reviewed changes
docs/user-guide.md Outdated
@@ -200,6 +200,7 @@ The currently supported options are:
| kompose.service.healthcheck.liveness.http_get_path | kubernetes liveness httpGet path |
| kompose.service.healthcheck.liveness.http_get_port | kubernetes liveness httpGet port |
| kompose.service.healthcheck.liveness.tcp_port | kubernetes liveness tcpSocket port |
| kompose.service.external-traffic-policy: local | kubernetes service external traffic 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
| kompose.service.external-traffic-policy: local | kubernetes service external traffic policy |
| kompose.service.external-traffic-policy | 'cluster', 'local', '' |

Let's just do this for now until we update the documentation on user guide. It's a bit of a mess right now to understand / comprehend and I plan to work on this, this month.

@AhmedGrati AhmedGrati force-pushed the feat-support-external-traffic-policy branch from 6366876 to 6be6fdd Compare February 13, 2023 13:59
@AhmedGrati AhmedGrati requested a review from cdrage February 13, 2023 14:00
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AhmedGrati, cdrage

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cdrage
Copy link
Member

cdrage commented Feb 13, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2023
@k8s-ci-robot k8s-ci-robot merged commit 607196c into kubernetes:main Feb 13, 2023
@AhmedGrati AhmedGrati mentioned this pull request Feb 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdrage cdrage cdrage approved these changes

@sebgoa sebgoa Awaiting requested review from sebgoa

@surajssd surajssd Awaiting requested review from surajssd

Assignees

@cdrage cdrage

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AhmedGrati @ZakariaNaaija @cdrage @k8s-ci-robot @TessaIO