-
Notifications
You must be signed in to change notification settings - Fork 327
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
feat(kmesh-cp) add kubernetes tags automatically #3439
Conversation
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
4815544
to
8f1a8df
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.
Nice! I have a couple of comments but this change LGTM!
@@ -12,6 +12,8 @@ import ( | |||
util_k8s "github.com/kumahq/kuma/pkg/plugins/runtime/k8s/util" | |||
) | |||
|
|||
const NamespaceTag = "k8s.kuma.io/namespace" |
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.
Can't we add a k8s.kuma.io/port
the point was to be able to use the port in virtual-outbound.
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.
And maybe k8s.kuma.io/service
which is the serviceName in k8s. That way you have the info in kuma.io/service
in separate tags.
Usage of k8s.kuma.io/service
would be for example adding policy to services exposed on all pods (.e.g metrics)
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.
@jakubdyszkiewicz said:
I think that k8s.kuma.io/port is a bit generic. It always raises the question of “but what port?“. Should this be k8s.kuma.io/service-port?
I don't really mind either way so if you think service-port is better go for it!
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.
added. I also put k8s.kuma.io/service-name
. Although -name
suffix sounds redundant, I find k8s.kuma.io/service
to be too close to kuma.io/service
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
aeed20c
to
a8cc1ce
Compare
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
(cherry picked from commit 19b4a04) # Conflicts: # pkg/plugins/runtime/k8s/controllers/inbound_converter.go
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Summary
Add
k8s.kuma.io/namespace
tag automatically so we can build policy based on it.Issues resolved
Fix #3367
Documentation
Testing
Backwards compatibility
backport-to-stable
label if the code is backwards compatible. Otherwise, list breaking changes.