DomainMapping Overriding the default HTTP behavior

[wei840222]

Fixes #12755

Proposed Changes

Support annotations 'networking.knative.dev/http-protocol' for Overriding the default HTTP behavior per service in DomainMapping CRD
https://knative.dev/docs/serving/services/http-protocol/#overriding-the-default-http-behavior

Release Note

Support annotations 'networking.knative.dev/http-protocol' for Overriding the default HTTP behavior per service in DomainMapping CRD.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

• pkg/reconciler/OWNERS

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

[knative-prow-robot]

Welcome @wei840222! It looks like this is your first PR to knative/serving 🎉

[knative-prow-robot]

Hi @wei840222. Thanks for your PR.

I'm waiting for a knative 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.

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.

[nak3]

/ok-to-test

[codecov]

Codecov Report

Merging #12786 (fa8a94d) into main (ad64980) will decrease coverage by 0.02%.
The diff coverage is 80.95%.

❗ Current head fa8a94d differs from pull request most recent head 7f8fa19. Consider uploading reports for the commit 7f8fa19 to get more accurate results

@@            Coverage Diff             @@
##             main   #12786      +/-   ##
==========================================
- Coverage   87.51%   87.49%   -0.03%     
==========================================
  Files         196      196              
  Lines        9782     9795      +13     
==========================================
+ Hits         8561     8570       +9     
- Misses        934      936       +2     
- Partials      287      289       +2     

Impacted Files	Coverage Δ
pkg/reconciler/domainmapping/reconciler.go	91.51% <80.95%> (-0.60%)	⬇️
pkg/reconciler/configuration/configuration.go	84.61% <0.00%> (-1.54%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad64980...7f8fa19. Read the comment docs.

[nak3]

Left one tiny suggestion but LGTM.

[nak3]

Could you edit #12786 (comment) and use your description for the Release Note draft as well?

Current DRAFT:

DomainMapping Overriding the default HTTP behavior

TOBE:

Support annotations 'networking.knative.dev/http-protocol' for Overriding the default HTTP behavior per service in DomainMapping CRD.

[wei840222]

hi @nak3
thx for your suggestion, and I've updated the ut code :)

[wei840222]

/retest-required

[wei840222]

hi @psschwei comment added

[psschwei]

looks good to me, pending the rest of the test suite running

[ZhiminXiang]

I just realized that actually we already have the similar logic

serving/pkg/reconciler/route/resources/ingress.go

Line 207 in 89ac2aa

func getHTTPProtocol(ctx context.Context, annotations map[string]string) (netv1alpha1.HTTPOption, error) {

.

Could we reuse this function? I think we could move the existing function to a shared place as a helper function so that it could be used by this file. WDYT?

[wei840222]

@ZhiminXiang I tried. But I found in these lines
https://github.com/knative/serving/blob/main/pkg/reconciler/route/resources/ingress.go#L220
https://github.com/knative/serving/pull/12786/files#diff-ebdda38d8b796c4f5e3b7e5ed54460483d22b3fd1d273de3b91a739c93aa59d6R337

The *config.Config are different type knative.dev/serving/pkg/reconciler/route/config.Config & knative.dev/serving/pkg/reconciler/domainmapping/config.Config
I think if method reuse is going to happen, maybe more changes will be introduced in this change.

[ZhiminXiang]

The *config.Config are different type knative.dev/serving/pkg/reconciler/route/config.Config & knative.dev/serving/pkg/reconciler/domainmapping/config.Config
I think if method reuse is going to happen, maybe more changes will be introduced in this change.

Could we modify the helper function to getHTTPOption(networkConfig *network.Config, annotations map[string]string) (netv1alpha1.HTTPOption, error). This function could be reused by DomainMapping and Route. I am inclined to share the logic between DomainMapping and Route because essentially the logic of getting HTTP option for those two objects should be the same.

[wei840222]

Hi
@ZhiminXiang I've changed the relevant code. plz check. thx :)

[ZhiminXiang]

Thanks @wei840222 for adopting the suggestion! It is in a good shape.

I left two small comments. After those two are addressed, this PR should be ready to merge. Thanks again!

[wei840222]

Hi, @ZhiminXiang changes applied, plz check. thx a lot :)

[ZhiminXiang]

/lgtm

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wei840222, ZhiminXiang

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 [ZhiminXiang]

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

[ZhiminXiang]

Hi, @ZhiminXiang changes applied, plz check. thx a lot :)

Thanks for making the contribution!