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

azure: azure container registry: fix login server #40142

Merged
merged 1 commit into from
Jan 25, 2017
Merged

azure: azure container registry: fix login server #40142

merged 1 commit into from
Jan 25, 2017

Conversation

colemickens
Copy link
Contributor

What this PR does / why we need it:

Fixes the Azure Container Registry integration

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
n/a

Special notes for your reviewer:

Before this change, if I created an ACR with name colemicktest, then the login server would be colemicktest-microsoft.azurecr.io. This code was concating to form colemicktest.azurecr.io which does not work.

The fix is to reach into RegistryProperties and read out the login server domain name directly.

Also, this should eliminate that existed when ACR gets to sovereign clouds.

Release note:

azure: fix Azure Container Registry integration

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 19, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@colemickens colemickens force-pushed the colemickens-acr-login-server branch from 063b6f5 to 15cf8ca Compare January 19, 2017 12:37
@colemickens
Copy link
Contributor Author

cc: @brendandburns

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake 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.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake 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.

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE Node e2e failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot cri node e2e test this. Please help us cut down flakes by linking to an open flake 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.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake 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.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake 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.

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

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.

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.

@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

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.

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.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake 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.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

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.

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.

@k8s-ci-robot
Copy link
Contributor

Jenkins Bazel Build failed for commit 063b6f504c9da4f25ea3de6c549aaa6e3bfe31b4. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake 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.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 19, 2017
@colemickens colemickens force-pushed the colemickens-acr-login-server branch from 15cf8ca to 1c72f2b Compare January 19, 2017 13:08
@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 1c72f2b. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake 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.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 1c72f2b. Full PR test history. cc @colemickens

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake 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.

@colemickens colemickens mentioned this pull request Jan 19, 2017
Closed
@colemickens
Copy link
Contributor Author

@k8s-bot cvm gke e2e test this
@k8s-bot gci gce e2e test this

@k8s-github-robot
Copy link

k8s-github-robot commented Jan 24, 2017 •
edited
Loading

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@colemickens
Copy link
Contributor Author

ping @erictune @liggitt for review. this should be a fast review.

cc: @brendandburns again as well.

liggitt
liggitt reviewed Jan 24, 2017
View reviewed changes
pkg/credentialprovider/azure/azure_credentials.go
}
return cfg
}

func getLoginServer(registry containerregistry.Registry) string {
return *(*registry.RegistryProperties).LoginServer
Copy link
Member

Choose a reason for hiding this comment

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

is there always a default? does it need a fallback to old behavior if it's empty?

Copy link
Member

Choose a reason for hiding this comment

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

looks like the old default was wrong. should this return (string, error) and error if LoginServer is unset?

Copy link
Contributor Author

@colemickens colemickens Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It is always set. There's lots of places that we deref without checking the value in here and the cloudprovider.

If you'd prefer, I can add an error return value, error check and skip the registry if getLoginServer errors out. LMK if you want that change.

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly if it's guaranteed to be set

@liggitt
Copy link
Member

liggitt commented Jan 25, 2017

/approve

@liggitt liggitt assigned liggitt and unassigned erictune Jan 25, 2017
@liggitt
Copy link
Member

liggitt commented Jan 25, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 37055, 40142)

@k8s-github-robot k8s-github-robot merged commit d62fca2 into kubernetes:master Jan 25, 2017
@colemickens
Copy link
Contributor Author

@saad-ali can we get this cherry-picked for 1.5.x? Thanks.

@colemickens colemickens deleted the colemickens-acr-login-server branch February 3, 2017 21:07
@colemickens colemickens mentioned this pull request Feb 10, 2017
Merged
@saad-ali saad-ali added this to the v1.5 milestone Feb 10, 2017
@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 Feb 10, 2017
k8s-github-robot pushed a commit that referenced this pull request Feb 10, 2017
Merge pull request #41233 from colemickens/automated-cherry-pick-of-#…
12c7f09 
…40142-kubernetes-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #40142

Cherry pick of #40142 on release-1.5.

#40142: azure: acr fix login server
@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

@liggitt liggitt liggitt left review comments

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.
8 participants
@colemickens @k8s-reviewable @k8s-ci-robot @k8s-github-robot @liggitt @k8s-cherrypick-bot @erictune @saad-ali