AWS: Store the OIDC documents in a S3 bucket #636

alvaroaleman · 2021-11-03T21:02:26Z

This change makes hypershift store the OIDC documents in a S3 bucket
rather than serving them from a dedicated ingress pointing to the
kube-apiserver. This is needed for two reasons:

Private clusters should not be publicly accessible
Tests (and probably more) assume that the serving cert of the
endpoint serving these documents is either publicly trusted or signed
by the guest clusters root ca

This implies that a hypershift setup that should manage AWS clusters
must be configured with a pre-existing S3 bucket and credentials to
write and delete objects in that bucket.

In detail, the following things were changed:

Hypershift install got optional arguments for a OIDC bucket and region
Hypershift install will always create a configmap with the data from
these arguments in the kube-public namespace, although it might be
empty if the flags were unset
If the flags were set, the hypershift operator will be configured with
flags for these settings
AWS Create IAM got optional flags for the bucket region and name that
will get defaulted to the values from the configmap in kube-public
AWS Create IAM will set the Issuer URL of the AWS OIDC provider to the
S3 Bucket url/$infra-id
The hypershift operator will for AWS clusters download
/.well-known/openid-configuration and /openid/v1/jwk from the
kube-apiserver and place them in the S3 bucket below $infra_id
Once it did that, it sets an additional finalizer on the hostedcluster
On cleanup, it will remove the two documents again from the bucket and
then remove the finalizer
The controlplaneoperator will not create an OIDC route anymore as we
don't need it

I've verified that this change both doesn't break the AWS OIDC provider
and fixes the [sig-auth] ServiceAccounts ServiceAccountIssuerDiscovery should support OIDC discovery of service account issuer
conformance testcase.

Ref https://issues.redhat.com/browse/HOSTEDCP-257
/cc @csrwng @sjenning

openshift-ci · 2021-11-03T21:03:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

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~~ [alvaroaleman]

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

sjenning · 2021-11-03T21:26:04Z

cmd/infra/aws/create_iam.go

@@ -63,12 +63,16 @@ func NewCreateIAMCommand() *cobra.Command {

 	cmd.Flags().StringVar(&opts.AWSCredentialsFile, "aws-creds", opts.AWSCredentialsFile, "Path to an AWS credentials file (required)")
 	cmd.Flags().StringVar(&opts.InfraID, "infra-id", opts.InfraID, "Infrastructure ID to use for AWS resources.")
+	cmd.Flags().StringVar(&opts.OIDCBucketName, "oidc-bucket-name", "", "The name of the bucket in which the OIDC discovery document is stored")


can we be opinionated about this using the infra-id?

I don't understand what you mean, can you elaborate?

nevermind, we are sharing the bucket among HCs. before, we had a bucket per HC and thus could just generate the bucket name and have an opinionated path within the bucket, thus nothing was required from the user.

hypershift-operator/main.go

alvaroaleman · 2021-11-04T15:50:01Z

/retest

enxebre · 2021-11-04T17:35:30Z

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

@@ -979,6 +995,11 @@ func (r *HostedClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, fmt.Errorf("failed to reconcile machine approver: %w", err)
 	}

+	// Reconcile the AWS OIDC discovery


I see the func returns nil if hcluster.Spec.Platform.Type is not aws but (at least for now) we might want to be consistent with the other platform specific reconcilers and gate the call with

switch hcluster.Spec.Platform.Type { case hyperv1.AWSPlatform:

Updated it, although I find it slightly silly to have a one-entry switch statement

yeh I agree, though I think being consistent with the pattern would make adding more platforms and refactor easier

enxebre · 2021-11-04T17:37:32Z

cmd/install/assets/hypershift_operator.go

@@ -558,3 +580,20 @@ func (r HypershiftRecordingRule) Build() *prometheusoperatorv1.PrometheusRule {
 	rule.Spec = recordingRuleSpec()
 	return rule
 }
+
+func AWSOIDCBucketConfigMap(bucketName, bucketRegion string) *corev1.ConfigMap {


Did I get it right this is putting all clusters config into the management cluster kube-public ns?

No, it is a configmap that contains the name and the region of the bucket that contains the OIDC docs. install iam needs this to be able to set up the OIDC provider

enxebre · 2021-11-04T17:38:33Z

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

+	}
+
+	if r.AWSOIDCBucketName == "" || r.S3Client == nil {
+		return errors.New("hypershift wasn't configured with a S3 bucket or credentials, this makes it unable to set up OIDC for AWS clusters. Please install hypershift with the --aws-oidc-bucket-name, --aws-oidc-bucket-region and --aws-creds flags set. The bucket must pre-exist and the credentials must be authorized to write into it")


This needs to be added as a Prerequisites to the Readme

I updated the readme with instructions on the bucket and how hypershift install needs to be parameterized now

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

enxebre · 2021-11-04T18:06:56Z

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

@@ -2822,6 +2843,10 @@ func (r *HostedClusterReconciler) delete(ctx context.Context, req ctrl.Request,
 	if err := r.Delete(ctx, ns); err != nil && !apierrors.IsNotFound(err) {
 		return false, fmt.Errorf("failed to delete namespace: %w", err)
 	}
+
+	if err := r.cleanupOIDCBucketData(ctx, hc); err != nil {


This call should be gated by

switch hcluster.Spec.Platform.Type { case hyperv1.AWSPlatform:

right?

It internally gates by checking for the finalizer and doing nothing if it is absent

alvaroaleman · 2021-11-05T14:06:02Z

/retest

alvaroaleman · 2021-11-08T19:34:30Z

@sjenning @enxebre I rebased this now that #637 merged, PTAL

sjenning · 2021-11-08T19:51:28Z

The discussion this morning got me thinking, this breaks a management cluster on platform None supporting HCs for platform AWS. You will still be required to have a management account in which to create the bucket, even though the management cluster doesn't run there. Is this correct?

alvaroaleman · 2021-11-08T19:54:38Z

The discussion this morning got me thinking, this breaks a management cluster on platform None supporting HCs for platform AWS. You will still be required to have a management account in which to create the bucket, even though the management cluster doesn't run there. Is this correct?

Yes, the management account with the Bucket is needed. However, it is not needed to actually run in AWS, so I would argue that this doesn't break the ability of a management cluster on platform none to run AWS clusters.

alvaroaleman · 2021-11-09T14:25:03Z

/retest

sjenning · 2021-11-09T15:57:03Z

Right now it ends up being implicit (s3 on AWS, none otherwise). Can we leave it at that for now?

Yes, just as long as you can set none on the provider and none of the other flags are required in that case. Thanks!

cmd/install/install.go

hypershift-operator/main.go

alvaroaleman · 2021-11-10T18:51:45Z

[2021-11-10T17:06:47Z]NoClusters: No clusters in pool are ready to be claimed
/retest

sjenning · 2021-11-10T23:04:13Z

/lgtm

This change makes hypershift store the OIDC documents in a S3 bucket rather than serving them from a dedicated ingress pointing to the kube-apiserver. This is needed for two reasons: 1. Private clusters should not be publicly accessible 2. Tests (and probably more) assume that the serving cert of the endpoint serving these documents is either publicly trusted or signed by the guest clusters root ca This implies that a hypershift setup that should manage AWS clusters must be configured with a pre-existing S3 bucket and credentials to write and delete objects in that bucket. In detail, the following things were changed: * Hypershift install got optional arguments for a OIDC bucket and region * Hypershift install will always create a configmap with the data from these arguments in the kube-public namespace, although it might be empty if the flags were unset * If the flags were set, the hypershift operator will be configured with flags for these settings * AWS Create IAM got optional flags for the bucket region and name that will get defaulted to the values from the configmap in kube-public * AWS Create IAM will set the Issuer URL of the AWS OIDC provider to the S3 Bucket url/$infra-id * The hypershift operator will for AWS clusters download /.well-known/openid-configuration and /openid/v1/jwk from the kube-apiserver and place them in the S3 bucket below $infra_id * Once it did that, it sets an additional finalizer on the hostedcluster * On cleanup, it will remove the two documents again from the bucket and then remove the finalizer * The controlplaneoperator will not create an OIDC route anymore as we don't need it I've verified that this change both doesn't break the AWS OIDC provider and fixes the `[sig-auth] ServiceAccounts ServiceAccountIssuerDiscovery should support OIDC discovery of service account issuer` conformance testcase.

alvaroaleman · 2021-11-11T14:35:06Z

@sjenning @csrwng had to rebase yet another time, could you re-lgtm it?

csrwng · 2021-11-11T16:25:22Z

/lgtm

openshift-ci bot requested review from csrwng and sjenning November 3, 2021 21:02

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2021

alvaroaleman mentioned this pull request Nov 3, 2021

WIP: Use a serving cert trusted by the root ca for the OIDC route #617

Closed

alvaroaleman force-pushed the oidc-bucket branch from c94041f to efe5db7 Compare November 3, 2021 21:06

alvaroaleman mentioned this pull request Nov 3, 2021

Add AWSEndpointService controller to hypershift-operator #637

Merged

sjenning reviewed Nov 3, 2021

View reviewed changes

hypershift-operator/main.go Outdated Show resolved Hide resolved

alvaroaleman force-pushed the oidc-bucket branch from efe5db7 to ecd3d53 Compare November 4, 2021 14:19

enxebre reviewed Nov 4, 2021

View reviewed changes

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go Show resolved Hide resolved

enxebre reviewed Nov 4, 2021

View reviewed changes

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go Outdated Show resolved Hide resolved

enxebre reviewed Nov 4, 2021

View reviewed changes

alvaroaleman force-pushed the oidc-bucket branch from ecd3d53 to bd6ebff Compare November 4, 2021 20:04

alvaroaleman force-pushed the oidc-bucket branch 2 times, most recently from 1fa8e1f to 261f57c Compare November 8, 2021 17:32

alvaroaleman force-pushed the oidc-bucket branch from 261f57c to e7c0836 Compare November 8, 2021 22:22

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2021

alvaroaleman force-pushed the oidc-bucket branch from e7c0836 to 509e2fa Compare November 8, 2021 22:25

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

alvaroaleman force-pushed the oidc-bucket branch from 509e2fa to 27602d9 Compare November 9, 2021 14:54

alvaroaleman force-pushed the oidc-bucket branch from 15c02ae to dc76dbf Compare November 9, 2021 15:50

sjenning reviewed Nov 9, 2021

View reviewed changes

cmd/install/install.go Outdated Show resolved Hide resolved

alvaroaleman force-pushed the oidc-bucket branch from dc76dbf to 72fb29a Compare November 9, 2021 16:07

sjenning reviewed Nov 9, 2021

View reviewed changes

hypershift-operator/main.go Outdated Show resolved Hide resolved

alvaroaleman force-pushed the oidc-bucket branch 2 times, most recently from b5c888d to dfe8e50 Compare November 9, 2021 16:44

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

alvaroaleman force-pushed the oidc-bucket branch from dfe8e50 to 962d485 Compare November 9, 2021 17:53

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2021

alvaroaleman force-pushed the oidc-bucket branch 4 times, most recently from 2388abf to 4561986 Compare November 10, 2021 17:02

alvaroaleman force-pushed the oidc-bucket branch 2 times, most recently from d193808 to 1460741 Compare November 10, 2021 21:34

openshift-ci bot assigned sjenning Nov 10, 2021

openshift-ci bot added lgtm 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 Nov 10, 2021

alvaroaleman force-pushed the oidc-bucket branch from 1460741 to 5cadb9c Compare November 11, 2021 05:23

openshift-ci bot removed lgtm 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 Nov 11, 2021

openshift-ci bot assigned csrwng Nov 11, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 11, 2021

openshift-merge-robot merged commit ab5c99b into openshift:main Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS: Store the OIDC documents in a S3 bucket #636

AWS: Store the OIDC documents in a S3 bucket #636

alvaroaleman commented Nov 3, 2021

openshift-ci bot commented Nov 3, 2021

sjenning Nov 3, 2021

alvaroaleman Nov 4, 2021

sjenning Nov 8, 2021

alvaroaleman commented Nov 4, 2021

enxebre Nov 4, 2021

alvaroaleman Nov 4, 2021

enxebre Nov 4, 2021

enxebre Nov 4, 2021

alvaroaleman Nov 4, 2021

enxebre Nov 4, 2021 •

edited

alvaroaleman Nov 4, 2021

enxebre Nov 4, 2021

alvaroaleman Nov 4, 2021

alvaroaleman commented Nov 5, 2021

alvaroaleman commented Nov 8, 2021

sjenning commented Nov 8, 2021

alvaroaleman commented Nov 8, 2021

alvaroaleman commented Nov 9, 2021

sjenning commented Nov 9, 2021

alvaroaleman commented Nov 10, 2021

sjenning commented Nov 10, 2021

alvaroaleman commented Nov 11, 2021

csrwng commented Nov 11, 2021

AWS: Store the OIDC documents in a S3 bucket #636

AWS: Store the OIDC documents in a S3 bucket #636

Conversation

alvaroaleman commented Nov 3, 2021

openshift-ci bot commented Nov 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Nov 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Nov 5, 2021

alvaroaleman commented Nov 8, 2021

sjenning commented Nov 8, 2021

alvaroaleman commented Nov 8, 2021

alvaroaleman commented Nov 9, 2021

sjenning commented Nov 9, 2021

alvaroaleman commented Nov 10, 2021

sjenning commented Nov 10, 2021

alvaroaleman commented Nov 11, 2021

csrwng commented Nov 11, 2021

enxebre Nov 4, 2021 •

edited