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

add targeting system and target command #5

Merged
merged 18 commits into from
Jun 8, 2021

Conversation

xrstf
Copy link
Contributor

@xrstf xrstf commented May 25, 2021

What this PR does / why we need it:
This PR introduces the "targeting" system from gardenctl v1, just slighly refactored. Targeting is used to free the user from having to specify the garden, seed and/or shoot name for most commands (e.g. free them from doing something like gardenctl ssh -g garden1 -s seed1 --ss shoot1 nodeNameXY). To achieve this, the user can use the target command and incrementally refine the goal for the upcoming commands, e.g.

gardenctl target garden mygarden
gardenctl target seed aws-seed
gardenctl target shoot my-shoot
gardenctl ssh nodeNameXY

The previous implementation chose to work with "stack"-like approach, however the new implementation uses a dedicated map instead. The code is also less generic, which seems like a downside, but if you look carefully at the v1 code, the generic parts were drowned in super specific hacks and exceptions and lots and lots of assumptions. That made the old code hard to read and understand:

// getKubeConfigOfClusterType return config of specified type
func getKubeConfigOfClusterType(clusterType TargetKind) (pathToKubeconfig string) {
	var target Target
	ReadTarget(pathTarget, &target)
	gardenName := target.Stack()[0].Name
	switch clusterType {
	case TargetKindGarden:
		pathToKubeconfig = TidyKubeconfigWithHomeDir(getGardenKubeConfig())
	case TargetKindSeed:
		if target.Target[1].Kind == "seed" {
			pathToKubeconfig = filepath.Join(pathGardenHome, "cache", gardenName, "seeds", target.Target[1].Name, "kubeconfig.yaml")
		} else {
			pathToKubeconfig = filepath.Join(pathGardenHome, "cache", gardenName, "seeds", getSeedForProject(target.Target[2].Name), "kubeconfig.yaml")
		}
	case TargetKindShoot:
		if target.Target[1].Kind == "seed" {
			pathToKubeconfig = filepath.Join(pathGardenHome, "cache", gardenName, "seeds", getSeedForProject(target.Target[2].Name), target.Target[2].Name, "kubeconfig.yaml")
		} else if target.Target[1].Kind == "project" {
			pathToKubeconfig = filepath.Join(pathGardenHome, "cache", gardenName, "projects", target.Target[1].Name, target.Target[2].Name, "kubeconfig.yaml")
		}
	}
	return pathToKubeconfig
}

The new v2 code makes no assumptions like that and simply has dedicated functions and fields for garden, seed, project and shoot.

Likewise, the new code separates the data and logic, with the Target interface only providing read access to target information, and the more complex logic being wrapped up in the Manager (so it's manager.GardenClient(target) instead of target.K8sClientForTarget(TargetKindGarden)). This is because we surely will rework the targeting in the future and this separation makes it easier to exchange the implementation.

I chose to provide controller-runtime clients, because I like their interface much better than the Kuberntes clientsets. One downside so far is that namespace targeting doesn't work yet, so targeting a project is currently pointless and returns a client to the garden cluster.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Added `target` command

@gardener-robot
Copy link

@xrstf Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels May 25, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 25, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added 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 May 25, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 25, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 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 May 25, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 25, 2021
@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 May 25, 2021
@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 May 25, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 25, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has 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 May 25, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 May 25, 2021
@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 May 25, 2021
@xrstf xrstf marked this pull request as ready for review May 25, 2021 15:30
@xrstf xrstf requested a review from a team as a code owner May 25, 2021 15:30
pkg/cmd/root/command.go Outdated Show resolved Hide resolved
pkg/cmd/root/command.go Outdated Show resolved Hide resolved
pkg/cmd/root/command.go Outdated Show resolved Hide resolved
pkg/cmd/target/options.go Show resolved Hide resolved
pkg/target/config.go Outdated Show resolved Hide resolved
pkg/target/manager.go Outdated Show resolved Hide resolved
@gardener-robot-ci-3 gardener-robot-ci-3 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 Jun 1, 2021
@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) 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 Jun 1, 2021
@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 Jun 1, 2021
@petersutter
Copy link
Member

@xrstf there are some lint errors that need to be fixed, otherwise lgtm

@petersutter
Copy link
Member

I just tested it locally and ran into an error because the home folder was not resolved for the garden kubeconfig.

cat ~/.garden/gardenctl-v2.yaml
gardens:
- name: my-garden
  kubeconfig: ~/path/to/kubeconfig.yaml 

@gardener-robot gardener-robot added the lifecycle/stale Nobody worked on this for 6 months (will further age) label Jun 2, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 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 Jun 3, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 Jun 3, 2021
@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 Jun 3, 2021
@xrstf
Copy link
Contributor Author

xrstf commented Jun 3, 2021

@petersutter linted and handled ~ in configuration now.

@gardener-robot-ci-3 gardener-robot-ci-3 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 Jun 3, 2021
Comment on lines 119 to 120
// prefer -session, then an explicit GCTL_HOME env,
// but fallback to the system-defined home directory
Copy link
Member

Choose a reason for hiding this comment

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

prefer -session

do not understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I've removed the notion of "sessions" again now. The comment was .. inspired, yet misleading. I am not actually sure how we want to allow users to switch between targets (we probably want to offer that, but how exactly?), so it's probably best if we do it in another PR.

@gardener-robot-ci-1 gardener-robot-ci-1 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 Jun 7, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 Jun 7, 2021
@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 Jun 8, 2021
@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 Jun 8, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 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 Jun 8, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 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 Jun 8, 2021
@petersutter petersutter self-requested a review June 8, 2021 12:32
Copy link
Member

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

/lgtm, let's get this basic target handling in and improve as we go

@petersutter petersutter merged commit 67c26eb into gardener:master Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Nobody worked on this for 6 months (will further age) 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 needs/second-opinion Needs second review by someone else size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants