Fix: egress rules & tags check existing #447
Conversation
* Currently this provider for isolated_network and tags do not
properly check for existing egress rules or tags. This causes
errors to happen in Cloudstack Events constantly saying the egress
rule or tag already exists
k8s-ci-robot
added
the
cncf-cla: no
✅ Deploy Preview for kubernetes-sigs-cluster-api-cloudstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Welcome @CodeBleu! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @CodeBleu. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
size/L
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CodeBleu, weizhouapache 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
CodeBleu
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
CodeBleu
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
* Adjusted some logic in isolated_network.go file so that the unit tests would work better for create Egress/Routing firewall rules
* Attempted to add/update unit tests
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues with egress rule and tag duplication by updating the logic to check for existing rules and tags before attempting to create new ones. Key changes include updating test expectations for tag retrieval, refactoring tag creation logic to only add missing tags, and improving firewall rule handling in isolated network operations.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cloud/vpc_test.go | Updated test flow to include list-tags expectations. |
| pkg/cloud/tags_test.go | Increased list-tags call expectations to assert duplicate calls. |
| pkg/cloud/tags.go | Removed redundant error suppression in tag creation. |
| pkg/cloud/isolated_network_test.go | Enhanced test coverage for both successful and error scenarios. |
| pkg/cloud/isolated_network.go | Refactored firewall rule logic with helper functions for clarity. |
| config/crd/bases/infrastructure.cluster.x-k8s.io_*.yaml | Added firewallRulesOpened field to CRD schemas. |
| api/v1beta3/cloudstackisolatednetwork_types.go | Updated status to include firewallRulesOpened field. |
| api/v1beta2/zz_generated.conversion.go and others | Updated conversion warnings to include the new field. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #447 +/- ##
==========================================
+ Coverage 25.66% 26.25% +0.58%
==========================================
Files 59 72 +13
Lines 5563 6844 +1281
==========================================
+ Hits 1428 1797 +369
- Misses 3996 4879 +883
- Partials 139 168 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
k8s-ci-robot
removed
the
cncf-cla: no
k8s-ci-robot
added
the
cncf-cla: yes
|
@rohityadavcloud @vishesh92 Is there anything else needed for this other than the worklow needs to run again after the changes were made from co-pilot review suggestions? I see a new tag for |
Issue #, if available:
Currently this provider for
isolated_networkandtagsdo not properly check for existing egress rules or tags. This causes errors to happen in Cloudstack Events constantly saying the egress rule or tag already existsIn Cloudstack Management logs (egress rules):
In Cloudstack Management logs (tags):
Description of changes:
Updating
tagsandisolated_networkto properly check for existing egress rules and tags and handle accordingly.Testing performed:
Ran
docker-buildanddocker-pushand then updated my image version on the capc provider to use new image. Observed that errors went away and that it still properly adds the egress rules and tags if removed and doesn't cause any errors in CloudstackBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.