Enh/domain targeting #43

grolu · 2021-10-08T14:18:56Z

What this PR does / why we need it:
This PR introduces basic domain targeting.
Shoots can be targeted via dashboard url or shoot api server url.
To enable domain targeting, you have to extend your garden configuration:

gardens:
  - name: foo-garden
    kubeconfig: ~/.kube/foo-kubeconfig
    shootDomain: shoot.gardener.cloud #shoot API sever domain
    dashboardDomain: dashboard.gardener.cloud #url to gardener dashboard

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

Special notes for your reviewer:

Release note:

holgerkoser · 2021-10-11T07:06:44Z

pkg/config/config.go

+ ShootDomain string `yaml:"shootDomain"`
+ DashboardDomain string `yaml:"dashboardDomain"`


Why don't we add a generic Patter Matching mechanism which also can handle technicalID. Add an Array of e.g shootRegexps or shootMatchPatterns. The implementation should be generic and could look like https://play.golang.org/p/q7JwrHDu283. I see the code of this more in the config package and not in the target package.

Yes, this sounds like a good Idea. We should document this well, so that users know how to configure their gardens. Would you add this per garden or global? If we have separate regexps per garden, we could keep the functionality of also targeting across multiple gardens with shoot / dashboard urls. Maybe we can have both?

We should document this well, so that users know how to configure their gardens

our goal should be that the user does not have to configure this and it should rather be fetched from somewhere, as we already discussed.

Would you add this per garden or global?

Per garden

If we have separate regexps per garden, we could keep the functionality of also targeting across multiple gardens with shoot / dashboard urls. Maybe we can have both?

I didn't get that

holgerkoser · 2021-10-11T07:11:48Z

pkg/target/manager.go

@@ -205,6 +207,14 @@ func (m *managerImpl) resolveProjectName(ctx context.Context, gardenClient clien
 return project, err
 }

+func (m *managerImpl) resolveProjectNamespace(ctx context.Context, gardenClient client.Client, projectNamespace string) (*corev1.Namespace, error) {


The wrapping of garden client function to make them more convenient is something we use in many locations. I think we overload the target manager if add all this convenient methods to him. I would propose to have a separate object/interface e.g GardenerClient.

holgerkoser · 2021-10-11T07:12:39Z

pkg/target/manager.go

@@ -282,8 +292,122 @@ func (m *managerImpl) validateSeed(ctx context.Context, seed *gardencorev1beta1.
 return nil
 }

+func domainsFromConfiguration(m *managerImpl) (map[string]string, map[string]string, error) {


Why is this a method on the manger. It doe not use the manger.

probably this happened during refactoring. I will fix this.

holgerkoser · 2021-10-11T07:16:36Z

pkg/target/manager.go

+ return "", ""
+}
+
+func targetFlagsFromDomain(ctx context.Context, m *managerImpl, t *targetImpl, name string) (TargetFlags, error) {


This should be a method of the managerImp. It should return a Target and not a TargetFlags. Why does it have in input parameter of type targteImpl. There is no need to depend on an impl of an interface. The only info we use is the gardenName.

holgerkoser · 2021-10-11T07:17:08Z

pkg/target/manager.go

+ return nil, nil
+}
+
+func isDomainFormat(name string) bool {


Why not strings.Contains

Yes you are right, I started with a more complex regex but simplified it more and more to support all cases and ended with contains-a-dot.

holgerkoser · 2021-10-11T07:21:39Z

I ask myself if it is a good idea if we pass this kind of identifiers to target shoot. This breaks shoot completions is is from my point of view something completely different because it rewrites the traget completely on not only the shoot.

grolu · 2021-10-11T08:03:50Z

I ask myself if it is a good idea if we pass this kind of identifiers to target shoot. This breaks shoot completions is is from my point of view something completely different because it rewrites the traget completely on not only the shoot.

I asked myself the same question. My first approach was to introduce a new target kind something like gardenctl target domain <shootdomain>, however I abandoned this attempt when I had another look at gardener-attic/gardenctl#499 where the domain targeting example does not have a dedicated target kind but uses target shoot <shootdomain> after thinking about it I decided that this makes sense as also with the current targeting it may happen that you change multiple values of your target (e.g. dropping seed and setting project).

Overall I feel that there are lots of things to discuss here

petersutter · 2021-10-11T08:45:34Z

internal/util/target.go

@@ -10,7 +10,6 @@ import (
 "context"
 "errors"
 "fmt"
-


revert
Did a tool that you used do this? Maybe it's misconfigured as we should be grouping imports by std, project and external

petersutter · 2021-10-11T08:53:38Z

pkg/config/config.go

+ ShootDomain string `yaml:"shootDomain"`
+ DashboardDomain string `yaml:"dashboardDomain"`


We should document this well, so that users know how to configure their gardens

our goal should be that the user does not have to configure this and it should rather be fetched from somewhere, as we already discussed.

Would you add this per garden or global?

Per garden

If we have separate regexps per garden, we could keep the functionality of also targeting across multiple gardens with shoot / dashboard urls. Maybe we can have both?

I didn't get that

petersutter · 2021-10-11T08:56:51Z

pkg/target/manager.go

+ err := gardenClient.Get(ctx, key, namespace)
+
+ return namespace, err


err := gardenClient.Get(ctx, key, namespace) return namespace, err

I have never seen such a pattern before, usually this is done:

if err := gardenClient.Get(ctx, key, namespace); err != nil { return nil, err } return namespace, nil

petersutter · 2021-10-11T09:17:26Z

pkg/target/manager.go

+ }
+
+ if len(dashboardDomains) == 0 && len(shootDomains) == 0 {
+ return nil, fmt.Errorf("no domain found in configured gardens. Shoot targeting via domain requires shoot and / or dashboard domain configuration for each garden that you want to enable domain targeting for")


if you need to format your error message use fmt.Errorf(), if you don't use errors.New().
-> in this case, use errors.New()

petersutter · 2021-10-11T09:21:36Z

pkg/target/manager.go

+ if namespace == nil {
+ return nil, fmt.Errorf("invalid namespace in shoot url")
+ }


I don't think this can ever happen

petersutter · 2021-10-11T09:24:31Z

pkg/target/manager.go

+
+ match := domainRegexp.FindStringSubmatch(domain)
+ if match != nil {
+ return NewTargetFlags(garden, match[2], "", match[1]), nil


I think it makes it hard to read what match[2] and match[1] really means, it probably makes more sense to extract it to a variable and name it appropriately

petersutter · 2021-10-11T09:29:40Z

pkg/target/manager.go

+ if targetFlags != nil {
+ t.Garden = targetFlags.GardenName()
+ t.Project = targetFlags.ProjectName()
+ t.Shoot = targetFlags.ShootName()
+ return nil
+ }
+
+ return fmt.Errorf("failed to target shoot with url: %s", shootName)


Nit: I'd prefer to turn it around

Suggested change

if targetFlags != nil {

t.Garden = targetFlags.GardenName()

t.Project = targetFlags.ProjectName()

t.Shoot = targetFlags.ShootName()

return nil

}

return fmt.Errorf("failed to target shoot with url: %s", shootName)

if targetFlags == nil {

return fmt.Errorf("failed to target shoot with url: %s", shootName)

}

t.Garden = targetFlags.GardenName()

t.Project = targetFlags.ProjectName()

t.Shoot = targetFlags.ShootName()

return nil

petersutter · 2021-10-11T09:46:57Z

maybe change this PR to draft as also the tests are missing?

grolu · 2021-10-14T15:41:56Z

closed in favor for #44

grolu added 3 commits October 7, 2021 14:07

Target shoot by dashboard or shoot url

b8f3087

fixes

b1b3b55

added error handling for missing domain configuration

b9a217b

grolu requested a review from a team as a code owner October 8, 2021 14:18

gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Oct 8, 2021

holgerkoser reviewed Oct 11, 2021

View reviewed changes

petersutter reviewed Oct 11, 2021

View reviewed changes

grolu closed this Oct 14, 2021

grolu deleted the enh/domain_targeting branch November 7, 2021 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enh/domain targeting #43

Enh/domain targeting #43

grolu commented Oct 8, 2021

holgerkoser Oct 11, 2021 •

edited

Loading

grolu Oct 11, 2021

petersutter Oct 11, 2021

holgerkoser Oct 11, 2021

holgerkoser Oct 11, 2021

grolu Oct 11, 2021

holgerkoser Oct 11, 2021

holgerkoser Oct 11, 2021

grolu Oct 11, 2021

holgerkoser commented Oct 11, 2021

grolu commented Oct 11, 2021 •

edited

Loading

petersutter Oct 11, 2021

petersutter Oct 11, 2021

petersutter Oct 11, 2021

petersutter Oct 11, 2021

petersutter Oct 11, 2021

petersutter Oct 11, 2021

petersutter Oct 11, 2021

petersutter commented Oct 11, 2021

grolu commented Oct 14, 2021

		ShootDomain string `yaml:"shootDomain"`
		DashboardDomain string `yaml:"dashboardDomain"`

		err := gardenClient.Get(ctx, key, namespace)

		return namespace, err

Enh/domain targeting #43

Enh/domain targeting #43

Conversation

grolu commented Oct 8, 2021

holgerkoser Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerkoser commented Oct 11, 2021

grolu commented Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petersutter commented Oct 11, 2021

grolu commented Oct 14, 2021

holgerkoser Oct 11, 2021 •

edited

Loading

grolu commented Oct 11, 2021 •

edited

Loading