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

ingress: Propagate labels from Ingress to LB Service #28598

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

log1cb0mb
Copy link
Contributor

@log1cb0mb log1cb0mb commented Oct 14, 2023

This PR adds the labels from Ingress to Load Balancer Service.

Fixes: #28576

Propagate prefixed labels from Ingress resource to LB service

@log1cb0mb log1cb0mb requested a review from a team as a code owner October 14, 2023 21: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 Oct 14, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 14, 2023
Copy link
Contributor

@youngnick youngnick 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 your contribution @log1cb0mb.

I have complicated feelings about this one. In general, I'm not in favor of copying labels or annotations around - I think it's a pretty dirty hack to fit config that should really be stored in a structured field somewhere into labels. But, because we don't have any way to set the settings we want to on the generated Service object, we're kinda stuck with it.

Additionally, we've tried to make this a little more specific with annotations - do you think that we could use a similar mechanism for labels and only copy labels that start with certain strings? I haven't looked at the labels in use for configuring things on Service for some time.

If we agree we want to blindly copy everything, though, the code change itself is fine, but I'd like to see a couple of extra things please?

Firstly, can you update the docs page in Documentation/network/servicemesh/ingress.rst with some information about how all labels will now be copied in addition to annotations?

Secondly, if you could have a look at the tests and see if you can make a test that checks that everything works as expected on the generated Service, that would be awesome. That's a non-blocking request though, so if you would like to merge this first, one of us can either work with you or pick it up to add it later.

So, tl;dr, three things:

  • Can you talk a little about copying everything? Would it be okay to use the same logic as the annotations and only copy some labels?
  • Please add some docs in the page I mentioned.
  • Extra task if you can, but non-blocking, see if you can add some tests?

Thanks again for the contribution, @log1cb0mb, really great to see and well done!

operator/pkg/ingress/ingress.go Show resolved Hide resolved
@log1cb0mb log1cb0mb requested a review from a team as a code owner October 19, 2023 09:41
@log1cb0mb log1cb0mb changed the title ingress: Propagate labels from Ingress to LB Service Draft: ingress: Propagate labels from Ingress to LB Service Oct 19, 2023
@maintainer-s-little-helper
Copy link

Commit 0e865b5 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 Oct 19, 2023
@log1cb0mb log1cb0mb requested review from a team as code owners October 19, 2023 19:48
@log1cb0mb log1cb0mb requested a review from squeed October 19, 2023 19:48
@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 Oct 19, 2023
@log1cb0mb log1cb0mb force-pushed the ingressLBLabels branch 2 times, most recently from af15e1f to acd0696 Compare October 19, 2023 20:00
@log1cb0mb log1cb0mb changed the title Draft: ingress: Propagate labels from Ingress to LB Service ingress: Propagate labels from Ingress to LB Service Oct 19, 2023
@log1cb0mb log1cb0mb force-pushed the ingressLBLabels branch 3 times, most recently from 1de4c58 to 4f53eec Compare October 19, 2023 21:21
@maintainer-s-little-helper
Copy link

Commit 11c08bd 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 Oct 19, 2023
@maintainer-s-little-helper
Copy link

Commit 11c08bd 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 removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 19, 2023
@aanm
Copy link
Member

aanm commented Nov 1, 2023

/test

@log1cb0mb log1cb0mb force-pushed the ingressLBLabels branch 2 times, most recently from 5eefda8 to 5ee6c55 Compare November 1, 2023 21:52
@meyskens
Copy link
Member

meyskens commented Nov 2, 2023

/test

Copy link
Member

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

changes to ingress re LGTM

@aanm
Copy link
Member

aanm commented Nov 3, 2023

/test

@joamaki
Copy link
Contributor

joamaki commented Nov 6, 2023

/test

@log1cb0mb
Copy link
Contributor Author

Is it possible to add backport-1.14 label to this?

@joamaki joamaki added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Nov 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.4 Nov 6, 2023
@log1cb0mb log1cb0mb force-pushed the ingressLBLabels branch 5 times, most recently from a3a1ebf to 6b045d7 Compare November 8, 2023 13:43
Signed-off-by: NabeelR <nabeelnrana@gmail.com>
@log1cb0mb
Copy link
Contributor Author

/test

@giorio94 giorio94 closed this Nov 9, 2023
@giorio94 giorio94 reopened this Nov 9, 2023
@giorio94
Copy link
Member

giorio94 commented Nov 9, 2023

Closed and reopened to see if travis gets triggered, as it was stuck

@giorio94
Copy link
Member

giorio94 commented Nov 9, 2023

Reviews are in, and checks are green. Marking as ready to merge.

@giorio94 giorio94 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 9, 2023
@tklauser tklauser merged commit 81613c6 into cilium:main Nov 9, 2023
101 checks passed
@aanm aanm removed the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Nov 9, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from main in 1.14.4 Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

CFP: Propagate labels from dedicated Ingress to LB service
9 participants