OCPBUGS-29027: dynamically obtain permissions required for passthrough

fxierh commented 7 months ago

On AWS, when CCO is in the default mode and the root credential is not sufficient for mint mode, the secret annotator controller checks if the root credential is good enough for passthrough against a static list of permissions https://github.com/openshift/cloud-credential-operator/blob/3cf3099b60fbd886dd9582c55373f36419c96fff/pkg/aws/utils.go#L35-L93 This list of perms is outdated.

openshift-ci[bot] commented 7 months ago

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

openshift-ci-robot commented 7 months ago

@fxierh: This pull request references Jira Issue OCPBUGS-29027, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to [this](https://github.com/openshift/cloud-credential-operator/pull/671): >On AWS, when CCO is in the default mode and the root credential is not sufficient for mint mode, the secret annotator controller checks if the root credential is good enough for passthrough against a static list of permissions >https://github.com/openshift/cloud-credential-operator/blob/3cf3099b60fbd886dd9582c55373f36419c96fff/pkg/aws/utils.go#L35-L93 >which is outdated. > >Same for GCP. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcloud-credential-operator). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.

openshift-ci[bot] commented 7 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fxierh Once this PR has been reviewed and has the lgtm label, please assign lleshchi 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: - **[OWNERS](https://github.com/openshift/cloud-credential-operator/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

fxierh commented 7 months ago

Before proceeding to code up a fix, I'd like to discuss about ways this issue can be solved: 1) Manually maintain the static list of perms required for passthrough. This is easy but it leaves us a maintenance burden. 2) Dynamically obtain the list of perms required for passthrough and check against it. The secret annotator controller will watch CredentialsRequests, and trigger a reconcile in one of the following scenarios:

A CR for the cluster's platform is created
A CR for the cluster's platform is deleted
A CR for the cluster's platform is updated AND its statementEntries are updated.
3) Do not check if the root credential is sufficient for passthrough at all in the secret annotator controller. If the root credential is not good enough for mint mode, leave the passthrough annotation on it without checking. This is what we are currently doing for Azure https://github.com/openshift/cloud-credential-operator/blob/3cf3099b60fbd886dd9582c55373f36419c96fff/pkg/operator/secretannotator/azure/reconciler.go#L127-L143 Logic that checks if the root credential is sufficient for passthrough already exists in the syncPassthrough func e.g., https://github.com/openshift/cloud-credential-operator/blob/3cf3099b60fbd886dd9582c55373f36419c96fff/pkg/gcp/actuator/actuator.go#L241-L288

fxierh commented 6 months ago

It looks like the installer is the only user of func CheckCloudCredPassthrough: https://github.com/openshift/installer/blob/3d78a2270aad5f71034bed9b7ab0ae992c6c83cd/pkg/asset/installconfig/aws/permissions.go#L310

Considering the above:

Option 1 has no impact on the installer.
Option 2 would require the installer:
- to be refactored in a way that extracts all CredentialsRequests (for the platform to install the cluster) from the release image and glean the permissions from them OR
- to remove the entire passthrough-permission-checking logic above
Option 3 would require the installer to remove the passthrough-permission-checking logic above.

I guess option 1 is the way to go.

@jstuever @dlom WDTY ?

jstuever commented 6 months ago

Sorry for the delay, I had to look into this one. I think we are stuck with option 1 for now. The credentialRequests can be obtained for a specific version with the following:

oc adm release extract --credentials-requests --cloud=aws to=./creds  --from <release>

Note: This will become complicated as these operators become optional components, so the installer might need to rework this in the future.

fxierh commented 5 months ago

@jstuever

Sorry for the late reply.

Note: This will become complicated as these operators become optional components, so the installer might need to rework this in the future.

Thanks for the reminder. This seems to be a common issue for both the CCO and the installer. Currently, among the 7 operators which request for cloud credentials on AWS, 4 are already optional: CCO, MAPI, ImageRegistry and Storage.

With the above taken into account, and if we go with option 1, we'd need to make the static list of permissions minimal, meaning that it will only contain permissions required by the non-optional operators. This makes CCO's secret annotator controller and installer's platform permission check kind of in-precise, even if we keep the static list of permissions up-to-date.

I'm thus wondering if it's good timing to solve this issue once and for all by

either passing an abstraction (e.g. interface) to CheckCloudCredPassthrough so the CCO and the installer could implement their own logic to obtain the list of permissions;
or letting both components obtain the list of permissions themselves somehow and then call CheckPermissionsAgainstActions directly.

openshift-ci[bot] commented 4 months ago

@fxierh: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).

openshift-bot commented 1 month ago

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot commented 1 week ago

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten /remove-lifecycle stale

openshift / cloud-credential-operator

OCPBUGS-29027: dynamically obtain permissions required for passthrough #671