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

Implement inband authentication #879

Closed
wants to merge 0 commits into from
Closed

Conversation

gbregman
Copy link
Contributor

@gbregman gbregman commented Oct 2, 2024

Implement inband authentication

@gbregman gbregman self-assigned this Oct 2, 2024
@gbregman gbregman requested review from baum and oritwas October 2, 2024 11:46
control/grpc.py Outdated Show resolved Hide resolved
control/grpc.py Outdated Show resolved Hide resolved
control/grpc.py Outdated Show resolved Hide resolved
@gbregman gbregman force-pushed the devel branch 22 times, most recently from 8c0fba4 to d5e87d2 Compare October 7, 2024 12:16
@baum
Copy link
Collaborator

baum commented Oct 7, 2024

@gbregman looks great! A couple of thoughts on cosmetics and code organization of demo test variants in build-container.yml:

In .github/workflows/build-container.yml, we could define a demo matrix with three tests:

  • 'demo',
  • 'demosecuredhchap'
  • 'demosecurepsk'

See pytest, ha matrices for instance.

For the Makefile:

  • We could move key management code (e.g., /tmp/temp-psk/, /tmp/temp-dhcha/) out of build-container.yml into the appropriate make targets.
  • We could move test logic (e.g., "verify connection list") out of build-container.yml into the appropriate make targets.
  • The "Run bdevperf" step could be a Makefile target, e.g., make demo_bdevperf, demosecuredhchap_bdevperf, demosecurepsk_bdevperf.

The goal is to keep the GitHub workflow YAML definition lean, clean, and maintainable for the future.

wdyt?

.github/workflows/build-container.yml Outdated Show resolved Hide resolved
.github/workflows/build-container.yml Outdated Show resolved Hide resolved
@gbregman
Copy link
Contributor Author

gbregman commented Oct 7, 2024

@baum we can't have the key files code in the make as this is done in the bdevperf container, not the gateway. The way it works is that the initiator (in this case bdevperf) needs to maintain the key files and keyring on his side. While the gateway code takes care of the target side. About the connection, it might be possible to move it I just followed what we had before. I don't see why it matters.

@gbregman gbregman requested review from baum and epuertat and removed request for oritwas October 7, 2024 12:39
@gbregman gbregman force-pushed the devel branch 3 times, most recently from 73806a8 to 0e400aa Compare October 7, 2024 16:40
control/grpc.py Outdated Show resolved Hide resolved
control/grpc.py Outdated Show resolved Hide resolved
control/grpc.py Outdated Show resolved Hide resolved
control/grpc.py Outdated Show resolved Hide resolved
@gbregman
Copy link
Contributor Author

gbregman commented Oct 8, 2024

@baum we can change the .yml file, but if we decide to do so I prefer to do it in a separate PR. I don't want to pack too many changes to one PR.

Copy link
Collaborator

@baum baum left a comment

Choose a reason for hiding this comment

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

Could you please address the test code issue before merging? Let me know if you need any assistance. Thank you!

@gbregman
Copy link
Contributor Author

gbregman commented Oct 8, 2024

Could you please address the test code issue before merging? Let me know if you need any assistance. Thank you!

I already said that I prefer to do it in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants