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

Rename root-certificate to brupop-selfsigned-ca #595

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

alekhrycaiko
Copy link

Issue number:

#486

Description of changes:

This attempts to address #486.

Changes root certificate referenced based on current findings of the issue.

Testing done:

Ran test suite.

Our own cluster already relies on brupop-selfsigned-ca. But I have not ran this specific PR against the cluster and could use an additional verification/eye there.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@yeazelm yeazelm self-requested a review February 26, 2024 02:50
@yeazelm
Copy link
Contributor

yeazelm commented Feb 26, 2024

Thanks @alekhrycaiko for cutting this PR! I'm just getting up to speed on this issue so I'll need to look at this a bit more to confirm it will work. I'll dig in and get back to you! Sorry for the delay in response!

Comment on lines +199 to +207
# Source: bottlerocket-update-operator/templates/controller-priority-class.yaml
apiVersion: scheduling.k8s.io/v1
kind: PriorityClass
metadata:
name: brupop-controller-high-priority
namespace: brupop-bottlerocket-aws
preemptionPolicy: Never
value: 1000000
---
Copy link
Contributor

Choose a reason for hiding this comment

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

When running make manifest this ends up at the end rather than here. We might just move this to the end of the file to avoid it creating a dirty tree when running it, functionally it works the same.

@@ -0,0 +1,193 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this file autogenerated? I'm not sure where it came from?

@@ -39,7 +39,7 @@ pub const PUBLIC_KEY_NAME: &str = "tls.crt";
pub const PRIVATE_KEY_NAME: &str = "tls.key";
pub const TLS_KEY_MOUNT_PATH: &str = "/etc/brupop-tls-keys";
// Certificate object name
pub const ROOT_CERTIFICATE_NAME: &str = "root-certificate";
pub const ROOT_CERTIFICATE_NAME: &str = "brupop-selfsigned-ca";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is probably safe but I'm digging into how we use cert-manager under the hood to ensure we aren't going to break something else down the line. Do you happen to have links to what led you to this change?

Copy link
Author

Choose a reason for hiding this comment

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

I do not unfortunately. I can just attest from what Jack mentions here that this resolved an issue on our end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants