-
Notifications
You must be signed in to change notification settings - Fork 52
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.
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
Internal Encryption fixes #860
Internal Encryption fixes #860
Conversation
@KauzClay: The label(s) In response to this:
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. |
Codecov Report
@@ Coverage Diff @@
## main #860 +/- ##
==========================================
+ Coverage 93.99% 94.08% +0.08%
==========================================
Files 7 7
Lines 783 794 +11
==========================================
+ Hits 736 747 +11
Misses 27 27
Partials 20 20
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
da86648
to
f2e4f81
Compare
503a2ee
to
00c84a9
Compare
00c84a9
to
c743fa4
Compare
132b52b
to
7657dfc
Compare
/assign @dprotaso |
/assign @evankanderson Since you were also involved on some of the previous internal-encryption PRs |
pkg/reconciler/contour/contour.go
Outdated
@@ -332,3 +333,11 @@ func (r *Reconciler) lbStatus(ctx context.Context, vis v1alpha1.IngressVisibilit | |||
} | |||
return | |||
} | |||
|
|||
func isDomainMapping(ing *v1alpha1.Ingress) bool { | |||
_, hasDmUIDLabelKey := ing.Labels[serving.DomainMappingUIDLabelKey] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to bring in a fair amount of serving code just to use these labels. Maybe there is a better way to import it that doesn't bring in so much?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you probably want to do is to either copy the constants here, or move them into networking
. When you have net-contour
import serving
directly, it makes it hard to cut a release, because the net-contour
package can't be cut until the serving
package is tagged.
Alternatively, you could look at the OwnerReferences
field to see whether the owner is a Knative Route or a DomainMapping.
The third option would be to add some explicit field to the KIngress indicating which type it is.
The fourth option would be to look at the spec.rules
-- if there's a single rule
with visibility: externalIP
, that must be a DomainMapping, because a Route always has an internal mapping: https://github.com/knative/serving/blob/main/pkg/reconciler/route/resources/ingress.go#L141
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, thanks for explaining that about importing serving.
I didn't even think about the owner reference, I think I will go for that. It seems the least intrusive but still pretty explicit. Then I can avoid having duplicated constants or adding more fields to the kingress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DomainMapping just sets HostRewrite
on the KIngress - https://github.com/knative/networking/blob/2473e65d69206ed544f32e47f994cf3635bebdbe/pkg/apis/networking/v1alpha1/ingress_types.go#L208-L212
You could look at that spec property and the target k8s service (it pointing to a contour's envoy service)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, well maybe not, the type definition for domainmappings is in the serving repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can define a constant for DomainMapping here, or add a helper in networking
to determine if a KIngress represents a DomainMapping.
It might be more robust, though, to look for the rewriteHeader
field and the presence of knative.dev/networking/pkg/http/header.OriginalHostKey
as a field in AppendHeaders
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I went with the check for rewriteheader + originalhost key, and put in a comment explaining it a bit.
This PR makes me realize that we need a special consideration for traffic going through I wonder if TLS passthrough makes more sense. |
I like that, except that we have no KIngress construct to describe this (and it's not clear that we should). |
/test integration-tests |
4c97bd6
to
6dfcff6
Compare
* unencrypt call back to envoy in domainmappings when internal encryption is enabled * identify domainmapping kingress via presence of Rewrite host and k-original-host header
b5ba695
to
d854098
Compare
Hey @evankanderson @dprotaso , do you have any other thoughts on this one? To me it sounds like any actions regarding Dave's comment
probably should happen in another PR. Is that what you had in mind? |
Yeah let's make an new issue and do it as a follow up /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, KauzClay 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 |
Changes
🐛 Fixes #862
🐛 internal encryption breaks http01 challenges
Release Note
Docs