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

Problems with charts with spaces in their name #3758

Closed
antgamdia opened this issue Nov 15, 2021 · 3 comments · Fixed by #3863 or #3998
Closed

Problems with charts with spaces in their name #3758

antgamdia opened this issue Nov 15, 2021 · 3 comments · Fixed by #3863 or #3998
Assignees
Labels
component/asset-syncer Issue related to kubeapps asset-syncer good first issue kind/bug An issue that reports a defect in an existing feature
Projects

Comments

@antgamdia
Copy link
Contributor

Description:

Packages including spaces as part of their names cannot be displayed in Kubeapps (well, the pkg details view at least).

Steps to reproduce the issue:

  1. Add a package with spaces in its name. For example, using Helm and Harbor:
  2. Go to https://demo.goharbor.io/harbor/projects an create a new project, e.g., kubeapps
  3. Create a new Helm chart, e.g., running helm create demo, then edit the Chart.yaml file to sth like name: foo bar
  4. Package the chart with helm package demo
  5. Upload the foo bar.tgz to the Harbor instance
  6. In Kubeapps, add a new repository: https://demo.goharbor.io/chartrepo/kubeapps
  7. Verify the package is being listed (pic below)
  8. image
  9. Click on the pkg to access its details and here's the error (next pic)

Describe the results you received:

image

Describe the results you expected:

Details should be displayed, as usual.

Additional information you deem important (e.g. issue happens only occasionally):

It is probably due to a missing encoding of the spaces (%20 instead of ), as we are actually doing with the / (%2F).
I did face an issue with encoding slashes in the past, so do not hesitate to ping me for further details.

Version of Helm, Kubeapps and Kubernetes:

N/A (current devel version)

@antgamdia antgamdia added kind/bug An issue that reports a defect in an existing feature good first issue priority/medium component/ui Issue related to kubeapps UI labels Nov 15, 2021
@antgamdia antgamdia added this to Inbox in Kubeapps Nov 15, 2021
@antgamdia antgamdia moved this from Inbox to Next iteration discussion in Kubeapps Nov 22, 2021
@ppbaena ppbaena moved this from Next iteration discussion to Committed in Kubeapps Nov 22, 2021
@castelblanque castelblanque moved this from Committed to In progress in Kubeapps Nov 29, 2021
@castelblanque
Copy link
Collaborator

Problem seems to be in the backend, not the dashboard.

Asset-syncer currently stores the charts with slashes encoded, but spaces unencoded. At least for Helm + Harbor here.
After syncing a chart with spaces in name, database shows for charts.chart_id something like
kubeapps-harbor-demo/really%20foo%20bar

Helm specifies that chart names must only contain alphanumeric characters, underscore, dash or dot. See Helm general conventions.
When trying to install directly with Helm CLI a chart with spaces in name, the following error is thrown:

Error: INSTALLATION FAILED: failed pre-install: warning: Hook pre-install chart with spaces/templates/pre-install-hook.yaml failed: Job.batch "test-a" is invalid: [metadata.labels: Invalid value: "chart with spaces-0.1.2": a valid label must be an empty string or consist of alphanumeric characters, '-', '' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9.])?[A-Za-z0-9])?'), spec.template.labels: Invalid value: "chart with spaces-0.1.2": a valid label must be an empty string or consist of alphanumeric characters, '-', '' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9.])?[A-Za-z0-9])?')]

Same error message appears in Kubeapps dashboard when chart name is synced having no %20:
image

Therefore, I suggest two possibilities:

  • Allow charts that contain spaces in a repo. We should then convert the spaces to - (as Helm recommends) whenever we treat chart data (e.g. before storing the asset in the DB when syncing). Tricky option, as the actual "external" name of the chart would still contain spaces, but inside kubeapps would be converted to using dashes.
  • Ignore charts that contain spaces in name. Filter them out automatically, or marked them somehow as "incompatible".

I don't think either option would mean backwards compatibility issues, as existing charts would not be affected, and charts with spaces don't work currently.

@castelblanque castelblanque self-assigned this Nov 29, 2021
@antgamdia
Copy link
Contributor Author

Thanks for the report!

Yep, that's true, Helm does not allow installing charts with spaces as part of their names. I personally think that rendering the Helm error output is ok, as it is a plugin-specific concern.
We had a similar problem in the past (with the slashes /) and we solved it by encoding end-2-end (/->%2F), so I guess the spaces can be encoded to %20 as you pointed out.

My concerns are twofold:

  • The main problem I found is that the error I described unable to fetch package: ... is thrown from kubeapps itself (I mean, just performing GET-alike actions), so perhaps it is not the best UX we want.
  • Although unlikely, what if another plugin does allow installing stuff with spaces?

Dropping some initial ideas/thoughts:

  • Is it possible for us to fetch the package details for a package with spaces? I'd give it a try at least.
  • If we are sure we cannot install packages with spaces, I'd disable the deploy button? Not sure if it's the best UX, perhaps throwing the plugin error is ok, I don't know...

@castelblanque
Copy link
Collaborator

The main problem I found is that the error I described unable to fetch package: ... is thrown from kubeapps itself (I mean, just performing GET-alike actions), so perhaps it is not the best UX we want.

@antgamdia This is due to the frontend sending correctly the ID with decoded spaces (not %20), but the ID in the database is stored encoded with %20. Therefore no record is found when SELECT.

Probably the cleanest solution by now is something like:

  • Storing charts data correctly into db. Meaning with spaces and not %20. This will allow to get to the package details page in the frontend (no need to do changes in Dashboard)
  • Show Helm's error in Dashboard when installing charts with spaces in name. If other plugins allow packages with spaces in name/Id, this approach should work.

This means that we allow to list packages with spaces, and also to click "Deploy" in Dashboard. In those cases, we throw the error from Helm.

castelblanque pushed a commit that referenced this issue Nov 30, 2021
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@castelblanque castelblanque added component/asset-syncer Issue related to kubeapps asset-syncer and removed component/ui Issue related to kubeapps UI labels Nov 30, 2021
@castelblanque castelblanque moved this from In progress to Waiting For Review in Kubeapps Nov 30, 2021
castelblanque pushed a commit that referenced this issue Dec 1, 2021
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Kubeapps automation moved this from Waiting For Review to Done Dec 1, 2021
castelblanque added a commit that referenced this issue Dec 1, 2021
* Allow to sync and store charts with spaces in name (#3758)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Fixed typo (#3758)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
castelblanque pushed a commit that referenced this issue Dec 24, 2021
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
castelblanque added a commit that referenced this issue Jan 3, 2022
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/asset-syncer Issue related to kubeapps asset-syncer good first issue kind/bug An issue that reports a defect in an existing feature
Projects
No open projects
Kubeapps
  
Done
2 participants