Tokenized Cloud Auth Enablement For Operators

[bentito]

I think this is ready for approval review: @sdodson, @joelanford, @bparees (you have one comment thread that seems maybe unaddressed as of 6/13 ?), @JoelSpeed

[gallettilance]

Thanks for getting this started!

[jharrington22]

Thanks for writing this up @bentito ! I've added some initial feedback, more to come. I'd like to generalize this a little more to be cloud agnostic. STS is a AWS service not really the name of an industry standard.

[sdodson]

Ugh, submitting pending comments from quite some time ago.

Also, I think we should broadly change from "STS" to "Token authentication" or "Token Based Workload Identity" or something in between unless we're specifically talking about AWS when we say STS.

[JoelSpeed]

If I read this correctly, there's an assumption that this new role-arn field could be populated by either the user or a controller, what happens if the user manually created the roles and then hasn't set the field?

[bparees]

Things that are still unclear to me:

1. what is responsible for creating the CredReq object? Is this just another resource defined in the CSV? as a custom resource alongside the CSV? Either way, if so, it sounds like olm needs updates to support it. Other possible components to create it could be the console(ew, relies on user having permission to create credreqs), the olm operator itself(creates its own credreq which could be messy)

2. discussion of how olm operators advertise the permissions/roles they need created(not just that they are STS-enabled), especially in light of the packagemanifest only showing information from the head bundles which may not be what the user is installing. Note there might be a phase 1 vs phase 2 aspect this (phase 1 is just install instructions, phase 2 is programmatic metadata)

3. what is responsible for creating the roles from (2) (possible answers include manually by the user, automatically by the ROSA cli, maybe other possible answers? Again, may have a phase1 and phase2 answer, but if users are creating this the roles/perms manually it's not clear to me how we've significantly moved the needle beyond where we are today by offering some minor ARN value passing)

[bentito]

If I read this correctly, there's an assumption that this new role-arn field could be populated by either the user or a controller, what happens if the user manually created the roles and then hasn't set the field?

Well-behaved operator will sit there waiting for info about those roles to be added the CredentialsRequest, that is waiting on the Secret to be present so it can use it to use the role info.

[joelanford]

new ... Subscription fields

Is this proposing a change to the OLM Subscription API?

Generate a manual ack for operator upgrade when cloud resources are involved.

It isn't clear to me what this means. If the Subscription has spec.installPlanApproval: Automatic, but an upgrade requires cloud API permission changes, it seems like we would be violating user intent by injecting a manual ack.

But this leads me to a boarder question: at what point of the OLM install/upgrade progression would this manual ack be injected, and how would it be surfaced to a user? Would OLM be expected to check the permissions so that the ack is only required if permissions need to change, or are we expecting users to manually ack every single install/upgrade involving an STS-supporting bundle deployed on an STS-enabled cluster?

[joelanford]

Is this some sort of static validation of the annotations? Or is this suggesting that SDK somehow assesses the requests the operator intends to make and warns/errors if they require increased permission?

[bparees]

yeah this needs more detail about what validation is being done and where/when/how

[JoelSpeed]

Maybe I need to re-read this again, but having just read through, there's lots of discussion about putting information in various places to trigger actions, eg putting role arn annotations, but, I couldn't work out onto what object or what specific annotations I'm supposed to be using.
Can we make sure the mechanics of this are clarified, perhaps within the workflows? or API changes?

I'd also like to understand why we are using annotations rather than fields on the objects (though I guess if those objects aren't owned by us this may be why).

[sdodson]

@bentito I fixed it in 9926a34 but that commit got stomped, just re apply those changes and it should be fine

[openshift-ci]

@bentito: all tests passed!

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

[sdodson]

@bparees I approve of this in its current form.

I understand that future refinements will include clarification around Hypershift to ensure CCO is compatible with use on management cluster and extension to add Azure as a supported platform, but those all seem very minor to me.

[bparees]

bunch of nits that i'd like to see cleaned up post-merge, but i'm going to merge this so it can land now. Thanks for the persistence, @bentito and everyone else who contributed

[bparees]

nit: supporting TAT isn't "more general" than clusters in manual mode, it's a different want of managing auth.

[bparees]

nit...."Extract the CredentialsRequest from the operator's image or codebase in order to know what IAM role is appropriate for the operator to assume and, if using the....... : "

(if they aren't using ccoctl, then they need to do other steps to create the roles and secret)

[bparees]

nit: would be better to make this a permalink since if we succeed w/ this EP, the EFS install instructions are likely going to change.

[bparees]

are we doing this in the initial implementation or not? I assume we are not. Can we update this to make it clear what's being done and what is being left as future improvements?

[bparees]

nit: needed by CCO. no webhook, since we abandoned that approach, right?

[bparees]

nit: by doc you mean a future enhancement?

[bparees]

New guidelines would include the following to use CCO has detected cluster is using time-based tokens:

nit: there's a typo or word missing here or something.

[bparees]

nit: i assume you mean cloud provider roles and not cluster rbac roles?

[bparees]

this isn't optional, it's not needed at all because it always happens automatically.

[bparees]

by "in the known location" you mean in the ARN field the Console displays and/or the Subscription object? if not, i don't know what this is referring to.

[bparees]

/approve

[bparees]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

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 [bparees]

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