✨ pkg/cloud/services/networking/securitygroups.go reimplement reconcilation #773

chrischdi · 2021-03-08T08:39:07Z

What this PR does / why we need it:
Reimplements the security group reconcilation by only creating and/or
deleting the rules desired / not desired instead of deleting all and creating all rules.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #771

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

/hold

k8s-ci-robot · 2021-03-08T08:39:16Z

Welcome @chrischdi!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-03-08T08:39:16Z

Hi @chrischdi. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

theopenlab-ci · 2021-03-08T10:37:29Z

Build failed.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : FAILURE in 34m 17s

chrischdi · 2021-03-08T13:09:13Z

I had to remove the RemoteIPPrefix: "0.0.0.0/0", which at the end has the same result as setting nothing: allow all.

Otherwise the reconcilation is getting a 409 because it would try to create another security group rule which has the same spec.

jichenjc · 2021-03-08T13:23:16Z

/ok-to-test

theopenlab-ci · 2021-03-08T13:34:31Z

Build failed.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : FAILURE in 34m 48s

jichenjc · 2021-03-09T02:07:33Z

recheck

theopenlab-ci · 2021-03-09T02:43:46Z

Build failed.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : FAILURE in 35m 03s

chrischdi · 2021-03-09T12:09:00Z

recheck

I'll debug it in a local devstack setup. From our environment I can see that the environment does not save the description.

chrischdi · 2021-03-09T13:41:56Z

I was able to fix the issue. By default there are already security group rules inside the security groups which have another description.

Because of that I changed the order to first delete not needed rules and create the new ones afterwards

theopenlab-ci · 2021-03-09T14:23:23Z

Build succeeded.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : SUCCESS in 44m 08s

jichenjc · 2021-03-11T02:06:34Z

reasonable change just curious about following statement, why are we creating an exactly same rule if we know
it's same, why we will create it?

I had to remove the RemoteIPPrefix: "0.0.0.0/0", which at the end has the same result as setting nothing: allow all.

Otherwise the reconcilation is getting a 409 because it would try to create another security group rule which has the same spec.

sbueringer · 2021-03-11T14:54:17Z

/retitle ✨ pkg/cloud/services/networking/securitygroups.go reimplement reconcilation

sbueringer · 2021-03-11T14:55:07Z

reasonable change just curious about following statement, why are we creating an exactly same rule if we know
it's same, why we will create it?
I had to remove the RemoteIPPrefix: "0.0.0.0/0", which at the end has the same result as setting nothing: allow all.

Otherwise the reconcilation is getting a 409 because it would try to create another security group rule which has the same spec.

I think the problem is that the deep equal does not detect that it's the same because the server will return something else compared to what we created before.

sbueringer · 2021-03-11T14:56:49Z

@chrischdi please rebase so that the tests are run against the new v1alpha4 master version

chrischdi · 2021-03-12T12:33:18Z

reasonable change just curious about following statement, why are we creating an exactly same rule if we know
it's same, why we will create it?
I had to remove the RemoteIPPrefix: "0.0.0.0/0", which at the end has the same result as setting nothing: allow all.

Otherwise the reconcilation is getting a 409 because it would try to create another security group rule which has the same spec.
I think the problem is that the deep equal does not detect that it's the same because the server will return something else compared to what we created before.

Exactly: on our OpenStack (which is "VMware Integrated OpenStack") the following happens:

security groups get reconciled
- rules get created
security groups get reconciled again later on
- rules previously created having RemoteIPPrefix: "0.0.0.0/0" will get deleted and created again, because the API returns nil instead of 0.0.0.0/0.

I can see the same result when using the openstack cli and checking the output of openstack security group rule list --debug <secgroup ID>

However: I think the response of the API is still okay, because nil will default to 0.0.0.0/0 in the backend according the cli docs:

$ openstack help security group rule create | grep -e ' --remote-ip' -A 3
  --remote-ip <ip-address>
                        Remote IP address block (may use CIDR notation;
                        default for IPv4 rule: 0.0.0.0/0, default for IPv6
                        rule: ::/0)

It is also ok for me to revert the removal of 0.0.0.0/0 on this PR if you like me to do so :-)

chrischdi · 2021-03-12T12:35:12Z

@chrischdi please rebase so that the tests are run against the new v1alpha4 master version

rebased to master :-)

theopenlab-ci · 2021-03-12T12:53:44Z

Build failed.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : FAILURE in 17m 46s

chrischdi · 2021-03-12T12:58:31Z

/retest

chrischdi · 2021-03-12T13:00:54Z

recheck

theopenlab-ci · 2021-03-12T13:23:33Z

Build failed.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : FAILURE in 21m 16s

chrischdi · 2021-03-12T13:42:48Z

Looks like we're hitting the same issues as in #779 , maybe I should wait for #759

…tion Reimplements the security group reconcilation by only creating and/or deleting the rules desired / not desired. Signed-off-by: Schlotter, Christian <christian.schlotter@daimler.com>

…tatus logs

… when matching 0.0.0.0/0 A RemoteIPPrefix of 0.0.0.0/0 is equal to the default (""/None). Because of that we do not set it to not trigger reconcilations.

…elete then create By default there are already rules which exist and do not match by name. Therefor we first delete not-matching rules and create the desired ones afterwards.

chrischdi · 2021-03-15T14:14:49Z

rebased to master again to have gcp based tests

chrischdi · 2021-03-15T14:27:04Z

/test help

sbueringer · 2021-03-15T14:27:12Z

/test pull-cluster-api-provider-openstack-make-conformance

k8s-ci-robot · 2021-03-15T14:27:18Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test pull-cluster-api-provider-openstack-build
/test pull-cluster-api-provider-openstack-test
/test pull-cluster-api-provider-openstack-e2e-test
/test pull-cluster-api-provider-openstack-make-conformance

Use /test all to run the following jobs:

pull-cluster-api-provider-openstack-build
pull-cluster-api-provider-openstack-test

In response to this:

/test help

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.

theopenlab-ci · 2021-03-15T14:34:34Z

Build failed.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : ERROR Unable to find playbook /var/lib/zuul/builds/19d03a8c35e1463eb6fef5ba93b69687/untrusted/project_0/github.com/kubernetes-sigs/cluster-api-provider-openstack/.zuul/playbooks/run.yaml in 8s

sbueringer

conformance test green now :)

k8s-ci-robot · 2021-03-15T18:30:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi, sbueringer

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbueringer]

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

jichenjc · 2021-03-16T00:25:27Z

recheck

theopenlab-ci · 2021-03-16T00:26:24Z

Build failed.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : ERROR Unable to find playbook /var/lib/zuul/builds/61e6b02960a04be2be1d0973e8f402be/untrusted/project_0/github.com/kubernetes-sigs/cluster-api-provider-openstack/.zuul/playbooks/run.yaml in 8s

jichenjc · 2021-03-16T00:26:28Z

reasonable change just curious about following statement, why are we creating an exactly same rule if we know
it's same, why we will create it?
I had to remove the RemoteIPPrefix: "0.0.0.0/0", which at the end has the same result as setting nothing: allow all.

Otherwise the reconcilation is getting a 409 because it would try to create another security group rule which has the same spec.
I think the problem is that the deep equal does not detect that it's the same because the server will return something else compared to what we created before.
Exactly: on our OpenStack (which is "VMware Integrated OpenStack") the following happens:
* security groups get reconciled
  
  * rules get created

* security groups get reconciled again later on
  
  * rules previously created having `RemoteIPPrefix: "0.0.0.0/0"` will get deleted and created again, because the API returns `nil` instead of `0.0.0.0/0`.
I can see the same result when using the openstack cli and checking the output of openstack security group rule list --debug <secgroup ID>

However: I think the response of the API is still okay, because nil will default to 0.0.0.0/0 in the backend according the cli docs:
$ openstack help security group rule create | grep -e ' --remote-ip' -A 3
  --remote-ip <ip-address>
                        Remote IP address block (may use CIDR notation;
                        default for IPv4 rule: 0.0.0.0/0, default for IPv6
                        rule: ::/0)
It is also ok for me to revert the removal of 0.0.0.0/0 on this PR if you like me to do so :-)

no need, just want to understand the logic here, all good :_)

jichenjc · 2021-03-16T00:28:15Z

/lgtm

jichenjc · 2021-03-16T00:28:51Z

wait for zuul job to merge

jichenjc · 2021-03-16T00:34:14Z

@sbueringer looks like we don't want to run openstack/check anymore as the playbook is missing
so we should go ahead merge this and overwrite the failure..

jichenjc · 2021-03-16T00:45:10Z

/hold cancel

sbueringer · 2021-03-16T05:23:40Z

@jichenjc Yes we have to ignore the OpenLab failures for now. I'll try to remove the OpenLab ASAP after we have the ProwJob on release-0.3

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 8, 2021

k8s-ci-robot requested review from hidekazuna and prankul88 March 8, 2021 08:39

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2021

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 8, 2021

k8s-ci-robot changed the title ~~✨ pkg/cloud/services/networking/securitygroups.go reimplement reconcilation~~ ✨ pkg/cloud/services/networking/securitygroups.go reimplement reconcilation Mar 11, 2021

chrischdi force-pushed the reimplement-securitygroup-reconcilation branch from 6c41df5 to a977e09 Compare March 12, 2021 12:34

chrischdi added 4 commits March 15, 2021 15:13

pkg/cloud/services/networking/securitygroups.go reimplement reconcila…

944a7df

…tion Reimplements the security group reconcilation by only creating and/or deleting the rules desired / not desired. Signed-off-by: Schlotter, Christian <christian.schlotter@daimler.com>

pkg/cloud/services/networking/securitygroups.go lower log level for s…

88c8e7e

…tatus logs

pkg/cloud/services/networking/securitygroups.go remove RemoteIPPrefix…

efd9a20

… when matching 0.0.0.0/0 A RemoteIPPrefix of 0.0.0.0/0 is equal to the default (""/None). Because of that we do not set it to not trigger reconcilations.

pkg/cloud/services/networking/securitygroups.go switch order, first d…

98ecc10

…elete then create By default there are already rules which exist and do not match by name. Therefor we first delete not-matching rules and create the desired ones afterwards.

chrischdi force-pushed the reimplement-securitygroup-reconcilation branch from a977e09 to 98ecc10 Compare March 15, 2021 14:14

sbueringer approved these changes Mar 15, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2021

k8s-ci-robot assigned jichenjc Mar 16, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2021

k8s-ci-robot merged commit 1a851a4 into kubernetes-sigs:master Mar 16, 2021

chrischdi mentioned this pull request May 18, 2021

REQUEST: New membership for chrischdi kubernetes/org#2717

Closed

6 tasks

✨ pkg/cloud/services/networking/securitygroups.go reimplement reconcilation #773

✨ pkg/cloud/services/networking/securitygroups.go reimplement reconcilation #773

Conversation

chrischdi commented Mar 8, 2021

k8s-ci-robot commented Mar 8, 2021

k8s-ci-robot commented Mar 8, 2021

theopenlab-ci bot commented Mar 8, 2021

chrischdi commented Mar 8, 2021

jichenjc commented Mar 8, 2021

theopenlab-ci bot commented Mar 8, 2021

jichenjc commented Mar 9, 2021

theopenlab-ci bot commented Mar 9, 2021

chrischdi commented Mar 9, 2021

chrischdi commented Mar 9, 2021

theopenlab-ci bot commented Mar 9, 2021

jichenjc commented Mar 11, 2021

sbueringer commented Mar 11, 2021

sbueringer commented Mar 11, 2021

sbueringer commented Mar 11, 2021

chrischdi commented Mar 12, 2021

chrischdi commented Mar 12, 2021

theopenlab-ci bot commented Mar 12, 2021

chrischdi commented Mar 12, 2021

chrischdi commented Mar 12, 2021

theopenlab-ci bot commented Mar 12, 2021

chrischdi commented Mar 12, 2021

chrischdi commented Mar 15, 2021

chrischdi commented Mar 15, 2021

sbueringer commented Mar 15, 2021

k8s-ci-robot commented Mar 15, 2021

theopenlab-ci bot commented Mar 15, 2021

sbueringer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 15, 2021

jichenjc commented Mar 16, 2021

theopenlab-ci bot commented Mar 16, 2021

jichenjc commented Mar 16, 2021

jichenjc commented Mar 16, 2021

jichenjc commented Mar 16, 2021

jichenjc commented Mar 16, 2021

jichenjc commented Mar 16, 2021

sbueringer commented Mar 16, 2021