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.

Sign up for GitHub

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

Jump to bottom

Add support for Azure Container Registry, update Azure dependencies #37783

Merged
merged 3 commits into from
Dec 10, 2016
Merged

Add support for Azure Container Registry, update Azure dependencies #37783

merged 3 commits into from
Dec 10, 2016

Conversation

brendandburns
Copy link
Contributor

@brendandburns brendandburns added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 1, 2016
@brendandburns brendandburns added this to the v1.5 milestone Dec 1, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 1, 2016
@k8s-oncall
Copy link

This change is Reviewable

@brendandburns
Copy link
Contributor Author

Work in progress, needs unit tests, and integration with the kubelet binary.

@brendandburns brendandburns changed the title Add support for Azure Container Registry, update Azure dependencies WIP: Add support for Azure Container Registry, update Azure dependencies Dec 1, 2016
@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 1, 2016
@colemickens
Copy link
Contributor

colemickens commented Dec 1, 2016 •
edited

  1. I assume that the user has to go to ACR and grant access for the SP that the cluster is using? Presumably it doesn't inherit the RBAC (similar to KV/storage/etc)...

  2. Maybe the config parse/environment determination/SPToken creation could be refactored and shared with cloudprovider?

I can spot some compile-time errors, will hold off reviewing more until you push more changes.

@bgrant0607 bgrant0607 assigned colemickens and unassigned bgrant0607 Dec 1, 2016
@bgrant0607
Copy link
Member

@brendandburns It's really late to be adding this to 1.5.

cc @saad-ali

@brendandburns
Copy link
Contributor Author

@bgrant0607 it's totally modular and low risk (off everywhere except Azure)

@bgrant0607
Copy link
Member

@brendandburns This is the exception process for non-bugfix changes:
https://github.com/kubernetes/features/blob/master/EXCEPTIONS.md

@saad-ali
Copy link
Member

saad-ali commented Dec 2, 2016

Like Brian said, we are past feature freeze and are only accepting bug fixes for 1.5.0 at the moment. I'm going to remove the 1.5 label for now. This can be considered for a 1.5.x patch release once 1.5.0 is out

@saad-ali saad-ali removed this from the v1.5 milestone Dec 2, 2016
@brendandburns brendandburns changed the title WIP: Add support for Azure Container Registry, update Azure dependencies Add support for Azure Container Registry, update Azure dependencies Dec 2, 2016
@brendandburns
Copy link
Contributor Author

@colemickens test added, code fixed. Refactor is a todo?

@saad-ali I'll propose a cherry-pick it into the 1.5.1 release.

@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit 6ba2883. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

colemickens
colemickens approved these changes Dec 2, 2016
View reviewed changes
Copy link
Contributor

@colemickens colemickens left a comment

Choose a reason for hiding this comment

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

few small comments, otherwise looks good

pkg/credentialprovider/azure/azure_credentials.go
)

// init registers the various means by which credentials may
// be resolved on GCP.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/GCP/azure/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops copy/paste...

pkg/credentialprovider/azure/azure_credentials.go
)

var (
flagConfigFile = pflag.String("azure-container-registry-config", "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these wind up exposed on kubelet then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes (switched to pointers so it will actually work...)

pkg/credentialprovider/azure/azure_credentials.go
a.config.AADClientSecret,
a.environment.ServiceManagementEndpoint)
if err != nil {
glog.Errorf("Failed to load azure credential file: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Error message looks copy/pasted from above, should probably mention "failed to get service principal oauth token" or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

pkg/credentialprovider/azure/azure_credentials.go
return cfg
}
for ix := range *res.Value {
// TODO: I don't think this will work for national clouds
Copy link
Contributor

Choose a reason for hiding this comment

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

related: Azure/go-autorest#102

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2016
@brendandburns
Copy link
Contributor Author

Comments addressed, rebased, please take another look.

colemickens
colemickens reviewed Dec 6, 2016
View reviewed changes
pkg/credentialprovider/azure/azure_credentials.go
var (
flagConfigFile = pflag.String("azure-container-registry-config", "",
"Path to the file container Azure container registry configuration information.")
flagConfigEmail = pflag.String("azure-container-registry-email", "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe consider a default value? I don't think ACR uses it at all (currently?) and even Docker moved away from it. I imagine we'd just put a dummy value here most of the time anyway.

@colemickens
Copy link
Contributor

One other nit comment.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 0420d1d8e6e86b874a3e6798500385d5893313f2. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 91f19e3. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@brendandburns
Copy link
Contributor Author

@k8s-bot gce etcd3 e2e test this issue: #33380

@brendandburns brendandburns added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2016
@brendandburns
Copy link
Contributor Author

re-lgtm on cole's LGTM above.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 025c57e into kubernetes:master Dec 10, 2016
@luxas
Copy link
Member

luxas commented Dec 10, 2016

This broke the cross-build, due to that we're vendoring an old version of github.com/docker/docker/pkg/term/windows/ that was coded against an old version of github.com/Azure/go-ansiterm. So now when we bumped the version of github.com/Azure/go-ansiterm here, breaking changes in go-ansiterm makes pkg/term/windows fail so we broke all windows builds at once.

We could of course try to vendor a newer version of github.com/docker/docker/pkg/term/windows/ alone, but that's risky and might take some time depending on how old version we have vendored now.

I would suggest we revert this ASAP and try merging a new version of this PR again later this week.
In the future, please comment "@k8s-bot build this" on all vendoring changes you make in order to be confident it won't break the queue.

cc @kubernetes/sig-testing @k8s-oncall

@luxas luxas mentioned this pull request Dec 10, 2016
Merged
@colemickens
Copy link
Contributor

@luxas I'm a bit confused. This PR bumps go-autorest but not go-ansiterm. Or is there a transitive dependency problem?

@luxas
Copy link
Member

luxas commented Dec 10, 2016

@colemickens This PR does indeed bump go-ansiterm: https://github.com/brendandburns/kubernetes/blob/91f19e3dfc1f5c282032122beefa71d7ab026561/Godeps/Godeps.json#L62

That makes the crossbuild fail: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-cross-build/211

The vendored package github.com/docker/docker/pkg/term/windows is coded against an old version of go-ansiterm, and now when go-ansiterm was bumped here and contained breaking changes compared to the earlier version Azure/go-ansiterm@70b2c90, so now github.com/docker/docker/pkg/term/windows fails to compile.

Can we revert this temporarily to unblock the crossbuild? We really need the cross-build to be healthy, especially tomorrow when kubeadm is gonna be released.

@colemickens
Copy link
Contributor

Sorry, I see it now. I guess if it's blocking a kubeadm release it would make sense to revert to unblock since this is currently targeting 1.5.1 anyway.

Though, I'm wondering, does it build if you revert just the bump to go-ansiterm, I'm wondering if @brendandburns bumped all github.com/Azure/... packages since azure-sdk-for-go and go-autorest don't seem to depend on go-ansiterm.

@luxas
Copy link
Member

luxas commented Dec 10, 2016

@colemickens I can test tomorrow, hopefully that will fix the issue. If it doesn't work, I have to revert unfortunately.

Until v1.6, kubeadm releases are dependent on the cross-build for getting the official binary builds, so this is important in that sense to get v1.5 out of the door. (Due to the split-out kubeadm from the monorepo process, etc.)
I'm kind of coordinating the kubeadm release process, see here for more info: kubernetes/kubeadm#70

In the future, we should build some automation that automatically runs the crossbuild ci on presubmit for PRs that touch vendor/ to avoid this situation. cc @kubernetes/sig-testing

@luxas
Copy link
Member

luxas commented Dec 10, 2016

Or wait, it might make sense to just revert, otherwise it won't be able to get cherrypicked safely into v1.5.
If this exact PR would go right into the 1.5 branch, it would block the release.
So to minimize the risk, it might be best to do this in a new PR, exactly the same thing but omitting go-ansiterm if possible.

WDYT @colemickens about that?

cc @saad-ali

@luxas luxas mentioned this pull request Dec 11, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Dec 12, 2016
Merge pull request #38584 from luxas/fix_vendored_azuredep
7671ccd 
Automatic merge from submit-queue

Revert to version 70b2c90b of the vendored package github.com/Azure/go-ansiterm

Fixes the problem described in #37783 (comment)

Fixes #38073

@ixdy @gmarek @colemickens @brendandburns
@colemickens
Copy link
Contributor

@brendandburns Did you already initiate the cherry pick process for this?

@luxas
Copy link
Member

luxas commented Dec 17, 2016

If it's gonna be cherrypicked, make sure you include the revert commit in PR as well

@saad-ali saad-ali added this to the v1.5 milestone Dec 20, 2016
@saad-ali saad-ali added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Dec 20, 2016
k8s-github-robot pushed a commit that referenced this pull request Dec 20, 2016
Merge pull request #38943 from brendandburns/automated-cherry-pick-of…
2c59bd9 
…-#37783-upstream-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #37783 upstream release 1.5

@saad-ali
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colemickens colemickens colemickens approved these changes

Assignees

@colemickens colemickens

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@brendandburns @k8s-oncall @colemickens @bgrant0607 @saad-ali @k8s-ci-robot @k8s-github-robot @luxas @k8s-cherrypick-bot