Skip to content

Disable terminal scheduling on non-managed seeds#2358

Closed
Lappihuan wants to merge 6 commits intogardener:masterfrom
Lappihuan:master
Closed

Disable terminal scheduling on non-managed seeds#2358
Lappihuan wants to merge 6 commits intogardener:masterfrom
Lappihuan:master

Conversation

@Lappihuan
Copy link
Copy Markdown
Contributor

@Lappihuan Lappihuan commented Mar 17, 2025

What this PR does / why we need it:
This disables the terminal on shoots that are not on a managed seed, using the suggestion from #2170 (comment)

Which issue(s) this PR fixes:
Fixes #2170

Special notes for your reviewer:
I'm unsure why there was a check on the garden namespace, but this excluded all shoots in the garden project.
https://github.com/gardener/dashboard/pull/2358/files#diff-6ed2ba0f6d68010cb296caa1dfe39da1cd2acf7dabd973d99fdf670d5849907fL307-R307
This might cause a bug elsewhere.

Release note:

Disable terminal scheduling on non-managed seeds

…n the garden namespace

Signed-off-by: Oriano de-Stefani <ode@exigo.org>
Signed-off-by: Oriano de-Stefani <ode@exigo.org>
@gardener-robot
Copy link
Copy Markdown

@Lappihuan Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 17, 2025
@gardener-robot-ci-1
Copy link
Copy Markdown
Contributor

Thank you @Lappihuan for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@gardener-robot
Copy link
Copy Markdown

@grolu, @holgerkoser You have pull request review open invite, please check

grolu
grolu previously approved these changes Mar 25, 2025
Copy link
Copy Markdown
Member

@grolu grolu left a comment

Choose a reason for hiding this comment

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

/lgtm

Removing namespace !== 'garden' should not break anything. If a seed is not managed, canLinkToSeed will be false. It was just an assumption that shoot resources in garden namespace will not have a seed that is managed but it is fair to remove it as this could be the case.
Also, using canLinkToSeed as an indicator for the terminal that the seed is managed is considered a workaround. It would be better to actually read the ManagedSeed resources in the backend and add a proper field to the shoot info. But we can target this in a separate PR.

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Mar 25, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 25, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 25, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 25, 2025
@grolu grolu self-requested a review March 25, 2025 15:40
@grolu grolu removed the reviewed/lgtm Has approval for merging label Mar 25, 2025
@grolu grolu dismissed their stale review March 25, 2025 15:48

After some internal discussions we found some issues with this PR:
Using the suggested approach, terminals would never be visible for regular users (non operators) as canLinkToSeed would always be false.
Also, for operators it should still be possible to schedule a terminal on the cluster even if the seed cannot be used to schedule a terminal. So disabling those options need to be implemented on the GCreateTerminalSessionDialog. Also, the operator needs to be informed (e.g. via tooltip) why scheduling a terminal on the seed is not possible.
See original comment in issue:

The dashboard should disable or hide the option to schedule a terminal on a Seed if it is not a managed seed. The frontend should detect whether a Seed is managed and adjust the available options accordingly.

@gardener-robot gardener-robot added the needs/review Needs review label Mar 25, 2025
@Lappihuan
Copy link
Copy Markdown
Contributor Author

Lappihuan commented Mar 25, 2025

It would be better to actually read the ManagedSeed resources in the backend and add a proper field to the shoot info.

It seems this is the way forward then.

So it depends on #1417

@grolu
Copy link
Copy Markdown
Member

grolu commented Mar 26, 2025

It seems this is the way forward then.
So it depends on #1417

So yes, in the long run this is the preferred solution. @petersutter already started working on watching seeds in the dashboard frontend. However, I cannot give an estimation on the timeline as we are currently blocked with other things.
However, you could still try to build an intermediate solution using canLinkToSeed. The important thing is that you don't disable the button to open the dialog but rather disable the options to schedule a terminal on the seed. This needs to be done on the GCreateTerminalSessionDialog and a tooltip message should be provided informing the operator, why he can only schedule a terminal on the shoot cluster and not on the control plane (seed). Also, the Terminal Runtime needs to be switched to Cluster.
I also noticed a couple of small issues in this context. I will have a look at it.

grolu added a commit that referenced this pull request Mar 26, 2025
@Lappihuan
Copy link
Copy Markdown
Contributor Author

@grolu is this now superseded by #2373 so i can close this PR?

@grolu
Copy link
Copy Markdown
Member

grolu commented Mar 27, 2025

@grolu is this now superseded by #2373 so i can close this PR?

Yes, I hope that we can merge PR #2373. But there are still some thinks to clarify first :) So I guess we can close this PR.

@Lappihuan Lappihuan closed this Mar 27, 2025
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Mar 27, 2025
grolu added a commit that referenced this pull request Apr 24, 2025
* Do not fail terminal config call if shoot API server is unreachable
Handle case when no shoot nodes are available (empty selection)
Disable create button in this case

* Moved alerts from subcomponents to GCreateTerminalSessionDialog to move them out of scrollable container
This ensures that they are always visible

* Apply suggestions from @Lappihuan PR #2358

* Proposal for disabling terminal scheduling on non-managed seeds

* Fixed create disabled for some scenarios
Added alert with error message in case no nodes are available to schedule terminal

* Disable terminal shortcuts for target control plane
Disable terminal shortcuts for target shoot for operators

* - Removed loading placeholder
- Fixed flaky initial default target
- Fixed terminal component initialization issue during navigation (loading state set too early, causing router-view to be returned before the navigation took place)

* PR Feedback

* PR Feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. status/closed Issue is closed (either delivered or triaged)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable terminal scheduling on non-managed seeds in the dashboard

5 participants