Skip to content
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

Copy parent HTTPOption to endpoint probe, if present. #713

Merged

Conversation

evankanderson
Copy link
Contributor

@evankanderson evankanderson commented Jan 21, 2022

Fixes #712

net-contour will now reconcile resources if only HTTPS is enabled

@knative-prow-robot knative-prow-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 21, 2022
@evankanderson
Copy link
Contributor Author

/assign @carlisia @nak3

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 21, 2022
@evankanderson
Copy link
Contributor Author

I have no idea how I bollixed up the first commit, corrected.

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #713 (afbbaf6) into main (a248e38) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #713      +/-   ##
==========================================
+ Coverage   93.43%   93.52%   +0.08%     
==========================================
  Files           7        7              
  Lines         518      525       +7     
==========================================
+ Hits          484      491       +7     
  Misses         16       16              
  Partials       18       18              
Impacted Files Coverage Δ
pkg/reconciler/contour/resources/httpproxy.go 97.24% <100.00%> (-0.02%) ⬇️
pkg/reconciler/contour/resources/kingress.go 96.87% <100.00%> (+0.44%) ⬆️

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 a248e38...afbbaf6. Read the comment docs.

@evankanderson
Copy link
Contributor Author

/retest

Copy link
Contributor Author

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

This is ready for review.

for _, tls := range ing.Spec.TLS {
for _, host := range tls.Hosts {
t := tls
hostToTLS[host] = &t
hostToTLS[host] = tls
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we were making a copy, there was no need to use references here anyway.

Comment on lines -50 to -51
// TODO: Probing against HTTP should be enough as it ensures Envoy's EDS?
// Need to verify it by scale-N test with HTTPS.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this converts things to probe HTTPS for HTTPS instances with certificates, I think that should be sufficient for this TODO.

@dprotaso
Copy link
Contributor

dprotaso commented Jan 24, 2022

Since we're opening up the ability to probe HTTPS endpoints we'll probably want the following

  1. The ability to included an addition CA cert/intermediate certs that the prober uses
    • This is for handling corporate environments with a non public CA
    • or we confirm and document SSL_CERT_FILE/SSL_CERT_DIR works as expected
  2. (optional) Configure an option to skip cert verification crypto/tls/Config.InsecureSkipVerify
    • Probably include big warnings that this should be off - or use only in testing
  3. E2E test covering this functionality

@evankanderson
Copy link
Contributor Author

(This change failed until I included the right hostnames in the child KIngress's spec.tls field, so I think we do have e2e test coverage.)

Re item 2, we already configure InsecureSkipVerify in the networking prober:

https://github.com/knative/networking/blob/57bee6625a8ac19f6a1f02261d23245facf98ed0/pkg/status/status.go#L368

I'm not sure what you're thinking of for item 1, given item 2.

@dprotaso
Copy link
Contributor

(This change failed until I included the right hostnames in the child KIngress's spec.tls field, so I think we do have e2e test coverage.)

Great

Re item 2, we already configure InsecureSkipVerify in the networking prober:

Oh interesting - thought it was stricter. Let's forgo 1 &2 then.

@dprotaso
Copy link
Contributor

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2022
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, evankanderson

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 [dprotaso,evankanderson]

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 8253890 into knative-extensions:main Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

net-contour fails to reconcile if only the HTTPS contour port is available
5 participants