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

restrict RBAC for kube controller secrets to the required namespace only #3602

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

vara2504
Copy link
Contributor

@vara2504 vara2504 commented Nov 19, 2024

Removed the secrets resource from the calico-kube-controllers and es-calico-kube-controllers ClusterRole and created a new Role with secrets resource in the tigera-operator and tigera-elasticsearch namespaces. In both the enterprise and cloud environments, I see that all secrets managed by the kube-controllers belong to either of these namespaces.

Tested in standalone cluster, mgmt and managed cluster .

Description

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

@marvin-tigera marvin-tigera added this to the v1.37.0 milestone Nov 19, 2024
@vara2504 vara2504 marked this pull request as ready for review November 20, 2024 21:13
@vara2504 vara2504 requested a review from a team as a code owner November 20, 2024 21:13
@tmjd
Copy link
Member

tmjd commented Nov 21, 2024

Please make sure you consider the multi-tenant configuration and understand if this impacts that configuration.

@vara2504
Copy link
Contributor Author

vara2504 commented Nov 22, 2024

Checked with the multi-tenant team, and we might not have any impact in the tenant namespace.
https://tigera.slack.com/archives/C04EKEGHE3C/p1732310285573909

// to get configmaps and manipulate secrets
func (m managedClusterLogStorage) linseedExternalRolesAndBindings() ([]*rbacv1.ClusterRole, []*rbacv1.RoleBinding, []*rbacv1.ClusterRoleBinding) {
func (m managedClusterLogStorage) externalRolesAndBindings() ([]*rbacv1.Role, []*rbacv1.ClusterRole, []*rbacv1.RoleBinding, []*rbacv1.ClusterRoleBinding) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if we kept this function scoped to Linseed, and introduced another one for the kube-controllers permissions.

}

// Bind the secrets permission to the operator namespace. This binding now adds permissions for kube controllers to create
// its public cert secret in the tigera-operator namespace
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why kube-controllers needs its public cert in the operator namespace? I'm struggling to recall what flow this is a part of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. management cluster only needs tigera-ee-*-elasticsearch-access secrets in the tigera-operator namespace not any public certs. Public certs are needed only in the managed cluster.

Copy link
Member

@caseydavenport caseydavenport left a comment

Choose a reason for hiding this comment

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

One minor comment but otherwise LGTM, thanks @vara2504 !

@rene-dekker rene-dekker merged commit cb000ca into tigera:master Nov 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants