[occm] feat : add load balancer listener tag using service annotation

[TheGh0xt]

• openstack-cloud-controller-manager (occm)

/kind feat

What this PR does / why we need it: this PR solves the add annotation for custom octavia listener tags problem

this is currently a draft PR that needs review, also i am a bit confuse on where to plug the method 😢

Which issue this PR fixes(if applicable): fixes #2327

[k8s-ci-robot]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

Hi @KingDaemonX. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[TheGh0xt]

@pierreprinetti can you take a look at this ??

[jichenjc]

/ok-to-test

[pierreprinetti]

Also: please don't forget to add tests! At a minimum, a unit test to exercise tag splitting (with various configurations of spaces in it) would probably be a good idea

[TheGh0xt]

thanks for the PR. Is it still WIP? If so, please add documentation and tests and mark this PR as WIP. See also some major findings

do you mean the comments on the issue ??

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign zetaab for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[TheGh0xt]

@pierreprinetti @kayrus please re-review

i am working currently working on the test case, hence the WIP

[TheGh0xt]

This isn't adding tags to load balancer listeners, only to the load balancer itself.

In my opinion we should allow tagging of all the LB resources being created, not only load balancers, so also: listeners, pools, members, health monitors and security groups. Search for "CreateOpts" in loadbalancer.go to find creations of the other resources.

We also need to account for updates - it should be possible to update the set of tags in the Service annotation and CPO should then update all the LB resources to reflect that. A tricky part here is we might want to keep the tags created by the user intact, which triggers a lost update problem for us, but let's deal with that later.

it just got really wide and clear to me now, thank you

[TheGh0xt]

This isn't adding tags to load balancer listeners, only to the load balancer itself.

In my opinion we should allow tagging of all the LB resources being created, not only load balancers, so also: listeners, pools, members, health monitors and security groups. Search for "CreateOpts" in loadbalancer.go to find creations of the other resources.

We also need to account for updates - it should be possible to update the set of tags in the Service annotation and CPO should then update all the LB resources to reflect that. A tricky part here is we might want to keep the tags created by the user intact, which triggers a lost update problem for us, but let's deal with that later.

can i create tags field for other resources,for example the security group and monitor doesn't have a tag

[dulek]

Nice, this looks better!

So according to Octavia API reference, the health monitors include tags [1]. I guess gophercloud doesn't support that yet, I'll look into that.

This is still missing tagging of the LB members here [2] and here [3].

The SGs are tagged in a different way - they're part of Neutron service, not Octavia. Something like this should work here [4]:

		err := neutrontags.Add(lbaas.network, "securitygroups", lbSecGroupID, "foo").ExtractErr()

And we should still think about how do we update resources with tags if annotation changes.

[1] https://docs.openstack.org/api-ref/load-balancer/v2/index.html?expanded=create-listener-detail#create-health-monitor [2] https://github.com/kubernetes/cloud-provider-openstack/blob/4ff1458c9a10dbd50bca90c1c33b2088745ba5fe/pkg/util/openstack/loadbalancer_serial.go#L71 [3] https://github.com/kubernetes/cloud-provider-openstack/blob/23291097decc2258985e4139e0de9d159f76a79d/pkg/openstack/loadbalancer.go#L1284-L1289 [4] https://github.com/kubernetes/cloud-provider-openstack/blob/23291097decc2258985e4139e0de9d159f76a79d/pkg/openstack/loadbalancer.go#L2270

[TheGh0xt]

Nice, this looks better!

So according to Octavia API reference, the health monitors include tags [1]. I guess gophercloud doesn't support that yet, I'll look into that.

This is still missing tagging of the LB members here [2] and here [3].

The SGs are tagged in a different way - they're part of Neutron service, not Octavia. Something like this should work here [4]:

		err := neutrontags.Add(lbaas.network, "securitygroups", lbSecGroupID, "foo").ExtractErr()

And we should still think about how do we update resources with tags if annotation changes.

[1] https://docs.openstack.org/api-ref/load-balancer/v2/index.html?expanded=create-listener-detail#create-health-monitor [2]

https://github.com/kubernetes/cloud-provider-openstack/blob/4ff1458c9a10dbd50bca90c1c33b2088745ba5fe/pkg/util/openstack/loadbalancer_serial.go#L71

[3] https://github.com/kubernetes/cloud-provider-openstack/blob/23291097decc2258985e4139e0de9d159f76a79d/pkg/openstack/loadbalancer.go#L1284-L1289

[4] https://github.com/kubernetes/cloud-provider-openstack/blob/23291097decc2258985e4139e0de9d159f76a79d/pkg/openstack/loadbalancer.go#L2270

thank you for the reference, i really missed those bit :)

i will push the fix in a bit

[jeffyjf]

Hi @KingDaemonX

If you addressed something, you can resolve the related comments.

[dulek]

I'm trying to build this and run a bit of tests, might get back with more comments.

Okay, nevermind, after changing "securitygroups" to "security-groups it just works.

[dulek]

Ah, one more thing, we should add these Neutron tag operations to metrics. Here's how you do it when tagging: https://github.com/kubernetes/cloud-provider-openstack/blob/release-1.27/pkg/openstack/loadbalancer.go#L769-L774

Let's name the context "security_group_tag", "replace" and "floating_ip_tag", "replace".

[TheGh0xt]

Unrelated change, please remove.

yeah i get you i will do that

[TheGh0xt]

i have made all the required changes to the PR @dulek

[TheGh0xt]

Seems like you've used security-group instead of security-groups. I'm fairly sure only the latter works, but please prove me wrong if you've tested it and it works.

Also seems like my remarks from this comment about metrics are not implemented.

i totally miss the remark on metric until now that you pointed it again i will fix them now

[kayrus]

Hm, I also don't like an idea that we need to tag all the resources at once. Neutron is modular and there is a possibility that it doesn't have tags module enabled, while octavia can have this module. If tags in neutron are not enabled, the OCCM will fail the reconciliation with an error. Though, cascade tagging of the octavia resources seems fine with me.

Also security groups and especially FIPs can have their own set of tags, e.g. FIP can be preallocated in advance and a special tag can be assigned on it. Once OCCM starts to manage this FIP, it will remove the tags assigned manually. This should not happen.

[k8s-ci-robot]

@KingDaemonX: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
openstack-cloud-csi-cinder-sanity-test	d726570eae48ab06146b79e5a50c133f8032a151	link	true	/test openstack-cloud-csi-cinder-sanity-test
openstack-cloud-csi-manila-e2e-test	d726570eae48ab06146b79e5a50c133f8032a151	link	true	/test openstack-cloud-csi-manila-e2e-test
openstack-cloud-csi-cinder-e2e-test	d726570eae48ab06146b79e5a50c133f8032a151	link	true	/test openstack-cloud-csi-cinder-e2e-test
openstack-cloud-csi-manila-sanity-test	d726570eae48ab06146b79e5a50c133f8032a151	link	true	/test openstack-cloud-csi-manila-sanity-test
openstack-cloud-keystone-authentication-authorization-test	d726570eae48ab06146b79e5a50c133f8032a151	link	true	/test openstack-cloud-keystone-authentication-authorization-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[dulek]

Hm, I also don't like an idea that we need to tag all the resources at once. Neutron is modular and there is a possibility that it doesn't have tags module enabled, while octavia can have this module. If tags in neutron are not enabled, the OCCM will fail the reconciliation with an error. Though, cascade tagging of the octavia resources seems fine with me.

True, we can do that and make sure that tagging will not happen when Neutron doesn't have the extension. Octavia is not modular, so it's just a matter of API version, but we should check for tags extension in Neutron case too.

@KingDaemonX - we need to add a function checking this and only attempt to tag FIPs and SGs when it exists. This is an example of how to do it: https://github.com/kubernetes/cloud-provider-openstack/blob/fdba36babb2c4b46e759c99cca50ac7eba2ee06f/pkg/openstack/openstack.go#L475-L484

We need to check for standard-attr-tag extension as explained in Neutron API reference.

Also security groups and especially FIPs can have their own set of tags, e.g. FIP can be preallocated in advance and a special tag can be assigned on it. Once OCCM starts to manage this FIP, it will remove the tags assigned manually. This should not happen.

Current code (after my remarks will be addressed) is only tagging FIPs on creation. Same happens with SGs, we assume it's always us creating it, if manage-security-groups=false that code won't be executed. Would that work for you? I also suggested to drop the tags replace call if user haven't defined any tags in the config.

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[dulek]

/remove-lifecycle stale

[dulek]

Ah wait, this is indeed stale. I'll just close this, we have other PRs looking at this.