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

Switch Nutanix Client to use Session Auth instead of Basic Auth #398

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

thunderboltsid
Copy link
Contributor

@thunderboltsid thunderboltsid commented Mar 22, 2024

This will ensure we make fewer basic auth requests to Prism Central IAM Services. For now this will reduce calls in a single reconcile cycle to session auth. #415 will help further optimize this by using a cache.

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 25.18%. Comparing base (8cef217) to head (89f298c).

Files Patch % Lines
pkg/client/client.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
+ Coverage   25.07%   25.18%   +0.11%     
==========================================
  Files          19       19              
  Lines        1332     1326       -6     
==========================================
  Hits          334      334              
+ Misses        998      992       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deepakm-ntnx
Copy link
Contributor

it makes sense to have sessionAuth set when we create the first connection by getting the cookies thru /users/me call here https://github.com/nutanix-cloud-native/prism-go-client/blob/main/internal/client.go#L276

@deepakm-ntnx
Copy link
Contributor

we need to add a test case somehow to test this as well

deepakm-ntnx
deepakm-ntnx previously approved these changes Mar 22, 2024
Copy link
Contributor

@deepakm-ntnx deepakm-ntnx left a comment

Choose a reason for hiding this comment

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

lets add a test case in this or thru other PR

@thunderboltsid
Copy link
Contributor Author

lets add a test case in this or thru other PR

Can you elaborate what you expect out of a test case?

@deepakm-ntnx
Copy link
Contributor

lets add a test case in this or thru other PR

Can you elaborate what you expect out of a test case?

we can start with following tests:

  • create connection with sessionAuth set and observe that cookie is set => ideally this can be part of prism-go-client
  • create 100 connections/clusters with sessionAuth set vs not set and see if there is any impact as scale was the main reason we are doing this
  • etc

yannickstruyf3
yannickstruyf3 previously approved these changes Mar 26, 2024
@thunderboltsid
Copy link
Contributor Author

/hold

@deepakm-ntnx
Copy link
Contributor

/hold

@thunderboltsid can we get this patch in and then add tests separately? we need to start the testing with scale test setup as well.

@thunderboltsid
Copy link
Contributor Author

/hold

@thunderboltsid can we get this patch in and then add tests separately? we need to start the testing with scale test setup as well.

any testing can be done using an image built from the branch. Merging this change alone won't fix anything.

@thunderboltsid
Copy link
Contributor Author

/retest

@thunderboltsid
Copy link
Contributor Author

/retest

1 similar comment
@thunderboltsid
Copy link
Contributor Author

/retest

@thunderboltsid thunderboltsid changed the title Switch Nutanix Client to using Session Auth Switch Nutanix Client to using Session Auth and Cached clients per cluster Apr 8, 2024
@thunderboltsid thunderboltsid requested review from dkoshkin and removed request for yannickstruyf3 April 8, 2024 12:04
@thunderboltsid
Copy link
Contributor Author

/retest

@thunderboltsid thunderboltsid changed the title Switch Nutanix Client to using Session Auth Switch Nutanix Client to use Session Auth instead of Basic Auth Apr 23, 2024
@thunderboltsid
Copy link
Contributor Author

/retest

1 similar comment
@thunderboltsid
Copy link
Contributor Author

/retest

@thunderboltsid thunderboltsid requested review from yannickstruyf3 and removed request for tuxtof and yannickstruyf3 April 23, 2024 20:31
Copy link
Contributor

@adiantum adiantum left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@thunderboltsid thunderboltsid merged commit 03c61d3 into main Apr 24, 2024
11 of 13 checks passed
@thunderboltsid thunderboltsid deleted the jira/krbn-8096 branch April 24, 2024 14:25
@nutanix-cn-prow-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adiantum, dkoshkin, thunderboltsid

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:
  • OWNERS [adiantum,thunderboltsid]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

thunderboltsid added a commit that referenced this pull request Apr 24, 2024
This will ensure we make fewer basic auth requests to Prism Central
IAM Services.
thunderboltsid added a commit that referenced this pull request Apr 29, 2024
This will ensure we make fewer basic auth requests to Prism Central
IAM Services.
thunderboltsid added a commit that referenced this pull request May 2, 2024
This will ensure we make fewer basic auth requests to Prism Central
IAM Services.
thunderboltsid added a commit that referenced this pull request May 2, 2024
* fix: improve error handling for Prism Client (#354)

* fix: use wrapper errors to clearly denote issues in client building

* fix: adds a function to properly sanitize the address

* fix: adds tests for ip address case given

* fix: uses a defined type for port error

* fix: clean up variable naming

* fix: remove validation here to be moved into prism-client

* refactor: task status file (#355)

* test: add unit tests for pkg/client/state

* refactor: use wait.Poll function waiting for task state

* refactor: use consistent task status names

* fixup! test: add unit tests for pkg/client/state

* fix: revert to previous behaviod polling forever

The ctx passed into WaitForTaskToSucceed is only used to cancel HTTP reqests and not to cancel the wait.

* chore: add license headers

* fix: better function name

* refactor: client.go file helper methods (#360)

* refactor: client.go file helper methods

Refactored the existing methods and functions to be unit testable.
Also made some methods that do not use the struct as generic functions.
The changes were primarily an effort to add unit test coverage.

* refactor: more testable read file function

* test: new nutanixcluster types unit tests

* test: additional test cases for errors

* fixup! refactor: client.go file helper methods

* fixup! refactor: client.go file helper methods

* fixup! refactor: more testable read file function

* fixup! refactor: client.go file helper methods

* Switch Nutanix Client to using Session Auth (#398)

This will ensure we make fewer basic auth requests to Prism Central
IAM Services.

---------

Co-authored-by: Faiq <faiq.raza@nutanix.com>
Co-authored-by: Dimitri Koshkin <dimitri.koshkin@nutanix.com>
thunderboltsid added a commit that referenced this pull request May 10, 2024
This will ensure we make fewer basic auth requests to Prism Central
IAM Services.
thunderboltsid added a commit that referenced this pull request May 14, 2024
This will ensure we make fewer basic auth requests to Prism Central
IAM Services.
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.

5 participants