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

clustermesh-apiserver: ExternalTrafficPolicy and internalTrafficPolicy can now be changed. #24166

Merged
merged 1 commit into from Mar 7, 2023
Merged

clustermesh-apiserver: ExternalTrafficPolicy and internalTrafficPolicy can now be changed. #24166

merged 1 commit into from Mar 7, 2023
+30 −0

Conversation

kahirokunn
Copy link
Contributor

@kahirokunn kahirokunn commented Mar 4, 2023 •
edited

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

If externalTrafficPolicy is set to Local, the IP address of the connection source can be retained.
This is very important for observer days, so you may want to enable it in many cases.

https://kubernetes.io/docs/tutorials/services/source-ip/#source-ip-for-services-with-type-nodeport

clustermesh-apiserver: ExternalTrafficPolicy and internalTrafficPolicy can now be changed.

@kahirokunn kahirokunn requested review from a team as code owners March 4, 2023 00:31
@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 4, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 4, 2023
@kahirokunn kahirokunn force-pushed the helm-add-values branch 3 times, most recently from 6c950b9 to 59541a1 Compare March 4, 2023 02:53
@kahirokunn kahirokunn requested a review from a team as a code owner March 4, 2023 02:53
@kahirokunn kahirokunn requested a review from qmonnet March 4, 2023 02:53
@kahirokunn
Copy link
Contributor Author

How can I get a CI through?

@gandro
Copy link
Member

gandro commented Mar 6, 2023

How can I get a CI through?

CI can only be triggered by Cilium committers (see https://docs.cilium.io/en/v1.13/community/governance/commit_access/). Just ping one of the assigned reviewers to run the tests for you.

@gandro
Copy link
Member

gandro commented Mar 6, 2023

/test

gandro
gandro requested changes Mar 6, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have left s ome feedback on Kubernetes compatibility.

Also, on a different note, the PR/commit description mentions ci: - we use the term ci typically for continuous integration. I think a better commit title here would be clustermesh-apiserver: ExternalTrafficPolicy and internalTrafficPolicy can now be changed

install/kubernetes/cilium/templates/clustermesh-apiserver/service.yaml Outdated Show resolved Hide resolved
qmonnet
qmonnet requested changes Mar 6, 2023
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

ci: ExternalTrafficPolicy and internalTrafficPolicy can now be changed.

It would be nice to add a pointer to the commit that enabled this change, in the commit description.

Please note that the smoke tests for the Helm charts are failing:

 please run 'make -C install/kubernetes' and submit your changes

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved
@kahirokunn kahirokunn changed the title ci: ExternalTrafficPolicy and internalTrafficPolicy can now be changed. clustermesh-apiserver: ExternalTrafficPolicy and internalTrafficPolicy can now be changed. Mar 6, 2023
@kahirokunn kahirokunn force-pushed the helm-add-values branch 4 times, most recently from 754612d to b323347 Compare March 6, 2023 13:27
@kahirokunn
Copy link
Contributor Author

I am getting an error in CI, but my commit message clearly does not exceed 75 characters.
Is this a bug?

make -C bpf checkpatch

=========================================================
[1/14] Running on b323347a6513d262dce69033275572f97dcd04d7
clustermesh-apiserver: ExternalTrafficPolicy and internalTrafficPolicy can now be changed.
=========================================================

"[PATCH] clustermesh-apiserver: ExternalTrafficPolicy and" has no obvious style problems and is ready for submission.

NOTE: Ignored message types: C99_COMMENT_TOLERANCE COMMIT_LOG_LONG_LINE COMMIT_MESSAGE COMPLEX_MACRO CONSTANT_CONVERSION CONST_STRUCT EMAIL_SUBJECT FILE_PATH_CHANGES FROM_SIGN_OFF_MISMATCH GIT_COMMIT_ID JIFFIES_COMPARISON LEADING_SPACE LONG_LINE_COMMENT MACRO_WITH_FLOW_CONTROL MULTISTATEMENT_MACRO_USE_DO_WHILE NOT_UNIFIED_DIFF PRINTK_WITHOUT_KERN_LEVEL TRAILING_SEMICOLON TRAILING_STATEMENTS VOLATILE
ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 90)

@gandro
Copy link
Member

gandro commented Mar 6, 2023

I am getting an error in CI, but my commit message clearly does not exceed 75 characters.

I think it does, currently it's set to

clustermesh-apiserver: ExternalTrafficPolicy and internalTrafficPolicy can now be changed.

which actually is 90 chars long

@gandro gandro added area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/misc This PR makes changes that have no direct user impact. area/helm Impacts helm charts and user deployment experience and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 6, 2023
@kahirokunn
Copy link
Contributor Author

Thx!

@kahirokunn
Copy link
Contributor Author

How about now?

gandro
gandro approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks!

qmonnet
qmonnet approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks

@gandro
Copy link
Member

gandro commented Mar 6, 2023

/test

@kahirokunn
Copy link
Contributor Author

Thanks too! 🙏

@kahirokunn
clustermesh-apiserver: Add helm values.
eed44f1 
* ExternalTrafficPolicy
* internalTrafficPolicy

Signed-off-by: kahirokunn <okinakahiro@gmail.com>
@kahirokunn
Copy link
Contributor Author

I think the CI test is failing in some areas that have nothing to do with my fix.
Does this happen often?

@gandro
Copy link
Member

gandro commented Mar 7, 2023

I think the CI test is failing in some areas that have nothing to do with my fix. Does this happen often?

Unfortunately, the push to the branch invalidated the results so I cannot check them anymore. But yes, there are a few flaky tests.

Let me re-run CI to check if the test failures are legitimate or not. Please do not push to the branch anymore (unless to fix bugs). Rebasing is usually not needed.

@gandro
Copy link
Member

gandro commented Mar 7, 2023

/test

@kahirokunn
Copy link
Contributor Author

okay

@gandro
Copy link
Member

gandro commented Mar 7, 2023

CI passed - the conformance ingress tests have been refactored and so will not be triggered in this branch. Old old test passed here: https://github.com/cilium/cilium/actions/runs/4350648456/jobs/7601557102

Marking ready to merge. Thanks @kahirokunn for your contribution!

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 7, 2023
@YutaroHayakawa YutaroHayakawa merged commit 0ee8015 into cilium:master Mar 7, 2023
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

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. 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/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kahirokunn @gandro @tommyp1ckles @qmonnet @YutaroHayakawa