Skip to content

Conversation

ialidzhikov
Copy link
Member

@ialidzhikov ialidzhikov commented May 29, 2025

How to categorize this PR?

/area security
/kind enhancement

What this PR does / why we need it:
gardener-operator creates kube-system/gardener-apiserver Service and kube-system/gardener-apiserver Endpoint in the virtual cluster:

  • func (g *gardenerAPIServer) service() *corev1.Service {
    return &corev1.Service{
    ObjectMeta: metav1.ObjectMeta{
    Name: serviceName,
    Namespace: metav1.NamespaceSystem,
    Labels: GetLabels(),
    },
    Spec: corev1.ServiceSpec{
    Type: corev1.ServiceTypeClusterIP,
    Selector: GetLabels(),
    Ports: []corev1.ServicePort{{
    Port: servicePort,
    Protocol: corev1.ProtocolTCP,
    TargetPort: intstr.FromInt32(port),
    }},
    },
    }
    }
  • func (g *gardenerAPIServer) endpoints(clusterIP string) *corev1.Endpoints {
    return &corev1.Endpoints{
    ObjectMeta: metav1.ObjectMeta{
    Name: serviceName,
    Namespace: metav1.NamespaceSystem,
    Labels: GetLabels(),
    },
    Subsets: []corev1.EndpointSubset{{
    Ports: []corev1.EndpointPort{{
    Port: servicePort,
    Protocol: corev1.ProtocolTCP,
    }},
    Addresses: []corev1.EndpointAddress{{
    IP: clusterIP,
    }},
    }},
    }
    }
  • virtualResources, err := virtualRegistry.AddAllAndSerialize(
    g.apiService(secretCAGardener, gardencorev1.SchemeGroupVersion.Group, gardencorev1.SchemeGroupVersion.Version),
    g.apiService(secretCAGardener, gardencorev1beta1.SchemeGroupVersion.Group, gardencorev1beta1.SchemeGroupVersion.Version),
    g.apiService(secretCAGardener, seedmanagementv1alpha1.SchemeGroupVersion.Group, seedmanagementv1alpha1.SchemeGroupVersion.Version),
    g.apiService(secretCAGardener, operationsv1alpha1.SchemeGroupVersion.Group, operationsv1alpha1.SchemeGroupVersion.Version),
    g.apiService(secretCAGardener, settingsv1alpha1.SchemeGroupVersion.Group, settingsv1alpha1.SchemeGroupVersion.Version),
    g.apiService(secretCAGardener, securityv1alpha1.SchemeGroupVersion.Group, securityv1alpha1.SchemeGroupVersion.Version),
    g.service(),
    g.endpoints(serviceRuntime.Spec.ClusterIP),
    g.clusterRole(),
    g.clusterRoleBinding(secretVirtualGardenAccess.ServiceAccountName),
    g.clusterRoleBindingAuthDelegation(secretVirtualGardenAccess.ServiceAccountName),
    g.roleBindingAuthReader(secretVirtualGardenAccess.ServiceAccountName),
    )

However, gardener admins currently don't have permissions to inspect or mutate these resources.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:
I don't know at all what is the use case of these Service and Endpoint. Let me know if they are not used. Let me know also what is their use case - it would be interesting to learn this.
EDIT: The APIService points to the kube-system/gardener-apiserver Service. The underlying Endpoint resolves to the clusterIP of the garden/gardener-apiserver Service in the runtime cluster.

Release note:

Gardener administrators are now allowed to inspect and manage Services and Endpoints in the garden cluster.

@gardener-prow gardener-prow bot added the area/security Security related label May 29, 2025
@gardener-prow gardener-prow bot requested a review from ScheererJ May 29, 2025 10:50
@gardener-prow gardener-prow bot added the kind/enhancement Enhancement, improvement, extension label May 29, 2025
@gardener-prow gardener-prow bot requested a review from timebertt May 29, 2025 10:50
@gardener-prow gardener-prow bot added cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels May 29, 2025
@ialidzhikov
Copy link
Member Author

/cc @vpnachev @dimityrmirchev @donistz

@gardener-prow gardener-prow bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 29, 2025
Copy link
Contributor

gardener-prow bot commented May 29, 2025

@ialidzhikov: GitHub didn't allow me to request PR reviews from the following users: donistz.

Note that only gardener members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @vpnachev @dimityrmirchev @donistz

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@dimityrmirchev
Copy link
Member

/assign

Copy link
Member

@dimityrmirchev dimityrmirchev left a comment

Choose a reason for hiding this comment

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

Nit: I have no strong opinion, but another approach would be to only allow access to the said service and endpoint. If that is too much work or if it would bring additional complexity w.r.t. role assignments I guess this change is also good enough as it is.

@ialidzhikov
Copy link
Member Author

/hold

@gardener-prow gardener-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2025
@gardener-ci-robot
Copy link
Contributor

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

  • After 15d of inactivity, lifecycle/stale is applied
  • After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Mark this PR as rotten with /lifecycle rotten
  • Close this PR with /close

/lifecycle stale

@gardener-prow gardener-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 13, 2025
@ialidzhikov
Copy link
Member Author

/remove-lifecycle stale

@gardener-prow gardener-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 16, 2025
@gardener-ci-robot
Copy link
Contributor

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

  • After 15d of inactivity, lifecycle/stale is applied
  • After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Mark this PR as rotten with /lifecycle rotten
  • Close this PR with /close

/lifecycle stale

@gardener-prow gardener-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 1, 2025
@ialidzhikov
Copy link
Member Author

/remove-lifecycle stale

@gardener-prow gardener-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 1, 2025
@ialidzhikov ialidzhikov force-pushed the enh/gardener-admin-clusterrole branch from 9a46c97 to 38aa177 Compare July 10, 2025 10:42
@gardener-prow gardener-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 10, 2025
@ialidzhikov
Copy link
Member Author

Nit: I have no strong opinion, but another approach would be to only allow access to the said service and endpoint. If that is too much work or if it would bring additional complexity w.r.t. role assignments I guess this change is also good enough as it is.

I was wondering the same when creating the PR. It might be also a feature for the gardener.cloud:admin ClusterRole not to be restricted only to the known Service and Endpoint.
If we have to apply the "minimal privileges" rule (create a dedicated Role and RoleBinding that are only for the resourceNames in the kube-system namespace) for the gardener.cloud:admin and gardener.cloud:viewer ClusterRoles which are for gardener administrators/operators only, then I can close the PR.

@ialidzhikov
Copy link
Member Author

/hold cancel

@gardener-prow gardener-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2025
@ialidzhikov
Copy link
Member Author

/test pull-gardener-unit

@gardener-ci-robot
Copy link
Contributor

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

  • After 15d of inactivity, lifecycle/stale is applied
  • After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Mark this PR as rotten with /lifecycle rotten
  • Close this PR with /close

/lifecycle stale

@gardener-prow gardener-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2025
@ialidzhikov
Copy link
Member Author

/remove-lifecycle stale

@gardener-prow gardener-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2025
@ialidzhikov
Copy link
Member Author

@dimityrmirchev @vpnachev @gardener/gardener-maintainers any feedback on this PR?

Copy link
Member

@dimityrmirchev dimityrmirchev left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2025
Copy link
Contributor

gardener-prow bot commented Jul 28, 2025

LGTM label has been added.

Git tree hash: ca0018bffd529b0162bc524461a705c0af2f4447

Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@tobschli tobschli left a comment

Choose a reason for hiding this comment

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

/approve

Copy link
Contributor

gardener-prow bot commented Jul 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dimityrmirchev, tobschli, vpnachev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2025
@gardener-prow gardener-prow bot merged commit 881f44e into gardener:master Jul 28, 2025
19 checks passed
@ialidzhikov ialidzhikov deleted the enh/gardener-admin-clusterrole branch July 31, 2025 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/security Security related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants