instance metadata issue fix #846
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @vdhanan. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| - name: CSI_NODE_NAME | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: spec.nodeName |
There was a problem hiding this comment.
also run make generate-kustomize (definitely we need a CI job for this, same as how we have make verify for keeping vendor dir up to date)
There was a problem hiding this comment.
done in new PR #855
pkg/cloud/metadata.go
Outdated
| } | ||
|
|
||
| // get node with k8s API | ||
| node, err := clientset.CoreV1().Nodes().Get(context.TODO(), nodeName, metav1.GetOptions{}) |
There was a problem hiding this comment.
doesn't the node need RBAC permission to do this? we will need to add clusterrole and clusterrolebinding bound to the node service account.
pkg/cloud/metadata.go
Outdated
| } | ||
|
|
||
| awsRegionRegex := "(?<=/)([a-z]{2}(-gov)?)-(central|(north|south)?(east|west)?)-[0-9]" | ||
| awsAvailabilityZoneRegex := "(?<=/)([a-z]{2}(-gov)?)-(central|(north|south)?(east|west)?)-[0-9][a-z]" |
There was a problem hiding this comment.
how much do we have to worry about special cases, will this work on an outpost node for example? see https://github.com/kubernetes/kubernetes/pull/99130/files#diff-f20dfc917eee752715e89727d403f28f27220826b73bc67ddcc224effe7d1836R270 by @ayberk.
(does sdk provide something so we dont have to write regex?)
pkg/cloud/cloud.go
Outdated
| @@ -224,7 +224,7 @@ type EC2 interface { | |||
| DescribeAvailabilityZonesWithContext(ctx aws.Context, input *ec2.DescribeAvailabilityZonesInput, opts ...request.Option) (*ec2.DescribeAvailabilityZonesOutput, error) | |||
| } | |||
|
|
|||
| type Cloud interface { | |||
| type AWSCloud interface { | |||
There was a problem hiding this comment.
fair enough. note that i do eventually want to reconcile our cloud implementation with the generic aws cloudprovider implementation https://github.com/kubernetes/cloud-provider-aws/blob/master/pkg/providers/v2/cloud.go#L63 so "Cloud" is not overly ambiguous, the current naming is probably fine. #806 (comment)
There was a problem hiding this comment.
my IDE was complaining about Cloud being declared here and in a 3P dependency, but that went away so I don't rename it in PR #855
|
/hold i can see unit tests in progress |
k8s-ci-robot
added
the
do-not-merge/hold
|
@vdhanan: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vdhanan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/XS
|
@vdhanan: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Is this a bug fix or adding new feature?
fixes #474
What is this PR about? / Why do we need it?
uses downward api and k8s api to populate needed metadata fields
What testing is done?
some unit and manual testing - need to add some more unit tests before merge