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.

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

Use internal-encryption to deploy internal certificates automatically #855

Merged
merged 2 commits into from
Jun 16, 2022
Merged

Use internal-encryption to deploy internal certificates automatically #855

merged 2 commits into from
Jun 16, 2022
+1,011 −271

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Jun 8, 2022

This patch is same purpose with knative/serving#13005.

Currently users have to deploy certificates manually with several options such as
activator-san, activator-ca, queue-proxy-ca etc.

Such deployment and management of the certificates is a big burden for users.

@knative-prow
Copy link

knative-prow bot commented Jun 8, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3

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 knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 8, 2022
@nak3 nak3 mentioned this pull request Jun 8, 2022
Merged
@nak3 nak3 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2022
@codecov
Copy link

codecov bot commented Jun 8, 2022 •
edited
Loading

Codecov Report

Merging #855 (c82cf34) into main (427434c) will decrease coverage by 0.43%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main     #855      +/-   ##
==========================================
- Coverage   81.83%   81.39%   -0.44%     
==========================================
  Files          18       18              
  Lines        1167     1172       +5     
==========================================
- Hits          955      954       -1     
- Misses        168      174       +6     
  Partials       44       44
Impacted Files Coverage Δ
pkg/config/config.go 0.00% <0.00%> (ø)
pkg/generator/ingress_translator.go 85.55% <100.00%> (ø)
pkg/reconciler/ingress/config/store.go 91.66% <100.00%> (-0.23%) ⬇️

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 427434c...c82cf34. Read the comment docs.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2022
@skonto
Copy link
Contributor

skonto commented Jun 14, 2022

@nak3 pls rebase.

@nak3
Copy link
Contributor Author

nak3 commented Jun 14, 2022

@skonto This also needs knative/networking#680.
Could you please review & merge knative/networking#680 first? Otherwise, I need to keep rebasing this PR.

skonto
skonto reviewed Jun 14, 2022
View reviewed changes
pkg/config/config.go
@@ -63,6 +63,10 @@ const (
// disableHTTP2AnnotationKey is the annotation key attached to a Knative Domain Mapping
// to indicate that http2 should not be enabled for it.
disableHTTP2AnnotationKey = "kourier.knative.dev/disable-http2"

// ServingNamespaceEnv is an env variable specifying where the serving is deployed.
// e.g. OpenShift deploys Kourier in different namespace so `system.Namespace()` does not work.
Copy link
Contributor

@skonto skonto Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Afaik this holds for backwards compatibility reasons. Would it be possible in the future to move everything to serving ns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible but it has a lot of risks to move the gateway into serving ns.

@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2022
@nak3 nak3 mentioned this pull request Jun 15, 2022
Closed
@nak3 nak3 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2022
@nak3
Copy link
Contributor Author

nak3 commented Jun 15, 2022

@skonto Could you please take a look?

@nak3 nak3 mentioned this pull request Jun 15, 2022
Merged
skonto
skonto reviewed Jun 15, 2022
View reviewed changes
pkg/generator/ingress_translator.go
@@ -313,7 +310,7 @@ func createUpstreamTLSContext(caCertificate []byte, activatorSAN string, alpnPro
},
MatchSubjectAltNames: []*envoymatcherv3.StringMatcher{{
MatchPattern: &envoymatcherv3.StringMatcher_Exact{
Exact: activatorSAN,
Exact: certificates.FakeDnsName,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if we ever have multiple-knative installations this will have to be changed?

@skonto
Copy link
Contributor

skonto commented Jun 15, 2022

/lgtm /hold for @evankanderson for any additional comments.

@skonto skonto added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 15, 2022
@nak3 nak3 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2022
@knative-prow knative-prow bot merged commit b9b1e8b into knative-extensions:main Jun 16, 2022
@nak3 nak3 mentioned this pull request Jun 16, 2022
Merged
openshift-ci bot pushed a commit to openshift-knative/serverless-operator that referenced this pull request Jun 17, 2022
@nak3
Inject SERVING_NAMESPACE env value to kourier controller (#1623)
acf5ab1 
Since knative-extensions/net-kourier#855, kourier
controller supports SERVING_NAMESPACE env value to specify the serving
namespace.

The feature is required when internal encryption is supported.
@psschwei psschwei mentioned this pull request Jun 24, 2022
Merged
@KauzClay KauzClay mentioned this pull request Oct 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skonto skonto

Assignees
No one assigned
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nak3 @skonto @knative-prow-robot