Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix RBAC permissions for the Antctl ClusterRole #5166

Merged
merged 5 commits into from
Jul 5, 2023

Conversation

antoninbas
Copy link
Contributor

The ClusterRole definition was no longer up-to-date, with some incorrect permissions and some missing permissions. As a consequence, it could not be used to run some antctl commands, such as supportbundle.

We fix the permissions and modify the Antctl e2e tests so that they use a Kubeconfig file generated for the antctl ServiceAccount, instead of the admin Kubeconfig file. Hopefully, this will help keep the ClusterRole definition up-to-date in the future.

A few other improvements were implemented in that process:

  • antctl now uses the typed SystemBundle K8s clientset, instead of a raw REST client. This helps define the correct RBAC permissions for antctl. Previous permissions were indeed incorrect as they used the "post" verb (instead of the "create" verb), which is only correct for non-resource endpoints.
  • supportbundle unit tests now use an in-memory filesystem, to avoid writing test outputs to the local machine.
  • the antctl ClusterRole is giving access to a few extra endpoints ("/metrics", "/debug/pprof/*") to increse the usefulness of the antctl proxy command.

Fixes #5136

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM, a few minor comments

pkg/antctl/raw/supportbundle/command.go Outdated Show resolved Hide resolved
pkg/antctl/raw/supportbundle/command.go Outdated Show resolved Hide resolved
// Secret contents to a file. Ideally, we would use a Pod to run antctl commands instead of
// running it from the Node (in that case, the Secret would be mounted to the Pod).
kubeconfigSecretKey := "kubeconfig"
// No need to worrky about deleting the Secret as it is created in the temporary test Namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// No need to worrky about deleting the Secret as it is created in the temporary test Namespace.
// No need to worry about deleting the Secret as it is created in the temporary test Namespace.

@antoninbas antoninbas force-pushed the fix-antctl-clusterrole branch 3 times, most recently from 83210a0 to 8f6c698 Compare July 1, 2023 00:24
tnqn
tnqn previously approved these changes Jul 3, 2023
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas
Copy link
Contributor Author

/test-all

The ClusterRole definition was no longer up-to-date, with some incorrect
permissions and some missing permissions. As a consequence, it could not
be used to run some antctl commands, such as supportbundle.

We fix the permissions and modify the Antctl e2e tests so that they use
a Kubeconfig file generated for the antctl ServiceAccount, instead of
the admin Kubeconfig file. Hopefully, this will help keep the
ClusterRole definition up-to-date in the future.

A few other improvements were implemented in that process:

* antctl now uses the typed SystemBundle K8s clientset, instead of a raw
  REST client. This helps define the correct RBAC permissions for
  antctl. Previous permissions were indeed incorrect as they used the
  "post" verb (instead of the "create" verb), which is only correct for
  non-resource endpoints.
* supportbundle unit tests now use an in-memory filesystem, to avoid
  writing test outputs to the local machine.
* the antctl ClusterRole is giving access to a few extra endpoints
  ("/metrics", "/debug/pprof/*") to increse the usefulness of the antctl
  proxy command.

Fixes antrea-io#5136

Signed-off-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Antonin Bas <abas@vmware.com>
'~' is not expanded with docker exec

Signed-off-by: Antonin Bas <abas@vmware.com>
@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas antoninbas merged commit 8913769 into antrea-io:main Jul 5, 2023
@antoninbas antoninbas deleted the fix-antctl-clusterrole branch July 5, 2023 04:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that build/charts/antrea/templates/antctl/clusterrole.yaml is up-to-date and stays up-to-date
2 participants