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

[operator] support cutom autocert-certPeriod time by days #1249

Merged
merged 8 commits into from
Jul 12, 2024

Conversation

JaredTan95
Copy link
Member

@JaredTan95 JaredTan95 commented Jul 8, 2024

As otel-operator matures, the default 365 days will be at risk when users choose to deploy the operator by autoGenerateCert. Therefore, this PR is intended to provide the ability to configure the expiration time.

I performed local verification in the following way:

  1. using custom values to install operator-chart, the same with examples/cutstom-autoGenerateCert-period/values.yaml
cd charts/opentelemetry-operator
helm install opentelemetry-operator . -f examples/cutstom-autoGenerateCert-period/values.yaml
manager:
  collectorImage:
    repository: "otel/opentelemetry-collector-k8s"

admissionWebhooks:
  certManager:
    enabled: false
  autoGenerateCert:
    enabled: true
    # If set to true, new webhook key/certificate is generated on helm upgrade.
    recreate: false
    # Cert period time in years. The default is 1(365 days).
    certPeriodDays: 200
  1. get tls.cert
kubectl get secret opentelemetry-operator-controller-manager-service-cert -o jsonpath='{.data.tls\.crt}' | base64 --decode > tls.crt
  1. use openssl to check the period time of tls.crt
openssl x509 -in tls.crt -noout -dates 

and I got this:

notBefore=Jul  9 02:57:00 2024 GMT
notAfter=Jan 25 02:57:00 2025 GMT # 👈 This is in line with my expectations

@JaredTan95 JaredTan95 requested review from Allex1 and a team as code owners July 8, 2024 14:03
@JaredTan95
Copy link
Member Author

sad, https://github.com/open-telemetry/opentelemetry-helm-charts/actions/runs/9841098833/job/27166919581?pr=1249#step:5:200

Since each CI will re-generated cutstom-autoGenerateCert-period example, can not be consistent, remove cutstom-autoGenerateCert-period?

charts/opentelemetry-operator/values.schema.json Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

the name of this example should be custom-auto-generate-cert-period IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

also, do we need to have a full example for setting this single variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think examples/cutstom-autoGenerateCert-period/values.yaml content is the full example

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, i'm not sure we need to make a whole new full example for this, i was mentioning that the naming with caps is a bit odd here too

@JaredTan95
Copy link
Member Author

sad, https://github.com/open-telemetry/opentelemetry-helm-charts/actions/runs/9841098833/job/27166919581?pr=1249#step:5:200

Since each CI will re-generated cutstom-autoGenerateCert-period example, can not be consistent, remove cutstom-autoGenerateCert-period?

@jaronoff97 hi, any idea about this?

@JaredTan95 JaredTan95 changed the title [operator] support cutom autocert-certPeriod time by years [operator] support cutom autocert-certPeriod time by days Jul 9, 2024
@jaronoff97
Copy link
Contributor

@JaredTan95 what do you get if you run the make generate locally?

@JaredTan95
Copy link
Member Author

every time run ``

@JaredTan95 what do you get if you run the make generate locally?

Similarly, the certificate is regenerated each time with CHARTS=opentelemetry-operator make generate-examples

@jaronoff97
Copy link
Contributor

@JaredTan95 i'll try running locally and see what happens.

@jaronoff97
Copy link
Contributor

@JaredTan95 seems like we have failures here too same thing.

@jaronoff97
Copy link
Contributor

ohhh i see the problem now... hmmm.....

@TylerHelmuth
Copy link
Member

Let's not add the new problematic example. The scenario is pretty straight forward (as in the values.yaml file isn't common or interesting) so I don't think we need a new example for it.

@TylerHelmuth TylerHelmuth merged commit 7ff3106 into open-telemetry:main Jul 12, 2024
4 checks passed
12ushan pushed a commit to giffgaff/opentelemetry-helm-charts that referenced this pull request Jul 22, 2024
…etry#1249)

* support cutom autocert certPeriod time by years

* revert

* polish

* update

* remove some example

---------

Co-authored-by: Jacob Aronoff <jaronoff97@users.noreply.github.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
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

4 participants