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 1 commit
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
44 changes: 44 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,28 @@ 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.
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.
type: string
encrypt:
description: encrypt specifies whether the registry stores the image in encrypted format or not. Optional, defaults to false.
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
keyID:
description: keyID is the KMS key ID to use for encryption. Optional, Encrypt must be true, or this parameter is ignored.
type: string
region:
description: region is the GCS location in which your bucket exists. Optional, will be set based on the installed GCS Region.
type: string
regionEndpoint:
description: regionEndpoint is the endpoint for S3 compatible storage services. Optional, defaults based on the Region that is provided.
type: string
pvc:
description: pvc represents configuration that uses a PersistentVolumeClaim.
type: object
Expand Down Expand Up @@ -949,6 +971,28 @@ 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.
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.
type: string
encrypt:
description: encrypt specifies whether the registry stores the image in encrypted format or not. Optional, defaults to false.
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
keyID:
description: keyID is the KMS key ID to use for encryption. Optional, Encrypt must be true, or this parameter is ignored.
type: string
region:
description: region is the GCS location in which your bucket exists. Optional, will be set based on the installed GCS Region.
type: string
regionEndpoint:
description: regionEndpoint is the endpoint for S3 compatible storage services. Optional, defaults based on the Region that is provided.
type: string
pvc:
description: pvc represents configuration that uses a PersistentVolumeClaim.
type: object
Expand Down
33 changes: 33 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,36 @@ type ImageRegistryConfigStorageIBMCOS struct {
ServiceInstanceCRN string `json:"serviceInstanceCRN,omitempty"`
}

// ImageRegistryConfigStorageOSS holds Alibaba Cloud OSS configuration.
Comment thread
menglingwei marked this conversation as resolved.
Outdated
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?

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 GCS location in which your bucket exists.
// Optional, will be set based on the installed GCS Region.
Comment thread
menglingwei marked this conversation as resolved.
Outdated
// +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?

Region string `json:"region,omitempty"`
// regionEndpoint is the endpoint for S3 compatible storage services.
Comment thread
menglingwei marked this conversation as resolved.
Outdated
// Optional, defaults based on the Region that is provided.
// +optional
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 the registry stores the image in encrypted
// format or not.
// Optional, defaults to false.
// +optional
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.

// keyID is the KMS key ID to use for encryption.
Comment thread
menglingwei marked this conversation as resolved.
Outdated
// Optional, Encrypt must be true, or this parameter is ignored.
// +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?

KeyID string `json:"keyID,omitempty"`
}

// ImageRegistryConfigStorage describes how the storage should be configured
// for the image registry.
type ImageRegistryConfigStorage struct {
Expand Down Expand Up @@ -333,6 +363,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.
// +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.

15 changes: 15 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.