dashboard icon indicating copy to clipboard operation
dashboard copied to clipboard

[BUG] Unable to create repository with SSH key authentication: "o is undefined"

Open lindhe opened this issue 1 year ago • 1 comments

Rancher Server Setup

  • Rancher version: v2.8.1
  • Installation option (Docker install/Helm Chart): Helm Chart
    • If Helm Chart, Kubernetes Cluster and version (RKE1, RKE2, k3s, EKS, etc): RKE2
  • Proxy/Cert Details: None

Information about the Cluster

  • Kubernetes version: v1.26.12+rke2r1
  • Cluster Type (Local/Downstream): Local

User Information

  • What is the role of the user logged in? Admin

Describe the bug

When I go to "Cluster Management > Advanced > Repositories" and try to add a new repo with a new SSH key for authentication, the creation fails and displays "o is undefined".

To Reproduce

  1. Go to "Cluster Management > Advanced > Repositories".
  2. Click "Create".
  3. Fill in repo info and choose "Create a new SSH Key Secret" under "Authentication".
  4. Fill in the public and private key info.
  5. Click "Create".

Result

o is undefined

Expected Result

Successful creation of the SSH key and adding the repo.

Screenshots

Screenshot 2024-02-02 075606

(relax, it's not my real key… 😉)

Screenshot 2024-02-02 075638

Additional context

I had the same issue on v2.7.6 and just upgraded to v2.8.1 hoping it would go away.

lindhe avatar Feb 02 '24 07:02 lindhe

Thanks @lindhe, I could replicate it in v2.8.1 as well. The workaroud would be to create the repository with empty authentication, and then edit the YAML file to add them.

torchiaf avatar Feb 02 '24 17:02 torchiaf

e2e test coverage LGTM however it might be worth testing this with legit SSH keys. moving to 'To Test' @izaac

yonasberhe23 avatar Mar 25 '24 19:03 yonasberhe23

We can test manually with a couple of encryption algos to be sure. RSA and other more modern.

izaac avatar Mar 26 '24 17:03 izaac

If we can include ed25519 in the test, that would be good for me. :)

lindhe avatar Mar 26 '24 20:03 lindhe

@izaac @yonasberhe23 @lindhe Thank you for reviewing the tests, I'm trying to understand why we would want to use real SSH keys or fake SSH key that follows the ed25519 standard? we're not verifying the active/successful state of the repo post-creation, we just check for the element to be there and then we delete the repo. Same applies for the basic auth case. Could you please provide further clarification?

momesgin avatar Apr 18 '24 01:04 momesgin

@momesgin sure. As I understand the expected behavior in this case is to create a working repository.

UI -> API -> ssh client/library -> repo with key auth

As part of the dev fix validation checking the UI -> API I think it would suffice.

On the other hand E2E we should make sure this feature is actually working after the fix. This require to at least validate the authentication is successful. This will confirm the fix works.

We've ran into these ssh authentication issues in the past where configuring these repos using kubectl worked but not from the UI and this can be only noticed on the repository status.

Here is an example of an issue: https://github.com/rancher/dashboard/issues/7746

In this case I worked on this issue in the past and I had different behavior with different algorithms for some reason. And that was working from kubectl

The E2E of this scenario is not too complex but we need a couple ssh keys using like rsa and ed25519. They don't necessarily need to be those, but those are very popular. In case this results in a backend issue after the E2E test we can open another for follow up on rancher/rancher

izaac avatar Apr 18 '24 01:04 izaac

@momesgin sure. As I understand the expected behavior in this case is to create a working repository.

UI -> API -> ssh client/library -> repo with key auth

As part of the dev fix validation checking the UI -> API I think it would suffice.

On the other hand E2E we should make sure this feature is actually working after the fix. This require to at least validate the authentication is successful. This will confirm the fix works.

We've ran into these ssh authentication issues in the past where configuring these repos using kubectl worked but not from the UI and this can be only noticed on the repository status.

Here is an example of an issue: #7746

In this case I worked on this issue in the past and I had different behavior with different algorithms for some reason. And that was working from kubectl

The E2E of this scenario is not too complex but we need a couple ssh keys using like rsa and ed25519. They don't necessarily need to be those, but those are very popular. In case this results in a backend issue after the E2E test we can open another for follow up on rancher/rancher

As I understand the bug is about the creation process being broken. Based on the current UX/UI design, the repo creation page allows the user to add any urls(e.g. https://github.com/your-company/charts.git), and for the SSH case only checks that the private key field is not empty(no further validation). Then it redirects the user to the list page, and if there's an issue with the created repo it'll be reflected on the UI by displaying the error status. So even if the repo is not working the UI still behaves correctly.

momesgin avatar Apr 18 '24 03:04 momesgin

@izaac @yonasberhe23 @bmdepesa I feel the automated tests here cover the bug that has been fixed here and will prevent regression.

The ask seems to be to add e2e tests that validate the underlying backend functionality - that has always been there and without automated tests.

I agree that ultimately we would want tests for this as an overall feature - but for this bug fix, it feels like the tests should be enough to not require manual test

nwmac avatar Apr 18 '24 16:04 nwmac

@nwmac agreed. I will add a QA task to the backlog for tracking

izaac avatar Apr 18 '24 16:04 izaac

@nwmac let's bring this up on the next sync up. We should be covering E2E like on provisioning.

Originally tests were only using Custer objects in the UI for Imported and Custom types. But now we have those with real clusters.

This should be the case for all features.

izaac avatar Apr 18 '24 16:04 izaac