-
Notifications
You must be signed in to change notification settings - Fork 423
Production deployment documentation #3712
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| garbageCollectionPolicy: Exponential | ||
| garbageCollectionPeriod: 43200s | ||
| deltaSnapshotPeriod: 300s | ||
| deltaSnapshotMemoryLimit: 1Gi |
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.
Not much important because this is about backups, but maybe it would be useful to have a comment why are these values chosen (e.g. if they're default, changed because it's better in prod this way, etc).
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.
This was taken from etcd-druid docs. would be good if they could comment on this. Never teted this myself :/
| - "*.alpha-peer.kcp-comer.svc" | ||
| - "*.alpha-peer.kcp-comer.svc.cluster.local" | ||
| issuerRef: | ||
| name: etcd-ca |
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.
Why is this not using etcd-ca from the kcp-comer namespace, similar to etcd-tls-ca?
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.
Might be miskate. Need to test before I change it. TBH re-testing this means rebuilding everything. So will take a note and once I do next rebuild - will check.
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.
Will leave it for now, and will creat follow-up issue to test this again.
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.
Does it make sense that we put comments such as # CHANGE ME for fields that should be changed (e.g. dnsNames)? That way, people can easier find it, and we can say in docs search for CHANGE ME and change those values.
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.
Change me confuses too. When you see #Change me. Its not always clear what it should be? Root url, shard url, workspace URL?
All this should be used as a reference, not "as is". So having real values gives more context that changes me and explanations. In general, this is a start, and once we get feedback, we can iterate.
269d3b4 to
c2befd5
Compare
Signed-off-by: Mangirdas Judeikis <[email protected]> On-behalf-of: @SAP [email protected]
c2befd5 to
8311dd9
Compare
xmudrii
left a comment
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.
Some final nits as of now. I would say, after these are fixed, we merge this PR and iterate as needed.
|
|
||
| # Reference to the ClusterIssuer | ||
| issuerRef: | ||
| name: kcp-comerletsencrypt-prod |
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 this is referenced in other places as letsencrypt-prod
| spec: | ||
| secretName: dex-tls | ||
| issuerRef: | ||
| name: kcp-comerletsencrypt-prod |
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 this is referenced in other places as letsencrypt-prod
|
|
||
| ### [kcp-vespucci](kcp-vespucci.md) - External Certificates | ||
| - **Best for**: Production environments requiring trusted certificates | ||
| - **Certificate approach**: Let's Encrypt for front-proxy with public shard access |
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.
Would this be correct?
| - **Certificate approach**: Let's Encrypt for front-proxy with public shard access | |
| - **Certificate approach**: Let's Encrypt for front-proxy, self-signed certificates for shards |
| # For other platforms, see: https://github.com/int128/kubelogin | ||
| ``` | ||
|
|
||
| **Solution**: Use OIDC authentication for external access. |
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.
This Solution: here looks weird to me, sounds like an incomplete sentence.
| # For other platforms, see: https://github.com/int128/kubelogin | ||
| ``` | ||
|
|
||
| ### Configure OIDC Credentials |
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.
Maybe we should have a test step like for kcp-dekker here, to make sure it works.
Summary
This PR adds detailed deployment documentation on how to deploy kcp in a production configuration.
/kind documentation
What Type of PR Is This?
Related Issue(s)
Fixes #
Release Notes