Skip to content

imageresitry support Alibabacloud oss #1009

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.

Sign up for GitHub

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

Closed
menglingwei wants to merge 7 commits into from
Closed

imageresitry support Alibabacloud oss #1009

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
72dbb1f
imageresitry support Alibabacloud oss
Sep 10, 2021
2ad6000
fix comments and add offical documentations link
Sep 16, 2021
0a38efa
Merge branch 'openshift:master' into master
menglingwei Nov 29, 2021
1f1b466
Merge branch 'openshift:master' into master
menglingwei Dec 7, 2021
e20bee9
To redefine the ImageRegistryConfigStorageAlibabaOSS
Dec 9, 2021
89df310
update crd
Dec 9, 2021
9158054
Adding updates from update-gofmt for go1.17
Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions imageregistry/v1/00-crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,25 @@ spec:
description: managementState indicates if the operator manages the underlying storage unit. If Managed the operator will remove the storage when this operator gets Removed.
type: string
pattern: ^(Managed|Unmanaged)$
oss:
description: OSS represents configuration that uses Alibaba Cloud Object Storage Service.
type: object
properties:
bucket:
description: bucket is the bucket name in which you want to store the registry's data. Optional, will be generated if not provided. About Bucket naming, more details you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/257087.htm)
type: string
encrypt:
description: encrypt specifies whether you would like your data encrypted on the server side. Defaults to false if not specified. Optional, defaults to false. More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)
type: boolean
internal:
description: internal specifies whether the registry use the OSS VPC internal endpoint Optional, defaults to false. if RegionEndpoint is specified, this config will be ignored
type: boolean
region:
description: region is the Alibaba Cloud Region in which your bucket exists. Optional, will be set based on the installed Alibaba Cloud Region. For a list of regions, you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/31837.html).
type: string
regionEndpoint:
description: regionEndpoint is the endpoint for OSS compatible storage services. Optional, defaults based on the Region that is provided. An endpoint which defaults to [bucket].[region].aliyuncs.com or [bucket].[region]-internal.aliyuncs.com (when internal=true). You can change the default endpoint by changing this value.
type: string
pvc:
description: pvc represents configuration that uses a PersistentVolumeClaim.
type: object
Expand Down Expand Up @@ -949,6 +968,25 @@ spec:
description: managementState indicates if the operator manages the underlying storage unit. If Managed the operator will remove the storage when this operator gets Removed.
type: string
pattern: ^(Managed|Unmanaged)$
oss:
description: OSS represents configuration that uses Alibaba Cloud Object Storage Service.
type: object
properties:
bucket:
description: bucket is the bucket name in which you want to store the registry's data. Optional, will be generated if not provided. About Bucket naming, more details you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/257087.htm)
type: string
encrypt:
description: encrypt specifies whether you would like your data encrypted on the server side. Defaults to false if not specified. Optional, defaults to false. More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)
type: boolean
internal:
description: internal specifies whether the registry use the OSS VPC internal endpoint Optional, defaults to false. if RegionEndpoint is specified, this config will be ignored
type: boolean
region:
description: region is the Alibaba Cloud Region in which your bucket exists. Optional, will be set based on the installed Alibaba Cloud Region. For a list of regions, you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/31837.html).
type: string
regionEndpoint:
description: regionEndpoint is the endpoint for OSS compatible storage services. Optional, defaults based on the Region that is provided. An endpoint which defaults to [bucket].[region].aliyuncs.com or [bucket].[region]-internal.aliyuncs.com (when internal=true). You can change the default endpoint by changing this value.
type: string
pvc:
description: pvc represents configuration that uses a PersistentVolumeClaim.
type: object
Expand Down
35 changes: 35 additions & 0 deletions imageregistry/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,38 @@ type ImageRegistryConfigStorageIBMCOS struct {
ServiceInstanceCRN string `json:"serviceInstanceCRN,omitempty"`
}

// ImageRegistryConfigStorageOSS holds Alibaba Cloud OSS configuration.
Comment thread
menglingwei marked this conversation as resolved.
Outdated
// the registry to use Alibaba Cloud Object Storage Service for backend storage.

@bparees bparees Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// the registry to use Alibaba Cloud Object Storage Service for backend storage.
// Configures the registry to use Alibaba Cloud Object Storage Service for backend storage.

?

// More about oss, you can look at the [official documentation](https://www.alibabacloud.com/help/product/31815.htm)
type ImageRegistryConfigStorageOSS struct {

@deads2k deads2k Dec 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since there's another push coming anyway, let's clean this up to ImageRegistryConfigStorageAlibaba. I think most of us won't recognize OSS

@menglingwei menglingwei Dec 8, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Got it.

// bucket is the bucket name in which you want to store the registry's
// data.
// Optional, will be generated if not provided.
Comment thread
menglingwei marked this conversation as resolved.
// +optional

@bparees bparees Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this come after the comment?

// About Bucket naming, more details you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/257087.htm)
Bucket string `json:"bucket,omitempty"`

@sttts sttts Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

which strings are allowed?

// region is the Alibaba Cloud Region in which your bucket exists.
// Optional, will be set based on the installed Alibaba Cloud Region.
// +optional

@bparees bparees Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this come after the comment?

// For a list of regions, you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/31837.html).
Region string `json:"region,omitempty"`
// regionEndpoint is the endpoint for OSS compatible storage services.
// Optional, defaults based on the Region that is provided.
// +optional
// An endpoint which defaults to [bucket].[region].aliyuncs.com or [bucket].[region]-internal.aliyuncs.com (when internal=true).
// You can change the default endpoint by changing this value.

@deads2k deads2k Dec 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you specifically called this out. Is the rest of this API immutable?

@menglingwei menglingwei Dec 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@deads2k I took a look at the AWS structure definition. We have capabilities that are pretty much the same as AWS.So. I thought, can we keep the same definition as AWS? As follows:

type EndpointAccessibility string
var (
   InternalEndpoint = EndpointAccessibility("Internal")
   PublicEndpoint = EndpointAccessibility("Public")
)


type ImageRegistryConfigStorageOSS struct {
	// bucket is the bucket name in which you want to store the registry's data.
	// Optional, will be generated if not provided.
	// +optional
	// About Bucket naming, more details you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/257087.htm)
	Bucket string `json:"bucket,omitempty"`
	// region is the Alibaba Cloud Region in which your bucket exists.
	// Optional, will be set based on the installed Alibaba Cloud Region.
	// +optional
	// For a list of regions, you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/31837.html).
	Region string `json:"region,omitempty"`
	// EndpointAccessibility specifies whether the registry use the OSS VPC internal endpoint
	// Optional, defaults to Internal.
	// +optional
	EndpointAccessibility EndpointAccessibility `json:"endpointAccessibility,omitempty"`
	// encrypt specifies whether the registry stores the image in encrypted
	// format or not.
	// Optional, defaults to false.
	// +optional
	// More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)
	Encrypt bool `json:"encrypt,omitempty"`
	// keyID is the KMS key ID to use for encryption.
	// Optional, Encrypt must be true, or this parameter is ignored.
	// +optional
	KeyID string `json:"keyID,omitempty"`
}

If you think this is ok, I will modify the code and resubmit the PR.

@menglingwei menglingwei Dec 7, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If the KeyID is set, we may set SSEAlgorithm to KMS ,Otherwise, use the default value（AES256).

@menglingwei menglingwei Dec 7, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The api document link https://www.alibabacloud.com/help/doc-detail/117914.htm?spm=a2c63.p38356.b99.1075.5c3e56989tiYEz

@deads2k deads2k Dec 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have capabilities that are pretty much the same as AWS.So. I thought, can we keep the same definition as AWS?

We cannot fix previous mistakes (this was in an operator repo when it was created), but we can avoid creating new instances of it. This API should follow our conventions.

@menglingwei menglingwei Dec 8, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Do we need to keep Encrypt as it used to be? Now the full definition is as follows, right?

type EndpointAccessibility string
var (
   InternalEndpoint = EndpointAccessibility("Internal")
   PublicEndpoint = EndpointAccessibility("Public")
)


type ImageRegistryConfigStorageOSS struct {
	// bucket is the bucket name in which you want to store the registry's data.
	// Optional, will be generated if not provided.
	// +optional
	// About Bucket naming, more details you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/257087.htm)
	Bucket string `json:"bucket,omitempty"`
	// region is the Alibaba Cloud Region in which your bucket exists.
	// Optional, will be set based on the installed Alibaba Cloud Region.
	// +optional
	// For a list of regions, you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/31837.html).
	Region string `json:"region,omitempty"`
	// EndpointAccessibility specifies whether the registry use the OSS VPC internal endpoint
	// Optional, defaults to Internal.
	// +optional
	EndpointAccessibility EndpointAccessibility `json:"endpointAccessibility,omitempty"`
	// encrypt specifies whether the registry stores the image in encrypted
	// format or not.
	// Optional, defaults to false.
	// +optional
	// More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)
	Encrypt bool `json:"encrypt,omitempty"`
}

RegionEndpoint string `json:"regionEndpoint,omitempty"`
// internal specifies whether the registry use the OSS VPC internal endpoint
// Optional, defaults to false. if RegionEndpoint is specified, this config will be ignored
Comment thread
menglingwei marked this conversation as resolved.
Outdated
// +optional
Internal bool `json:"internal,omitempty"`

@deads2k deads2k Sep 14, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nothing is a bool. Use an enum string please, with values like Internal and its counterpoint.

@menglingwei menglingwei Sep 15, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think it's better to keep it the same as registry storage-driver for oss. https://github.com/docker/docker.github.io/blob/master/registry/storage-drivers/oss.md

@deads2k deads2k Dec 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The registry storage driver isn't an openshift API. openshift APIs look like kube APIs and we prefer to have enumerations where the value clearly indicates what it does and the list of values provides helpful description of what the alternatives are.

This field looks like "EndpointAccessibility" with values "Internal" or "Public". Looking at clear enums like that, the default ought to be internal.

@menglingwei menglingwei Dec 8, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So we might want to change Internal to EndpointAccessibility, and EndpointAccessibility is an enums filed.

var (
   InternalEndpoint = EndpointAccessibility("Internal")
   PublicEndpoint = EndpointAccessibility("Public")
)

type ImageRegistryConfigStorageOSS struct{
...
    EndpointAccessibility EndpointAccessibility `json:"endpointAccessibility,omitempty"`

}

Is ok?

@deads2k deads2k Dec 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is ok?

Yes, that looks good. Please be careful about resolving threads without a push. These comments are disappearing from view.

// encrypt specifies whether you would like your data encrypted on the server side. Defaults to false if not specified.
// Optional, defaults to false.
// +optional

@bparees bparees Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this come after the comment?

// More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)

@bparees bparees Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)
// More details, you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)

Encrypt bool `json:"encrypt,omitempty"`

@deads2k deads2k Sep 14, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You have two encryption related fields? this and keyID? You need to describe the interaction and likely produce an API that makes it impossible to specify an invalid combination.

@deads2k deads2k Sep 14, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why would we allow a non-encrypted option?

@menglingwei menglingwei Sep 15, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Here the documation https://www.alibabacloud.com/help/doc-detail/117914.htm?spm=a2c63.p38356.b99.1075.5c3e56989tiYEz.
Encrypt means enable server-side encryption. The default server-side encryption method.
Valid values: KMS, AES256. So if you not set keyID ,AES256 is used for default server-side encryption method.

@menglingwei menglingwei Sep 16, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

keep same as. https://github.com/docker/docker.github.io/blob/master/registry/storage-drivers/oss.md.
image

@deads2k deads2k Dec 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The registry storage driver isn't an openshift API. This API is an openshift API and should conform to our standards. Encryption with values of "ServerSideEncryption" and "ClearText" is very clear.

@menglingwei menglingwei Dec 8, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@kwoodson @deads2k From the above discussion. I think i need to change internal to an Enum filed like "EndpointAccessibility" , it may be Internal or Public, the default value is Internal. And the Encrty change to an Enum filed like Encryption,It is used to define the server-side encryption algorithm, KMS or AES256, and same time ,add KeyID , if the Encryption value is KMS, the KeyID must be set. So the final structure should be


type EndpointAccessibility string
var (
   InternalEndpoint = EndpointAccessibility("Internal")
   PublicEndpoint = EndpointAccessibility("Public")
)

type Encryption string
var(
 AES256 = Encryption("AES256")
 KMS = Encryption("KMS")
)

type ImageRegistryConfigStorageOSS struct {
	// bucket is the bucket name in which you want to store the registry's data.
	// Optional, will be generated if not provided.
	// +optional
	// About Bucket naming, more details you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/257087.htm)
	Bucket string `json:"bucket,omitempty"`
	// region is the Alibaba Cloud Region in which your bucket exists.
	// Optional, will be set based on the installed Alibaba Cloud Region.
	// +optional
	// For a list of regions, you can look at the [official documentation](https://www.alibabacloud.com/help/doc-detail/31837.html).
	Region string `json:"region,omitempty"`
	// EndpointAccessibility specifies whether the registry use the OSS VPC internal endpoint
	// Optional, defaults to Internal.
	// +optional
	EndpointAccessibility EndpointAccessibility `json:"endpointAccessibility,omitempty"`
	// encrypt specifies whether the registry stores the image in encrypted
	// format or not.
	// Optional, defaults to false.
	// +optional
	// More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)
	Encrypt Encryption `json:"encrypt,omitempty"`
	// keyID is the KMS key ID to use for encryption.
	// +optional
	KeyID string `json:"keyID,omitempty"`
}

I wonder if my understanding is correct? If it's correct, I'll do it this way

@menglingwei menglingwei Dec 8, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If user don't set Encrypt or out of range ,the default value is AES256. If user set Encrypt to KMS and KeyID is not empty, use KMS. Follow the api documents

type Encryption string
var(
AES256 = Encryption("AES256")
KMS = Encryption("KMS")
)

Encrypt Encryption `json:"encrypt,omitempty"`
KeyID string `json:"keyID,omitempty"`

@deads2k deads2k Dec 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't realize that keyID was related to Encryption before.

type ImageRegistryConfigStorageAlibaba struct{

}


type Encryption string
var(
    ClearText = Encryption("ClearText")
    AES256 = Encryption("AES256")
    KMS = Encryption("KMS") 
)


// this a union type in kube parlance.  Depending on the value for the encryptionType,
// different pointers may be used
type EncryptionAlibaba struct{
    EncryptionType Encryption `json:"encryptionType"`

    KMSEncryptionAlibaba *KMSEncryptionAlibaba  `json:"kms"`
}

type KMSEncryptionAlibaba struct{
    KeyID string
}

@menglingwei menglingwei Dec 8, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Got it.

@deads2k deads2k Dec 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this thread got resolved before a push, unresolving so we can see it.

}

// ImageRegistryConfigStorage describes how the storage should be configured
// for the image registry.
type ImageRegistryConfigStorage struct {
Expand Down Expand Up @@ -333,6 +365,9 @@ type ImageRegistryConfigStorage struct {
// ibmcos represents configuration that uses IBM Cloud Object Storage.
// +optional
IBMCOS *ImageRegistryConfigStorageIBMCOS `json:"ibmcos,omitempty"`
// OSS represents configuration that uses Alibaba Cloud Object Storage Service.

@dmage dmage Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These comments will be visible by customers and should contain YAML names, so

Suggested change
// OSS represents configuration that uses Alibaba Cloud Object Storage Service.
// oss represents configuration that uses Alibaba Cloud Object Storage Service.

// +optional
OSS *ImageRegistryConfigStorageOSS `json:"oss,omitempty"`
// managementState indicates if the operator manages the underlying
// storage unit. If Managed the operator will remove the storage when
// this operator gets Removed.
Expand Down
21 changes: 21 additions & 0 deletions imageregistry/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions imageregistry/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.