Skip to content
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

Gke service account generator #3914

Merged

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented May 21, 2021

This PR implements a generator for GKE service accounts.

It is based on the resource in step 6 of "Authenticating to Google Cloud" here: https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity#yaml

ALLOW_MODULE_SPAN

@natasha41575 natasha41575 requested review from justinsb and monopole May 21, 2021 18:41
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 21, 2021
@k8s-ci-robot k8s-ci-robot requested review from droot and KnVerey May 21, 2021 18:41
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2021
@natasha41575 natasha41575 removed the request for review from droot May 21, 2021 18:41
@natasha41575 natasha41575 force-pushed the GkeServiceAccountGenerator branch from bf25938 to 8b4cabd Compare May 21, 2021 18:46
@max-sixty
Copy link
Contributor

@natasha41575 asked me to comment in kubernetes-sigs/cli-experimental#158

One issue we face is that GKE service accounts are a resource that requires the project, which isn't known until the final aggregation of manifests. So I think this solves that well, because the project can be patched in once at the final aggregation.

It doesn't solve the general problem though, IIUC; here's another example: GoogleCloudPlatform/k8s-config-connector#271. (this is no longer be an issue for us, since we don't use the tool, though we'd like to at some point)

There are also a couple of other areas where we'd like to be able to patch arbitrary strings; to the extent that's possible without complicating the very nice abstractions that kustomize offers.

@natasha41575
Copy link
Contributor Author

@monopole had a chat with about this with @justinsb offline. Will summarize the convo in a doc tomorrow and we can discuss

Copy link
Contributor

@monopole monopole left a comment

Choose a reason for hiding this comment

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

this is going to be very useful!

api/types/kustomization.go Outdated Show resolved Hide resolved
api/filters/gkesagenerator/doc.go Outdated Show resolved Hide resolved
resources:
- resource.yaml

gkeSaGenerator:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say put all the effort into the filter test for now.

and/or drive this via the generators field e.g.

  • chartinflatorplugin_test.go
  • inlinetransformer_test.go

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this is cloud-provider-specific, does it need to be a top-level field, or could we make it generators-only? Is there a need for a similar generator for other cloud providers? Are the inputs required similar enough to share an abstraction? If not will we accept a bunch of top-level fields?

Copy link
Contributor Author

@natasha41575 natasha41575 May 26, 2021

Choose a reason for hiding this comment

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

@KnVerey had a chat with @monopole this morning. We are thinking having one top level field that has an option like cloud: gke to tell us which cloud provider to generate for. When we want to have something for other cloud providers, we can add more options to this generator.

@natasha41575 natasha41575 force-pushed the GkeServiceAccountGenerator branch from 8b4cabd to 5a2a770 Compare May 26, 2021 23:54
@natasha41575 natasha41575 requested a review from monopole May 26, 2021 23:56
@natasha41575
Copy link
Contributor Author

natasha41575 commented May 26, 2021

@monopole I've updated this PR based on your comments

I would like to update this generator to add GKE resources such as IAMServiceAccount and IAMPolicyMember in a follow-up PR.

@natasha41575 natasha41575 requested a review from justinsb May 27, 2021 00:04
Copy link
Contributor

@monopole monopole left a comment

Choose a reason for hiding this comment

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

/lgtm

great start!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: monopole, natasha41575

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 [monopole,natasha41575]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants