Instance roles for service accounts (IRSA) contd #10756

olemarkus · 2021-02-07T20:30:42Z

This is a continuation of #9271

The workflow is changed a little bit. Users now only need to provide a publicsDataStore VFS (only S3 works now) and kOps will handle the rest.

This PR also works with #9352. dns-controller gets a dedicated IAM role and can assume it using the projected tokens.

Further work:

Unfortunately, I could not figure out a way to make the uploaded files public so that is still a manual step.
With the number of addons we'd like to provision roles for, the remap logic probably doesn't scale that well. But we can improve this using the IRSA webhook.

k8s-ci-robot · 2021-02-07T20:30:43Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

olemarkus · 2021-02-07T20:31:12Z

/cc @rifelpet

pkg/model/awsmodel/oidc_provider.go

olemarkus · 2021-02-09T22:05:25Z

Added in using https://api. as issuer as k8s 1.20 requires this.

The broken test had to be removed as PublicJWKS won't work with FS VFS. Since we try to call the https url to get the cert. If this gets merged, we can do a kubetest of this. We have the rights to write the jwks files to a public store, I believe (we are writing other assets somewhere).

The commits are a bit of a mess, so I should clean this up first.
But reviews would be welcomed.

olemarkus · 2021-02-10T09:28:02Z

/test pull-kops-e2e-kubernetes-aws

olemarkus · 2021-02-12T06:28:03Z

/kind office-hours

pkg/model/awsmodel/oidc_provider.go

justinsb · 2021-03-19T15:34:51Z

pkg/model/awsmodel/oidc_provider.go

+		SigningKey: skTask,
+	}
+
+	discovery, err := buildDicoveryJSON(serviceAccountIssuer)


Nit: typo "dicovery"

justinsb · 2021-03-19T15:36:25Z

pkg/model/awsmodel/oidc_provider.go

+		Contents:  fi.NewBytesResource(discovery),
+		Lifecycle: b.Lifecycle,
+		Location:  fi.String("oidc/.well-known/openid-configuration"),
+		Name:      fi.String("discovery.json"),


Nit: maybe public-oidc-configuration (and similar prefix on keys.json). It matters most for the terraform, I suspect (and primarily only optics there)

justinsb · 2021-03-19T15:38:27Z

pkg/model/awsmodel/oidc_provider.go

+}
+
+type KeyResponse struct {
+	Keys []jose.JSONWebKey `json:"keys"`


Nit: are taking the dependency on the jose library only for the structs? Should we just embed them?

justinsb · 2021-03-19T15:41:59Z

upup/pkg/fi/cloudup/awstasks/iamoidcprovider.go

@@ -35,7 +35,7 @@ type IAMOIDCProvider struct {
 	Lifecycle *fi.Lifecycle

 	ClientIDs   []*string
-	Thumbprints []fi.Resource
+	Thumbprints []*string


I thought we needed this for the initial preview... I guess if tests are passing then we don't (or we have a gap in our tests!)

We used to because the thumbprint came from the cluster CA cert. Now they are just well-known strings.

I suspect this may be something we can change back in order to support anonymousauth+public apiserver as oidc provider.

justinsb · 2021-03-19T15:42:39Z

upup/pkg/fi/cloudup/bootstrapchannelbuilder_test.go

@@ -53,20 +53,6 @@ func TestBootstrapChannelBuilder_BuildTasks(t *testing.T) {
 	runChannelBuilderTest(t, "awsiamauthenticator", []string{"authentication.aws-k8s-1.12"})
 }

-func TestBootstrapChannelBuilder_PublicJWKS(t *testing.T) {


Is there are reason to drop this test?

Yeah. This PR only supports using s3 as store for jwks, so this fails because filesystem VFS cannot be used for https.

justinsb · 2021-03-19T15:44:33Z

Had some questions/nits but this LGTM. As this is feature-flagged, I propose we merge and then I'll probably send a follow-on PR to try to e.g. add the test back if we do need it.

/approve
/lgtm

Generate and upload keys.json + discovery.json to public store Don't enable anonymous auth on publicjwks Remove tests that won't work using FS VFS anymore

k8s-ci-robot · 2021-03-19T19:55:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, rifelpet

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~~ [justinsb,rifelpet]

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

We removed them from kubernetes#10756, but they can be re-added.

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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 7, 2021

olemarkus requested review from justinsb and johngmyers and removed request for justinsb February 7, 2021 20:30

k8s-ci-robot requested review from hakman and zetaab February 7, 2021 20:31

k8s-ci-robot added area/api area/provider/aws Issues or PRs related to aws provider approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 7, 2021

k8s-ci-robot requested a review from rifelpet February 7, 2021 20:31

rifelpet mentioned this pull request Feb 7, 2021

[WIP] Add API fields for AWS OIDC Provider #9271

Closed

olemarkus commented Feb 7, 2021

View reviewed changes

pkg/model/awsmodel/oidc_provider.go Outdated Show resolved Hide resolved

olemarkus force-pushed the irsa branch from 9bb1955 to b3f5e2c Compare February 9, 2021 22:00

olemarkus marked this pull request as ready for review February 9, 2021 22:01

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 Feb 9, 2021

olemarkus force-pushed the irsa branch from b3f5e2c to 02e05ce Compare February 9, 2021 22:12

k8s-ci-robot added the kind/office-hours label Feb 12, 2021

olemarkus force-pushed the irsa branch 2 times, most recently from be328f1 to 941c49e Compare February 19, 2021 20:08

olemarkus force-pushed the irsa branch from 941c49e to c4b9625 Compare February 27, 2021 10:15

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2021

olemarkus requested a review from justinsb February 27, 2021 10:16

olemarkus assigned justinsb Feb 27, 2021

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

rifelpet reviewed Mar 9, 2021

View reviewed changes

pkg/model/awsmodel/oidc_provider.go Show resolved Hide resolved

olemarkus force-pushed the irsa branch from af85499 to 6ccea6d Compare March 9, 2021 14:23

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

justinsb reviewed Mar 19, 2021

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 19, 2021

olemarkus force-pushed the irsa branch from 6ccea6d to ab94742 Compare March 19, 2021 18:22

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 19, 2021

rifelpet and others added 7 commits March 19, 2021 20:03

Generate and upload keys.json + discovery.json to public store

7c900b7

Generate and upload keys.json + discovery.json to public store Don't enable anonymous auth on publicjwks Remove tests that won't work using FS VFS anymore

Use well-known s3 fingerprints

ed16631

Do not need thumbprints to be resources

2c1f88f

Fix bug with deleting OIDC providers

5a8d47d

Upload JWKS files as world readable

1900548

Comment where the CA sha1s come from

5178571

Fix go.mod

2e4cb85

olemarkus force-pushed the irsa branch from ab94742 to 2e4cb85 Compare March 19, 2021 19:11

Fix comments from review

397f58d

rifelpet approved these changes Mar 19, 2021

View reviewed changes

k8s-ci-robot assigned rifelpet Mar 19, 2021

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

k8s-ci-robot merged commit ac657c4 into kubernetes:master Mar 19, 2021

justinsb added a commit to justinsb/kops that referenced this pull request Mar 20, 2021

Re-add integration tests for jwks

1fc7d44

We removed them from kubernetes#10756, but they can be re-added.

justinsb mentioned this pull request Mar 20, 2021

Re-add integration tests for jwks #11087

Merged

justinsb added a commit to justinsb/kops that referenced this pull request Mar 20, 2021

Re-add integration tests for jwks

0a01640

We removed them from kubernetes#10756, but they can be re-added.

justinsb added a commit to justinsb/kops that referenced this pull request Mar 21, 2021

Re-add integration tests for jwks

3ebe8d0

We removed them from kubernetes#10756, but they can be re-added.

justinsb added a commit to justinsb/kops that referenced this pull request Mar 21, 2021

Re-add integration tests for jwks

c75e084

We removed them from kubernetes#10756, but they can be re-added.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instance roles for service accounts (IRSA) contd #10756

Instance roles for service accounts (IRSA) contd #10756

olemarkus commented Feb 7, 2021 •

edited

Loading

k8s-ci-robot commented Feb 7, 2021

olemarkus commented Feb 7, 2021

olemarkus commented Feb 9, 2021

olemarkus commented Feb 10, 2021

olemarkus commented Feb 12, 2021

justinsb Mar 19, 2021

justinsb Mar 19, 2021

justinsb Mar 19, 2021

justinsb Mar 19, 2021

olemarkus Mar 19, 2021

justinsb Mar 19, 2021

olemarkus Mar 19, 2021

justinsb commented Mar 19, 2021

k8s-ci-robot commented Mar 19, 2021

Instance roles for service accounts (IRSA) contd #10756

Instance roles for service accounts (IRSA) contd #10756

Conversation

olemarkus commented Feb 7, 2021 • edited Loading

k8s-ci-robot commented Feb 7, 2021

olemarkus commented Feb 7, 2021

olemarkus commented Feb 9, 2021

olemarkus commented Feb 10, 2021

olemarkus commented Feb 12, 2021

justinsb Mar 19, 2021

Choose a reason for hiding this comment

justinsb Mar 19, 2021

Choose a reason for hiding this comment

justinsb Mar 19, 2021

Choose a reason for hiding this comment

justinsb Mar 19, 2021

Choose a reason for hiding this comment

olemarkus Mar 19, 2021

Choose a reason for hiding this comment

justinsb Mar 19, 2021

Choose a reason for hiding this comment

olemarkus Mar 19, 2021

Choose a reason for hiding this comment

justinsb commented Mar 19, 2021

k8s-ci-robot commented Mar 19, 2021

olemarkus commented Feb 7, 2021 •

edited

Loading