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 pull secrets to service accounts #8582

Merged
merged 1 commit into from
May 27, 2015
Merged

add pull secrets to service accounts #8582

merged 1 commit into from
May 27, 2015

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented May 20, 2015

This adds ImagePullSecrets to the ServiceAccount type and restricts a pod's ability to reference ImagePullSecrets. If a pod lacks ImagePullSecrets, but has a service account that references ImagePullSecrets, the pod gets its list from the service account.

ImagePullSecrets is distinct from the Secrets on a ServiceAccount because Secrets may be mounted into the pod, but ImagePullSecrets are only available the kubelet.

@lavalamp Since you reviewed the pod pull secrets
@liggitt because it touches service accounts

/cc @smarterclayton @pmorie

@deads2k deads2k mentioned this pull request May 20, 2015
Closed
9 tasks
liggitt
liggitt reviewed May 20, 2015
View reviewed changes
pkg/kubelet/kubelet.go
secretRefs := []api.LocalObjectReference{}
secretRefs = append(secretRefs, pod.Spec.ImagePullSecrets...)

if len(pod.Spec.ServiceAccount) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

should the merge happen here or in the serviceaccounts admission controller?

@deads2k deads2k force-pushed the add-pull-secrets-to-service-account branch 2 times, most recently from 915d98a to 3d494e9 Compare May 21, 2015 18:55
liggitt
liggitt reviewed May 21, 2015
View reviewed changes
plugin/pkg/admission/serviceaccount/admission.go
}
for i, pullSecretRef := range pod.Spec.ImagePullSecrets {
if !pullSecrets.Has(pullSecretRef.Name) {
return fmt.Errorf(`imagePullSecrets["%d"].name="%s" is not allowed because service account %s does not reference that imagePullSecret`, i, pullSecretRef.Name, serviceAccount.Name)
Copy link
Member

Choose a reason for hiding this comment

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

don't quote the index in ["%d"]

@deads2k deads2k force-pushed the add-pull-secrets-to-service-account branch 2 times, most recently from def6e73 to dce9035 Compare May 21, 2015 19:32
@deads2k
Copy link
Contributor Author

deads2k commented May 21, 2015

comment addressed.

@pmorie
Copy link
Member

pmorie commented May 21, 2015

I am stoked that this in progress, won't have review bw until tomorrow
On Thu, May 21, 2015 at 3:33 PM David Eads notifications@github.com wrote:

comment addressed.

—
Reply to this email directly or view it on GitHub
#8582 (comment)
.

lavalamp
lavalamp reviewed May 21, 2015
View reviewed changes
plugin/pkg/admission/serviceaccount/admission.go
@@ -186,6 +187,10 @@ func (s *serviceAccount) Admit(a admission.Attributes) (err error) {
}
}

if len(pod.Spec.ImagePullSecrets) == 0 {
pod.Spec.ImagePullSecrets = serviceAccount.ImagePullSecrets
Copy link
Member

Choose a reason for hiding this comment

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

You should copy these, otherwise they could be modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should copy these, otherwise they could be modified.

Done.

@k8s-bot
Copy link

k8s-bot commented May 21, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

@deads2k
Copy link
Contributor Author

deads2k commented May 22, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

This is new. Is it similar to [test] in openshift? That would be awesome.

@ixdy
Copy link
Member

ixdy commented May 22, 2015

@deads2k it's intended to automatically run the e2e tests against GCE on our internal Jenkins instance. It's still a WIP, and was way spammier than I intended when I turned it on. It should be tuned now to only update the commit status (similar to Travis/Shippable) rather than spamming comments all the time.

Right now all it will do is just build everything, but I hope to turn on running tests sometime tomorrow.

@deads2k
Copy link
Contributor Author

deads2k commented May 22, 2015

it's intended to automatically run the e2e tests against GCE on our internal Jenkins instance.

That would be awesome. I'd really like rights to use it. Being able to do some e2e checks and another option run the whole e2e bucket would also be a nice to have feature.

@deads2k deads2k force-pushed the add-pull-secrets-to-service-account branch from dce9035 to f108465 Compare May 22, 2015 12:04
@pmorie
Copy link
Member

pmorie commented May 22, 2015

@k8s-bot ok to test

@deads2k
Copy link
Contributor Author

deads2k commented May 22, 2015

@pmorie surely you trust the user..... :)

lavalamp
lavalamp reviewed May 22, 2015
View reviewed changes
plugin/pkg/admission/serviceaccount/admission_test.go
err := admit.Admit(attrs)
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to mutate the pod's secret name and make sure the service account's secret name doesn't change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be nice to mutate the pod's secret name and make sure the service account's secret name doesn't change.

check added.

@deads2k deads2k force-pushed the add-pull-secrets-to-service-account branch 2 times, most recently from 5206874 to 590bd04 Compare May 22, 2015 18:05
@pmorie
Copy link
Member

pmorie commented May 22, 2015

This PR LGTM, @lavalamp, I'll leave it to you to tag.

@lavalamp
Copy link
Member

LGTM

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2015
@deads2k deads2k mentioned this pull request May 26, 2015
Merged
@deads2k
Copy link
Contributor Author

deads2k commented May 26, 2015

bump?

@mfojtik
Copy link
Contributor

mfojtik commented May 26, 2015

LGTM as well

@deads2k deads2k mentioned this pull request May 26, 2015
Merged
@smarterclayton
Copy link
Contributor

Rerunning Travis then will merge

@smarterclayton
Copy link
Contributor

Oh wait, on call is doing merges now. Leaving it, David, ping the on call when they get up.

@lavalamp
Copy link
Member

It looks like there's half a dozen PRs in the queue ahead of you, sorry.

@pmorie
Copy link
Member

pmorie commented May 27, 2015

@lavalamp any chance someone can take a look at this one tomorrow?

@pmorie
Copy link
Member

pmorie commented May 27, 2015

Nvm, @a-robinson explained the situation w/ the merge hangover from last week.

thockin added a commit that referenced this pull request May 27, 2015
@thockin
Merge pull request #8582 from deads2k/add-pull-secrets-to-service-acc…
a15c697 
…ount

add pull secrets to service accounts
@thockin thockin merged commit a15c697 into kubernetes:master May 27, 2015
@pmorie pmorie mentioned this pull request May 29, 2015
Closed
@deads2k deads2k deleted the add-pull-secrets-to-service-account branch June 11, 2015 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@deads2k @pmorie @k8s-bot @ixdy @lavalamp @mfojtik @smarterclayton @liggitt @googlebot @thockin