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

IR-366,IR-367,IR-411: allow users to configure private storage accounts in Azure #930

Merged
merged 6 commits into from Nov 27, 2023

Conversation

flavianmissi
Copy link
Member

@flavianmissi flavianmissi commented Oct 2, 2023

Testing

This PR introduces support for private storage accounts in Azure by configuring a private endpoint when requested by the user.
Making a storage account private can happen in one of two ways:

  1. user provided vnet and subnet names
  2. operator discovers the vnet and subnet names

Testing user provided vnet and subnet names

  1. Provision a cluster with this PR on Azure - cluster bot example:
launch 4.15,openshift/cluster-image-registry-operator#930 azure
  1. Get the vnet and subnet names used in the cluster (save it to a text file or some other accessible place):
oc get machinesets -nopenshift-machine-api -ojson|jq '.items[1].spec.template.spec.providerSpec.value | (.vnet, .subnet)'
  1. Edit the registry operator config
oc edit configs.imageregistry/cluster
  1. Configure the private endpoint in the registry operator config using vnet and subnet names:
spec:
  [...]
   storage:
      azure:
        [...]
        networkAccess:
          type: Internal
          internal:
            subnetName: <subnet-name>
            vnetName: <vnetnet-name>
  1. Wait for the operator to configure the private endpoint (the command will return the endpoint name - this may take a couple minutes):
oc get configs.imageregistry/cluster -o=jsonpath="{.spec.storage.azure.privateEndpointName}" -w
  1. Enable the default route and disable redirect on the operator config
oc patch configs.imageregistry cluster --type=merge -p '{"spec":{"defaultRoute": true, "disableRedirect": true}}'
  1. Verify that you can pull an image directly from the registry
REGISTRY=$(oc get route default-route -n openshift-image-registry --template='{{ .spec.host }}')
podman login --tls-verify=false -u unused -p $(oc whoami -t) ${REGISTRY}
podman pull --tls-verify=false ${REGISTRY}/openshift/tools
  1. Enable redirect and verify that you cannot pull an image from the registry (the registry will redirect to the storage account, and the storage account is now private so this is expected to not work)
oc patch configs.imageregistry cluster --type=merge -p '{"spec":{"disableRedirect": false}}'
# this will take a minute to propagate, check that the pods have recycled
oc get pods -nopenshift-image-registry -w
podman rmi ${REGISTRY}/openshift/tools
podman pull --tls-verify=false ${REGISTRY}/openshift/tools # this will fail with:
Error: parsing image configuration: fetching blob: StatusCode: 403, <?xml version="1.0" encoding="utf-8"?><Error><C...

Testing operator discovers the vnet and subnet names

  1. Provision a cluster with this PR on Azure (if you already have a cluster from the previous test you may reuse it but you need to set the registry to "Removed", wait for the transition, then set it to "Managed" again) - cluster bot example:
launch 4.15,openshift/cluster-image-registry-operator#930 azure
  1. Edit the registry operator config
oc edit configs.imageregistry/cluster
  1. Configure the private endpoint in the registry operator config using vnet and subnet names (note that we purposefully omit vnetName and subnetName):
spec:
  [...]
   storage:
      azure:
        [...]
        networkAccess:
          type: Internal
  1. Wait for the operator to configure the private endpoint (the command will return the endpoint name - this may take a couple minutes):
oc get configs.imageregistry/cluster -o=jsonpath="{.spec.storage.azure.privateEndpointName}" -w
  1. Enable the default route and disable redirect on the operator config
oc patch configs.imageregistry cluster --type=merge -p '{"spec":{"defaultRoute": true, "disableRedirect": true}}'
  1. Verify that you can pull an image directly from the registry
REGISTRY=$(oc get route default-route -n openshift-image-registry --template='{{ .spec.host }}')
podman login --tls-verify=false -u unused -p $(oc whoami -t) ${REGISTRY}
podman pull --tls-verify=false ${REGISTRY}/openshift/tools
  1. Enable redirect and verify that you cannot pull an image from the registry (the registry will redirect to the storage account, and the storage account is now private so this is expected to not work)
oc patch configs.imageregistry cluster --type=merge -p '{"spec":{"disableRedirect": false}}'
# this will take a minute to propagate, check that the pods have recycled
oc get pods -nopenshift-image-registry -w
podman rmi ${REGISTRY}/openshift/tools
podman pull --tls-verify=false ${REGISTRY}/openshift/tools # this will fail with:
Error: parsing image configuration: fetching blob: StatusCode: 403, <?xml version="1.0" encoding="utf-8"?><Error><C...

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 2, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 2, 2023

@flavianmissi: This pull request references IR-366 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

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.

@flavianmissi
Copy link
Member Author

this is a work in progress.
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 2, 2023

@flavianmissi: This pull request references IR-366 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

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.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 2, 2023

@flavianmissi: This pull request references IR-366 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

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.

@openshift-merge-robot
Copy link
Contributor

@flavianmissi: 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/verify bfc1f3e link true /test verify
ci/prow/e2e-azure-operator bfc1f3e link false /test e2e-azure-operator
ci/prow/e2e-azure-ovn bfc1f3e link false /test e2e-azure-ovn
ci/prow/e2e-openstack bfc1f3e link false /test e2e-openstack
ci/prow/images bfc1f3e link true /test images
ci/prow/e2e-aws-ovn-upgrade bfc1f3e link true /test e2e-aws-ovn-upgrade
ci/prow/e2e-hypershift-conformance bfc1f3e link true /test e2e-hypershift-conformance
ci/prow/unit bfc1f3e link true /test unit
ci/prow/e2e-aws-operator bfc1f3e link true /test e2e-aws-operator
ci/prow/e2e-aws-ovn-image-registry bfc1f3e link true /test e2e-aws-ovn-image-registry
ci/prow/e2e-aws-ovn bfc1f3e link true /test e2e-aws-ovn
ci/prow/e2e-hypershift bfc1f3e link true /test e2e-hypershift

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.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 30, 2023

@flavianmissi: This pull request references IR-366 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

Testing

  1. Provision a cluster with this PR on Azure - cluster bot example:
launch 4.15,openshift/cluster-image-registry-operator#930 azure
  1. Get the vnet and subnet names used in the cluster (save it to a text file or some other accessible place):
oc get machinesets <machineset-name> -nopenshift-machine-api -oyaml|yq '.spec.template.spec.providerSpec.value | (.vnet, .subnet)'
  1. Edit the registry operator config
$ oc edit configs.imageregistry/cluster
  1. Configure the private endpoint in the registry operator config using vnet and subnet names:
spec:
 [...]
  storage:
     azure:
       [...]
       networkAccess: Internal
       subnetName: <subnet-name>
       vnetName: <vnetnet-name>

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.

@flavianmissi
Copy link
Member Author

this needs to go in after openshift/api#1607
/hold

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Nov 21, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 21, 2023

@flavianmissi: This pull request references IR-366 which is a valid jira issue.

This pull request references IR-367 which is a valid jira issue.

This pull request references IR-411 which is a valid jira issue.

In response to this:

Testing

This PR introduces support for private storage accounts in Azure by configuring a private endpoint when requested by the user.
Making a storage account private can happen in one of two ways:

  1. user provided vnet and subnet names
  2. operator discovers the vnet and subnet names

Testing user provided vnet and subnet names

  1. Provision a cluster with this PR on Azure - cluster bot example:
launch 4.15,openshift/cluster-image-registry-operator#930 azure
  1. Get the vnet and subnet names used in the cluster (save it to a text file or some other accessible place):
oc get machinesets -nopenshift-machine-api -ojson|jq '.items[1].spec.template.spec.providerSpec.value | (.vnet, .subnet)'
  1. Edit the registry operator config
oc edit configs.imageregistry/cluster
  1. Configure the private endpoint in the registry operator config using vnet and subnet names:
spec:
 [...]
  storage:
     azure:
       [...]
       networkAccess:
         type: Internal
         internal:
           subnetName: <subnet-name>
           vnetName: <vnetnet-name>
  1. Wait for the operator to configure the private endpoint (the command will return the endpoint name - this may take a couple minutes):
oc get configs.imageregistry/cluster -o=jsonpath="{.spec.storage.azure.privateEndpointName}" -w
  1. Enable the default route and disable redirect on the operator config
oc patch configs.imageregistry cluster --type=merge -p '{"spec":{"defaultRoute": true, "disableRedirect": true}}'
  1. Verify that you can pull an image directly from the registry
REGISTRY=$(oc get route default-route -n openshift-image-registry --template='{{ .spec.host }}')
podman login --tls-verify=false -u unused -p $(oc whoami -t) ${REGISTRY}
podman pull --tls-verify=false ${REGISTRY}/openshift/tools
  1. Enable redirect and verify that you cannot pull an image from the registry (the registry will redirect to the storage account, and the storage account is now private so this is expected to not work)
oc patch configs.imageregistry cluster --type=merge -p '{"spec":{"disableRedirect": false}}'
# this will take a minute to propagate, check that the pods have recycled
oc get pods -nopenshift-image-registry -w
podman rmi ${REGISTRY}/openshift/tools
podman pull --tls-verify=false ${REGISTRY}/openshift/tools # this will fail with:
Error: parsing image configuration: fetching blob: StatusCode: 403, <?xml version="1.0" encoding="utf-8"?><Error><C...

Testing operator discovers the vnet and subnet names

  1. Provision a cluster with this PR on Azure (if you already have a cluster from the previous test you may reuse it but you need to set the registry to "Removed", wait for the transition, then set it to "Managed" again) - cluster bot example:
launch 4.15,openshift/cluster-image-registry-operator#930 azure
  1. Edit the registry operator config
oc edit configs.imageregistry/cluster
  1. Configure the private endpoint in the registry operator config using vnet and subnet names (note that we purposefully omit vnetName and subnetName):
spec:
 [...]
  storage:
     azure:
       [...]
       networkAccess:
         type: Internal
  1. Wait for the operator to configure the private endpoint (the command will return the endpoint name - this may take a couple minutes):
oc get configs.imageregistry/cluster -o=jsonpath="{.spec.storage.azure.privateEndpointName}" -w
  1. Enable the default route and disable redirect on the operator config
oc patch configs.imageregistry cluster --type=merge -p '{"spec":{"defaultRoute": true, "disableRedirect": true}}'
  1. Verify that you can pull an image directly from the registry
REGISTRY=$(oc get route default-route -n openshift-image-registry --template='{{ .spec.host }}')
podman login --tls-verify=false -u unused -p $(oc whoami -t) ${REGISTRY}
podman pull --tls-verify=false ${REGISTRY}/openshift/tools
  1. Enable redirect and verify that you cannot pull an image from the registry (the registry will redirect to the storage account, and the storage account is now private so this is expected to not work)
oc patch configs.imageregistry cluster --type=merge -p '{"spec":{"disableRedirect": false}}'
# this will take a minute to propagate, check that the pods have recycled
oc get pods -nopenshift-image-registry -w
podman rmi ${REGISTRY}/openshift/tools
podman pull --tls-verify=false ${REGISTRY}/openshift/tools # this will fail with:
Error: parsing image configuration: fetching blob: StatusCode: 403, <?xml version="1.0" encoding="utf-8"?><Error><C...

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.

@flavianmissi
Copy link
Member Author

/retest

1 similar comment
@flavianmissi
Copy link
Member Author

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2023
@flavianmissi
Copy link
Member Author

need another update on openshift/api, so using a replace again. holding until it merges.
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023
…ork resource group

when users bring their our network, they will commonly have a dedicated
resource group where the network resources are. vnet and subnet ids
contain the resource group of where these resources exist in, so the
operator needs to distinguish between the cluster resource group, and
the user provided network resource group.

this commit adds support for such use-case.
note that it's importante that the private endpoint is created within
the cluster resource group, which is managed by the installer. this is
so that the private endpoint is guaranteed to be destroyed when the
cluster itself is destroyed.
@dmage
Copy link
Member

dmage commented Nov 23, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 23, 2023
Copy link
Contributor

openshift-ci bot commented Nov 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, flavianmissi

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:

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

@flavianmissi
Copy link
Member Author

/retest

1 similar comment
@flavianmissi
Copy link
Member Author

/retest

Copy link
Contributor

openshift-ci bot commented Nov 24, 2023

@flavianmissi: The following test 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-vsphere f4fef9b link false /test e2e-vsphere

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.

@flavianmissi
Copy link
Member Author

/retest

@flavianmissi
Copy link
Member Author

I'm adding the required labels in the interest of merging this before code freeze. I've notified PX.
/label acknowledge-critical-fixes-only
/label px-approved

@openshift-ci openshift-ci bot added acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. px-approved Signifies that Product Support has signed off on this PR labels Nov 27, 2023
@flavianmissi
Copy link
Member Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 0c6fdc3 into openshift:master Nov 27, 2023
13 checks passed
@flavianmissi flavianmissi deleted the IR-366 branch November 27, 2023 13:47
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-image-registry-operator-container-v4.15.0-202311271455.p0.g0c6fdc3.assembly.stream for distgit ose-cluster-image-registry-operator.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants