Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nsxserviceaccount: update PI cert when there is PI while there is no cluster-control-plane #1020

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

liu4480
Copy link
Contributor

@liu4480 liu4480 commented Feb 10, 2025

Each time when nsx-operator reconciles NSXServiceAccount, it creates a new cert, and uses the new cert to create PI, then create the cluster-control-plane resource.

If the PI creation was successful, but cluster-control-plane creation fails, it doesn't delete the PI, but it continue with next retry. The next retry will generate a new cert, but the new cert is not updated to the existing PI, and the new cert is used for creating cluster-control-plane. In this bug, at first the license is missing, so PI is created with an older cert, and then license is configured, so the cluster-control-plane is created with a newer cert. That's why the clusters created before the license was applied run into such symptom.

the key problem is this code snippet:

else if !hasPI != (piObj == nil) {
                return "", fmt.Errorf("old PI exists")
}

This code is originally for checking whether a cluster with the same name has already been registered but it's with a different uuid. This means probably there is some leftover data in NSX.

Leftover data case:
hasPI: false
!hasPI: true
(piObj == nil): false
!hasPI != (piObj == nil): true
So it reports old PI exists error.

PI creation successful but cluster-control-plane creation failure case:
hasPI: true
!hasPI: false
(piObj == nil): false
!hasPI != (piObj == nil): false
So it doesn't report error, in this case, nsxserviceaccounts server shall update the PI with new cert

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 86.48649% with 5 lines in your changes missing coverage. Please review.

Project coverage is 74.47%. Comparing base (4307d92) to head (c41a588).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/nsx/services/nsxserviceaccount/cluster.go 86.48% 3 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1020   +/-   ##
=======================================
  Coverage   74.47%   74.47%           
=======================================
  Files         118      118           
  Lines       16285    16313   +28     
=======================================
+ Hits        12128    12149   +21     
- Misses       3389     3393    +4     
- Partials      768      771    +3     
Flag Coverage Δ
unit-tests 74.47% <86.48%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
pkg/nsx/services/nsxserviceaccount/cluster.go 79.33% <86.48%> (-0.29%) ⬇️

@liu4480 liu4480 force-pushed the topic/biliu/nsxserviceaccounts branch from 8806f8c to a00773a Compare February 11, 2025 03:02
@liu4480
Copy link
Contributor Author

liu4480 commented Feb 11, 2025

@edwardbadboy @dantingl @lxiaopei can you have a look? thanks

Copy link
Contributor

@gran-vmv gran-vmv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@liu4480 liu4480 force-pushed the topic/biliu/nsxserviceaccounts branch 2 times, most recently from 41ea0e7 to a22a122 Compare February 17, 2025 09:53
@liu4480 liu4480 force-pushed the topic/biliu/nsxserviceaccounts branch from a22a122 to c41a588 Compare February 18, 2025 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants