Skip to content

add bound serviceaccount token to machine-controller #743

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

Merged
openshift-merge-robot merged 1 commit into from
Dec 5, 2020
Merged

add bound serviceaccount token to machine-controller #743

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions pkg/operator/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,21 @@ func newPodTemplateSpec(config *OperatorConfig, features map[string]bool) *corev
},
},
},
{
Name: "bound-sa-token",
VolumeSource: corev1.VolumeSource{
Projected: &corev1.ProjectedVolumeSource{
Sources: []corev1.VolumeProjection{
{
ServiceAccountToken: &corev1.ServiceAccountTokenProjection{
Audience: "openshift",
Copy link
Contributor

@michaelgugino michaelgugino Nov 4, 2020

Choose a reason for hiding this comment

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

I don't understand why we need this.

Copy link
Contributor Author

@sjenning sjenning Nov 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JoelSpeed JoelSpeed Nov 12, 2020

Choose a reason for hiding this comment

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

Should this be setting the expiry? IIRC that's a field in this struct right? What happens if we don't set the expiry?

Copy link
Contributor Author

@sjenning sjenning Nov 13, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JoelSpeed JoelSpeed Nov 13, 2020

Choose a reason for hiding this comment

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

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?

Path: "token",
},
},
},
},
},
},
}
volumes = append(volumes, newRBACConfigVolumes()...)

Expand Down Expand Up @@ -675,6 +690,11 @@ func newContainers(config *OperatorConfig, features map[string]bool) []corev1.Co
Name: "trusted-ca",
ReadOnly: true,
},
{
MountPath: "/var/run/secrets/openshift/serviceaccount",
Name: "bound-sa-token",
ReadOnly: true,
},
},
},
{
Expand Down