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

❇️ Populate system default root-ca-certificate with ca-bundle #40

Merged

Conversation

swapnilgm
Copy link

@swapnilgm swapnilgm commented Apr 14, 2020

Signed-off-by: Swapnil Mhamane [email protected]

What this PR does / why we need it:

  • This PR removes the bootstrap script dependency over apk to download appropriate wget version, which makes it self-contained.

Which issue(s) this PR fixes:
Fixes #39

Special notes for your reviewer:
Please test whether certificate based operation are performed properly on etcd container.
Release note:

:sparkle: Populate etcd container root-ca-certificates list with the provided self-signed ca-bundle for communication with backup-restore sidecar

@swapnilgm swapnilgm requested a review from a team as a code owner April 14, 2020 08:44
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 14, 2020
Copy link
Contributor

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Overall LGTM. I've suggested a minor naming change to keep consistent with other naming changes.

charts/etcd/templates/etcd-statefulset.yaml Outdated Show resolved Hide resolved
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 14, 2020
Copy link

@georgekuruvillak georgekuruvillak left a comment

Choose a reason for hiding this comment

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

Nice find. I have tested it out. Works well. LGTM

@swapnilgm
Copy link
Author

swapnilgm commented Apr 15, 2020

@georgekuruvillak Will you please merge this PR. And release the hotfix. This is urgently needed fix for alicloud deployment.

@georgekuruvillak georgekuruvillak merged commit 9c70aa4 into gardener:master Apr 15, 2020
@swapnilgm swapnilgm deleted the feature/remove-wget-dependency branch April 15, 2020 15:28
@ialidzhikov
Copy link
Member

Change in the bootstrap ConfigMap leads the checksum annotation checksum/etcd-bootstrap-configmap to be updated which leads the etcd Pod to be restarted. For future cases I would appreciate if we mention similar details in the release notes (for example this PR will lead to etcd Pod restart (in the maintenance time window)).

@swapnilgm
Copy link
Author

@ialidzhikov Sure. We will try to maintain those details in release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Avoid fetching resource from external resources on internet during bootstrap
6 participants