Skip to content

Support internal-encryption to deploy internal certificates automatically#680

Merged
knative-prow[bot] merged 3 commits into
knative:mainfrom
nak3:add-cert-controller-config
Jun 14, 2022
Merged

Support internal-encryption to deploy internal certificates automatically#680
knative-prow[bot] merged 3 commits into
knative:mainfrom
nak3:add-cert-controller-config

Conversation

@nak3
Copy link
Copy Markdown
Contributor

@nak3 nak3 commented Jun 8, 2022

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.

Hence, this patch supports internal-encryption config to deploy
internal certificates automatically.

knative-extensions/net-kourier#855 and knative/serving#13005 demonstrated this change.

@knative-prow knative-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 8, 2022
@knative-prow
Copy link
Copy Markdown

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

Details 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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 8, 2022

Codecov Report

Merging #680 (28356d9) into main (edfa211) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #680      +/-   ##
==========================================
- Coverage   94.85%   94.75%   -0.10%     
==========================================
  Files          41       41              
  Lines        1243     1221      -22     
==========================================
- Hits         1179     1157      -22     
  Misses         52       52              
  Partials       12       12              
Impacted Files Coverage Δ
pkg/config/config.go 84.81% <100.00%> (-1.86%) ⬇️

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 edfa211...28356d9. Read the comment docs.

@nak3
Copy link
Copy Markdown
Contributor Author

nak3 commented Jun 8, 2022

/cc @evankanderson @psschwei @skonto @rhuss

net-kourier's unit test is failing but it can be fixed as knative-extensions/net-kourier#855
Note, I dropped the old configurations but the alpha feature can be dropped without notice https://knative.dev/docs/serving/configuration/feature-flags/#lifecyle

#
# NOTE: This flag is in an alpha state and is mostly here to enable internal testing
# for now. Use with caution.
activator-ca: ""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess all that will be in the knative-serving-certs and SAN is set to data-plane.knative.dev

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the SAN include a per-namespace component?

@skonto
Copy link
Copy Markdown
Contributor

skonto commented Jun 14, 2022

LGTM let's wait for @evankanderson's approval.

@psschwei
Copy link
Copy Markdown
Member

Note, I dropped the old configurations but the alpha feature can be dropped without notice https://knative.dev/docs/serving/configuration/feature-flags/#lifecyle

My initial thought was issue to mark the old configs as deprecated for a release and then drop them in the next one, but it looks like these flags were only added a couple of months ago in #648 , so don't think there's any problem with dropping.

Copy link
Copy Markdown
Member

@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.

I love the simplification in configuration! One concern -- I think we should have different SANs per-namespace, to prevent any sort of network-confusion attacks

#
# NOTE: This flag is in an alpha state and is mostly here to enable internal testing
# for now. Use with caution.
activator-ca: ""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the SAN include a per-namespace component?

@evankanderson
Copy link
Copy Markdown
Member

Note, I dropped the old configurations but the alpha feature can be dropped without notice https://knative.dev/docs/serving/configuration/feature-flags/#lifecyle

My initial thought was issue to mark the old configs as deprecated for a release and then drop them in the next one, but it looks like these flags were only added a couple of months ago in #648 , so don't think there's any problem with dropping.

I'm sort of in the same boat as Paul here -- I could go either way given how new this is and how hard the old way was to set up.

I think my larger / largest concern about keeping vs dropping values is whether it makes it hard to integrate from this repo to the net-kourier repo.

@evankanderson
Copy link
Copy Markdown
Member

/lgtm

@knative-prow knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2022
@evankanderson
Copy link
Copy Markdown
Member

(since the SAN is not actually specified in this PR, just mentioned by @skonto 's comment)

@knative-prow knative-prow Bot merged commit 07c9d76 into knative:main Jun 14, 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants