support extension on Garden resource#280
Conversation
hown3d
force-pushed
the
garden-extension
branch
4 times, most recently
from
677f076 to
6cc819a
Compare
timebertt
left a comment
timebertt
left a comment
There was a problem hiding this comment.
Partial review together with @MichaelEischer @mstueer @hammadzf
| - get | ||
| - list | ||
| - watch | ||
| {{- end }} |
There was a problem hiding this comment.
Please double-check if we need all of the remaining permissions in the garden case.
There was a problem hiding this comment.
I think having a seperate roles that clearly define which permissions are granted for the extensions is easier to read. I will refactor this.
| var gardenCluster cluster.Cluster | ||
| if kFile := os.Getenv("GARDEN_KUBECONFIG"); kFile != "" { | ||
| var err error | ||
| gardenCluster, err = setupGardenCluster(mgr, kFile) |
There was a problem hiding this comment.
This should happen in app.go not in the controller package
There was a problem hiding this comment.
I agree, but there where would I put the cluster to pass it through to the predicate?
The controllers are registered via controllerSwitches which expects the AddToManger(ctx, mgr) signature.
Binding to AddOptions could be an option, but this seems a bit weird. WDYT?
There was a problem hiding this comment.
I suggest passing a new cluster.Cluster param to ControllerSwitches, then you can wrap the actual AddToManager(ctx, mgr, cluster) call in a func(context.Context, manager.Manager)
There was a problem hiding this comment.
This does not work well because the controller switches are registered before the manager is setup and I would like to setup the cluster next to the manager.
hown3d
force-pushed
the
garden-extension
branch
7 times, most recently
from
cac3025 to
0e64847
Compare
|
Sorry for all the force pushes, it was quite exhausting to rebase this to main. |
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
Signed-off-by: Lukas Hoehl <lukas.hoehl@stackit.cloud>
| if err := a.client.List(ctx, gardens); err != nil { | ||
| return nil, err | ||
| } | ||
| if len(gardens.Items) == 0 { |
There was a problem hiding this comment.
maybe add a check if > 1 gardens are found (which should never happen i think)?
There was a problem hiding this comment.
This is not supported by gardener operator and will be denied by the operator webhooks.
I don't think we need to check that
| log, | ||
| ex.GetNamespace(), | ||
| extSpec, | ||
| cluster, |
There was a problem hiding this comment.
since cluster is nil (line 149 ff.) if hasClassGarden() == false:
wouldn't it be better to have a function createFiltersGarden() without the cluster variable, and rename createFilters() to createFiltersShoot() ?'
and then do something like if hasGardenClass(ex) { createFiltersGarden() } else { createFiltersShoot() }?
| return a.createManagedResource(ctx, namespace, mrName, "seed", renderer, ChartNameSeed, namespace, values.AsMap(), nil, charts.Seed) | ||
| } | ||
|
|
||
| type values struct { |
There was a problem hiding this comment.
can we have
| type values struct { | |
| type envoyFilterSpecValues struct { |
or helmChartValues or similar, to make the name clearer?
3 participants
What this PR does / why we need it:
Allows to use the ACL extension to restrict access to the virtual garden API server
Which issue(s) this PR fixes:
Fixes gardener/hackathon#47
Special notes for your reviewer: