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

No longer assume that K8s creates a token for all ServiceAccounts #3730

Conversation

antoninbas
Copy link
Contributor

The assumption that a secret token is automatically created for each
ServiceAccount is not valid anymore, starting with K8s v1.24.

We make the following changes:

  • create a Secret in our reference Prometheus manifest for the prometheus
    ServiceAccount; this addresses the failure in Prometheus e2e tests.
  • create a Secret in the Antrea manifest for the antctl and antrea-agent
    ServiceAccount (but not for the antrea-controller ServiceAccount). Note that
    the token for the antrea-agent ServiceAccount is only really necessary when
    running the Antrea Agent as a process on Windows Nodes, so in the future we
    may want to conditionally generate the Secret only for that use case.
  • remove the manual installation instructions: the instructions require a token
    for the antrea-controller ServiceAccount, but we don't have one anymore. The
    instructions are also outdated (e.g., we no longer have ready-to-use RBAC
    manifests), and running the Agent manually is likely to work sub-optimally (no
    downward API so missing environment variables).

Fixes #3729

Signed-off-by: Antonin Bas abas@vmware.com

@antoninbas
Copy link
Contributor Author

@tnqn let me know if you think we should keep the manual installation document

@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label May 4, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #3730 (a154c0f) into main (8f451f7) will decrease coverage by 18.19%.
The diff coverage is 12.58%.

❗ Current head a154c0f differs from pull request most recent head ffe26cf. Consider uploading reports for the commit ffe26cf to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3730       +/-   ##
===========================================
- Coverage   64.44%   46.24%   -18.20%     
===========================================
  Files         278      245       -33     
  Lines       39513    35770     -3743     
===========================================
- Hits        25463    16541     -8922     
- Misses      12068    17596     +5528     
+ Partials     1982     1633      -349     
Flag Coverage Δ
e2e-tests 46.24% <12.58%> (?)
kind-e2e-tests ?
unit-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/openflow/framework.go 77.00% <0.00%> (-12.70%) ⬇️
pkg/agent/openflow/pod_connectivity.go 42.26% <5.79%> (-26.06%) ⬇️
pkg/agent/openflow/client.go 66.56% <8.33%> (-5.62%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 30.56% <22.22%> (-48.52%) ⬇️
pkg/agent/openflow/pipeline.go 67.90% <100.00%> (-6.91%) ⬇️
pkg/ipfix/ipfix_process.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/ipfix/ipfix_registry.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/ipfix/ipfix_intermediate.go 0.00% <0.00%> (-90.91%) ⬇️
...lowaggregator/clickhouseclient/clickhouseclient.go 0.00% <0.00%> (-90.77%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 4.58% <0.00%> (-88.08%) ⬇️
... and 169 more

The assumption that a secret token is automatically created for each
ServiceAccount is not valid anymore, starting with K8s v1.24.

We make the following changes:
- create a Secret in our reference Prometheus manifest for the `prometheus`
  ServiceAccount; this addresses the failure in Prometheus e2e tests.
- create a Secret in the Antrea manifest for the antctl and antrea-agent
  ServiceAccount (but not for the antrea-controller ServiceAccount). Note that
  the token for the antrea-agent ServiceAccount is only really necessary when
  running the Antrea Agent as a process on Windows Nodes, so in the future we
  may want to conditionally generate the Secret only for that use case.
- remove the manual installation instructions: the instructions require a token
  for the antrea-controller ServiceAccount, but we don't have one anymore. The
  instructions are also outdated (e.g., we no longer have ready-to-use RBAC
  manifests), and running the Agent manually is likely to work sub-optimally (no
  downward API so missing environment variables).

Fixes antrea-io#3729

Signed-off-by: Antonin Bas <abas@vmware.com>
@antoninbas antoninbas force-pushed the stop-relying-on-autogenerated-tokens-for-serviceaccounts branch from ed39351 to ffe26cf Compare May 4, 2022 23:41
@@ -176,13 +176,14 @@ using the authentication token of the `antctl` ServiceAccount:

```bash
# Get the token value of antctl account.
TOKEN=$(kubectl get secrets -n kube-system -o jsonpath="{.items[?(@.metadata.annotations['kubernetes\.io/service-account\.name']=='antctl')].data.token}"|base64 --decode)
TOKEN=$(kubectl get secret/antctl-service-account-token -n kube-system -o jsonpath="{.data.token}"|base64 --decode)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will not work for old versions of Antrea. The alternative is:

kubectl get secrets -o jsonpath="{.items[?(@.metadata.annotations['kubernetes\.io/service-account\.name']=='sa-test')].data.token}" | awk '{print $1}' | base64 --decode

but that requires awk

Unfortunately K8s jsonpath does not support selecting the first element after applying a filter expression. If we don't use awk, we may get 2 tokens for K8s versions before v1.24 (auto-generated secret, plus secret/antctl-service-account-token), which will not work.

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

LGTM. Quan should review.

Copy link
Contributor

@xliuxu xliuxu 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

@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

@tnqn
Copy link
Member

tnqn commented May 5, 2022

@tnqn let me know if you think we should keep the manual installation document

I'm fine with removing it given that it has been outdated for a while and there was no complaint about it.

@antoninbas
Copy link
Contributor Author

/test-all

Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assumption that a secret token is automatically created for each ServiceAccount not valid anymore
6 participants