Add DomainMapping CRUD commands #1267

dsimansk · 2021-03-17T14:53:14Z

Description

This PR adds CRUD operations for DomainMapping resource.

TODO:

I've covered only Knative Service as the only target, but going through the updated docs I should probably reflect the other targets as well.
knative/serving@8eee938
I'll take a look into Servings test, if we can do a real world domain E2E test.

Changes

🎁 Add top-level cmd domain with create, update, describe, delete, list ops

Reference

Fixes #1197

knative-prow-robot

@dsimansk: 15 warnings.

In response to this:

Description

This PR adds CRUD operations for DomainMapping resource.

TODO:

I've covered only Knative Service as the only target, but going through the updated docs I should probably reflect the other targets as well.
knative/serving@8eee938

I'll take a look into Servings test, if we can do a real world domain E2E test.

Changes

🎁 Add top-level cmd domain with create, update, describe, delete, list ops

Reference

Fixes #1197

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.

pkg/kn/commands/domain/create.go

pkg/serving/v1alpha1/client.go

pkg/kn/commands/domain/human_readable_flags.go

pkg/serving/v1alpha1/client_mock.go

dsimansk · 2021-03-17T14:53:47Z

@julz FYI.

itsmurugappan

I gave it a try and it looks good. Just a few nits.

itsmurugappan · 2021-03-19T00:46:02Z

pkg/kn/commands/domain/create.go

+			err = client.CreateDomainMapping(builder.Build())
+			if err != nil {
+				return knerrors.GetError(err)
+			}


Suggested change

err = client.CreateDomainMapping(builder.Build())

if err != nil {

return knerrors.GetError(err)

}

if err := client.CreateDomainMapping(builder.Build()); err != nil {

return knerrors.GetError(err)

}

Thanks for the suggestion. However, I recall from some previous PRs that if-s with inline declarations aren't very popular in client's code.
@maximilien or @rhuss wdyt?

Yeah, I like the simpler none if err := ... but that's just me :)

itsmurugappan · 2021-03-19T00:47:27Z

pkg/kn/commands/domain/delete.go

+			err = client.DeleteDomainMapping(name)
+			if err != nil {
+				return knerrors.GetError(err)
+			}


Suggested change

err = client.DeleteDomainMapping(name)

if err != nil {

return knerrors.GetError(err)

}

if err := client.DeleteDomainMapping(name); err != nil {

return knerrors.GetError(err)

}

pkg/kn/commands/domain/delete_test.go

itsmurugappan · 2021-03-19T00:54:08Z

pkg/serving/v1alpha1/client.go

+	_, err := cl.client.DomainMappings(cl.namespace).Create(context.TODO(), domainMapping, v1.CreateOptions{})
+	if err != nil {
+		return knerrors.GetError(err)
+	}


Suggested change

_, err := cl.client.DomainMappings(cl.namespace).Create(context.TODO(), domainMapping, v1.CreateOptions{})

if err != nil {

return knerrors.GetError(err)

}

if _, err := cl.client.DomainMappings(cl.namespace).Create(context.TODO(), domainMapping, v1.CreateOptions{}); err != nil {

return knerrors.GetError(err)

}

itsmurugappan · 2021-03-19T01:01:44Z

Few suggestions

While creation, can ksvc be made the owner of this domain mapping, so that when ksvc is deleted this also gc'ed
AutoTLS can be disabled per route based on annotations, in follow up PR we can support annotations to domain mapping

apiVersion: serving.knative.dev/v1alpha1
kind: DomainMapping
metadata:
 annotations:
  networking.knative.dev/disableAutoTLS: "true"
 name: domainmapping.example.com
spec:
 ref:
   name: domain-mapping-svc
   kind: Service
   apiVersion: serving.knative.dev/v1

julz · 2021-03-19T09:53:44Z

While creation, can ksvc be made the owner of this domain mapping, so that when ksvc is deleted this also gc'ed

FWIW I'm not sure this is what we should do. If the DM is deleted when the ksvc is deleted then it's possible for someone to swoop in and grab a domain mapping that you want to reuse for another ksvc later. I would understand doing this more if we created the ksvc and domain at the same time (eg if we added a field to ksvc to spit out a domainmapping), but it seems a bit surprising to me at first glance for us to do this implicitly. OTOH if this matches the behaviour for things like --sink that'd probably change my mind.. do we automatically set the ksvc as the owner of a source when you pass --sink?

dsimansk · 2021-03-19T13:22:19Z

While creation, can ksvc be made the owner of this domain mapping, so that when ksvc is deleted this also gc'ed

I have the same concern as @julz that it'd break the reuse use case. In addition to --sink handling question kn isn't adding any OwnerRef wiring for sources, channels etc., except very specific case in service import that's trying to recreate the relationship.

However, such flag can still optional per explicit request to add OwenerRef to the resource. It might prove to be useful elsewhere as well.

AutoTLS can be disabled per route based on annotations, in follow up PR we can support annotations to domain mapping

Thanks, that's good point I haven't checked the annotations yet.

maximilien

Various comments... please address. Great contribution BTW

CC: @julz who did the work on serving side as FYI

maximilien · 2021-03-17T22:04:43Z

docs/cmd/kn_domain_delete.md

+Delete a domain mapping
+
+```
+kn domain delete FQDN


What’s “FQDN”? Can we use something simpler, e.g., ‘hello’

I intended to promote that fully qualified domain name is expected. It'd be better to explain in the example description probably.

Thx clearer the better

maximilien · 2021-03-17T22:23:56Z

pkg/kn/commands/domain/create.go

+			if err != nil {
+				return err
+			}
+			destination, err := refFlags.ResolveSink(dynamicClient, namespace)


A bit confuse here. Why are we resolving the destination as an event sink?

I've reused the sink flag handling here, but it's not entirely correct. I'll come up with something better. :)

pkg/kn/commands/domain/delete.go

pkg/kn/commands/domain/domain.go

pkg/kn/commands/domain/domain_test.go

pkg/kn/commands/domain/list.go

pkg/kn/commands/domain/list_test.go

pkg/kn/commands/domain/update.go

pkg/serving/v1alpha1/client.go

dsimansk · 2021-03-23T17:04:05Z

I've updated target reference resolution with df9c889.

Do we want to support type:name:namespace resolution as was added for --sink flag? Or rather have something like --ref-namespace.
I lean more towards the first option.

navidshaikh · 2021-03-23T17:41:36Z

Do we want to support type:name:namespace resolution as was added for --sink flag?

+1

dsimansk · 2021-03-24T12:28:28Z

From the DomainMapping docs:

// Ref specifies the target of the Domain Mapping.
//
// The object identified by the Ref must be an Addressable with a URL of the
// form {name}.{namespace}.{domain} where {domain} is the cluster domain,
// and {name} and {namespace} are the name and namespace of a Kubernetes
// Service.
//
// This contract is satisfied by Knative types such as Knative Services and
// Knative Routes, and by Kubernetes Services.

Therefore I've added the following handling for 3 reference types.

Addressable target reference for Domain Mapping. You can specify a Knative service, a Knative soute or a Kubernetes service.Examples: '--ref ksvc:hello' or simply '--ref hello' for a Knative service 'hello', '--ref kroute:hello' for a Knative route 'hello', '--ref svc:hello' for a Kubernetes service 'hello'. If a prefix is not provided, it is considered as a Knative service.

However, per discussion with @navidshaikh and @itsmurugappan yesterday there're some doubts. The addition of Kubernetes Service wtih svc: prefix breaks our previous convention for --sink flag, that doesn't support K8s Service. Even though there're 3 types that satisfy the contract, probably Knative Service is the most popular in our context.

Should we only support reference as Knative Service for now to keep it simple. And add additional support if there's request for it? Wdyt? @rhuss @maximilien

navidshaikh · 2021-03-26T10:17:20Z

Should we only support reference as Knative Service for now to keep it simple. And add additional support if there's request for it?

@dsimansk : Let's support knative resources, ksvc and kroute ?

If we're planning to support prefix for k8s service, let's do it for sink flag as well? This way we've consistent prefix across flags.

dsimansk · 2021-03-26T19:22:56Z

Should we only support reference as Knative Service for now to keep it simple. And add additional support if there's request for it?

@dsimansk : Let's support knative resources, ksvc and kroute ?

If we're planning to support prefix for k8s service, let's do it for sink flag as well? This way we've consistent prefix across flags.

Update the resolve func accordingly. In addition the PR is rebased and context param added.

knative-metrics-robot · 2021-03-26T19:24:31Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/domain/create.go	Do not exist	85.2%
pkg/kn/commands/domain/delete.go	Do not exist	88.2%
pkg/kn/commands/domain/describe.go	Do not exist	85.4%
pkg/kn/commands/domain/domain.go	Do not exist	96.4%
pkg/kn/commands/domain/human_readable_flags.go	Do not exist	90.0%
pkg/kn/commands/domain/list.go	Do not exist	80.0%
pkg/kn/commands/domain/update.go	Do not exist	81.2%
pkg/kn/commands/types.go	53.6%	55.1%	1.5
pkg/serving/v1alpha1/client.go	Do not exist	88.4%
pkg/serving/v1alpha1/client_mock.go	Do not exist	90.9%

maximilien · 2021-03-30T22:48:45Z

/lgtm
/approve
/hold to let others complete their feedback

rhuss · 2021-04-01T15:16:15Z

Let's get that merged now, if now one objects (we can clean up later, but we need to get this in before the release)

/approve
/lgtm
/unhold

knative-prow-robot · 2021-04-01T15:16:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, rhuss

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien,rhuss]

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

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 17, 2021

knative-prow-robot reviewed Mar 17, 2021

View reviewed changes

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 17, 2021

dsimansk force-pushed the pr/domain-mapping branch from 113b513 to ff5f028 Compare March 18, 2021 09:49

itsmurugappan suggested changes Mar 19, 2021

View reviewed changes

maximilien reviewed Mar 19, 2021

View reviewed changes

dsimansk force-pushed the pr/domain-mapping branch 2 times, most recently from c7fbd6d to db96581 Compare March 26, 2021 13:44

dsimansk added 6 commits March 26, 2021 18:41

Add DomainMapping CRUD commands

2d0dbe8

fix: Reflect review feedback

16d4382

fix: Fix prealloc lint error

8822680

fix: Add specific reference resolution

4a3afa7

fix: Remove Kubernetes support in ref

0549eeb

chore: Add context param to client functions

b57b7e3

dsimansk force-pushed the pr/domain-mapping branch from db96581 to b57b7e3 Compare March 26, 2021 19:21

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

knative-prow-robot assigned maximilien Mar 30, 2021

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 30, 2021

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

knative-prow-robot assigned rhuss Apr 1, 2021

knative-prow-robot merged commit 10ea0df into knative:main Apr 1, 2021

dprotaso mentioned this pull request Apr 15, 2021

DomainMapping CRD (Tracking Issue) knative/serving#9713

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DomainMapping CRUD commands #1267

Add DomainMapping CRUD commands #1267

dsimansk commented Mar 17, 2021

knative-prow-robot left a comment

dsimansk commented Mar 17, 2021

itsmurugappan left a comment

itsmurugappan Mar 19, 2021

dsimansk Mar 22, 2021

maximilien Mar 30, 2021 •

edited

Loading

itsmurugappan Mar 19, 2021

itsmurugappan Mar 19, 2021

itsmurugappan commented Mar 19, 2021

julz commented Mar 19, 2021

dsimansk commented Mar 19, 2021 •

edited

Loading

maximilien left a comment

maximilien Mar 17, 2021

dsimansk Mar 22, 2021

maximilien Mar 30, 2021

maximilien Mar 17, 2021

dsimansk Mar 22, 2021

dsimansk commented Mar 23, 2021 •

edited

Loading

navidshaikh commented Mar 23, 2021

dsimansk commented Mar 24, 2021

navidshaikh commented Mar 26, 2021

dsimansk commented Mar 26, 2021

knative-metrics-robot commented Mar 26, 2021

maximilien commented Mar 30, 2021

rhuss commented Apr 1, 2021

knative-prow-robot commented Apr 1, 2021

Add DomainMapping CRUD commands #1267

Add DomainMapping CRUD commands #1267

Conversation

dsimansk commented Mar 17, 2021

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

dsimansk commented Mar 17, 2021

itsmurugappan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itsmurugappan commented Mar 19, 2021

julz commented Mar 19, 2021

dsimansk commented Mar 19, 2021 • edited Loading

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Mar 23, 2021 • edited Loading

navidshaikh commented Mar 23, 2021

dsimansk commented Mar 24, 2021

navidshaikh commented Mar 26, 2021

dsimansk commented Mar 26, 2021

knative-metrics-robot commented Mar 26, 2021

maximilien commented Mar 30, 2021

rhuss commented Apr 1, 2021

knative-prow-robot commented Apr 1, 2021

maximilien Mar 30, 2021 •

edited

Loading

dsimansk commented Mar 19, 2021 •

edited

Loading

dsimansk commented Mar 23, 2021 •

edited

Loading