user-configurable IAM roles for ServiceAccounts #11016

olemarkus · 2021-03-11T20:11:08Z

This PR allows users to configure IRSA for their own workloads. It is the kOps equivalent of https://eksctl.io/usage/iamserviceaccounts/#usage-with-config-files

/cc @justinsb @rifelpet

olemarkus · 2021-03-12T10:45:13Z

/retest

rifelpet

I don't see the OIDC provider being created in the integration test's terraform output. any idea why that is?

rifelpet · 2021-04-05T17:37:59Z

pkg/apis/kops/validation/legacy.go

+		}
+		if sa.IAMPolicyARN != "" && sa.InlinePolicy != "" {
+			allErrs = append(allErrs, field.Forbidden(p, "cannot set both inlinePolicy and iamPolicyARN"))
+		}


nit, might be worth validating the policy arn:

kops/pkg/apis/kops/validation/instancegroup.go

Lines 267 to 271 in b03b50f

parsedARN, err := arn.Parse(instanceProfileARN)

if err != nil || !strings.HasPrefix(parsedARN.Resource, "instance-profile/") {

allErrs = append(allErrs, field.Invalid(fldPath.Child("profile"), instanceProfileARN,

"Instance Group IAM Instance Profile must be a valid aws arn such as arn:aws:iam::123456789012:instance-profile/KopsExampleRole"))

}

pkg/model/iam.go

olemarkus · 2021-04-06T05:18:05Z

PublicJWKS feature flag isn't set on this particular test, so no OIDC config is created.

rifelpet · 2021-04-06T05:23:30Z

The iam roles' assume role policies reference arn:aws:iam::123456789012:oidc-provider/api.internal.minimal.example.com if that's not being created in kubernetes.tf nor specified anywhere in the cluster spec then where did it come from? Is this even a valid configuration?

olemarkus · 2021-04-06T05:40:15Z

Not really. Those two flags would almost always be used together.

It depends a bit what we want here. My idea was to add an enable flag on this struct, and that would mean provision OIDC (i.e PublicJWKS). Having this enabled, would also use IRSA on all kops addons that supports it (i.e UseServiceAccountIAM).

Question is if we want to force that? Or do we want users to enable IRSA, but not for the kops addons? Do we want users to enable IRSA individually on all addons?

rifelpet · 2021-04-06T05:58:30Z

Hm for migration purposes I could see the benefit of being able to migrate individual addons to IRSA. It'd be safer to migrate one at a time rather than be forced to do them all at once. I do like the idea of an enable field here that would supplement PublicJWKS. Perhaps we update the integration test to use both feature flags, and (potentially in a followup PR) add api validation to ensure PublicJWKS is enabled if UseServiceAccountIAM is enabled? And if we want to relax that restriction down the road (eg. to allow specifying an existing OIDC provider ARN in the cluster spec) then we can do that.

olemarkus · 2021-04-06T12:26:06Z

/retest

rifelpet · 2021-04-29T12:39:25Z

Does Cluster API or any of the relevant Cluster API providers support similar functionality? If so we could consider modeling our design off of theirs to reduce friction when adopting Cluster API support.

rifelpet · 2021-04-29T13:50:17Z

A summary of the discussion from office hours:

Move inlinePolicy and iamPolicyARNs to an aws substruct
Rename serviceAccountMappings to something indicating these are mappings to external permissions (cloud providers, etc.) serviceAccountExternalMappings was proposed.
We need to investigate if multiple fields are supported as a merge key for patch operations since an element in the serviceAccountMappings list would be keyed by name + namespace

As an aside, I think if we move iamPolicyARNs to an aws substruct we could probably drop the iam prefix from the field name, to be more consistent with inlinePolicy and because the presence of the aws field provides sufficient context that these policy ARNs are for AWS IAM policies.

olemarkus · 2021-04-29T18:53:09Z

Amended according to our discussions today.

Note that right now, one can configure this without enabling AWS OIDC provider. Once that is configurable, we may want to add validation for that. Easy thing that can be done in a follow-up.

olemarkus · 2021-04-30T16:36:24Z

/test pull-kops-e2e-cni-amazonvpc

johngmyers · 2021-04-30T17:38:43Z

Why use the word "mappings" for these? That word appears to be overly uninformative as to what is being provisioned. What is being provisioned is something more like "permissions" or "roles". They're the out-of-cluster analogue of Role and/or RoleBinding.

johngmyers · 2021-04-30T17:42:44Z

Okay, at least that word isn't exposed through the API.

pkg/apis/kops/cluster.go

johngmyers · 2021-04-30T17:57:59Z

pkg/apis/kops/validation/validation_test.go

+	featureflag.ParseFlags("+PublicJWKS")
+	unsetFeatureFlags := func() {
+		featureflag.ParseFlags("-PublicJWKS")
+	}


Would be good to not require the feature flag. Not blocking.

Apply suggestions from code review Co-authored-by: John Gardiner Myers <[email protected]>

johngmyers · 2021-05-01T18:47:18Z

pkg/apis/kops/cluster.go

+	PolicyARNs   []string `json:"policyARNs,omitempty"`
+	InlinePolicy string   `json:"inlinePolicy,omitempty"`


Could use comments on these.

pkg/apis/kops/validation/validation.go

johngmyers · 2021-05-01T19:00:25Z

pkg/featureflag/featureflag.go

 	UseServiceAccountIAM = New("UseServiceAccountIAM", Bool(false))
-	// PublicJWKS enables public jwks access.  This is generally not as secure as republishing.
+	// PublicJWKS enables public jwks access.


These changes are technically out of scope of this PR. Not blocking.

johngmyers · 2021-05-01T19:03:50Z

pkg/model/awsmodel/iam.go

@@ -124,26 +125,64 @@ func (b *IAMModelBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 	}

+	iamSpec := b.Cluster.Spec.IAM
+	if iamSpec != nil {
+		for _, sa := range iamSpec.ServiceAccountExternalPermissions {


Do we have code to delete the relevant AWS objects when items are removed from this list?

No, as usual, kOps is quite bad at this. I am wondering if we need a more generic solution that. Maybe we can tag all RenderAWS-created resources with kops.k8s.io/task=taskname and then at the end of cloudup, remove all resources that match tasks that wasn't created.

I think we need to look at that separately.

johngmyers · 2021-05-01T19:05:36Z

pkg/model/awsmodel/iam.go

+	if iamSpec != nil {
+		for _, sa := range iamSpec.ServiceAccountExternalPermissions {
+			var p *iam.Policy
+			aws := sa.AWS


We might consider handling aws == nil to save overlooking when someone adds a second external system.

Co-authored-by: John Gardiner Myers <[email protected]>

k8s-ci-robot · 2021-05-02T17:26:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [johngmyers]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from justinsb and rifelpet March 11, 2021 20:11

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/api labels Mar 11, 2021

olemarkus force-pushed the irsa-custom branch 3 times, most recently from 3c22b11 to 04e0253 Compare March 12, 2021 10:30

olemarkus changed the title ~~WIP user-configurable IAM roles for ServiceAccounts~~ user-configurable IAM roles for ServiceAccounts Mar 12, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2021

olemarkus added the kind/office-hours label Mar 20, 2021

olemarkus added this to the v1.21 milestone Apr 1, 2021

olemarkus force-pushed the irsa-custom branch from 04e0253 to f80cd0e Compare April 5, 2021 09:21

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2021

rifelpet reviewed Apr 5, 2021

View reviewed changes

olemarkus force-pushed the irsa-custom branch 3 times, most recently from c7f8abc to 93fbd3b Compare April 6, 2021 11:42

olemarkus mentioned this pull request Apr 19, 2021

Feature - Support AWS IAM Roles for Service Accounts #8264

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2021

johngmyers removed the kind/office-hours label Apr 23, 2021

olemarkus force-pushed the irsa-custom branch from 93fbd3b to fa7fb8a Compare April 24, 2021 08:39

olemarkus force-pushed the irsa-custom branch from 925df72 to fe8ced4 Compare April 29, 2021 18:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2021

olemarkus requested review from rifelpet and johngmyers April 30, 2021 12:54

olemarkus force-pushed the irsa-custom branch from fe8ced4 to 3df5506 Compare April 30, 2021 15:42

k8s-ci-robot added the area/provider/aws Issues or PRs related to aws provider label Apr 30, 2021

olemarkus force-pushed the irsa-custom branch from 3df5506 to 0bcfe0b Compare April 30, 2021 15:44

johngmyers requested changes Apr 30, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2021

olemarkus force-pushed the irsa-custom branch from 0bcfe0b to de8a7da Compare May 1, 2021 07:42

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2021

olemarkus force-pushed the irsa-custom branch 2 times, most recently from 06851c5 to cf410b1 Compare May 1, 2021 12:19

Add support for IRSA in he api

6f8b364

Apply suggestions from code review Co-authored-by: John Gardiner Myers <[email protected]>

olemarkus force-pushed the irsa-custom branch from cf410b1 to 6f8b364 Compare May 1, 2021 14:03

olemarkus requested a review from johngmyers May 1, 2021 18:35

johngmyers requested changes May 1, 2021

View reviewed changes

Apply suggestions from code review

6199174

Co-authored-by: John Gardiner Myers <[email protected]>

olemarkus force-pushed the irsa-custom branch from 1c2211b to 6199174 Compare May 2, 2021 05:57

johngmyers approved these changes May 2, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2021

k8s-ci-robot merged commit b054fb3 into kubernetes:master May 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

user-configurable IAM roles for ServiceAccounts #11016

user-configurable IAM roles for ServiceAccounts #11016

olemarkus commented Mar 11, 2021

olemarkus commented Mar 12, 2021

rifelpet left a comment

rifelpet Apr 5, 2021

olemarkus commented Apr 6, 2021

rifelpet commented Apr 6, 2021 •

edited

Loading

olemarkus commented Apr 6, 2021

rifelpet commented Apr 6, 2021

olemarkus commented Apr 6, 2021

rifelpet commented Apr 29, 2021

rifelpet commented Apr 29, 2021

olemarkus commented Apr 29, 2021

olemarkus commented Apr 30, 2021

johngmyers commented Apr 30, 2021

johngmyers commented Apr 30, 2021

johngmyers Apr 30, 2021

johngmyers May 1, 2021

johngmyers May 1, 2021

johngmyers May 1, 2021

olemarkus May 2, 2021

johngmyers May 1, 2021

k8s-ci-robot commented May 2, 2021

	parsedARN, err := arn.Parse(instanceProfileARN)
	if err != nil \|\| !strings.HasPrefix(parsedARN.Resource, "instance-profile/") {
	allErrs = append(allErrs, field.Invalid(fldPath.Child("profile"), instanceProfileARN,
	"Instance Group IAM Instance Profile must be a valid aws arn such as arn:aws:iam::123456789012:instance-profile/KopsExampleRole"))
	}

		PolicyARNs []string `json:"policyARNs,omitempty"`
		InlinePolicy string `json:"inlinePolicy,omitempty"`

user-configurable IAM roles for ServiceAccounts #11016

user-configurable IAM roles for ServiceAccounts #11016

Conversation

olemarkus commented Mar 11, 2021

olemarkus commented Mar 12, 2021

rifelpet left a comment

Choose a reason for hiding this comment

rifelpet Apr 5, 2021

Choose a reason for hiding this comment

olemarkus commented Apr 6, 2021

rifelpet commented Apr 6, 2021 • edited Loading

olemarkus commented Apr 6, 2021

rifelpet commented Apr 6, 2021

olemarkus commented Apr 6, 2021

rifelpet commented Apr 29, 2021

rifelpet commented Apr 29, 2021

olemarkus commented Apr 29, 2021

olemarkus commented Apr 30, 2021

johngmyers commented Apr 30, 2021

johngmyers commented Apr 30, 2021

johngmyers Apr 30, 2021

Choose a reason for hiding this comment

johngmyers May 1, 2021

Choose a reason for hiding this comment

johngmyers May 1, 2021

Choose a reason for hiding this comment

johngmyers May 1, 2021

Choose a reason for hiding this comment

olemarkus May 2, 2021

Choose a reason for hiding this comment

johngmyers May 1, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented May 2, 2021

rifelpet commented Apr 6, 2021 •

edited

Loading