-
Notifications
You must be signed in to change notification settings - Fork 32
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
Introduce support for Projected Volumes kubernetes ServiceAccount Tokens #416
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: jjotah <[email protected]>
Thanks @JJotah , we will take a look. |
Fixes linkerd/linkerd2#12573 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JJotah , this makes a lot of sense!
Can I suggest introducing a variable analog to cni_conf_sha
but tracking the sha for the token file? So we avoid introducing that new if/else in sync()
. This would require two additional refactors though:
- Pass the entire filepath to the function instead of using
filepath="${HOST_CNI_NET}/$filename"
- Under your new
inotifywait
call when you callsync
, you'll have to add some logic to figure what's the actual config file name being used (as per my comment below).
log "Ignoring event: $ev $filepath; no real changes detected" | ||
# If there is no previous SHA (as in the case of the token), reinstall cni. | ||
log "Processing event for $filename with no SHA comparison needed." | ||
install_cni_conf "$DEFAULT_CNI_CONF_PATH" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't be sure at this point if the config is held on $DEFAULT_CNI_CONF_PATH
, which is a .conf
file, or a .conflist
file.
@@ -298,6 +304,11 @@ monitor() { | |||
cni_conf_sha="$(sha256sum "$directory/$filename" | while read -r s _; do echo "$s"; done)" | |||
fi | |||
fi | |||
done & | |||
inotifywait -m "/var/run/secrets/kubernetes.io/serviceaccount/token" -e create,delete,moved_to,modify | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In create_cni_conf()
we have SERVICE_ACCOUNT_PATH=/var/run/secrets/kubernetes.io/serviceaccount
. Can you make that a global variable, so we can reuse it here?
@@ -298,6 +304,11 @@ monitor() { | |||
cni_conf_sha="$(sha256sum "$directory/$filename" | while read -r s _; do echo "$s"; done)" | |||
fi | |||
fi | |||
done & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to be careful with backgrounding these tasks -- we want to ensure that inotifywait
failures are propagated so that the program can fail if any of these calls fail.
There are other changes we probably need to make to this script first, but we'll want to ensure that we preserve PIDs and wait
on them to surface errors.
I did some more research on this, thinking it's been a while since k8s injects projected kube-api-access volumes, so why is linkerd-cni still working past the token expiration (1 hour by default)? - name: kube-api-access-729gv
projected:
defaultMode: 420
sources:
- serviceAccountToken:
expirationSeconds: 3607
path: token
- configMap:
items:
- key: ca.crt
path: ca.crt
name: kube-root-ca.crt
- downwardAPI:
items:
- fieldRef:
apiVersion: v1
fieldPath: metadata.namespace
path: namespace I could also observe how the token file got indeed recycled every hour. But actually inspecting those tokens revealed they're good for one year! So even if the token file gets renewed, linkerd-cni keeps on using the first one, which will continue to be valid.
This flag still shows as default |
Add support for Projected Volumes kubernetes ServiceAccount Tokens. In some cloud providers we can't change the expiration token and linkerd is adding the projected volumes. So the install-cni should watch like istio did checking the serviceaccount token