refactor: allow variants to implement their own k8s providerID parsing logic #938
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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-sigs/prow repository. |
k8s-ci-robot
added
the
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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
cncf-cla: no
|
Welcome @sanjams2! |
k8s-ci-robot
added
needs-triage
|
Hi @sanjams2. Thanks for your PR. I'm waiting for a kubernetes 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
k8s-ci-robot
added
cncf-cla: yes
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@cartermckinnon any way you could give this a quick glance and see if there is anything glaring? thanks in advance! |
| import "github.com/aws/aws-sdk-go/aws" | ||
|
|
||
| // NodeID is the ID of a node used to uniquely identify that node within an AWS service | ||
| type NodeID string |
There was a problem hiding this comment.
I think this could be more useful as an interface, we have too many string alias types in here already 😛
Something like:
type AWSProviderID interface {
ComputeID() string
}
Then we can implement an EC2ProviderID, FargateProviderID, etc.
The func-s in the Variant interface can accept an AWSProviderID and cast to the relevant type as necessary, providerID.(EC2ProviderID)
There was a problem hiding this comment.
hmm I actually am not quite following the need for an interface here. While the NodeID might have different formats, I dont see why there would be different implementations required for those different formats. Along that same line, while it sounds potentially useful to have strongly typed NodeID's, there is currently not a need for it — the casting you mention wouldnt actually be necessary anywhere. That to me signals that an interface may be overkill, but perhaps Im not thinking about a future use-case where having an interface would be ideal.
So as it stands now, I actually think the ergonomics of the string alias type is fits the needs quite well. Let me know your thoughts.
There was a problem hiding this comment.
also, much less of a concern, but moving to an interface would substantially increase the size of the changes in this commit.
pkg/providers/v1/instances.go
Outdated
| // the following form | ||
| // - aws:///<zone>/<awsInstanceId> | ||
| // - aws:////<awsInstanceId> | ||
| // - aws:///<zone>/fargate-<eni-ip-address> | ||
| // - <awsInstanceId> | ||
| type KubernetesInstanceID string | ||
| type KubernetesProviderID string |
There was a problem hiding this comment.
This type doesn't seem very useful to me, can we get rid of it in favor of something like:
func ParseProviderID(providerID string) (AWSProviderID, error)
?
There was a problem hiding this comment.
Agree and makes sense. Will fix
…g logic == Motivation == Allow further variant specific customization == Details == This change adds the ability for variants to implement their own logic to parse a k8s providerID into an identifier specific to that variant. This is done by adding a new method 'NodeId' on each variant that takes the providerID (in pre-parsed url format) and returns a respective NodeId. NodeId is a refactor of the previous InstanceId which was inadequately named for variants other than EC2 instances (e.g. fargate). I have also taken this opportunity to add a bit more strong typing to the variant methods to make these methods a bit easier to grok and safer to implement. I have also renamed the KubernetesInstanceId to KubernetesProviderId which felt a more adequate name as well. Finally, in order to prevent circular depedencies and to maintain (what I felt) was a more logical concept of a NodeId, we have created a new submodule in the v1 package named 'awsnode' which contains the NodeId type. This allows both the base v1 module and the variant submodule to use this NodeId type withou circular dependencies. == Testing == make
|
The test errors seem unrelated to my change. I also cannot find an existing issue noting a similar symptom to these failures. |
|
/retest |
|
@sanjams2: The following tests 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-sigs/prow repository. I understand the commands that are listed here. |
| @@ -20,6 +20,7 @@ import ( | |||
| "context" | |||
| "fmt" | |||
| "io" | |||
| "k8s.io/cloud-provider-aws/pkg/providers/v1/awsnode" | |||
There was a problem hiding this comment.
nit: can we group the import correctly ?
| func (name KubernetesInstanceID) MapToAWSInstanceID() (InstanceID, error) { | ||
| s := string(name) | ||
|
|
||
| func ParseProviderID(providerID string) (awsnode.NodeID, error) { |
There was a problem hiding this comment.
i observed that we are making call to get variant followed by this method in most places. Can we return the variant also such that the caller can get both at the same time ?
|
/kind feature |
k8s-ci-robot
added
the
kind/feature
k8s-ci-robot
added
triage/accepted
k8s-ci-robot
added
the
needs-rebase
|
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-sigs/prow repository. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
== Motivation ==
Allow further variant specific customization
== Details ==
This change adds the ability for variants to implement their own logic to parse a k8s providerID into an identifier specific to that variant. This is done by adding a new method 'NodeId' on each variant that takes the providerID (in pre-parsed url format) and returns a respective NodeId. That said, the behavior of the existing Fargate variant and EC2 node are left unchanged.
NodeId is a refactor of the previous InstanceId which was inadequately named for variants other than EC2 instances (e.g. fargate). I have also taken this opportunity to add a bit more strong typing to the variant methods to make these methods a bit easier to grok and safer to implement. I have also renamed the KubernetesInstanceId to KubernetesProviderId which felt a more adequate name as well. Finally, in order to prevent circular depedencies and to maintain (what I felt) was a more logical concept of a NodeId, we have created a new submodule in the v1 package named 'awsnode' which contains the NodeId type. This allows both the base v1 module and the variant submodule to use this NodeId type withou circular dependencies.
I have opted not to change variables named
instanceIdor similar for the time being given the bloat that would cause on this CR size.Im pushing this change now to get feedback quickly. Can work on adding the necessary testing steps/release notes as required. Based on #917, it seems this may not be required for this change.