Fail NodePublishVolume with actionable error in case Workload Identity is not yet enabled on node level #533
Conversation
…ith actionable error in case WI is not yet enabled on node level
hungnguyen243
force-pushed
the
addWICheckForNode
branch
from
72cc716 to
e81b057
Compare
hungnguyen243
force-pushed
the
addWICheckForNode
branch
from
e81b057 to
978412b
Compare
| nodeObj.Spec = corev1.NodeSpec{} | ||
| nodeObj.Status = corev1.NodeStatus{} | ||
| nodeObj.ObjectMeta.Annotations = nil | ||
| nodeObj.ObjectMeta.Labels = newLabels |
There was a problem hiding this comment.
A couple questions, why do we set nodeObj.ObjectMeta.Annotations = nil, and clear out the nodeObj.spec / status? What are we going to use to get the machineType of the node?
Also , is it intentional we replace the nodeObj.ObjectMeta.Labels to only be the GkeMetaDataServerKey? Is that the part where we "We are filtering only for relevant Node annotations to optimize memory usage." ? Or where does the filtering happen.
Comment says we do filter for releavant node annotations (its labels), but if GkeMetaDataServerKey isn't there, I tink we
There was a problem hiding this comment.
IIUC the point of the function is to fetch only the necessary information we want (i.e. constructing a new Node/Pod object with just the info/fields we need from the actual k8s object, rather than passing the whole object around). This is just my guess looking at the way ConfigurePodLister is currently working :)
When we want to get machineType from the labels, we can do similar processing and add the 'machineType' label ("node.kubernetes.io/instance-type") in newLabels.
There was a problem hiding this comment.
Ok Sounds good. Thanks!
There was a problem hiding this comment.
this is an optimization trick we used to reduce informer mem usage
|
/lgtm |
|
/cherry-pick release 1.13 |
…533-upstream-release-1.13 Automated cherry pick of #533: Fail NodePublishVolume with actionable error in case Workload Identity is not yet enabled on node level
…tionable error in case Workload Identity is not yet enabled on node level"
…-cherry-pick-of-#533-upstream-release-1.13 Revert "Automated cherry pick of #533: Fail NodePublishVolume with actionable error in case Workload Identity is not yet enabled on node level"
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add a check for GKE metadata server label on node to fail MountVolume (NodePublishVolume) with an actionable error. The target scenario is when an user enabled Workload Identity on an existing cluster, but didn't update their node pool to use GKE metadata server. Currently, this error takes more effort to debug since NodePublishVolume still succeeds, but we would see R/W errors from the sidecar due to PermissionDenied as the default SA lacks the appropriate permission for the GCS bucket.
Testing documentation with custom driver image: https://paste.googleplex.com/5532151901323264
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: