openshift / cluster-image-registry-operator

The image registry operator installs+maintains the internal registry on a cluster
Apache License 2.0
58 stars 123 forks source link

[release-4.14] OCPBUGS-39100: Avoid Shared Access Key usage for Azure Storage Account when using Managed Identity based auth #1112

Closed rajdeepc2792 closed 2 months ago

rajdeepc2792 commented 2 months ago

Alternate PR to https://github.com/openshift/cluster-image-registry-operator/pull/1107 Note that other than running go mod vendor (second commit in this PR), I also needed to make one manual change to this PR:

Bring the pkg/storage/azure/azureclient over to 4.14. This package was created as part of an operator feature available on OCP >= 4.15, and so it doesn't exist on the 4.14 branch and had to be added manually. Go.mod and vendor files also needed to be updated to support this new package.

No need to bump go.opentelemetry.io/otel/exporters/otlp/otlptrace and go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc to v1.19.0 as per https://github.com/open-telemetry/opentelemetry-go/issues/4586. go mod vendor and go mod tidy were successful without it.

Note for QE Because 4.14 is GA, we cannot backport the "containers/delete" permission. @rajdeepc2792 wrote the code in a way that storage account deletion should still proceed [when setting managementState: Removed] even after container deletion fails due to lack of permission, so please double check that is the case. If you find that not to be the case, we will still go ahead with this backport but we will create a pre-emptive bug describing the issue with a documented workaround (which we will need to figure out). Though I'm hoping it won't come to that 😅

openshift-ci-robot commented 2 months ago

@rajdeepc2792: This pull request references Jira Issue OCPBUGS-39100, which is invalid:

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/cluster-image-registry-operator/pull/1112): >Alternate PR to https://github.com/openshift/cluster-image-registry-operator/pull/1107 >Note that other than running go mod vendor (second commit in this PR), I also needed to make one manual change to this PR: > >Bring the pkg/storage/azure/azureclient over to 4.14. This package was created as part of an operator feature available on OCP >= 4.15, and so it doesn't exist on the 4.14 branch and had to be added manually. Go.mod and vendor files also needed to be updated to support this new package. > >No need to bump go.opentelemetry.io/otel/exporters/otlp/otlptrace and go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc to v1.19.0 as per https://github.com/open-telemetry/opentelemetry-go/issues/4586. `go mod vendor` and `go mod tidy` were successful without it. > > >Note for QE >Because 4.14 is GA, we cannot backport the "containers/delete" permission. @rajdeepc2792 wrote the code in a way that storage account deletion should still proceed [when setting managementState: Removed] even after container deletion fails due to lack of permission, so please double check that is the case. If you find that not to be the case, we will still go ahead with this backport but we will create a pre-emptive bug describing the issue with a documented workaround (which we will need to figure out). Though I'm hoping it won't come to that 😅 Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-image-registry-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 2 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rajdeepc2792 Once this PR has been reviewed and has the lgtm label, please assign adambkaplan 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/cluster-image-registry-operator/blob/release-4.14/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci[bot] commented 2 months ago

@rajdeepc2792: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-azure-operator fe3d767c3e1b9f4f0bf5d3fe90675da42fc0690b link false /test e2e-azure-operator
ci/prow/e2e-azure-ovn fe3d767c3e1b9f4f0bf5d3fe90675da42fc0690b link false /test e2e-azure-ovn

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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
flavianmissi commented 2 months ago

Closed the other backport PR in favor of this one. /retitle [release-4.14] OCPBUGS-39100: Avoid Shared Access Key usage for Azure Storage Account when using Managed Identity based auth

flavianmissi commented 2 months ago

I may have found the source of the missing authentication header. The operator uses https://github.com/jongio/azidext as an adaptor so that it can use recent auth types with our older azure sdk version. release-4.14 vendors version 0.4.0 of that library, while release-4.15 onwards use version 0.5.0. In the 0.5.0 release of azidext I found this bug reported: https://github.com/jongio/azidext/pull/57. The fix for this bug is the only thing new no 0.5.0. I'll try to clone this PR, bump that lib and see where that takes us.

flavianmissi commented 2 months ago

The fix on https://github.com/openshift/cluster-image-registry-operator/pull/1114 solved the missing auth header issue 🎉 /close

openshift-ci-robot commented 2 months ago

@rajdeepc2792: This pull request references Jira Issue OCPBUGS-39100. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to [this](https://github.com/openshift/cluster-image-registry-operator/pull/1112): >Alternate PR to https://github.com/openshift/cluster-image-registry-operator/pull/1107 >Note that other than running go mod vendor (second commit in this PR), I also needed to make one manual change to this PR: > >Bring the pkg/storage/azure/azureclient over to 4.14. This package was created as part of an operator feature available on OCP >= 4.15, and so it doesn't exist on the 4.14 branch and had to be added manually. Go.mod and vendor files also needed to be updated to support this new package. > >No need to bump go.opentelemetry.io/otel/exporters/otlp/otlptrace and go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc to v1.19.0 as per https://github.com/open-telemetry/opentelemetry-go/issues/4586. `go mod vendor` and `go mod tidy` were successful without it. > > >Note for QE >Because 4.14 is GA, we cannot backport the "containers/delete" permission. @rajdeepc2792 wrote the code in a way that storage account deletion should still proceed [when setting managementState: Removed] even after container deletion fails due to lack of permission, so please double check that is the case. If you find that not to be the case, we will still go ahead with this backport but we will create a pre-emptive bug describing the issue with a documented workaround (which we will need to figure out). Though I'm hoping it won't come to that 😅 Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fcluster-image-registry-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 2 months ago

@flavianmissi: Closed this PR.

In response to [this](https://github.com/openshift/cluster-image-registry-operator/pull/1112#issuecomment-2328829456): >The fix on https://github.com/openshift/cluster-image-registry-operator/pull/1114 solved the missing auth header issue 🎉 >/close 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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.