这是indexloc提供的服务，不要输入任何密码
Skip to content

Add DomainMapping labels to Ingress #10370

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Dec 30, 2020
Merged

Add DomainMapping labels to Ingress #10370

merged 2 commits into from
Dec 30, 2020

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Dec 15, 2020 •
edited
Loading

When DomainMapping controller creates Ingress, Ingress does not have
the label who created it.
It is not consistent with Ingress created by Route and so it is
difficult to list Ingresses resources created by DomainMapping.

To fix it, this patch adds the DomainMapping labels to Ingress.

BEFORE:

$ kubectl get king --show-labels
NAME            READY   REASON   LABELS
hello-example   True             serving.knative.dev/route=hello-example,serving.knative.dev/routeNamespace=default,serving.knative.dev/service=hello-example
mydomain.com    True             <none>

AFTER:

$ kubectl get king --show-labels
NAME            READY   REASON   LABELS
hello-example   True             serving.knative.dev/route=hello-example,serving.knative.dev/routeNamespace=default,serving.knative.dev/service=hello-example
mydomain.com    True             serving.knative.dev/domainmapping=mydomain.com

Release Note

Introduce `serving.knative.dev/domainmapping` label for Ingress generated by DomainMapping.

/cc @julz @markusthoemmes

@nak3
Add DomainMapping labels to Ingress
cdcb2dd 
When DomainMapping controller creates Ingress, Ingress does not have
the label who created it.
It is not consistent with Ingress created by Route and so it is
difficult to list Ingresses resources created by DomainMapping.

BEFORE:
```
$ kubectl get king --show-labels
NAME            READY   REASON   LABELS
hello-example   True             serving.knative.dev/route=hello-example,serving.knative.dev/routeNamespace=default,serving.knative.dev/service=hello-example
mydomain.com    True             <none>
```

AFTER:
```
$ kubectl get king --show-labels
NAME            READY   REASON   LABELS
hello-example   True             serving.knative.dev/route=hello-example,serving.knative.dev/routeNamespace=default,serving.knative.dev/service=hello-example
mydomain.com    True             serving.knative.dev/domainmapping=mydomain.com,serving.knative.dev/domainmappingNamespace=default
```
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 15, 2020
@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers labels Dec 15, 2020
@codecov
Copy link

codecov bot commented Dec 15, 2020 •
edited
Loading

Codecov Report

Merging #10370 (8336077) into master (0c60aeb) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10370      +/-   ##
==========================================
- Coverage   88.12%   87.99%   -0.14%     
==========================================
  Files         186      186              
  Lines        8710     8720      +10     
==========================================
- Hits         7676     7673       -3     
- Misses        801      809       +8     
- Partials      233      238       +5
Impacted Files Coverage Δ
pkg/reconciler/domainmapping/resources/ingress.go 100.00% <ø> (ø)
pkg/reconciler/route/reconcile_resources.go 80.67% <100.00%> (-6.94%) ⬇️
pkg/reconciler/route/traffic/rollout.go 100.00% <100.00%> (ø)
pkg/reconciler/configuration/configuration.go 86.71% <0.00%> (-1.57%) ⬇️
pkg/reconciler/autoscaling/kpa/scaler.go 88.57% <0.00%> (-1.43%) ⬇️

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 6b15a49...1a1ca74. Read the comment docs.

@nak3
Copy link
Contributor Author

nak3 commented Dec 15, 2020

For the use case & background of this request.
Our downstream(OpenShift) generates OpenShift Route resource from Ingress.
We are currently list the OpenShift Route based on Ingress by the route labels in Ingress as:

func (r *Reconciler) routeList(ing *v1alpha1.Ingress) ([]*ocproutev1.Route, error) {
        ingressLabels := ing.GetLabels()
        return r.ocprouteLister.List(labels.SelectorFromSet(map[string]string{
                networking.IngressLabelKey:     ing.GetName(),
                serving.RouteLabelKey:          ingressLabels[serving.RouteLabelKey],
                serving.RouteNamespaceLabelKey: ingressLabels[serving.RouteNamespaceLabelKey],
        }))
}

But it does not work on Ingress created from DomainMapping's Ingress.
I think tt has a workaround by adding a new label on OpenShift Route
but I wonder that it would be better that Ingress has the consistent label (serving.knative.dev/{route or domainmapping}=hello-example) with Ingress by Route.

markusthoemmes
markusthoemmes reviewed Dec 15, 2020
View reviewed changes
pkg/apis/serving/register.go Outdated

// DomainMappingNamespaceLabelKey is the label key attached to a Ingress
// by a DomainMapping to indicate which namespace the DomainMapping was created in.
DomainMappingNamespaceLabelKey = GroupName + "/domainmappingNamespace"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary, given the Ingress is always in the same namespace (per the comment on MakeIngress)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct. It is not necessary. Updated.

@nak3 nak3 mentioned this pull request Dec 16, 2020
Merged
tcnghia
tcnghia approved these changes Dec 30, 2020
View reviewed changes
Copy link
Contributor

@tcnghia tcnghia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 30, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3, tcnghia

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:

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 knative-prow-robot merged commit 99cf808 into knative:master Dec 30, 2020
@julz julz mentioned this pull request Mar 2, 2021
Merged
@julz julz mentioned this pull request May 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markusthoemmes markusthoemmes markusthoemmes left review comments

@tcnghia tcnghia tcnghia approved these changes

@julz julz Awaiting requested review from julz

Assignees

@tcnghia tcnghia

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nak3 @knative-prow-robot @tcnghia @markusthoemmes