🐛: securitygroup: fix comparison of ingress rules sets. #5024

r4f4 · 2024-06-13T16:55:06Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

We are comparing sets that are not compatible, so rules will always be revoked and authorized during reconciliation. We need to expand the rules from the spec so there is one rule for each item in cidrBlock/sourceSecurityGroupIds.

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 #5023

Special notes for your reviewer:

Checklist:

Release note:

Fix comparison of ingress rules during Security Group reconciliation to avoid unnecessary revokes and authorizes.

k8s-ci-robot · 2024-06-13T16:55:16Z

Hi @r4f4. 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-sigs/prow repository.

r4f4 · 2024-06-13T17:16:07Z

Without this change, I'd see a constant stream of revoke-authorize ingress rules in the debug logs [1]. Now I see the rules authorized once and no revokes (will add logs once I have more signal from openshift CI).

[1] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_installer/8545/pull-ci-openshift-installer-master-e2e-aws-ovn/1801267394513997824/artifacts/e2e-aws-ovn/ipi-install-install/artifacts/.openshift_install-1718293235.log

mtulio

overall LGTM, very good fix/improvement in the provision phase. 🎉

@r4f4 Do you think we are covered in the ReconcileSecurityGroups or would be hard to reproduce in new case as revoke is in the same function, and/or would be nice to create a test dedicated to exercise expandIngressRules()?

r4f4 · 2024-06-13T17:30:12Z

Do you think we are covered in the ReconcileSecurityGroups or would be hard to reproduce in new case as revoke is in the same function, and/or would be nice to create a test dedicated to exercise expandIngressRules()?

Good call. I can, at a minimum, add unit tests for the expandIngressRules() function. Will do that while I wait for CI results.

We are comparing sets that are not compatible, so rules will always be revoked and authorized during reconciliation. We need to expand the rules from the spec so there is one rule for each item in cidrBlock/sourceSecurityGroupIds.

r4f4 · 2024-06-13T19:42:07Z

Updated: add a few fixes and unit tests.

r4f4 · 2024-06-14T07:02:02Z

Openshift CI results are all green! openshift/installer#8596. Looking at the logs, the only rule being revoked is the one we explicitely removed from the cluster spec:

time="2024-06-13T21:27:14Z" level=debug msg="I0613 21:27:14.705844     356 securitygroups.go:181] \"Revoked ingress rules from security group\" controller=\"awscluster\" controllerGroup=\"infrastructure.cluster.x-k8s.io\" controllerKind=\"AWSCluster\" AWSCluster=\"openshift-cluster-api-guests/ci-op-x5n2dn57-2d061-fxccx\" namespace=\"openshift-cluster-api-guests\" name=\"ci-op-x5n2dn57-2d061-fxccx\" reconcileID=\"1eebbaa2-4a91-478f-91df-6d0cf652d71c\" cluster=\"openshift-cluster-api-guests/ci-op-x5n2dn57-2d061-fxccx\" revoked-ingress-rules=[{\"description\":\"Bootstrap SSH Access\",\"protocol\":\"tcp\",\"fromPort\":22,\"toPort\":22,\"cidrBlocks\":[\"0.0.0.0/0\"]}] security-group-id=\"sg-0193bb8269722f9e3\""
time="2024-06-13T21:27:14Z" level=debug msg="I0613 21:27:14.705926     356 securitygroups.go:150] \"second pass security group reconciliation\" controller=\"awscluster\" controllerGroup=\"infrastructure.cluster.x-k8s.io\" controllerKind=\"AWSCluster\" AWSCluster=\"openshift-cluster-api-guests/ci-op-x5n2dn57-2d061-fxccx\" namespace=\"openshift-cluster-api-guests\" reconcileID=\"1eebbaa2-4a91-478f-91df-6d0cf652d71c\" cluster=\"openshift-cluster-api-guests/ci-op-x5n2dn57-2d061-fxccx\" group-id=\"sg-06d4855d6bc549b25\" name=\"ci-op-x5n2dn57-2d061-fxccx-lb\" role=\"lb\""

https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_installer/8596/pull-ci-openshift-installer-master-altinfra-e2e-aws-ovn/1801355744365252608/artifacts/e2e-aws-ovn/ipi-install-install/artifacts/.openshift_install-1718314761.log

r4f4 · 2024-06-14T13:03:09Z

@damdo would you mind enabling tests on this PR?

r4f4 · 2024-06-14T14:03:18Z

/cc @alexander-demicev
I see you've recently changed this code. WDYT of this fix?

This test checks that target ingress rules that have already been authorized are not revoked and then authorized again.

r4f4 · 2024-06-16T19:07:50Z

@mtulio following your suggestion, I've added a unit test to check that we are not revoking ingress rules unnecessarily.

mtulio · 2024-06-18T21:46:19Z

/lgtm

k8s-ci-robot · 2024-06-18T21:46:23Z

@mtulio: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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-sigs/prow repository.

nrb · 2024-06-20T15:59:17Z

/ok-to-test

r4f4 · 2024-06-20T17:01:19Z

Tests are all green here as well!

r4f4 · 2024-06-20T17:01:52Z

/test ?

k8s-ci-robot · 2024-06-20T17:02:06Z

@r4f4: The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-aws-build
/test pull-cluster-api-provider-aws-build-docker
/test pull-cluster-api-provider-aws-test
/test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-aws-apidiff-main
/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-e2e-clusterclass
/test pull-cluster-api-provider-aws-e2e-conformance
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
/test pull-cluster-api-provider-aws-e2e-eks
/test pull-cluster-api-provider-aws-e2e-eks-gc
/test pull-cluster-api-provider-aws-e2e-eks-testing

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-aws-apidiff-main
pull-cluster-api-provider-aws-build
pull-cluster-api-provider-aws-build-docker
pull-cluster-api-provider-aws-test
pull-cluster-api-provider-aws-verify

In response to this:

/test ?

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-sigs/prow repository.

r4f4 · 2024-06-20T17:02:44Z

/test pull-cluster-api-provider-aws-e2e pull-cluster-api-provider-aws-e2e-blocking

r4f4 · 2024-06-20T17:34:30Z

aws-e2e failure doesn't seem to be cause by the PR changes:

   Should've eventually succeeded creating an AWS CloudFormation stack
  Expected
      <bool>: false
  to be true
  In [SynchronizedBeforeSuite] at: /home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws/test/e2e/shared/suite.go:147 @ 06/20/24 17:24:37.437

/test pull-cluster-api-provider-aws-e2e

r4f4 · 2024-06-20T19:53:30Z

aws-e2e: the 2 e2e failures don't look related to the PR changes but will retest again to be sure.

Test from this PR

In the log of this PR no revokes until the cluster is deleted.

Test from another PR

Looking at an old test log:

I0611 16:19:08.741022       1 securitygroups.go:179] "Revoked ingress rules from security group" controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="self-hosted-yr5v5v/self-hosted-70i0jt-klt6x" namespace="self-hosted-yr5v5v" name="self-hosted-70i0jt-klt6x" reconcileID="df380180-8af8-45d2-a1c0-fd7d1108558e" cluster="self-hosted-yr5v5v/self-hosted-70i0jt" revoked-ingress-rules=[{"description":"Kubernetes API","protocol":"tcp","fromPort":6443,"toPort":6443,"sourceSecurityGroupIds":["sg-04c31b37099612208"]},{"description":"Kubernetes API","protocol":"tcp","fromPort":6443,"toPort":6443,"sourceSecurityGroupIds":["sg-02a4d2faaf7102a28"]},{"description":"Kubernetes API","protocol":"tcp","fromPort":6443,"toPort":6443,"sourceSecurityGroupIds":["sg-0799511b5638468a1"]},{"description":"bgp (calico)","protocol":"tcp","fromPort":179,"toPort":179,"sourceSecurityGroupIds":["sg-02a4d2faaf7102a28"]},{"description":"bgp (calico)","protocol":"tcp","fromPort":179,"toPort":179,"sourceSecurityGroupIds":["sg-0799511b5638468a1"]},{"description":"IP-in-IP (calico)","protocol":"4","fromPort":0,"toPort":0,"sourceSecurityGroupIds":["sg-02a4d2faaf7102a28"]},{"description":"IP-in-IP (calico)","protocol":"4","fromPort":0,"toPort":0,"sourceSecurityGroupIds":["sg-0799511b5638468a1"]}] security-group-id="sg-02a4d2faaf7102a28"
I0611 16:19:09.000272       1 securitygroups.go:193] "Authorized ingress rules in security group" controller="awscluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="AWSCluster" AWSCluster="self-hosted-yr5v5v/self-hosted-70i0jt-klt6x" namespace="self-hosted-yr5v5v" name="self-hosted-70i0jt-klt6x" reconcileID="df380180-8af8-45d2-a1c0-fd7d1108558e" cluster="self-hosted-yr5v5v/self-hosted-70i0jt" authorized-ingress-rules=[{"description":"bgp (calico)","protocol":"tcp","fromPort":179,"toPort":179,"sourceSecurityGroupIds":["sg-02a4d2faaf7102a28","sg-0799511b5638468a1"]},{"description":"IP-in-IP (calico)","protocol":"4","fromPort":-1,"toPort":65535,"sourceSecurityGroupIds":["sg-02a4d2faaf7102a28","sg-0799511b5638468a1"]},{"description":"Kubernetes API","protocol":"tcp","fromPort":6443,"toPort":6443,"sourceSecurityGroupIds":["sg-04c31b37099612208","sg-02a4d2faaf7102a28","sg-0799511b5638468a1"]}] security-group-id="sg-02a4d2faaf7102a28"

So the following rules are being revoked:

[{"description":"Kubernetes API","protocol":"tcp","fromPort":6443,"toPort":6443,"sourceSecurityGroupIds":["sg-04c31b37099612208"]},{"description":"Kubernetes API","protocol":"tcp","fromPort":6443,"toPort":6443,"sourceSecurityGroupIds":["sg-02a4d2faaf7102a28"]},{"description":"Kubernetes API","protocol":"tcp","fromPort":6443,"toPort":6443,"sourceSecurityGroupIds":["sg-0799511b5638468a1"]}

Then being authorized as:

{"description":"Kubernetes API","protocol":"tcp","fromPort":6443,"toPort":6443,"sourceSecurityGroupIds":["sg-04c31b37099612208","sg-02a4d2faaf7102a28","sg-0799511b5638468a1"]}

r4f4 · 2024-06-20T21:23:59Z

/test pull-cluster-api-provider-aws-e2e

r4f4 · 2024-06-21T07:43:41Z

/test pull-cluster-api-provider-aws-e2e

r4f4 · 2024-06-21T09:28:00Z

/assign @nrb

Tests look good. This is ready for review.

alexander-demicev

Thanks taking care of this
/lgtm

richardcase · 2024-07-10T10:52:34Z

Thanks @r4f4

/approve

k8s-ci-robot · 2024-07-10T10:52:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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~~ [richardcase]

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

* Brings in this fix kubernetes-sigs/cluster-api-provider-aws#5024 to avoid unnecessary revoke-authorize of ingress rules.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Jun 13, 2024

k8s-ci-robot requested review from dlipovetsky and fiunchinho June 13, 2024 16:55

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 13, 2024

mtulio reviewed Jun 13, 2024

View reviewed changes

r4f4 added 2 commits June 13, 2024 21:37

🌱: add unit tests for expandIngressRules

ce5aaf7

r4f4 force-pushed the fix-5023 branch from 414dac3 to ce5aaf7 Compare June 13, 2024 19:41

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 Jun 13, 2024

mtulio mentioned this pull request Jun 14, 2024

OCPBUGS-35054: capi/aws/byo-vpc/multi-cidr: fix group rules openshift/installer#8591

Merged

k8s-ci-robot requested a review from alexander-demicev June 14, 2024 14:03

🌱: securitygroup: add unit test against unnecessary revokes

b2d1d3d

This test checks that target ingress rules that have already been authorized are not revoked and then authorized again.

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 Jun 20, 2024

k8s-ci-robot assigned nrb Jun 21, 2024

alexander-demicev reviewed Jun 21, 2024

View reviewed changes

k8s-ci-robot assigned alexander-demicev Jun 21, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2024

k8s-ci-robot merged commit d375b7d into kubernetes-sigs:main Jul 10, 2024
25 checks passed

r4f4 added a commit to r4f4/installer that referenced this pull request Jul 10, 2024

OCPBUGS-35440: capi/aws: bump provider for ingress rules fix

707acef

* Brings in this fix kubernetes-sigs/cluster-api-provider-aws#5024 to avoid unnecessary revoke-authorize of ingress rules.

r4f4 mentioned this pull request Jul 10, 2024

OCPBUGS-35440: capi/aws: bump provider for ingress rules fix openshift/installer#8596

Merged

r4f4 added a commit to r4f4/installer that referenced this pull request Jul 14, 2024

OCPBUGS-35440: capi/aws: bump provider for ingress rules fix

defd47f

* Brings in this fix kubernetes-sigs/cluster-api-provider-aws#5024 to avoid unnecessary revoke-authorize of ingress rules.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛: securitygroup: fix comparison of ingress rules sets. #5024

🐛: securitygroup: fix comparison of ingress rules sets. #5024

r4f4 commented Jun 13, 2024 •

edited

Loading

k8s-ci-robot commented Jun 13, 2024

r4f4 commented Jun 13, 2024

mtulio left a comment •

edited

Loading

r4f4 commented Jun 13, 2024

r4f4 commented Jun 13, 2024

r4f4 commented Jun 14, 2024

r4f4 commented Jun 14, 2024

r4f4 commented Jun 14, 2024

r4f4 commented Jun 16, 2024

mtulio commented Jun 18, 2024

k8s-ci-robot commented Jun 18, 2024

nrb commented Jun 20, 2024

r4f4 commented Jun 20, 2024

r4f4 commented Jun 20, 2024

k8s-ci-robot commented Jun 20, 2024

r4f4 commented Jun 20, 2024

r4f4 commented Jun 20, 2024

r4f4 commented Jun 20, 2024

r4f4 commented Jun 20, 2024

r4f4 commented Jun 21, 2024

r4f4 commented Jun 21, 2024

alexander-demicev left a comment

richardcase commented Jul 10, 2024

k8s-ci-robot commented Jul 10, 2024

🐛: securitygroup: fix comparison of ingress rules sets. #5024

🐛: securitygroup: fix comparison of ingress rules sets. #5024

Conversation

r4f4 commented Jun 13, 2024 • edited Loading

k8s-ci-robot commented Jun 13, 2024

r4f4 commented Jun 13, 2024

mtulio left a comment • edited Loading

Choose a reason for hiding this comment

r4f4 commented Jun 13, 2024

r4f4 commented Jun 13, 2024

r4f4 commented Jun 14, 2024

r4f4 commented Jun 14, 2024

r4f4 commented Jun 14, 2024

r4f4 commented Jun 16, 2024

mtulio commented Jun 18, 2024

k8s-ci-robot commented Jun 18, 2024

nrb commented Jun 20, 2024

r4f4 commented Jun 20, 2024

r4f4 commented Jun 20, 2024

k8s-ci-robot commented Jun 20, 2024

r4f4 commented Jun 20, 2024

r4f4 commented Jun 20, 2024

r4f4 commented Jun 20, 2024

Test from this PR

Test from another PR

r4f4 commented Jun 20, 2024

r4f4 commented Jun 21, 2024

r4f4 commented Jun 21, 2024

alexander-demicev left a comment

Choose a reason for hiding this comment

richardcase commented Jul 10, 2024

k8s-ci-robot commented Jul 10, 2024

r4f4 commented Jun 13, 2024 •

edited

Loading

mtulio left a comment •

edited

Loading