Add Inventory Controller watchers and VM tag management for VKS IDPS support#1399
Draft
yuntanghsu wants to merge 1 commit intovmware-tanzu:mainfrom
Draft
Add Inventory Controller watchers and VM tag management for VKS IDPS support#1399yuntanghsu wants to merge 1 commit intovmware-tanzu:mainfrom
yuntanghsu wants to merge 1 commit intovmware-tanzu:mainfrom
Conversation
Contributor
|
Can one of the admins verify this patch? |
yuntanghsu
force-pushed
the
yuntanghsu/idps
branch
2 times, most recently
from
386e4cc to
5097af1
Compare
yuntanghsu
changed the title
yuntanghsu
force-pushed
the
yuntanghsu/idps
branch
from
5097af1 to
4a6f13c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1399 +/- ##
==========================================
- Coverage 76.77% 74.81% -1.96%
==========================================
Files 151 154 +3
Lines 21315 21600 +285
==========================================
- Hits 16364 16160 -204
+ Misses 3782 3781 -1
- Partials 1169 1659 +490
🚀 New features to boost your workflow:
|
yuntanghsu
force-pushed
the
yuntanghsu/idps
branch
3 times, most recently
from
4c908fe to
854f629
Compare
| } | ||
| } | ||
|
|
||
| if nsxSA.Status.Phase != nsxvmwarecomv1alpha1.NSXServiceAccountPhaseRealized { |
There was a problem hiding this comment.
With the exception of the above realization check, handleNSXServiceAccount and handleNSXServiceAccountDelete are exactly the same.
I think it should be ok to have this check even in the deletion handler, so maybe could we just remove handleNSXServiceAccountDelete?
Contributor
Author
There was a problem hiding this comment.
Could the NSXServiceAccount somehow become unrealized? I separated them because if the NSXServiceAccount becomes unrealized, the update and delete functions will skip it, and the VM's tag won't be removed.
| func (s *InventoryService) findRealizedNSXServiceAccount(namespace string) (*nsxvmwarecomv1alpha1.NSXServiceAccount, error) { | ||
| nsxSAList := &nsxvmwarecomv1alpha1.NSXServiceAccountList{} | ||
| if err := s.Client.List(context.TODO(), nsxSAList, &client.ListOptions{ | ||
| Namespace: namespace, |
There was a problem hiding this comment.
dumb question: do you know if it's possible to pass a condition on ownerreferences to listoptions?
This should allow us to simply fetch the service account for a cluster
Contributor
Author
There was a problem hiding this comment.
K8s ListOptions does not support filtering by ownerReferences fields. I think it should be fine as we only have very few NSXServiceAccounts per namespace?
yuntanghsu
force-pushed
the
yuntanghsu/idps
branch
4 times, most recently
from
8211719 to
acab59f
Compare
…support Add NSXServiceAccount and VirtualMachine watchers to the Inventory Controller, along with add/remove VM tag functionality in the Inventory Service. This enables tagging NSX Inventory VMs with nsx-op/cluster-name to support IDPS correlation of Kubernetes objects to IPs in VKS clusters. Key changes: - Add SupervisorClusterName field to NSXServiceAccountStatus and CRD schema, with backfill logic for already-realized ServiceAccounts - Implement VirtualMachine informer that enqueues running VKS VMs for tag processing, with a dedicated delete handler to ensure taggedVMs store cleanup (avoiding memory leak) - Implement NSXServiceAccount informer that enqueues VMs on SA realization and deletion for tag add/remove - Add SyncVirtualMachineTag with idempotent add/remove logic using NSX Fabric API (POST update_tags), backed by in-memory taggedVMs store rehydrated from NSX on startup via initTaggedVMs - Fix HandleHTTPResponse to accept 204 No Content and empty bodies - Add comprehensive unit tests for all handlers and service logic VGL-51541 Signed-off-by: Yun-Tang Hsu <yun-tang.hsu@broadcom.com>
yuntanghsu
force-pushed
the
yuntanghsu/idps
branch
from
acab59f to
8a497b9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add NSXServiceAccount and VirtualMachine watchers to the Inventory Controller, along with add/remove VM tag functionality in the Inventory Service. This enables tagging NSX Inventory VMs with nsx-op/cluster-name to support IDPS correlation of Kubernetes objects to IPs in VKS clusters.
Key changes:
VGL-51541
Signed-off-by: Yun-Tang Hsu yun-tang.hsu@broadcom.com