Skip to content
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

Add Alibaba Cloud Provider #696

Merged
merged 7 commits into from
Aug 31, 2018
Merged

Conversation

xianlubird
Copy link
Contributor

Add Alibaba Cloud provided

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 30, 2018
@xianlubird xianlubird changed the title Add Alibaba Cloud provided Add Alibaba Cloud Provider Aug 30, 2018
@xianlubird xianlubird force-pushed the aliyun branch 2 times, most recently from c45bbf2 to 0d992c4 Compare August 30, 2018 10:32
@njuettner
Copy link
Member

@xianlubird thanks for open the PR 🎉 and a new provider 🙂.
We'll have a look.

@xianlubird
Copy link
Contributor Author

@njuettner
For the continuous-integration/travis-ci/pr fail message

!!! 'gofmt -s' needs to be run on the following files: 
./provider/google.go
./provider/google_test.go
FAILED   /home/travis/gopath/src/github.com/kubernetes-incubator/external-dns/vendor/github.com/kubernetes/repo-infra/verify/go-tools/verify-gofmt.sh	0s

I didn't change /provider/google.go and ./provider/google_test.go , and in my workspace, I run verify shell, it was fine.

SUCCESS  /go/src/github.com/kubernetes-incubator/external-dns/vendor/github.com/kubernetes/repo-infra/verify/go-tools/verify-gofmt.sh	1s

So cloud you give me some advice on how to fix it, thanks.

@njuettner
Copy link
Member

It fails due to a change from go 1.10 to go 1.11. gofmt behaves differently.
If you have already go 1.11 could you run gofmt -s for those files (google.go, google_test.go)?

@@ -0,0 +1,365 @@
# Setting up ExternalDNS for Services on Alibaba Cloud

This tutorial describes how to setup ExternalDNS for usage within a Kubernetes cluster on Alibaba Cloud. Make sure to use **>=0.4** version of ExternalDNS for this tutorial
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to change 0.4 to version 0.5.6. This would be the next version incl. Alibaba Cloud


if cfg.RoleName != "" {
provider.setNextExpire(cfg.ExpireTime)
go provider.refreshStsToken(1 * time.Second)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this could be also configurable via flag or something. refreshing every 1 second sounds eager to me. WDTY?

Copy link
Contributor Author

@xianlubird xianlubird Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • roleName is a Alibaba Cloud STS Token role, it can be fetch from ECS internal API, so it don't need to be config via flag.
  • refreshStsToken, the first time is 1 second, but from the next time it will be turned to 9 minute.
func (p *AlibabaCloudProvider) refreshStsToken(sleepTime time.Duration) {
    for {
        time.Sleep(sleepTime)
        now := time.Now()
        sleepTime = p.nextExpire.Sub(nowTime)
        log.Infof("Distance expiration time %v", sleepTime)
        if sleepTime < time.Duration(10*time.Minute) {
	       sleepTime = time.Duration(time.Second * 1)
        } else {
	       sleepTime = time.Duration(9 * time.Minute)
       }
     }
}

So in most situation, the refresh loop time will be 9 minute. I think it's fine.

@njuettner
Copy link
Member

Another question I just saw that your tests cover roughly 60% of your code, do you feel confident enough that everything works as expected?

Other than my small notes it looks good to me.

@njuettner njuettner self-assigned this Aug 30, 2018
@xianlubird
Copy link
Contributor Author

xianlubird commented Aug 31, 2018

@njuettner

  • I have already change version to 0.5.6 according to your comments
  • Foe the test case, I add some, now it's 68.1%. It has cover most situation including add, update, delete public & private zone. And I also test it in product environment, we have support many users in Alibaba Cloud. I will also keep improve features & bug.

Thanks for your review. :-)

@njuettner
Copy link
Member

Agreed, thanks for your hard work and offering Alibaba Cloud as new provider 🚀 .
As I've already mentioned it will released with v0.5.6.

@njuettner njuettner merged commit 0f46801 into kubernetes-sigs:master Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. new-provider size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants