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

TypeError reading 'forEach' #615

Open
jrbe228 opened this issue Oct 7, 2024 · 34 comments
Open

TypeError reading 'forEach' #615

jrbe228 opened this issue Oct 7, 2024 · 34 comments
Assignees
Labels
documentation Improvements or additions to documentation UI Update on the UI

Comments

@jrbe228
Copy link

jrbe228 commented Oct 7, 2024

Describe the bug
I'm unable to create modules from the Helm chart for Argo Workflows. Trying a popular chart like Argo was a debugging step after seeing the same error from various charts in private repos.

To Reproduce
Steps to reproduce the behavior:

  1. In CyclopsUI, go to 'Templates'
  2. Click on 'Add template reference'
  3. Enter the following:
Repository URL:  https://github.com/argoproj/argo-helm
Path:  charts/argo-workflows
  1. Attempt to create a module from this template. See error.

Expected behavior
No errors. Template options displayed for Argo Workflows.

Screenshots
Resulting error:

TypeError: Cannot read properties of null (reading 'forEach')
Check if Cyclops backend is available on: http://cyclops-ctrl.cyclops:8080

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser: Edge
  • Version: 129.0.2792.79
  • CyclopsUI version: v0.13.1
  • Kubernetes distro: K3s installed with Rancher Desktop

Additional Context
On the bright side, the new private templates feature works as expected.

@petar-cvit
Copy link
Collaborator

Hey @jrbe228, thanks for raising this!

Cyclops renders UIs based on the helm chart schema in values.schema.json. You are seeing this issue on argo workflows chart because those charts do not contain the mentioned file.

There are tools that will allow you to generate the schema from your helm charts -> https://github.com/dadav/helm-schema
Cerbos and Grafana K6 use it, and we find it straightforward and simple to use. It will read your values.yaml and generate values.schema.json based on it. You can then push the schema json along with your chart, and you should be able to use your UIs.

Let us know if the issue persists after you add the values.schema.json in case we need to dive deeper.

Nonetheless, we will update the error message so it is easier to determine the cause of the error.

@petar-cvit petar-cvit self-assigned this Oct 7, 2024
@petar-cvit
Copy link
Collaborator

@jrbe228, we added a short section to our docs on how to add a helm schema here.

We fixed the error you got, and Cyclops will now allow you to install Helm charts that do not contain the schema.

Also, we added a new warning banner when a template without a schema is used. It links to Helm docs about schema files and our newly added docs on schema generation. It should help future users mitigate the issue you had. This banner will be included from version v0.14.1

Screenshot 2024-10-10 at 12 39 51

@petar-cvit petar-cvit added documentation Improvements or additions to documentation UI Update on the UI labels Oct 10, 2024
@jrbe228
Copy link
Author

jrbe228 commented Oct 12, 2024

@petar-cvit
Thank you for the quick response! I should have feedback later this month.

@ghorvathansys
Here is one issue which may be resolved now. I need to re-test after returning from Europe.

@jrbe228
Copy link
Author

jrbe228 commented Oct 20, 2024

We fixed the error you got.

Confirmed on my side. Now TypeError... is replaced by warning banner in your screenshot.

Cyclops will now allow you to install Helm charts that do not contain the schema.

This is a great feature! Quick sanity testing by deploying any public repo and with the default Helm values. Working for me with ArgoWF.

There is one more related feature critical for our use cases. When I deploy the ArgoWF Helm release from CyclopsUI's module page, everything works. The module is displayed:

image

However when I deploy the ArgoWF Helm release without using CyclopsUI, using simple command line instead:
helm repo add argo https://argoproj.github.io/argo-helm
helm install my-release argo/argo-workflows

Nothing is displayed in the CyclopsUI dashboard. Do I need some annotation for the release to be recognized? My apologies if this feature is already explained in the docs.

This feature is valuable for our compatibility with legacy systems. We have many existing pipelines which deploy Helm releases. During deployment, the pipelines inject Helm data from internal company servers. So it's difficult to convert this process to a Cyclops format quickly. We hope to continue using the legacy pipelines for Helm release deployment. Then use Cyclops for lifecycle management once the release is deployed.

@petar-cvit
Copy link
Collaborator

Hey @jrbe228,

Cyclops currently shows only modules, which are a Cyclops CRD, that reference a Helm chart and values to be used with the chart. It doesn't show existing Helm releases and their resources in the cluster.

I see two possible solutions for managing Helm releases through Cyclops:

  1. Implement the same support for Helm releases as we have for Cyclops modules
  2. Show all existing Helm releases and support easy migration from an existing Helm release to a Cyclops Module. The migration would be done through the UI or the cli and would retain all existing values and resources

I am more in favor of the second option since we wanted to implement something like this a while back, but I would love to hear what you think.

Let me know which option sounds best for your case!

@jrbe228
Copy link
Author

jrbe228 commented Oct 21, 2024

Either option would be an improvement! Hopefully we can get input from the community also. I am admittedly biased towards convenience for our use cases. To give more context from our side, we see apps progressing as follows:

  • Initial development in a local container
  • Intermediate development on transient Kubernetes releases, managed via Cyclops
  • Advanced (staging / pre-production) development on persistent Kubernetes releases, managed via ArgoCD

@petar-cvit
Copy link
Collaborator

petar-cvit commented Oct 21, 2024

@jrbe228 I implemented a first draft of what the Helm chart management would look like and attached a screen recording.
We are considering adding a new tab on the sidebar with all the Helm releases detected in the cluster. For each release, you could see its name and namespace and its chart.

When you select one release, you can check the resources deployed from it. From there, you could edit the release directly and apply updated resources or migrate the helm chart to a Cyclops module with a couple of clicks, giving you a more robust solution through our Module CRD.

Cyclops will also pick up all newly installed helm releases.

cyclops-helm-management.mp4

Regarding the input from the community, you can join our discord and start a thread there. Let me know if something like this would help your use case!

@jrbe228
Copy link
Author

jrbe228 commented Oct 21, 2024

Thread started on the Cyclops Discord channel.

My first impression is the new tab looks very useful.

  • Migration to Cyclops Module is an option, but no longer a requirement for visibility.
  • I guess advanced features like "rollback" can be accessed after migration to Modules. These are less relevant for our use cases with short lifecycles.
  • Having a "Delete" option for releases would be convenient. But ok if migration is required first.
  • I'm thinking how we would expose critical info from specific resources, such as a Service external IP.

@petar-cvit
Copy link
Collaborator

petar-cvit commented Oct 22, 2024

I guess advanced features like "rollback" can be accessed after migration to Modules

@jrbe228 my thoughts as well. We could implement editing releases directly, but you can migrate to modules if you need more capabilities.

We can add the delete release option, no problem 👍

Also, we can add external IPs to the Service view or any other field you think might be useful. In the long run, we are also thinking of implementing a dynamic view of the resources so anybody can configure how their resources are visualized or do it through plugins. For now, you can suggest any fields you would like to see in the UI, and we can add them.

@jrbe228
Copy link
Author

jrbe228 commented Oct 22, 2024

At the moment, Service external IP is the only critical info to add. Ports are already summarized nicely:

image

I see Cyclops already has Auth and RBAC on the roadmap. We would benefit from these features in the long term. However the platform is usable without them, assuming we trust our Devs to be responsible 😂

@petar-cvit
Copy link
Collaborator

@jrbe228, yeah, RBAC is in the works! There is already an open PR for it we are still working on - #361

We added an external IPs section to the service, which gets all the IPs (and hostnames) from .status.loadBalancer.ingress of the service, along with IPs from .spec.externalIPs.

Screenshot 2024-10-23 at 14 27 51

I will create a release candidate version with external IPs and Helm release management, which you can play around with. I will notify you once the rc is built and how to install it.

I would love to hear if it would help your use case and if there is anything else you would like to see included 😄

@petar-cvit petar-cvit mentioned this issue Oct 24, 2024
2 tasks
@petar-cvit
Copy link
Collaborator

Hey @jrbe228, as promised, we made a release candidate v0.15.0-rc.3 🔥

You can install it with the following command:

kubectl apply -f https://raw.githubusercontent.com/cyclops-ui/cyclops/v0.15.0-rc.3/install/cyclops-install.yaml && 
kubectl apply -f https://raw.githubusercontent.com/cyclops-ui/cyclops/v0.15.0-rc.3/install/demo-templates.yaml

With this rc, you should be able to edit the values of your releases through the UI and delete releases. The UI will render if your installed helm charts contain the values.schema.json, if not, you will get the same warning we mentioned above. You can also find external IPs sections on your services :)

Currently, the rc fetches your releases from secrets, which is Helm's default behavior. If you are storing your releases through some other Helm driver, let me know, and we can support it.

Also, not to break your K8s API, for the rc version, we limit which kinds are fetched when you check release details (image below). Here you can find a list of resource kinds that are displayed.

Screenshot 2024-10-24 at 20 15 30

Let me know if you encounter any issues so we can add this to the next main release

@jrbe228
Copy link
Author

jrbe228 commented Oct 27, 2024

A few initial observations.

  1. Overall everything works as expected. Non-Cyclops Helm releases are discovered. Service IPs are displayed. Cyclops is fully usable for our purposes in the current state. Any other changes would be optimizing the experience.

image

  1. Our primary use case is managing legacy Helm releases. These already exist in specific namespaces on a large multi-tenant cluster. Ideally we would like Cyclops to show releases from these namespaces, but not from other namespaces. Probably not from kube-system😄

image

  1. If simpler on your side, you could consider an installer flag for 1-to-1 namespace isolation. So if I supply --namespace=hpc01, then Cyclops installs in "hpc01" and only shows Helm releases from "hpc01". This may also be a workaround for the missing Auth / RBAC features. Limiting Cyclops actions to 1 namespace should prevent users from doing anything destructive to the cluster. Also it could greatly reduce the service account permissions required for Cyclops installation. Currently the service account needs many cluster-scope permissions, which our IT admins prefer to avoid.

Note that 2) and 3) are somewhat related to this issue and this issue.

@petar-cvit
Copy link
Collaborator

petar-cvit commented Oct 27, 2024

@jrbe228, thanks for the update!

Regarding points 2 and 3, we could scope Cyclops to a single namespace through an environment variable. We already have similar logic to scope Modules reconciler to updates from a specific namespace. I'm keen to add a separate env variable from the one controlling the reconcile, something like WATCH_NAMESPACE_HELM.

I agree with the point that the service account has too many permissions in our install yaml. We purposely made it that way so anything can be deployed through Cyclops, but once we implement the scoping of Helm releases, you can limit permissions to only a single namespace :)

We will include Helm release management in the next release (v0.15.0) and implement the namespace scoping through environment variables.

@jrbe228
Copy link
Author

jrbe228 commented Nov 1, 2024

@petar-cvit - looking forward to testing v0.15.0+ next week! Regarding limited install permissions, could you update the docs with specifics on what K8s SA config should work for a single namespace? I see the WATCH_NAMESPACE and WATCH_NAMESPACE_HELM variables in .env. Our IT group is asking how to configure a minimal service account which will be sufficient for Cyclops.

@petar-cvit
Copy link
Collaborator

Sure thing, @jrbe228. Will update the docs with a new page for the namespace scope, including env variables and permissions.

Since you will be using Cyclops for Helm releases, I was thinking about adding an environment variable that would disable the Module reconciler so you can't accidentally deploy a new app

@jrbe228
Copy link
Author

jrbe228 commented Nov 4, 2024

I think you can leave the Module reconciler running. Deploying a new app is actually useful in either of these namespaces - WATCH_NAMESPACE or WATCH_NAMESPACE_HELM. Over time, we may migrate legacy Helm releases to deployment via Cyclops. The only concern is cluster-wide permissions, which should be solved soon. We only need to prevent Cyclops from deploying new apps to arbitrary namespaces.

@petar-cvit
Copy link
Collaborator

I think you can leave the Module reconciler running

Thanks for the feedback. We won't implement it for now

We only need to prevent Cyclops from deploying new apps to arbitrary namespaces

Still tweaking some things that were queried on cluster scope, and we will get back to you with Cyclops configuration and Roles/Service accounts for single namespace setup ASAP

@petar-cvit
Copy link
Collaborator

Hey @jrbe228, we released v0.15.2 with all the necessary changes for Cyclops to work namespaced scoped.
Regarding the service account permissions, Cyclops can now work with a Role that has full access to a single namespace and that namespace alone.

We updated our helm chart so you can configure the Role, Role Binding, and all the Cyclops controller env variables to scope it to a single namespace.

There is also a new page in our docs on how to limit Cyclops to a namespace here.

TL;DR You can generate all the resources for a single namespace with the following command

helm template cyclops oci://registry-1.docker.io/cyclopsui/cyclops-chart \
--namespace <your-namespace> \
--include-crds \
--set global.singleNamespaceScope.enabled=true \
--set global.singleNamespaceScope.namespace=<your-namespace>

Let me know if permissions configured like this are ok with you and if Cyclops is working as expected on your end 😄

@jrbe228
Copy link
Author

jrbe228 commented Nov 9, 2024

My initial reaction - the core capabilities work. I can install to the cyclops namespace, then monitor various other namespaces for Helm releases or Cyclops modules. Just add the appropriate Role + RoleBinding, and everything shows up.

I did notice one issue (likely caused by my config) with resource discovery. Helm releases show no resources when I deploy in narrow-namespace mode. I will let you know if I find out more info.

image

@petar-cvit
Copy link
Collaborator

@jrbe228 thanks for the feedback!

For the resource discovery on releases, Cyclops lists all of the resources with the following label values:

  • app.kubernetes.io/instance=<name of the release>
  • app.kubernetes.io/managed-by=Helm

Check if your Helm chart adds those labels. We might make the selector labels configurable so that they are not the only ones used to find resources.

I see the release name contains testpod. Does it contain only a single Pod? If yes, Cyclops currently fetches resources only from these GVRs, and it does not fetch pods as a single resource, but only as part of Deployments/StatefulSets/DaemonSets.

You can also check if your resources (Deployments, Services...) are deployed in the correct namespace. When Cyclops is namespaced scoped, it fetches stuff only from the specified namespace.

Let us know if any of these fix your problem, and we can patch it up on our end

@jrbe228
Copy link
Author

jrbe228 commented Nov 9, 2024

Bad example 😄 Isolated Pods would indeed be ignored by Cyclops. Moving on to an actual Helm release in the cluster, this Service format is a little different than Cyclops is expecting:

metadata:
  annotations:
    meta.helm.sh/release-name: smastora-rocky810-smastora-1149
    meta.helm.sh/release-namespace: dev-container
    ...
  labels:
    app.kubernetes.io/managed-by: Helm
    ...

We could probably add labels on our side. I'm just wondering what makes sense in general. Would other Cyclops users be likely to have the same annotation meta.helm.sh/release-name... instead of a label? Maybe caused by different versions of Helm during installation.

Completely separate question.
For TemplateAuthRule objects, is it possible to list multiple Repos or an Org?
https://cyclops-ui.com/docs/templates/private_templates

@petar-cvit
Copy link
Collaborator

Isolated Pods would indeed be ignored by Cyclops

Would this be a blocker for you, or can you use Deployments or something else?

Regarding labels, we decided to query based on the app.kubernetes.io/instance since it is one of Helm's standard labels. We tested this with bitnami and a bunch of other popular charts, and it worked fine, so other users shouldn't be affected.

For TemplateAuthRule objects, is it possible to list multiple Repos or an Org?

This isn't documented (sorry about that, we will add it to the docs), but you can just set the repo to https://github.com/<your-org>/.*, and it will inject the credentials to any template request that matches the regex.

@jrbe228
Copy link
Author

jrbe228 commented Nov 9, 2024

No need for isolated pod discovery. That was an irrelevant example. Everything we need Cyclops to manage is composed of Deployment / Service / etc.

About missing the standard labels, it looks like our charts need them added. Will try that soon.

Great to know about the TemplateAuthRule regex feature!

@jrbe228
Copy link
Author

jrbe228 commented Nov 11, 2024

It seems we have an unintentional feature 😄 The label app.kubernetes.io/instance was missing from many of our Helm chart templates. Some template objects we prefer to keep hidden. Now I can selectively add the label only for objects (like Services) which should be visible to Cyclops users.

@jrbe228
Copy link
Author

jrbe228 commented Nov 11, 2024

Unrelated to other items on this issue...

  1. Since private repos are now supported, this page of the docs can be updated -

image

  1. Usage of the "Load values from file" feature is unclear to me:

image

Initially I tried to supply a local Windows file path, which gave the above error. Maybe only Unix file paths are supported? Or maybe it should be a Git repo URL. Also I tried to copy / paste values directly into the text field. These values did not seem to be loaded.

@petar-cvit
Copy link
Collaborator

petar-cvit commented Nov 12, 2024

@jrbe228, thanks for the feedback on the docs. We will remove the warning!

If you want to load a YAML file into the form, you need to provide a URL to a YAML file. For example, if you have a file on GitHub that you would like to use, you can provide a URL to the raw version of it (e.g. https://raw.githubusercontent.com/cyclops-ui/templates/refs/heads/main/demo/values.yaml). You currently can't import values from local files.

We talked about removing this feature altogether since it is not that widely used and comes from a time before we introduced templates as a concept. We found some issues with this feature, and it adds a lot of complexity to the values flow. One issue is that even though your Cyclops instance can access a private repo, this feature cannot fetch your values from that same repo. You can also encounter problems when the template and the values mismatch, so I would not recommend using this feature.

If you would like to use this feature, let us know, and we will try to revamp it. If not, we will likely remove it in the next version.

@jrbe228
Copy link
Author

jrbe228 commented Nov 12, 2024

Copy/pasting raw text to the values importer could be nice for quick testing (but not super useful in general). Copy/paste would avoid the private repo limitation and the local file limitation. Lack of private repo support would generally block us from using the YAML import feature. Most of our repos are private / internal.

Generally each Helm chart has many Values files for users to select. Each Values.yaml has defaults for the specific cluster or specific use case:

image

So I am considering how to handle these defaults. We can't set them as chart defaults, since they only work for specific use cases. Perhaps the best option is setting up 1 Cyclops template for 1 values.yaml file. Surprisingly the Helm schema generator does not seem to preserve defaults, unless there is some option flag I missed. Here is the resulting module creation in Cyclops, after generating a values-schema.json from the above screenshot Values file:

image

In summary I don't know how to supply all these defaults in a Cyclops module, unless I manually edit the values-schema.json.

@petar-cvit
Copy link
Collaborator

Copy/paste would avoid the private repo limitation and the local file limitation.

@jrbe228 I agree. We will then leave the feature so you can paste your YAML config, but we will remove the option to fetch it since it would be confusing if it couldn't resolve a URL to a private repo.

 Perhaps the best option is setting up 1 Cyclops template for 1 values.yaml file

Yeah, sounds good. You could have separate branches for your different use cases that each have their own values.yaml. You could then reference those as different templates in Cyclops.

I don't know how to supply all these defaults in a Cyclops module, unless I manually edit the values-schema.json.

Just to be clear, all the values that come filled out when you select a template come from the values.yaml, so if you didn't add values.yaml to your chart, your UI would be empty.

Surprisingly the Helm schema generator does not seem to preserve defaults, unless there is some option flag I missed

The schema gen tool will preserve the defaults, but Cyclops does not take them into account. I will look into that so you can have that default in values.schema.json instead of only values.yaml.

@jrbe228
Copy link
Author

jrbe228 commented Nov 14, 2024

  • Feel free to leave the URL fetch feature, if you think it has value to other users. Although it would help to have a label in the web form saying "Supports public repos URLs only" or similar.

  • We could do 1 branch per values file.. We would need some Github Actions workflows to keep everything in sync. Somewhat risky 😄 We might also handle it via enum values in the schema json. We can think about options here. I wonder if other Cyclops users run into this situation.

  • Understood about "no values file = empty UI". We did try that accidentally!

  • Thanks for looking into Cyclops preserving the defaults from the schema.

@petar-cvit
Copy link
Collaborator

@jrbe228, we released v0.15.3 in which we revamped loading from file feature. We decided to remove loading by a URL reference until we figure out an easy way to load private YAMLs. Until then, you can paste your YAML values into the editor and populate the UI that way.

Screenshot 2024-11-22 at 17 39 08

I did some digging on the default values in the helm schema, and it seems that helm does not respect those - helm/helm#8218

Because of that, we will not support them to be consistent with Helm and not to introduce confusion for other users. If you see some other way to solve this that would require our help, let me know 😄

@jrbe228
Copy link
Author

jrbe228 commented Nov 22, 2024

Nice! The copy / paste feature should work for our simpler use cases. It implicitly supports concatenating multiple values files, which is fairly common. I assume multiple values files was not supported by the legacy "loading from file" feature.

About defaults in the helm schema, we might try to find a creative way to handle that. Maybe with enumeration in the json schema.. or some other approach. But it's less urgent thanks to the copy / paste feature.

@jrbe228
Copy link
Author

jrbe228 commented Nov 27, 2024

One thing I noticed on the install page -

image

It seems there is a consistent 0.1 difference between docs and latest release version -

image

@petar-cvit
Copy link
Collaborator

petar-cvit commented Nov 27, 2024

Thanks @jrbe228, I just bumped it. We will figure out a way to bump this automatically so it's not lagging behind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation UI Update on the UI
Projects
None yet
Development

No branches or pull requests

2 participants