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

Updating SkupperPodmanSite.Status to check if site.AuthMode is internal. #1453

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

Conversation

Karen-Schoener
Copy link
Collaborator

Fixes #1389

@Karen-Schoener
Copy link
Collaborator Author

Please see below for manual test results. Tests ran on fedora.

Test case: configure console, --console-auth unsecured:

$ skupper --platform podman init  --enable-console --console-auth unsecured  --enable-flow-collector 

$ ./skupper --platform podman status
Skupper is enabled for "kschoener" with site name "fedora-kschoener-e2de0". It is not connected to any other sites. It has no exposed services.
The site console url is:  https://0.0.0.0:8010

Test case: configure console with internal auth:

$ skupper --platform podman init --enable-console --enable-flow-collector

$ ./skupper --platform podman status
Skupper is enabled for "kschoener" with site name "fedora-kschoener-e7c35". It is not connected to any other sites. It has no exposed services.
The site console url is:  https://0.0.0.0:8010
The credentials for internal console-auth mode are held in podman volume: 'skupper-console-users'

@Karen-Schoener Karen-Schoener force-pushed the fix_1389_unsecured_podman_site_with_console_reports_credentials branch from 207b873 to e56f82d Compare April 23, 2024 19:44
Copy link
Member

@fgiorgetti fgiorgetti left a comment

Choose a reason for hiding this comment

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

I noticed that there are two spaces after The site console url is: (this is not related to your change, but it would be nice to have it fixed as well).

There is also an extra line being displayed at the end.
Do you mind removing it?

@Karen-Schoener Karen-Schoener force-pushed the fix_1389_unsecured_podman_site_with_console_reports_credentials branch from e56f82d to 5d04831 Compare April 24, 2024 14:38
@Karen-Schoener
Copy link
Collaborator Author

I noticed that there are two spaces after The site console url is: (this is not related to your change, but it would be nice to have it fixed as well).

There is also an extra line being displayed at the end. Do you mind removing it?

Thanks for the review.

Updated with the suggestions. Hope it's ok that I used fmt.Printf("...\n") instead of fmt.Println().

Re-ran test case by hand: configure console, --console-auth unsecured:

[kschoener@fedora skupper]$ ./skupper --platform podman status
Skupper is enabled for "kschoener" with site name "fedora-kschoener-1468e". It is not connected to any other sites. It has no exposed services.
The site console url is: https://0.0.0.0:8010
[kschoener@fedora skupper]$

Re-ran test case by hand: configure console with internal auth:

[kschoener@fedora skupper]$ ./skupper --platform podman status
Skupper is enabled for "kschoener" with site name "fedora-kschoener-0edbc". It is not connected to any other sites. It has no exposed services.
The site console url is: https://0.0.0.0:8010
The credentials for internal console-auth mode are held in podman volume: 'skupper-console-users'
[kschoener@fedora skupper]$

@Karen-Schoener
Copy link
Collaborator Author

Out of curiosity, I was wondering if this first line of skupper status should be split into 3 separate lines?

Skupper is enabled for "kschoener" with site name "fedora-kschoener-0edbc". It is not connected to any other sites. It has no exposed services.

Just thought I'd double-check...

Planning to leave as-is unless there's feedback.

@nluaces
Copy link
Member

nluaces commented Apr 24, 2024

@Karen-Schoener thanks for your pull request!

Out of curiosity, I was wondering if this first line of skupper status should be split into 3 separate lines?

At present it would affect all the tests that check the output of the skupper status, I would not recommend it for now.

@Karen-Schoener Karen-Schoener force-pushed the fix_1389_unsecured_podman_site_with_console_reports_credentials branch from 5d04831 to e1e6a05 Compare May 2, 2024 18:51
@Karen-Schoener Karen-Schoener force-pushed the fix_1389_unsecured_podman_site_with_console_reports_credentials branch from e1e6a05 to 80902cf Compare May 2, 2024 19:46
@Karen-Schoener
Copy link
Collaborator Author

Karen-Schoener commented May 2, 2024

Rebased with the the latest in skupper/main.

@Karen-Schoener
Copy link
Collaborator Author

I believe that the review comments have been addressed.

@fgiorgetti when time allows, would you mind letting me know if the PR looks ok? Thanks in advance, Karen

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.

unsecured podman site with console reports credentials
4 participants