add bound serviceaccount token to machine-controller #743
Conversation
| Sources: []corev1.VolumeProjection{ | ||
| { | ||
| ServiceAccountToken: &corev1.ServiceAccountTokenProjection{ | ||
| Audience: "openshift", |
There was a problem hiding this comment.
I don't understand why we need this.
There was a problem hiding this comment.
the token or the openshift audience?
We need the token to present to STS to do the AssumeRoleWIthWebIdentity calll, which will be validated by the OpenID Connect Provider registered in IAM
We need to set the audience to the same audience the OpenID Connect Provider was registered to accept. openshift is an arbitrary, but platform-neutral choice.
There was a problem hiding this comment.
Should this be setting the expiry? IIRC that's a field in this struct right? What happens if we don't set the expiry?
There was a problem hiding this comment.
It defaults to 1h, which is in alignment with STS guidelines
https://github.com/kubernetes/kubernetes/blob/4b59044b8d2a3502ea490ba2c958008a098511a3/pkg/apis/core/v1/defaults.go#L273
Which is in alignment with default STS AssumeRole
https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html
By default, the temporary security credentials created by AssumeRole last for one hour.
We can explicitly set it if you prefer. Just let me know.
There was a problem hiding this comment.
I'd prefer to see it explicitly set with a comment saying this is the recommended default and linking to the AWS docs if that's ok?
joelddiaz
left a comment
joelddiaz
left a comment
There was a problem hiding this comment.
This looks good. I like the naming scheme for /var/run/secrets/openshift/serviceaccount.
|
/retest |
Add a projected service account token to the ingress-operator container in the ingress-operator pod. We will use this token to get STS creds when that is wired in. This is inert until then. see https://issues.redhat.com/browse/CO-1256 The audience `openshift` chosen is arbitrary, but platform-neutral choice. also see openshift/machine-api-operator#743 for similar consistent change.
Add a projected service account token to the ingress-operator container in the ingress-operator pod. We will use this token to get STS creds when that is wired in. This is inert until then. see https://issues.redhat.com/browse/CO-1256 The audience `openshift` chosen is arbitrary, but platform-neutral choice. also see openshift/machine-api-operator#743 for similar consistent change.
|
/hold I don't want to merge this until there is an enhancement or other formalized documentation that lays out exactly what these components are for and how they fit into the larger system. |
openshift-ci-robot
added
the
do-not-merge/hold
|
Would like to see the counterpart PR that implements this being used in the AWS code linked here as well, is that being worked on yet? |
|
@JoelSpeed i'm working on that PR now. will link here when i have it. |
|
@michaelgugino I think the details you're asking for are available in https://github.com/openshift/enhancements/blob/master/enhancements/cloud-integration/aws/aws-pod-identity.md and https://docs.google.com/document/d/1M8wij7fCD6XILkUX6CQezv2svgVT5cabteKWYBjVpn4/edit?ts=5fac13dd# |
Add a projected service account token to the ingress-operator container in the ingress-operator pod. We will use this token to get STS creds when that is wired in. This is inert until then. see https://issues.redhat.com/browse/CO-1256 The audience `openshift` chosen is arbitrary, but platform-neutral choice. also see openshift/machine-api-operator#743 for similar consistent change.
Add a projected service account token to the ingress-operator container in the ingress-operator pod. We will use this token to get STS creds when that is wired in. This is inert until then. see https://issues.redhat.com/browse/CO-1256 The audience `openshift` chosen is arbitrary, but platform-neutral choice. also see openshift/machine-api-operator#743 for similar consistent change.
Add a projected service account token to the ingress-operator container in the ingress-operator pod. We will use this token to get STS creds when that is wired in. This is inert until then. see https://issues.redhat.com/browse/CO-1256 The audience `openshift` chosen is arbitrary, but platform-neutral choice. also see openshift/machine-api-operator#743 for similar consistent change.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/lgtm |
|
/hold cancel We worked out some details in chat, we'll need to capture those in some kind of docs in our repo in the near future. |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
8 participants
@joelddiaz @michaelgugino
Add a projected service account token to the machine-controller container in the machine-api-operator pod. We will use this token to get STS creds when that is wired in.
This is inert until then.
https://issues.redhat.com/browse/CO-1254 subtask https://issues.redhat.com/browse/CO-1274