Alibaba Cloud Provider: introducing imageregistry types by kwoodson · Pull Request #1082 · openshift/api

kwoodson · 2021-12-10T18:36:41Z

Summary: This PR adds the additional fields necessary to provide support for Alibaba imageregistry.

Updated the descriptions
updated the kubebuilder fields
cleaned up comments on the fields
cleaned up the constant / enumerable types to match OpenShift patterns.

This PR replaces the previous #1009.

sttts · 2021-12-10T19:04:04Z

+type KMSEncryptionAlibaba struct {
+	// KeyID holds the KMS encryption key ID
+	// +kubebuilder:validation:Required
+	KeyID string `json:"keyID"`


can this be empty? Which format?

When the Type is set to KMS then a key would be required. When it is not, it can be empty.

The CMK ID that must be specified when SSEAlgorithm is set to KMS and a specified CMK is used for encryption. In other cases, this element must be set to null.

sttts · 2021-12-10T19:24:16Z

+	PublicEndpoint EndpointAccessibility = "Public"
+
+	// PlainText is an AlibabaEncryptionMode. This means no encryption
+	PlainText AlibabaEncryptionMode = "PlainText"


should this be the default value? Then declare a default via marker.

Declared below where Type is defined.

Set as default

sttts · 2021-12-10T19:28:15Z

+
+	// KMS (key management service) is an encryption type provided to encrypt the provider
+	// +optional
+	KMS *KMSEncryptionAlibaba `json:"kms,omitempty"`


if this is only to be set when type matches, you have to add a patch file (can be follow-up), setting

anyOf: - properties: type: not: enum: ["KMS"] not: required: ["kms"] - properties: type: enum: ["KMS"] required: ["kms"]

Double check the logic.

Let me research how to do this.

From our discussion in slack https://github.com/openshift/api/pull/1082/files#diff-3e8a362860cb72aadab809e7fdb9a0d112ed3e6f09c50c89ac77cf3453189880

sttts · 2021-12-10T21:44:57Z

+	// EndpointAccessibility specifies whether the registry use the OSS VPC internal endpoint
+	// Empty value means no opinion and the platform chooses the a default, which is subject to change over time.
+	// Currently the default is `Internal`.
+	// +kubebuilder:validation:Enum="Internal";"Public"


also need "", because client side validation does not see the default.

sttts · 2021-12-10T21:45:40Z

+	// Currently the default is `PlainText`.
+	// +kubebuilder:validation:Enum="PlainText";"KMS";"AES256"
+	// +kubebuilder:default="PlainText"
+	// +kubebuilder:validation:Required


with the default this must be optional now because client-side validation does not see the default.

Updated to optional.

sttts · 2021-12-10T22:31:27Z

/lgtm
/approve

/hold
Please test in a real cluster and verify that it behaves as it should.

kwoodson · 2021-12-10T22:37:54Z

@sttts Thanks for the help. I'll get a test cluster going now.

kwoodson · 2021-12-14T01:42:10Z

Fixed typo in description and nothing else.

kwoodson · 2021-12-14T01:56:08Z

/test verify

kwoodson · 2021-12-14T02:22:52Z

Verify had a few of these. Wondering if something wrong with the CI.

W1214 01:48:40.137749   14982 clientconn.go:1223] grpc: addrConn.createTransport failed to connect to {http://127.0.0.1:2379  <nil> 0 <nil>}. Err :connection error: desc = "transport: Error while dialing dial tcp 127.0.0.1:2379: connect: connection refused". Reconnecting...

kwoodson · 2021-12-14T02:23:10Z

/test verify

kwoodson · 2021-12-14T03:53:07Z

@dmage @sttts In order to correctly apply the kubebuilder/openAPI patch to the CRD I had to rename the 00-crd.yaml to 00-imageregistry.crd.yaml. I have noticed that this CRD no longer gets deployed when testing this latest pull request.

Does this require an openshift/client-go and an openshift/library-go update?

kwoodson · 2021-12-14T15:07:12Z

I have updated the 00- and 01- CRD files to have a meaningful name 00_imageregistry.crd.yaml as well as 01_imagepruner.crd.yaml. This allows the patch to be applied.

Testing these changes now.

mtulio · 2021-12-14T19:00:53Z

+	// Mode defines the different encrytion types available
+	// Empty value means no opinion and the platform chooses the a default, which is subject to change over time.
+	// Currently the default is `AES256`.
+	// +kubebuilder:validation:Enum="PlainText";"KMS";"AES256"
+	// +kubebuilder:default="AES256"
+	// +optional
+	Mode AlibabaEncryptionMode `json:"mode"`


@sttts @kwoodson I am concerning about the field name mode, looking the Alibaba Console, bucket encryption configuration, we have three methods of encryption:

None

OSS-Managed

KMS

Then it has only one encryption algorithm:

AES256

I am concerned if we could rename this field from mode to method, with the default value of OSS-Managed.

I am not sure if we need the field algorithm, but it could be interesting for future capabilities. For now, it will assume only one value: AES256

I updated the Type to Method to align with the API. I updated the default to AES256 after @mtulio's tests. This seems to be what we wanted it to be by default.

Here is the reference where we are setting to default, if KMS is not set:
https://github.com/openshift/cluster-image-registry-operator/pull/724/files#diff-e0f51602f2f4156d47e688910139da09ff81fe2ac10efa43d7d453b06ee08ec8R502

Have nothing against these changes, but be quick. This should land before FF on Friday (I am out, but David Eads can approve).

@sttts Looks like we have been bitten by the yaml_patch issue. I have submitted a PR #1086 to fix it. We would like to merge that one so that I can generate code properly in this PR. Would you please TAL?

cc @deads2k

@sttts Awesome, thanks for your help!

mtulio · 2021-12-14T19:03:08Z

/lgtm /approve

/hold Please test in a real cluster and verify that it behaves as it should.

@kwoodson @sttts please take a look at those tests: openshift/cluster-image-registry-operator#724 (comment)

It's working as expected, but I left some comments regarding the naming conventions of those fields, considering the Console/UI. Please let me know wdyt.

kwoodson · 2021-12-14T19:16:48Z

/hold

I have submitted #1086 to fix the test verify that has been failing.

kwoodson · 2021-12-14T20:00:24Z

Rebased to clean up commits from the #1086 merge.

kwoodson · 2021-12-14T20:01:34Z

/hold cancel

openshift-ci · 2021-12-14T20:11:59Z

@kwoodson: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

mtulio · 2021-12-14T20:12:41Z

/lgtm

openshift-ci · 2021-12-14T20:14:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwoodson, mtulio, sttts

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [sttts]

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

openshift-ci Bot requested review from bparees and derekwaynecarr December 10, 2021 18:37

kwoodson force-pushed the alibaba_reg_updates branch 3 times, most recently from d157107 to 3de2385 Compare December 10, 2021 19:01