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

Copy auth plugin to client-go repo #33334

Merged
merged 2 commits into from
Sep 27, 2016

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented Sep 22, 2016

client-go doesn't copy the auth plugin. This causes user cannot access cluster run by GKE. User will see error "No Auth Provider found for name gcp".

This PR fixes this issue. It's marked as WIP because I'll need to rebase after #32906 gets merged. Also, the fix needs to be cherry-picked into 1.4 branch to update client-go/1.4.


This change is Reviewable

@caesarxuchao caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Sep 22, 2016
@caesarxuchao
Copy link
Member Author

I created the PR to see if it can pass the gke smoke test. There is a twist there.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 22, 2016
@@ -131,7 +131,7 @@ function mvfolder {
# the first rule is to convert import lines like `restclient "k8s.io/client-go/pkg/client/restclient"`,
# where a package alias is the same the package name.
find "${CLIENT_REPO}" -type f -name "*.go" -print0 | \
xargs -0 sed -i "s,${src_package} \"${CLIENT_REPO_FROM_SRC}/${src},${dst_package} \"${CLIENT_REPO_FROM_SRC}/${dst},g"
xargs -0 sed -i "s,\<${src_package} \"${CLIENT_REPO_FROM_SRC}/${src},${dst_package} \"${CLIENT_REPO_FROM_SRC}/${dst},g"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this change is doing. Could you explain?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's to avoid rewriting such import:

import (
fcache "k8s.io/client-go/pkg/client/cache"
)

to

import (
fcore "k8s.io/client-go/core"
)

because when rewriting invocations, the script search for whole word, it does not rewrite the invocation from fcache to fcore.

find "${CLIENT_REPO}" -type f -name "*.go" -print0 | xargs -0 sed -i "s,\<${src_package}\.\([a-zA-Z]\),${dst_package}\.\1,g"
fi

{ grep -Rl "\"${CLIENT_REPO_FROM_SRC}/${src}" "${CLIENT_REPO}" || true ; } | while read target ; do
Copy link
Contributor

Choose a reason for hiding this comment

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

read -r so it doesn't mangle any backslashes

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

# rewrite imports
# the first rule is to convert import lines like `restclient "k8s.io/client-go/pkg/client/restclient"`,
# where a package alias is the same the package name.
sed -i "s,\<${src_package} \"${CLIENT_REPO_FROM_SRC}/${src},${dst_package} \"${CLIENT_REPO_FROM_SRC}/${dst},g" ${target}
Copy link
Contributor

Choose a reason for hiding this comment

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

double quote ${target} here and below

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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

Jenkins Kubemark GCE e2e failed for commit aba615cd39efa9df5695302e31031a6cffc0bba3. Full PR test history.

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.

@caesarxuchao
Copy link
Member Author

@k8s-bot e2e test this, issue #33175

@caesarxuchao caesarxuchao changed the title [WIP] Copy auth plugin to client-go repo Copy auth plugin to client-go repo Sep 23, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit aba615cd39efa9df5695302e31031a6cffc0bba3. Full PR test history.

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

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit aba615cd39efa9df5695302e31031a6cffc0bba3. Full PR test history.

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

@caesarxuchao
Copy link
Member Author

Tests failed with,

group apps has not been registered

Looks like I need to update some import_known_versions.go.

@caesarxuchao
Copy link
Member Author

@krousey this PR is ready. PTAL.

The commit "remove special clientrepo code from main repository gcp plugin" maybe the a little mysterious. Because our e2e test uses client-go, I used to add the special initialization code for auth plugin for client-go. It's not needed anymore. Because client-go/kubernetes/clientset.go will initialize the auth plugin for client-go.

Please let me know if there are other non-obvious changes. Thanks!

@krousey krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2016
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Sep 26, 2016
@caesarxuchao caesarxuchao added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Sep 26, 2016
Chao Xu added 2 commits September 26, 2016 15:40
rename plugin/pkg/client/auth/plugins.go package name to auth

add the plugin import line in client-gen

update import_known_versions for release_1_5 clientset

change copy.sh
run copy.sh
@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 26, 2016
@caesarxuchao
Copy link
Member Author

Reapplying the lgtm after squash.

@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

@k8s-github-robot k8s-github-robot merged commit 1e7fa1f into kubernetes:master Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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.

5 participants