imageresitry support Alibabacloud oss by menglingwei · Pull Request #1009 · openshift/api

menglingwei · 2021-09-10T02:35:36Z

No description provided.

openshift-ci · 2021-09-10T02:35:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: menglingwei
To complete the pull request process, please assign jwforres after the PR has been reviewed.
You can assign the PR to them by writing /assign @jwforres in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

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 · 2021-09-10T02:36:03Z

Hi @menglingwei. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

kwoodson · 2021-09-10T18:00:03Z

/ok-to-test

deads2k · 2021-09-14T14:07:52Z

+	// internal specifies whether the registry use the OSS VPC internal endpoint
+	// Optional, defaults to false. if RegionEndpoint is specified, this config will be ignored
+	// +optional
+	Internal bool `json:"internal,omitempty"`


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

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

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.

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?

Is ok?

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

deads2k · 2021-09-14T14:09:43Z

+	// format or not.
+	// Optional, defaults to false.
+	// +optional
+	Encrypt bool `json:"encrypt,omitempty"`


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.

Why would we allow a non-encrypted option?

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.

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

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.

@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

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"`

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 }

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

dmage · 2021-09-14T14:35:02Z

The registry team isn't allowed to merge PRs without approvals from QE/Docs/Px (i.e. QE should confirm that the feature works as expected, Docs can confirm that they have capacity to document it), so I'd expect to have at least a draft PR for cluster-image-registry-operator. You can use replace in your go.mod to create a draft PR with these changes.

menglingwei · 2021-09-17T05:33:33Z

replace

ok.

menglingwei · 2021-09-22T06:30:55Z

The registry team isn't allowed to merge PRs without approvals from QE/Docs/Px (i.e. QE should confirm that the feature works as expected, Docs can confirm that they have capacity to document it), so I'd expect to have at least a draft PR for cluster-image-registry-operator. You can use replace in your go.mod to create a draft PR with these changes.

I post a new PR openshift/cluster-image-registry-operator#724.

menglingwei · 2021-09-23T07:36:22Z

@dmage I posted a draf PR openshift/cluster-image-registry-operator#724. In cluster-image-reigstry-operator, I need the structure defined in openshift/api. Same as other storage registry privders, such as AWS, GCP, etc

menglingwei · 2021-10-28T01:59:02Z

hi,any questions about this PR?

mtulio · 2021-11-25T14:24:30Z

@menglingwei we are making sure that the PR openshift/cluster-image-registry-operator#724 will work with the tests described here using new credentials from CCO. Then when image-registry is validated we can follow up on this one. All the builds is including this change.

menglingwei · 2021-11-26T02:25:57Z

@menglingwei we are making sure that the PR openshift/cluster-image-registry-operator#724 will work with the tests described here using new credentials from CCO. Then when image-registry is validated we can follow up on this one. All the builds is including this change.

@mtulio got it.

deads2k · 2021-12-08T21:51:04Z

+// ImageRegistryConfigStorageOSS holds Alibaba Cloud OSS configuration.
+// 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 {


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

menglingwei · 2021-12-09T01:20:09Z

/retest

menglingwei · 2021-12-09T05:14:41Z

/retest

menglingwei · 2021-12-09T07:10:37Z

/retest

menglingwei · 2021-12-09T07:42:32Z

/retest

menglingwei · 2021-12-09T08:56:14Z

I find this job(ci/prow/verify ) fails all the time, I don't know why? @deads2k @kwoodson Who can help me to solve this task?

menglingwei · 2021-12-09T09:00:10Z

In my dev environment,. when i run make verify. The following error occurs

hack/../hack/update-deepcopy.sh
Generating deepcopy funcs
F1209 16:50:52.181929 38267 main.go:82] Error: Failed executing generator: some packages had errors:
errors in package "github.com/openshift/api/user/v1":
output for "v1/zz_generated.deepcopy.go" differs; first existing/expected diff:
"go:build !ignore_autogenerated\n// +build !ignore_autogenerated\n\n// Code generated by deepcopy-gen. D"
" +build !ignore_autogenerated\n\n// Code generated by deepcopy-gen. DO NOT EDIT.\n\npackage v1\n\nimport ("

menglingwei · 2021-12-09T11:43:18Z

/retest

menglingwei · 2021-12-09T12:11:49Z

/retest

kwoodson · 2021-12-09T14:35:05Z

@menglingwei This is caused by the generated code and golang 1.17. When I run make update-gofmt I receive a single update:

 make update-gofmt
Running `gofmt -s -l -w` on 352 file(s).
./imageregistry/v1/zz_generated.deepcopy.go

 git diff
diff --git a/imageregistry/v1/zz_generated.deepcopy.go b/imageregistry/v1/zz_generated.deepcopy.go
index 00748800..0cb96ed9 100644
--- a/imageregistry/v1/zz_generated.deepcopy.go
+++ b/imageregistry/v1/zz_generated.deepcopy.go
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 // Code generated by deepcopy-gen. DO NOT EDIT.

Please rerun with golang1.17, add the change, and push.

menglingwei · 2021-12-10T01:04:53Z

@menglingwei This is caused by the generated code and golang 1.17. When I run make update-gofmt I receive a single update:

 make update-gofmt
Running `gofmt -s -l -w` on 352 file(s).
./imageregistry/v1/zz_generated.deepcopy.go

 git diff
diff --git a/imageregistry/v1/zz_generated.deepcopy.go b/imageregistry/v1/zz_generated.deepcopy.go
index 00748800..0cb96ed9 100644
--- a/imageregistry/v1/zz_generated.deepcopy.go
+++ b/imageregistry/v1/zz_generated.deepcopy.go
@@ -1,3 +1,4 @@
+//go:build !ignore_autogenerated
 // +build !ignore_autogenerated
 
 // Code generated by deepcopy-gen. DO NOT EDIT.

Please rerun with golang1.17, add the change, and push.
@kwoodson Thank you.

menglingwei · 2021-12-10T01:21:27Z

@deads2k @kwoodson I updated the api defination,please help to reiveiw it. If the changes are acceptable, I wiil update the cluster-image-registry-operator dependency. Thank you.

kwoodson · 2021-12-10T01:44:01Z

@menglingwei Thanks for the fix! I have pinged the API team and we should receive a review.

menglingwei · 2021-12-10T02:10:01Z

@menglingwei Thanks for the fix! I have pinged the API team and we should receive a review.

@kwoodson Do I need to update cluster-image-registry-operator now or later?

kwoodson · 2021-12-10T03:10:12Z

@menglingwei After this merges, please update the cluster-image-registry-operator PR by removing your repository and update the openshift/api library in the go.mod file.

sttts · 2021-12-10T17:49:18Z

+type Encryption string
+
+var (
+	ClearText = Encryption("ClearText")


usual style is ClearText Encryption = "ClearText"

and they should be defined as const, not var.

sttts · 2021-12-10T17:49:42Z

+// EncryptionAlibaba 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"`


needs OpenAPI markers

is it required?

~~nvmd. These are native types, right? Not CRDs.~~

sttts · 2021-12-10T17:50:37Z

+type EncryptionAlibaba struct {
+	EncryptionType Encryption `json:"encryptionType"`
+
+	KMSEncryptionAlibaba *KMSEncryptionAlibaba `json:"kms"`


It's convenient when field names match their json names. So I'd recommend

KMS *KMSEncryptionAlibaba `json:"kms,omitempty"`

bparees · 2021-12-10T17:43:27Z

+// EncryptionAlibaba 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"`


doc this field and the valid values

bparees · 2021-12-10T17:44:40Z

+}
+
+type KMSEncryptionAlibaba struct {
+	KeyID string `json:"keyID"`


doc this field (key id of what/from where?)

is this security sensitive?

bparees · 2021-12-10T17:49:41Z

+	// +optional
+	EndpointAccessibility EndpointAccessibility `json:"endpointAccessibility,omitempty"`
+	// encrypt specifies whether you would like your data encrypted on the server side. Defaults to false if not specified.
+	// Optional, defaults to AES256.


this says "defaults to false" and then "defaults to aes256". Since it's not a boolean, I assume "defaults to false" is not accurate.

bparees · 2021-12-10T17:50:50Z

+	// encrypt specifies whether you would like your data encrypted on the server side. Defaults to false if not specified.
+	// Optional, defaults to AES256.
+	// +optional
+	// More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)


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)

bparees · 2021-12-10T17:54:13Z

+}
+
+// ImageRegistryConfigStorageAlibabaOSS holds Alibaba Cloud OSS configuration.
+// the registry to use Alibaba Cloud Object Storage Service for backend storage.


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.

?

bparees · 2021-12-10T17:54:56Z

+	// bucket is the bucket name in which you want to store the registry's
+	// data.
+	// Optional, will be generated if not provided.
+	// +optional


shouldn't this come after the comment?

bparees · 2021-12-10T17:55:00Z

+	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


shouldn't this come after the comment?

bparees · 2021-12-10T17:55:06Z

+	EndpointAccessibility EndpointAccessibility `json:"endpointAccessibility,omitempty"`
+	// encrypt specifies whether you would like your data encrypted on the server side. Defaults to false if not specified.
+	// Optional, defaults to AES256.
+	// +optional


shouldn't this come after the comment?

dmage · 2021-12-10T18:07:08Z

+// EncryptionAlibaba 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"`


So it'll look like

encrypt: encryptionType: KMS kms: keyID: ...

Maybe it's better to name it as

encryption: type: KMS kms: keyID: ...

?

I will update to type. This reduces verbiage and reads more clearly.

dmage · 2021-12-10T18:09:36Z

 	// 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.


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.

sttts · 2021-12-10T18:34:37Z

+	// Optional, defaults to AES256.
+	// +optional
+	// More details, you can look cat the [official documentation](https://www.alibabacloud.com/help/doc-detail/117914.htm)
+	Encrypt EncryptionAlibaba `json:"encrypt,omitempty"`


omitempty + non-point struct does not work.

You probably want a pointer to the struct.

sttts · 2021-12-10T18:35:03Z

+	// EndpointAccessibility specifies whether the registry use the OSS VPC internal endpoint
+	// Optional, defaults to Internal.
+	// +optional
+	EndpointAccessibility EndpointAccessibility `json:"endpointAccessibility,omitempty"`


needs enum marker

sttts · 2021-12-10T18:35:19Z

+	// 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"`


which strings are allowed?

kwoodson · 2021-12-10T19:48:04Z

Closing this one in favor of #1082

cc @menglingwei

imageresitry support Alibabacloud oss

72dbb1f

openshift-ci Bot requested review from knobunc and sttts September 10, 2021 02:35

openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 10, 2021

menglingwei mentioned this pull request Sep 10, 2021

imageresitry support Alibabacloud oss #1004

Closed

openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 10, 2021