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

update backupbucket fields #931

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Conversation

kon-angelo
Copy link
Contributor

@kon-angelo kon-angelo commented Aug 1, 2024

How to categorize this PR?

/area control-plane
/kind enhancement
/platform azure

What this PR does / why we need it:
Publish the BackupBucket domain name, to enable different Azure landscapes.
See: gardener/etcd-backup-restore#756

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

The BackupBucket controller now adds an additional field in the generated secret to indicate the blob storage service domain. This can be used to create blobs in other Azure environments like CN or USGov.

@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension platform/azure Microsoft Azure platform/infrastructure needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Aug 1, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 1, 2024
AndreasBurger
AndreasBurger previously approved these changes Aug 1, 2024
Copy link

@unmarshall unmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments

// the new options _also_ allow configuring the cloud instance.
switch {
case cloudConfiguration == nil:
return "blob.core.windows.net", nil

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to make constants for these? (It is a pity that designers of the SDK do not think of these things)

const (
  BlobStoreGlobalDomain = "blob.core.windows.net"
  BlobStoreChinaDomain = "blob.core.chinacloudapi.cn"
  BlobStoreUSGovDomain = "blob.core.usgovcloudapi.net"

switch {
case cloudConfiguration == nil:
return "blob.core.windows.net", nil
case strings.EqualFold(cloudConfiguration.Name, "AzurePublic"):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can club 2 cases into 1:

case cloudConfiguration == nil:
case strings.EqualFold(cloudConfiguration.Name, "AzurePublic"):
  return "blob.core.windows.net", nil

Copy link
Member

@renormalize renormalize Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fallthrough must be used to achieve this functionality in Go, but there's a more idiomatic way of doing this.

case cloudConfiguration == nil, strings.EqualFold(cloudConfiguration.Name, "AzurePublic"):
    return "blob.core.windows.net", nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you would need to use fallthrough for that. Also in a very recent PR of mine in gardener core, they voted to merge conditions by ||. I will probably do the same here for coherence

if err != nil {
return nil, fmt.Errorf("failed to parse service url: %v", err)
}
blobclient, err := azblob.NewClientWithSharedKeyCredential(storageEndpointURL.String(), credentials, nil)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the azure-sdk-for-go the nomenclature is a bit different. They provide 3 different clients AFAIK:

  1. Blob client which is created with a specific blob URL (see here)
  2. Container client which is created for a specific container/bucket ( see here).
  3. Service client to manage buckets/containers (see here)

So what you have here is a ContainerClient or a BucketClient but you call it a blobClient. This confused me a bit.

Copy link
Contributor Author

@kon-angelo kon-angelo Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to look closer for that one. The blob client that we use, creates all the clients internally, so e.g. from the interfaces that we use the azure SDK does something like:

func (c *Client) DeleteBlob(ctx context.Context, containerName string, blobName string, o *DeleteBlobOptions) (DeleteBlobResponse, error) {
	return c.svc.NewContainerClient(containerName).NewBlobClient(blobName).Delete(ctx, o)
}

So from our perspective we only use the top level interfaces and all the container/bucket client is hidden from us - I guess that's why the name was blobClient. I will think how to do the names better.

@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 1, 2024
@kon-angelo kon-angelo marked this pull request as ready for review August 1, 2024 13:53
@kon-angelo kon-angelo requested review from a team as code owners August 1, 2024 13:53
@kon-angelo
Copy link
Contributor Author

/hold

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Aug 1, 2024
@kon-angelo
Copy link
Contributor Author

tested with http://europe-docker.pkg.dev/gardener-project/snapshots/gardener/etcd-druid:v0.22.4-dev-c692c9db187df47bc45c2d9815e5e31108b30faf successfully.

/unhold

@gardener-robot gardener-robot removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Aug 2, 2024
@AndreasBurger AndreasBurger merged commit a812fef into gardener:master Aug 14, 2024
12 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review needs/second-opinion Needs second review by someone else platform/azure Microsoft Azure platform/infrastructure size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants