Bug 1411421 added information on linking pods to serviceaccounts #3475

bfallonf · 2017-01-11T05:04:28Z

As per: https://bugzilla.redhat.com/show_bug.cgi?id=1411421

@jim-minter I'll ask again in the BZ, but can I get an ack this fulfills the request? I am wondering if I've gotten some of the wording wrong. It's hard to tell with the info that's there...

Thanks!

jim-minter

some suggestions; the change to dev_guide/managing_images.adoc needs to be reverted. Thanks!

jim-minter · 2017-01-11T08:45:59Z

dev_guide/builds.adoc

@@ -947,6 +947,15 @@ source secrets used. Access is granted with the following command:
 $ oc secrets link builder mysecret
 ----

+[NOTE]
+====
+As of {product-title} version 3.0.2, limiting secrets to only the service


("3.0.2" only applies when {product-title} is OCP)

slightly clumsy wording, maybe just easier to say something along the lines of: if limitSecretReferences is false (default), this step is not required ?

jim-minter · 2017-01-11T08:46:42Z

dev_guide/builds.adoc

+accounts that reference them is disabled by default. This means that unless
+`serviceAccountConfig.limitSecretReferences` is set to `true` in the master
+configuration file, linking secrets to a service account will do
+nothing.


"is not required" may be better than "will do nothing"

jim-minter · 2017-01-11T08:53:12Z

dev_guide/managing_images.adoc

@@ -477,8 +477,9 @@ $ oc secrets new-dockercfg <pull_secret_name> \
    --docker-password=<password> --docker-email=<email>
 ----

-To use a secret for pulling images for pods, you must add the secret to your
-service account. The name of the service account in this example should match
+If `serviceAccountConfig.limitSecretReferences` is set to `true` in the master


I believe this paragraph change is wrong and should be reverted: --for=pull and serviceAccountConfig.limitSecretReferences are separate things.

@jim-minter Sorry, maybe I'm missing something. Is that not what you meant in the BZ:

"https://docs.openshift.com/container-platform/3.3/dev_guide/managing_images.html / Allowing Pods to Reference Images from Other Secured Registries

the advice about 'oc secrets link' is superfluous unless serviceAccountConfig.limitSecretReferences is on, which it is probably not in the majority of installs."

Am I missing something obvious?

The BZ wasn't desperately clear (sorry), but did mention not to review the 'oc secrets link --for=pull' case(s), of which this is one. serviceAccountConfig.limitSecretReferences being enabled affects the requirement to run 'oc secrets link' or 'oc secrets link --for=mount', but not 'oc secrets link --for=pull'.

@jim-minter OK. I'm still a bit confused why this was mentioned in the initial BZ, but I've reverted it. Let me know if this still is not what you're requesting.

jim-minter · 2017-01-11T08:53:51Z

dev_guide/service_accounts.adoc

@@ -170,6 +170,15 @@ To allow a secret to be mounted by a service account's pods, run:
 $ oc secrets link --for=mount <serviceaccount-name> <secret-name>
 ----

+[NOTE]
+====
+As of {product-title} version 3.0.2, limiting secrets to only the service


same notes as above apply

jim-minter · 2017-01-11T08:54:05Z

install_config/registry/securing_and_exposing_registry.adoc

@@ -76,6 +76,15 @@ $ oc secrets link registry registry-secret
 $ oc secrets link default  registry-secret
 ----
 +
+[NOTE]
+====
+As of {product-title} version 3.0.2, limiting secrets to only the service


same notes as above apply

jim-minter · 2017-01-11T08:56:43Z

dev_guide/service_accounts.adoc

@@ -170,6 +170,15 @@ To allow a secret to be mounted by a service account's pods, run:
 $ oc secrets link --for=mount <serviceaccount-name> <secret-name>
 ----

+[NOTE]


here you follow a section discussing --for=pull and --for=mount; it may be good to be clearer that the NOTE refers to --for=mount (which is also implied if no --for= is specified) only.

@jim-minter I see what you mean, which is why I made sure the last part of the note box says "...mounting secrets to a service account's pods is not required." Or did you mean that it should be more obvious?

Ah, I hadn't spotted the wording change (so IMO it's not clear enough ;). Now I re-read it, I'm concerned that the following statement is actually not true and doesn't make sense:

unless serviceAccountConfig.limitSecretReferences is set to true in the master configuration file, mounting secrets to a service account's pods will do nothing.

Something like the following would make sense:

if the admin has set limitSecretReferences to false (default), it is not required to use oc link --for=mount to enable use of a mountable secret by a service account. However oc link --for=pull is always required to enable use of an image pull secret, regardless of the value of limitSecretReferences.

OK. I combined the two. Should be better now.

bfallonf · 2017-01-16T05:20:57Z

@gaurav-nelson @ahardin-rh @bmcelvee This should be ok for peer review now ⭐

jim-minter · 2017-01-16T11:48:45Z

dev_guide/builds.adoc

+====
+Limiting secrets to only the service accounts that reference them is disabled by
+default. This means that if `serviceAccountConfig.limitSecretReferences` is set
+to `false` (the default setting)) in the master configuration file, linking


jim-minter · 2017-01-16T11:48:54Z

install_config/registry/securing_and_exposing_registry.adoc

+====
+Limiting secrets to only the service accounts that reference them is disabled by
+default. This means that if `serviceAccountConfig.limitSecretReferences` is set
+to `false` (the default setting)) in the master configuration file, linking


jim-minter · 2017-01-16T11:49:16Z

apart from the double ))s, lgtm, thanks

gaurav-nelson · 2017-01-17T01:41:42Z

dev_guide/builds.adoc

@@ -947,6 +947,14 @@ source secrets used. Access is granted with the following command:
 $ oc secrets link builder mysecret
 ----

+[NOTE]
+====
+Limiting secrets to only the service accounts that reference them is disabled by


to only the → only to the

gaurav-nelson · 2017-01-17T01:43:16Z

dev_guide/service_accounts.adoc

@@ -170,6 +170,17 @@ To allow a secret to be mounted by a service account's pods, run:
 $ oc secrets link --for=mount <serviceaccount-name> <secret-name>
 ----

+[NOTE]
+====
+Limiting secrets to only the service accounts that reference them is disabled by


same as above for all instances

gaurav-nelson · 2017-01-17T01:45:05Z

minor change, everything else is fine. ☑️

bfallonf · 2017-01-17T02:16:18Z

@gaurav-nelson As discussed face-to-face, Going to leave it as is.

If there's nothing else, I'll merge this.

bfallonf · 2017-01-17T02:23:39Z

[rev_history]
|xref:../dev_guide/builds.adoc#dev-guide-builds[Builds]
|Added note box about mounting secrets to service accounts.
%
[rev_history]
|xref:../dev_guide/service_accounts.adoc#dev-guide-service-accounts[Service Accounts]
|Added note box about mounting secrets to service accounts.
%
[rev_history]
|xref:../install_config/registry/securing_and_exposing_registry.adoc#install-config-registry-securing-exposing[Setting up the Registry -> Securing and Exposing the Registry]
|Added note box about mounting secrets to service accounts.
%

bfallonf force-pushed the secrets_1411421 branch from 20155fc to a3b48a0 Compare January 11, 2017 05:07

jim-minter suggested changes Jan 11, 2017

View reviewed changes

bfallonf force-pushed the secrets_1411421 branch 2 times, most recently from 38c2ada to 33bbba1 Compare January 13, 2017 01:54

jim-minter reviewed Jan 16, 2017

View reviewed changes

added information on linking pods to serviceaccounts

817b9f6

bfallonf force-pushed the secrets_1411421 branch from 33bbba1 to 817b9f6 Compare January 16, 2017 23:48

gaurav-nelson reviewed Jan 17, 2017

View reviewed changes

bfallonf added branch/dedicated branch/enterprise-3.2 branch/enterprise-3.3 branch/enterprise-3.4 branch/online labels Jan 17, 2017

bfallonf added this to the Next Release milestone Jan 17, 2017

bfallonf merged commit 15dfa6c into openshift:master Jan 17, 2017

bfallonf deleted the secrets_1411421 branch January 17, 2017 02:24

vikram-redhat modified the milestones: Next Release, Staging, OCP 3.4 GA Jan 18, 2017

vikram-redhat removed this from the Staging milestone Jan 18, 2017

adellape modified the milestones: OCP 3.4 GA, Staging, Temp Feb 1, 2017

adellape mentioned this pull request Feb 3, 2017

Revert source-secret-match-uri commits for OCP 3.3 #3658

Merged

vikram-redhat modified the milestones: OCP 3.4 GA, Dedicated-Staging Mar 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1411421 added information on linking pods to serviceaccounts #3475

Bug 1411421 added information on linking pods to serviceaccounts #3475

bfallonf commented Jan 11, 2017

jim-minter left a comment

jim-minter Jan 11, 2017

jim-minter Jan 11, 2017

jim-minter Jan 11, 2017

jim-minter Jan 11, 2017

bfallonf Jan 12, 2017

jim-minter Jan 12, 2017

bfallonf Jan 13, 2017

jim-minter Jan 11, 2017

jim-minter Jan 11, 2017

jim-minter Jan 11, 2017

bfallonf Jan 12, 2017

jim-minter Jan 12, 2017

bfallonf Jan 13, 2017

bfallonf commented Jan 16, 2017

jim-minter Jan 16, 2017

jim-minter Jan 16, 2017

jim-minter commented Jan 16, 2017

gaurav-nelson Jan 17, 2017

gaurav-nelson Jan 17, 2017

gaurav-nelson commented Jan 17, 2017

bfallonf commented Jan 17, 2017

bfallonf commented Jan 17, 2017

Bug 1411421 added information on linking pods to serviceaccounts #3475

Bug 1411421 added information on linking pods to serviceaccounts #3475

Conversation

bfallonf commented Jan 11, 2017

jim-minter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfallonf commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaurav-nelson commented Jan 17, 2017

bfallonf commented Jan 17, 2017

bfallonf commented Jan 17, 2017