Add domain and realm CRDs by andrew-su · Pull Request #12 · knative/networking

andrew-su · 2020-06-09T15:18:53Z

This adds the CRDs for Realm and Domain.

googlebot · 2020-06-09T15:37:06Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

shashwathi · 2020-06-09T15:37:30Z

@googlebot I consent.

googlebot · 2020-06-09T15:37:35Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

shashwathi · 2020-06-09T22:14:51Z

/assign @tcnghia

vagababov · 2020-06-09T22:36:54Z

+func (i *Domain) SetDefaults(ctx context.Context) {
+	i.Spec.SetDefaults(apis.WithinSpec(ctx))
+}


Suggested change

func (i *Domain) SetDefaults(ctx context.Context) {

i.Spec.SetDefaults(apis.WithinSpec(ctx))

}

func (d *Domain) SetDefaults(ctx context.Context) {

d.Spec.SetDefaults(apis.WithinSpec(ctx))

}

vagababov · 2020-06-09T22:37:43Z

+}
+
+// GetGroupVersionKind returns SchemeGroupVersion of an Ingress
+func (i *Domain) GetGroupVersionKind() schema.GroupVersionKind {


Suggested change

func (i *Domain) GetGroupVersionKind() schema.GroupVersionKind {

func (*Domain) GetGroupVersionKind() schema.GroupVersionKind {

vagababov · 2020-06-09T22:38:50Z

+	// Istio Gateways associated with this Domain. This could be a reference of a ConfigMap
+	// owned by the implementation as well.
+	// +optional
+	Config []map[string]string `json:"config,omitempty"`


Some people think we should not use maps in the apis
/cc @mattmoor

cc @dprotaso can you please suggest the best practice to represent maps here?

This is an array of maps - is that intended?

best practice to represent maps here?

Maybe a few criteria to think about

Patching

Although, CRDs don't support strategic merge patches the different types help reason about the different patch strategies that could open up in the future (although it's been shut down a bunch of times)

https://pkg.go.dev/k8s.io/kube-openapi/pkg/idl?tab=doc#ListType
https://pkg.go.dev/k8s.io/kube-openapi/pkg/idl?tab=doc#MapType
https://pkg.go.dev/k8s.io/kube-openapi/pkg/idl?tab=doc#StructType

JSON Patch & JSON Merge Patch are applicable to CRDs.

Future extensibility

Right now using an array with maps above don't let you add new typed fields - this may be fine but something to consider.

ie. knative/serving#7373 (comment)

For conditions there's also
kubernetes/community#4521 (comment)

Thanks for detailed explanation @dprotaso. Updated the type to use array.

vagababov · 2020-06-09T22:43:04Z

+// SetDefaults populates default values in Ingress
+func (i *Realm) SetDefaults(ctx context.Context) {
+	i.Spec.SetDefaults(apis.WithinSpec(ctx))
+}


Suggested change

// SetDefaults populates default values in Ingress

func (i *Realm) SetDefaults(ctx context.Context) {

i.Spec.SetDefaults(apis.WithinSpec(ctx))

}

// SetDefaults populates default values in Ingress

func (r *Realm) SetDefaults(ctx context.Context) {

r.Spec.SetDefaults(apis.WithinSpec(ctx))

}

tcnghia · 2020-06-11T00:08:07Z

/assign @dprotaso
who can provide more guidance on API _types.go best practices here

dprotaso · 2020-06-11T18:02:25Z

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+apiVersion: apiextensions.k8s.io/v1beta1


use v1 CRD API now please

dprotaso · 2020-06-11T18:34:34Z

+	// associated with this Domain.  This is used in automatic DNS provisioning like
+	// configuration of magic DNS or creating ExternalName services for cluster-local
+	// access.
+	LoadBalancers []LoadBalancerIngressStatus `json:"loadBalancers"`


Weird to see a type named *Status in the spec - make a copy?

dprotaso · 2020-06-11T18:51:03Z

+	// Istio Gateways associated with this Domain. This could be a reference of a ConfigMap
+	// owned by the implementation as well.
+	// +optional
+	Config []map[string]string `json:"config,omitempty"`


This is an array of maps - is that intended?

best practice to represent maps here?

Maybe a few criteria to think about

Patching

Although, CRDs don't support strategic merge patches the different types help reason about the different patch strategies that could open up in the future (although it's been shut down a bunch of times)

https://pkg.go.dev/k8s.io/kube-openapi/pkg/idl?tab=doc#ListType
https://pkg.go.dev/k8s.io/kube-openapi/pkg/idl?tab=doc#MapType
https://pkg.go.dev/k8s.io/kube-openapi/pkg/idl?tab=doc#StructType

JSON Patch & JSON Merge Patch are applicable to CRDs.

Future extensibility

Right now using an array with maps above don't let you add new typed fields - this may be fine but something to consider.

ie. knative/serving#7373 (comment)

googlebot · 2020-06-16T03:03:18Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

shashwathi · 2020-06-16T03:03:42Z

@googlebot I consent

googlebot · 2020-06-16T15:04:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

tcnghia · 2020-06-23T18:15:35Z

/approve

knative-prow-robot · 2020-06-23T18:15:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrew-su, tcnghia

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~~ [tcnghia]

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

dprotaso · 2020-06-23T20:51:13Z

/hold cancel

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

Signed-off-by: Andrew Su <andrew.su.01@gmail.com>

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

knative-metrics-robot · 2020-06-23T21:17:15Z

The following is the coverage report on the affected files.
Say /test pull-knative-networking-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/networking/v1alpha1/domain_defaults.go	Do not exist	100.0%
pkg/apis/networking/v1alpha1/domain_lifecycle.go	Do not exist	100.0%
pkg/apis/networking/v1alpha1/domain_types.go	Do not exist	100.0%
pkg/apis/networking/v1alpha1/domain_validation.go	Do not exist	100.0%
pkg/apis/networking/v1alpha1/realm_defaults.go	Do not exist	100.0%
pkg/apis/networking/v1alpha1/realm_lifecycle.go	Do not exist	100.0%
pkg/apis/networking/v1alpha1/realm_types.go	Do not exist	100.0%
pkg/apis/networking/v1alpha1/realm_validation.go	Do not exist	100.0%

tcnghia · 2020-06-23T23:01:20Z

/lgtm

knative-prow-robot requested a review from shashwathi June 9, 2020 15:18

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 9, 2020

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 9, 2020

mattmoor reviewed Jun 9, 2020

View reviewed changes

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Jun 9, 2020

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Jun 9, 2020

mattmoor reviewed Jun 9, 2020

View reviewed changes

knative-prow-robot assigned tcnghia Jun 9, 2020

vagababov reviewed Jun 9, 2020

View reviewed changes

knative-prow-robot requested a review from mattmoor June 9, 2020 22:56

knative-prow-robot assigned dprotaso Jun 11, 2020

dprotaso reviewed Jun 11, 2020

View reviewed changes

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 16, 2020

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Jun 16, 2020

andrew-su force-pushed the ingress/crds branch from a5adf25 to 8cdd16d Compare June 16, 2020 15:03

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 16, 2020

googlebot removed the cla: no Indicates the PR's author has not signed the CLA. label Jun 16, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2020

knative-prow-robot assigned vagababov Jun 16, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2020

andrew-su and others added 15 commits June 23, 2020 14:08

Add domain and realm CRDs

0eecd65

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

Update copyright headers

94dcc7c

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

Add unit tests for domain and realm lifecycle

4f0ecd6

Signed-off-by: Andrew Su <andrew.su.01@gmail.com>

Add unit tests for domain and realm types

58f156b

Signed-off-by: Andrew Su <andrew.su.01@gmail.com>

Add unit tests for realm and domain defaults

bcbc3de

Signed-off-by: Andrew Su <andrew.su.01@gmail.com>

Update copyright header

f27fbbc

Signed-off-by: Andrew Su <andrew.su.01@gmail.com>

Address comments

98981c3

Update CRD verison to v1

c8f6f25

Make a copy of LBIngressStatus as LBImgressspec

a14dc2d

gen reconciler changes

a660c03

Update tests

acff51a

Use IngressConfig struct instead of map type

6cd8073

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

Address comments

6cc6d0f

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

Address comments

643435e

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

Update codegen

f8592c3

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

shashwathi force-pushed the ingress/crds branch from f45aebe to f8592c3 Compare June 23, 2020 21:15

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2020

knative-prow-robot merged commit 53e376c into knative:master Jun 23, 2020

nak3 mentioned this pull request Sep 14, 2020

Add Domain and Realm CRDs for kingress visibility knative/serving#8184

Closed

	func (i *Domain) GetGroupVersionKind() schema.GroupVersionKind {
	func (*Domain) GetGroupVersionKind() schema.GroupVersionKind {

Conversation

andrew-su commented Jun 9, 2020

Uh oh!

googlebot commented Jun 9, 2020

Uh oh!

shashwathi commented Jun 9, 2020

Uh oh!

googlebot commented Jun 9, 2020

Uh oh!

shashwathi commented Jun 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Patching

Future extensibility

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcnghia commented Jun 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Patching

Future extensibility

Uh oh!

googlebot commented Jun 16, 2020

Uh oh!

shashwathi commented Jun 16, 2020

Uh oh!

googlebot commented Jun 16, 2020

Uh oh!

tcnghia commented Jun 23, 2020

Uh oh!

knative-prow-robot commented Jun 23, 2020

Uh oh!

dprotaso commented Jun 23, 2020

Uh oh!

knative-metrics-robot commented Jun 23, 2020

Uh oh!

tcnghia commented Jun 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

dprotaso Jun 11, 2020 •

edited

Loading