-
Notifications
You must be signed in to change notification settings - Fork 579
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
🌱 Support using custom endpoints for AWS services #1858
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
Welcome @Promaethius! |
Hi @Promaethius. 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. |
I'm thinking that this might have to extend to the
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
Think this one might be tricky if we need to modify the cloud controller manager on the created workload cluster. As @detiber notes, this might be more appropriate as part of the multi-tenancy work in progress. Will get back to you on how to deal with this. |
@randomvariable did you get direction? I got word back from aws-go-sdk that C2S endpoints should be specified through this custom parser. I'm worried about scope now cause that may mean if I define custom service endpoints here that they should become available to the Python CLI and the kubelet Cloud provider plugin. |
@Promaethius I didn't get round to it this week, but promise to answer on Monday. That said, the custom service endpoints won't have downstream impacts on other processes. The main issue we have here is how to make sure the cloud provider also follows the endpoints. It may be the answer is that you use a kubeadmconfigtemplate and KCP with the appropriate settings. The wider issue is that we're implementing a CRD that will hold different types of AWS credentials, and we might want to extend it to also include the service endpoints. This is being tracked for v0.6.1 in #1753 |
@randomvariable what I'm thinking for configuring endpoints of the aws cli and kubelet cloud provider:
Now here's where I have an issue with what @detiber pointed out above. In the case of multi-tennant workloads that require different sets of credentials, there must be logic on the templating side that selects a CRD which corresponds to a machine pool and renders the correct service endpoint. However, pools don't have the ability to select credentials; they're only passed a Profile parameter which is separate from the Manager's Credentials. Unsure of how to approach this. |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
I backtracked the session handler functionality through both EKS and
Machine so it should be working for both.
As for the resolver, this is pretty much lifted out from the aws resolver
example.
…On Fri, Sep 25, 2020, 7:09 AM Naadir Jeewa ***@***.***> wrote:
***@***.**** commented on this pull request.
Thanks for this. Some minor changes.
Also, can we please wire this up in the EKS and bootstrap controllers, so
we have consistency across them?
------------------------------
In pkg/cloud/scope/session.go
<#1858 (comment)>
:
> + resolver := func(service, region string, optFns ...func(*endpoints.Options)) (endpoints.ResolvedEndpoint, error) {
+ for _, s := range endpoint {
+ if service == s.ServiceID {
+ return endpoints.ResolvedEndpoint{
+ URL: s.URL,
+ SigningRegion: s.SigningRegion,
+ }, nil
+ }
+ }
+ return endpoints.DefaultResolver().EndpointFor(service, region, optFns...)
+ }
Move this anonymous function out. Might be the former ruby dev in me, but
easier to unit test this way.
------------------------------
In pkg/cloud/endpoints/endpoints.go
<#1858 (comment)>
:
> @@ -0,0 +1,122 @@
+/*
+Copyright 2018 The Kubernetes Authors.
⬇️ Suggested change
-Copyright 2018 The Kubernetes Authors.
+Copyright 2020 The Kubernetes Authors.
I know it's my copy-pasta, soz.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1858 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFFNN3ABYF7RZE3YTJRFBLSHR3AJANCNFSM4PT6YNEA>
.
|
thanks @Promaethius . could you also update the copyright header to the correct year? |
Need to add the command line flag to the respective main.gos in |
@randomvariable Added it to |
Can you rebase the branch rather than merging master into it? |
|
/test ? |
@randomvariable: The following commands are available to trigger jobs:
Use
In response to this:
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. |
/test pull-cluster-api-provider-aws-e2e |
I think there's something wrong with the resultant bootstrap script. Please take a look, and verify with a local run against AWS. |
the bash if statement didn't evaluate correctly if there weren't spaces around the quotes :P |
/test pull-cluster-api-provider-aws-e2e |
1 similar comment
/test pull-cluster-api-provider-aws-e2e |
Ah, the classic |
@randomvariable e2e tests passed :D |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: randomvariable The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it: Allows custom endpoints for aws services to be specified for the aws provider. Not only would this allow for local stack testing but would pave the road for Cluster API into AWS IC partitions which require FIPS named endpoints.
Which issue(s) this PR fixes:
Fixes #1855