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

PWX-38512 Skip token refresh verification if host-pid not enabled #1635

Merged
merged 3 commits into from
Aug 12, 2024

Conversation

ssz1997
Copy link
Collaborator

@ssz1997 ssz1997 commented Aug 9, 2024

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/

@nikolaypopov
Copy link
Collaborator

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?

Copy link
Collaborator

@nikolaypopov nikolaypopov left a comment

Choose a reason for hiding this comment

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

LGTM with few nit comments

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also we can probably just do this, less heavier operation

Suggested change
if len(pxSaSecret.Data[core.ServiceAccountTokenKey]) == 0 {
if pxSaSecret.Data[core.ServiceAccountTokenKey] == "" {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a byte array, not a string, so checking the length would be better in this case?

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.59%. Comparing base (6e2096d) to head (f8dc862).

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.
📢 Have feedback on the report? Share it here.

shsun_pure added 3 commits August 11, 2024 21:11
Signed-off-by: shsun_pure <shsun@purestorage.com>
Signed-off-by: shsun_pure <shsun@purestorage.com>
Copy link
Contributor

@nikita-bhatia nikita-bhatia left a comment

Choose a reason for hiding this comment

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

LGTM

@nikita-bhatia nikita-bhatia merged commit 09fcd70 into master Aug 12, 2024
9 checks passed
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 <shsun@purestorage.com>

* validate px serviceaccount token secret created

Signed-off-by: shsun_pure <shsun@purestorage.com>

* update error message

---------

Signed-off-by: shsun_pure <shsun@purestorage.com>
Co-authored-by: shsun_pure <shsun@purestorage.com>
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 <shsun@purestorage.com>

* validate px serviceaccount token secret created

Signed-off-by: shsun_pure <shsun@purestorage.com>

* update error message

---------

Signed-off-by: shsun_pure <shsun@purestorage.com>
Co-authored-by: shsun_pure <shsun@purestorage.com>
Signed-off-by: shsun_pure <shsun@purestorage.com>
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 <shsun@purestorage.com>
Co-authored-by: shsun_pure <shsun@purestorage.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants