-
Notifications
You must be signed in to change notification settings - Fork 3.4k
ingress: Propagate labels from Ingress to LB Service #28598
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
Conversation
There was a problem hiding this 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!
ed65a3f
to
c45cf6e
Compare
c45cf6e
to
1444a8a
Compare
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 |
0e865b5
to
8ce0a19
Compare
af15e1f
to
acd0696
Compare
1de4c58
to
4f53eec
Compare
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 |
11c08bd
to
e0c935a
Compare
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 |
e0c935a
to
e5db8e7
Compare
e5db8e7
to
e705fd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @log1cb0mb !
/test |
5eefda8
to
5ee6c55
Compare
/test |
There was a problem hiding this 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
5ee6c55
to
a4ce4a7
Compare
/test |
a4ce4a7
to
b1531c7
Compare
/test |
b1531c7
to
93139c3
Compare
Is it possible to add backport-1.14 label to this? |
a3a1ebf
to
6b045d7
Compare
Signed-off-by: NabeelR <nabeelnrana@gmail.com>
6b045d7
to
df8a452
Compare
/test |
Closed and reopened to see if travis gets triggered, as it was stuck |
Reviews are in, and checks are green. Marking as ready to merge. |
This PR adds the labels from Ingress to Load Balancer Service.
Fixes: #28576