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

refactor(kubernetes-dashboard): move override to separate values.yaml file #179

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

muse-sisay
Copy link
Contributor

@muse-sisay muse-sisay commented Oct 29, 2024

Description

  • store override in a separate values.yaml file
  • upgrade chart to version 7.9.0
  • append cluster name to dashboard endpoint

Related Issue(s)

  • Resources in components directory were installed in the local cluster. As a result ingress resource wasn't available for downstream clusters.
  • Conflicting DNS names for multiple instance of Kubernetes dashboard

How to test

Follow steps in Contributing.md to deploy the app.

- store override in a separate values.yaml file
- upgrade to version 7.9.0
Copy link
Contributor

@CristhianF7 CristhianF7 left a comment

Choose a reason for hiding this comment

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

ArgoCD will complain because of having the same name for resources and since apps can be installed in different clusters we will have the same problem

subjects:
- kind: ServiceAccount
name: <CLUSTER_NAME>-kubernetes-dashboard
name: dashboard-user
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: dashboard-user
name: <CLUSTER_NAME>-dashboard-user

apiVersion: v1
kind: ServiceAccount
metadata:
name: dashboard-user
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: dashboard-user
name: <CLUSTER_NAME>-dashboard-user

@muse-sisay
Copy link
Contributor Author

Hey @CristhianF7

Indeed, ArgoCD raises an error if two Applications are managing the same resource. (i.e the resource has the same name, destination cluster and type ). In this PR though, the service-account and role binding are created in downstream cluster. Even though in multiple installation of the app the resource have the same name they are not the same resource.

There is going to be a problem if a user installs two copies of the app in the same downstream cluster. Luckily that is not a feature we support and don't have to worry about, for now.

bonus point : Because the resources are name the same across different downstream accounts, if we ever write script/automation or ask user to ran some sort of command (eg. kubectl -n kubernetes-dashboard create token dashboard-user), they don't have to modify the resource name to match the cluster they are running against.

Muse

@CristhianF7
Copy link
Contributor

Hey @CristhianF7

Indeed, ArgoCD raises an error if two Applications are managing the same resource. (i.e the resource has the same name, destination cluster and type ). In this PR though, the service-account and role binding are created in downstream cluster. Even though in multiple installation of the app the resource have the same name they are not the same resource.

There is going to be a problem if a user installs two copies of the app in the same downstream cluster. Luckily that is not a feature we support and don't have to worry about, for now.

bonus point : Because the resources are name the same across different downstream accounts, if we ever write script/automation or ask user to ran some sort of command (eg. kubectl -n kubernetes-dashboard create token dashboard-user), they don't have to modify the resource name to match the cluster they are running against.

Muse

Hey @muse-sisay

Thanks for clarifying. I just wanted to double check because we faced this error several times

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.

2 participants