-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
✨ (go/v3) create and bind to a non-default service account #2070
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,6 @@ roleRef: | |
name: proxy-role | ||
subjects: | ||
- kind: ServiceAccount | ||
name: default | ||
name: controller-manager | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to change the default value? Can we not only address the change without change the names of what is scaffold currently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the manager's namespace is changed to a one shared by more than one access-controlled object, but the service account remains as "default", RBAC already bound to "default" will be applied to the manager, on top of the manager's RBAC. This is insecure from an admin perspective. A more secure default is to create a manager-specific service account so RBAC is never polluted. |
||
namespace: system | ||
` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
Copyright 2021 The Kubernetes Authors. | ||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
http://www.apache.org/licenses/LICENSE-2.0 | ||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package rbac | ||
|
||
import ( | ||
"path/filepath" | ||
|
||
"sigs.k8s.io/kubebuilder/v3/pkg/model/file" | ||
) | ||
|
||
var _ file.Template = &ServiceAccount{} | ||
|
||
// ServiceAccount scaffolds a file that defines the service account the manager is deployed in. | ||
type ServiceAccount struct { | ||
file.TemplateMixin | ||
} | ||
|
||
// SetTemplateDefaults implements file.Template | ||
func (f *ServiceAccount) SetTemplateDefaults() error { | ||
if f.Path == "" { | ||
f.Path = filepath.Join("config", "rbac", "service_account.yaml") | ||
} | ||
|
||
f.TemplateBody = serviceAccountTemplate | ||
|
||
return nil | ||
} | ||
|
||
const serviceAccountTemplate = `apiVersion: v1 | ||
kind: ServiceAccount | ||
metadata: | ||
name: controller-manager | ||
namespace: system | ||
` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,11 @@ var _ = Describe("kubebuilder", func() { | |
}) | ||
|
||
It("should generate a runnable project", func() { | ||
// go/v3 uses a unqiue-per-project service account name, | ||
// while go/v2 still uses "default". | ||
tmp := kbc.Kubectl.ServiceAccount | ||
kbc.Kubectl.ServiceAccount = "default" | ||
defer func() { kbc.Kubectl.ServiceAccount = tmp }() | ||
GenerateV2(kbc) | ||
Run(kbc) | ||
}) | ||
|
@@ -166,7 +171,7 @@ func Run(kbc *utils.TestContext) { | |
_, err = kbc.Kubectl.Command( | ||
"create", "clusterrolebinding", fmt.Sprintf("metrics-%s", kbc.TestSuffix), | ||
fmt.Sprintf("--clusterrole=e2e-%s-metrics-reader", kbc.TestSuffix), | ||
fmt.Sprintf("--serviceaccount=%s:default", kbc.Kubectl.Namespace)) | ||
fmt.Sprintf("--serviceaccount=%s:%s", kbc.Kubectl.Namespace, kbc.Kubectl.ServiceAccount)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the kind of PR that will require changes in the SDK side such as:
So, instead of manually change ansible and helm regards the changes in the config then, I'd like to see if we can first:
See that how much changes like this one we merge before that, harder will be for us to align and keep maintained the SDK side. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The addition of a service account is straightforward, improves default security, and is an often requested feature by users. The PR’s/steps you linked are unrelated and need a lot of review. Other PR’s aren’t being blocked by them so the argument that this one should doesn’t hold water. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, are you planning to get it merged and as follow up;
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would interpret this as: In order to apply all these changes downstream, there is a lot of work involved. Plugins phase 1.5 would enable us to reduce the wwork needed downstream to pinning to a kubebuilder version that contains a feature like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that true though? Why would merging this now prevent ansible/helm plugins from receiving the same changes later through phase 1.5 or some other machination mentioned above? Wouldn't they just get this feature for free then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would definetely work the way you mention. I just meant that once phase 1.5 is implemented, the process will be much easier as changing them in one place will update all (after a kubebuilder bump). I think the point of this conversation was just to show how right now they need to be updated in 3 or 4 different places while with 1.5 would only require to be updated in one place. But by no means does that mean that it is blocked, just that it requires more work as it is right now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I guess my point that this PR adds a feature that doesn’t necessarily need to be added to other plugins right now wasn’t clear. For example, neither of the operator-sdk’s own plugins support component configs yet the Go plugin does, and that disparity was acceptable at the time. Also, a PR here shouldn’t be blocked by downstream stuff. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My comment here was we will spend a lot of effort which shows unnecessary if we agree to merge the open prs. Then, if it is not urgent(can wait until Tue) why spend all this effort at all? However, if it gets merged before that and we do not apply these changes for helm/ansible in sdk the, it means that it also needs to remember to do that as well to address the pr to sync #2015 with sdk. Otherwise, it is fine. I am ok with the changes. /lgtm /hold I hope that clarifies my comments and motivations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm not mistaken this should be merged after #2071, right? It seems to have the same changes also inside here. By the way this also has my LGTM. Even if phase 1.5 was implemented, we would have to do this here, so that doesn't make a difference. |
||
ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
||
_ = curlMetrics(kbc) | ||
|
@@ -263,18 +268,23 @@ func Run(kbc *utils.TestContext) { | |
// curlMetrics curl's the /metrics endpoint, returning all logs once a 200 status is returned. | ||
func curlMetrics(kbc *utils.TestContext) string { | ||
By("reading the metrics token") | ||
b64Token, err := kbc.Kubectl.Get(true, "secrets", "-o=jsonpath={.items[0].data.token}") | ||
// Filter token query by service account in case more than one exists in a namespace. | ||
query := fmt.Sprintf(`{.items[?(@.metadata.annotations.kubernetes\.io/service-account\.name=="%s")].data.token}`, | ||
kbc.Kubectl.ServiceAccount, | ||
) | ||
b64Token, err := kbc.Kubectl.Get(true, "secrets", "-o=jsonpath="+query) | ||
ExpectWithOffset(2, err).NotTo(HaveOccurred()) | ||
token, err := base64.StdEncoding.DecodeString(strings.TrimSpace(b64Token)) | ||
ExpectWithOffset(2, err).NotTo(HaveOccurred()) | ||
ExpectWithOffset(2, len(token)).To(BeNumerically(">", 0)) | ||
|
||
By("creating a curl pod") | ||
cmdOpts := []string{ | ||
"run", "--generator=run-pod/v1", "curl", "--image=curlimages/curl:7.68.0", "--restart=OnFailure", "--", | ||
"run", "--generator=run-pod/v1", "curl", "--image=curlimages/curl:7.68.0", "--restart=OnFailure", | ||
"--serviceaccount=" + kbc.Kubectl.ServiceAccount, "--", | ||
"curl", "-v", "-k", "-H", fmt.Sprintf(`Authorization: Bearer %s`, token), | ||
fmt.Sprintf("https://e2e-%v-controller-manager-metrics-service.e2e-%v-system.svc:8443/metrics", | ||
kbc.TestSuffix, kbc.TestSuffix), | ||
fmt.Sprintf("https://e2e-%s-controller-manager-metrics-service.%s.svc:8443/metrics", | ||
kbc.TestSuffix, kbc.Kubectl.Namespace), | ||
} | ||
_, err = kbc.Kubectl.CommandInNamespace(cmdOpts...) | ||
ExpectWithOffset(2, err).NotTo(HaveOccurred()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,4 +52,5 @@ spec: | |
requests: | ||
cpu: 100m | ||
memory: 20Mi | ||
serviceAccountName: controller-manager | ||
terminationGracePeriodSeconds: 10 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
apiVersion: v1 | ||
kind: ServiceAccount | ||
metadata: | ||
name: controller-manager | ||
namespace: system |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,4 +50,5 @@ spec: | |
requests: | ||
cpu: 100m | ||
memory: 20Mi | ||
serviceAccountName: controller-manager | ||
terminationGracePeriodSeconds: 10 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
apiVersion: v1 | ||
kind: ServiceAccount | ||
metadata: | ||
name: controller-manager | ||
namespace: system |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,4 +52,5 @@ spec: | |
requests: | ||
cpu: 100m | ||
memory: 20Mi | ||
serviceAccountName: controller-manager | ||
terminationGracePeriodSeconds: 10 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
apiVersion: v1 | ||
kind: ServiceAccount | ||
metadata: | ||
name: controller-manager | ||
namespace: system |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,4 +52,5 @@ spec: | |
requests: | ||
cpu: 100m | ||
memory: 20Mi | ||
serviceAccountName: controller-manager | ||
terminationGracePeriodSeconds: 10 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
apiVersion: v1 | ||
kind: ServiceAccount | ||
metadata: | ||
name: controller-manager | ||
namespace: system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using a prefix/suffix with the project name so that multiple managers created in different projects don't conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you
kustomize build config/default
, the namePrefix is applied to the ServiceAccount’s name and all references to it since that name is a builtin nameReference.