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

✨ feat: Add IRSA Support for Self Managed Clusters (2024 edition) #5101

Closed
wants to merge 14 commits into from

Conversation

sl1pm4t
Copy link

@sl1pm4t sl1pm4t commented Aug 21, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds IRSA functionality to an IaaS cluster. This will bring the IaaS clusters inline in functionality with Managed clusters which provide most of this functionality out of the box.

With this PR, the following new resources are created:

  • S3 bucket ( if not already created for the Ignition feature )
  • Two public world readable objects in the S3 bucket:
    • <cluster_name>/.well-known/openid-configuration - OpenID Connect discovery document
    • <cluster_name>/openid/v1/jwks - Service Account signing public key
  • AWS IAM Identity Provider, configured to trust the Issuer found at the S3 URL where the OIDC discovery doc and keys are published.

This is a continuation of an old unmerged PR #4094 - with some fixes and some functionality removed to reduce the scope of the PR.

The functionality removed includes:

  • This PR no longer deploys the amazon-pod-identity-webhook addon to the workload cluster. I felt there are already many ways to manage cluster addons, including ClusterResourceSets or CAAPH, and that it was unnecessary to install the addon via the controller which then becomes an ongoing maintenance burden. Instead, the requirement for the addon has been added to the documentation.
  • This PR no longer modifies the API Server service-account-issuer argument through kubeadm patches. This is easily covered in the documentation and only requires a single line of config to be added to the AWSCluster resource, but also during testing I experienced issues with this being applied inconsistently, resulting in different values across the control plane nodes.

Which issue(s) this PR fixes:
Fixes #3560
Supersedes #4094

Special notes for your reviewer:

Adding a new ReconcileOIDCProvider to the AWSCluster reconciliation loop.

  • Created a new IAM service, in the future the same logic for EKS could be combined as previously the oidc code was buried in the EKS service. Details on the reconciler can be found in comments.
  • Updated the S3 service to take keys instead of Machines and added a CreatePublic for the JWKS/OIDC config
  • Exposed a ManagementClient and RemoteClient for both cluster types and exported Client.
  • Moved OIDCProvider status type to v1beta2 and migrated out of the EKS API to make one type both clusters can reference a single type.
  • This PR adds a new Experiment feature flag to enable this functionality. This feature is dependent on the S3 bucket associated with Ignition node configuration, but it felt unintuitive to need to enable the Ignition feature flag to get OIDC support.
  • I have tried to preserve the commit history from the original PR.
  • This cyclomatic showed up during testing so I refactored reconcileNormal
controllers/awsmachine_controller.go:435:1: cyclomatic complexity 38 of func `(*AWSMachineReconciler).reconcileNormal` is high (> 30) (gocyclo)
func (r *AWSMachineReconciler) reconcileNormal(ctx context.Context, machineScope *scope.MachineScope, clusterScope cloud.ClusterScoper, ec2Scope scope.EC2Scope, elbScope scope.ELBScope, objectStoreScope scope.S3Scope) (ctrl.Result, error) {

Checklist:

  • squashed commits -- NOTE: I squashed my commits up until merging main into my branch, but also left the original commits by @luthermonson untouched so his contributions are not lost
  • includes documentation
  • includes emojis
  • adds unit tests
  • adds or updates e2e tests

Release note:

Add IRSA support for self-hosted clusters

luthermonson and others added 12 commits October 24, 2023 08:19
# Conflicts:
#	api/v1beta2/types.go
#	api/v1beta2/zz_generated.deepcopy.go
#	config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
#	config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
#	controllers/awscluster_controller.go
#	controllers/awsmachine_controller.go
#	controllers/awsmachine_controller_test.go
#	controllers/awsmachine_controller_unit_test.go
#	controlplane/eks/api/v1beta2/awsmanagedcontrolplane_types.go
#	go.mod
#	go.sum
#	pkg/cloud/scope/machine.go
#	pkg/cloud/services/interfaces.go
#	pkg/cloud/services/s3/s3.go
#	pkg/cloud/services/s3/s3_test.go
This was blocking cluster deletion, because the OIDC provider no longer existed and was returning an error.
# Conflicts:
#	api/v1beta2/types.go
# Conflicts:
#	controllers/awsmachine_controller.go
* build oidc discovery doc
* remove pod-identity-webhook deployment
* Add documentation for SA Issuer Discovery & IRSA
* remove unused reconcileKubeAPIParameters()
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Aug 21, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ankitasw for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/contains-merge-commits labels Aug 21, 2024
@k8s-ci-robot
Copy link
Contributor

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-priority needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 21, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @sl1pm4t!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 21, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @sl1pm4t. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2024
@sl1pm4t sl1pm4t changed the title Add IRSA Support for Self Managed Clusters (2024 edition) ✨ feat: Add IRSA Support for Self Managed Clusters (2024 edition) Aug 27, 2024
@sl1pm4t sl1pm4t closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/contains-merge-commits kind/feature Categorizes issue or PR as related to a new feature. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for non-EKS IRSA
4 participants