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

Fix grafana login issue #623

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Fix grafana login issue #623

merged 1 commit into from
Jan 5, 2024

Conversation

w13915984028
Copy link
Member

@w13915984028 w13915984028 commented Jan 2, 2024

Problem:

The grafana login page showed 404.

A main reason is, when accesing grafana from the local cluster, the general URL is https://1.2.3.4/k8s/clusters/local/api/v1/...., the Rancher will trim /k8s/clusters/local/, this works fine for normal link. But for login, it is another story, the grafana will return a 302 code with a new Location, unfortunately, the Rancher will NOT add /k8s/clusters/local back to the Location. When grafana's front page code runs, it will get a different appSubUrl and assume error, then showed 404 page.

For rancher managed cluster, the regex processing in appSubUrl is not perfect to deal with 302.

Solution:

(1) For local cluster, add a fallback item in the nginx sub-filter
(2) For rancher-managed cluster, use long match in regex to avoid duplicate URL elements.

Related Issue:
harvester/harvester#4756

Test plan:

  1. Install a new Harvester v1.3.0 cluster, enable rancher-monitoring addon. Or use this workaround [BUG] Grafana (Addon: rancher-monitoring) sign in is not working in Harvester 1.2.1 harvester#4756 (comment) in existing v1.2.1/v1.3.0 cluster.
  2. Click grafana from the Dashboard page, which will open a new page.
  3. Click login, try login; or return to previous page, all should run smoothly.

In Rancher-integration scenario, repeat the same processes.

Signed-off-by: Jian Wang <[email protected]>
@bk201
Copy link
Member

bk201 commented Jan 3, 2024

@w13915984028 If the user turns on the add-on in v1.2.1, does this fix work if he upgrades to the newer version?

@w13915984028
Copy link
Member Author

w13915984028 commented Jan 3, 2024

@bk201

For v1.2.1 -> v1.3.0,
After harvester/harvester#4941, the rancher-monitoring-crd will have a newer version, the upgrade process will make sure the rancher-monitoring-crd managedchart is upgraded with new value, for all PODs in cattle-monitoring-system, they will be replaced when NODE is rebooted/addon is upgraded, the bug will be fixed after upgrade.

For v1.2.1 -> potential v1.2.2, we will not bump a newer version of rancher-monitoring chart, the fleet-agent may not be able to upgrade the rancher-monitoring-crd managedchart as the chart version is not changed, then the configmap is not updated. Tracked in harvester/harvester#4942.

@bk201
Copy link
Member

bk201 commented Jan 4, 2024

@bk201

For v1.2.1 -> v1.3.0, After harvester/harvester#4940, the rancher-logging-crd will have a newer version, the upgrade process will make sure the rancher-logging-crd managedchart is upgraded with new value, for all PODs in cattle-logging-system, they will be replaced when NODE is rebooted/addon is upgraded, the bug will be fixed after upgrade.

For v1.2.1 -> potential v1.2.2, we will not bump a newer version of rancher-logging chart, the fleet-agent may not be able to upgrade the rancher-logging-crd managedchart as the chart version is not changed, then the configmap is not updated. Tracked in harvester/harvester#4756.

Thanks for the explanation, but did you mean the rancher-monitoring chart, not the logging one?

@w13915984028
Copy link
Member Author

@bk201 You are right :) , it is rancher-monitoring not rancher-logging related, the description is updated.

Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

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

LGTM!

@bk201 bk201 merged commit 03ad687 into harvester:master Jan 5, 2024
5 checks passed
@bk201
Copy link
Member

bk201 commented Jan 5, 2024

@Mergifyio backport v1.2

Copy link

mergify bot commented Jan 5, 2024

backport v1.2

✅ Backports have been created

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.

3 participants