PWX-38512 Skip token refresh verification if host-pid not enabled #1635
Conversation
This file contains 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
|
Do you think we should maybe check for annotation and not validate the actual token, if the annotation is missing, but maybe we should still validate up to the point that the secret exists, at least we will be sure that it gets created? |
ssz1997
force-pushed
the
fixIntegrationTest
branch
from
f4c4d5a
to
c0db204
Compare
nikolaypopov
approved these changes
Aug 9, 2024
pkg/util/test/util.go
Outdated
| if err != nil || !pidEnabled { | ||
| pxSaSecret, err := coreops.Instance().GetSecret(pxSaTokenSecretName, cluster.Namespace) | ||
| if err != nil { | ||
| return fmt.Errorf("px serviceaccount token validation failed. Unable to get px serviceaccount secret. Err: %w", err) |
There was a problem hiding this comment.
I would suggest to print errors like this, from my experience, it helps later on to debug simple things just from logs
Suggested change
| return fmt.Errorf("px serviceaccount token validation failed. Unable to get px serviceaccount secret. Err: %w", err) | |
| return fmt.Errorf("failed to get px serviceaccount secret [%s] in namespace [%s]. Err: %w", pxSaTokenSecretName, cluster.Namespace, err) |
| if err != nil { | ||
| return fmt.Errorf("px serviceaccount token validation failed. Unable to get px serviceaccount secret. Err: %w", err) | ||
| } | ||
| if len(pxSaSecret.Data[core.ServiceAccountTokenKey]) == 0 { |
There was a problem hiding this comment.
Also we can probably just do this, less heavier operation
Suggested change
| if len(pxSaSecret.Data[core.ServiceAccountTokenKey]) == 0 { | |
| if pxSaSecret.Data[core.ServiceAccountTokenKey] == "" { |
There was a problem hiding this comment.
It's a byte array, not a string, so checking the length would be better in this case?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1635 +/- ##
=======================================
Coverage 75.59% 75.59%
=======================================
Files 77 77
Lines 20862 20862
=======================================
Hits 15771 15771
Misses 3967 3967
Partials 1124 1124 ☔ View full report in Codecov by Sentry. |
added 3 commits
August 11, 2024 21:11
Signed-off-by: shsun_pure <[email protected]>
Signed-off-by: shsun_pure <[email protected]>
ssz1997
force-pushed
the
fixIntegrationTest
branch
from
50170bd
to
f8dc862
Compare
ssz1997
added a commit
that referenced
this pull request
Aug 12, 2024
) * skip token refresh verification if host-pid not enabled Signed-off-by: shsun_pure <[email protected]> * validate px serviceaccount token secret created Signed-off-by: shsun_pure <[email protected]> * update error message --------- Signed-off-by: shsun_pure <[email protected]> Co-authored-by: shsun_pure <[email protected]>
ssz1997
added a commit
that referenced
this pull request
Aug 12, 2024
) * skip token refresh verification if host-pid not enabled Signed-off-by: shsun_pure <[email protected]> * validate px serviceaccount token secret created Signed-off-by: shsun_pure <[email protected]> * update error message --------- Signed-off-by: shsun_pure <[email protected]> Co-authored-by: shsun_pure <[email protected]> Signed-off-by: shsun_pure <[email protected]>
ssz1997
added a commit
that referenced
this pull request
Aug 12, 2024
) (#1641) * skip token refresh verification if host-pid not enabled * validate px serviceaccount token secret created * update error message --------- Signed-off-by: shsun_pure <[email protected]> Co-authored-by: shsun_pure <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does / why we need it:
host-pid is required to run nsenter with runc. nsenter without runc is fine is the reason why other verifications are fine.
Which issue(s) this PR fixes (optional)
Closes PWX-38512
A full set of integration test is being run at https://jenkins.pwx.dev.purestorage.com/job/Operator/view/Operator%20Release%20Dashboard/job/op-basic-hotfix/