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.

Sign up for GitHub

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

Jump to bottom

cert manager update #679

Merged
merged 2 commits into from
Nov 17, 2022
Merged

cert manager update #679

merged 2 commits into from
Nov 17, 2022

Conversation

badhrinathpa
Copy link
Contributor

No description provided.

@onf-bot
Copy link

onf-bot commented Sep 28, 2022

Can one of the admins verify this patch?

adibrastegarnia
adibrastegarnia reviewed Sep 28, 2022
View reviewed changes
onos-config/templates/deployment.yaml
@@ -47,6 +47,14 @@ spec:
env:
# *_NAMESPACE and *_NAME environment variables are recognized by onos-lib-go utilities.
# These variables should always be defined.
- name: GRPC_GO_LOG_VERBOSITY_LEVEL
Copy link
Contributor

Choose a reason for hiding this comment

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

what loggers are these? log levels for onos-config and onos-topo can be controlled from the values in the helm chart.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to remove them to avoid confusion with our loggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the logs under flag.

adibrastegarnia
adibrastegarnia requested changes Sep 28, 2022
View reviewed changes
onos-umbrella/Chart.yaml Outdated
@@ -17,6 +17,11 @@ maintainers:
- name: ONOS Support
email: support@opennetworking.org
dependencies:
- name: ca-issuer-charts
condition: import.ca-issuer.enabled
#intel-innersource/frameworks.edge.one-intel-edge.maestro-app.roc.rocaas-charts/ca-issuer-charts
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the intel inner source thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

adibrastegarnia
adibrastegarnia requested changes Sep 28, 2022
View reviewed changes
onos-config/values.yaml Outdated
@@ -108,6 +117,12 @@ logging:
output:
stdout:
sink: stdout
grpc:
Copy link
Contributor

@adibrastegarnia adibrastegarnia Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We should not add it here. It should be separate. We use a specific format for internal loggers and grpc stuff is not part of that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

adibrastegarnia
adibrastegarnia requested changes Sep 28, 2022
View reviewed changes
onos-config/templates/secret.yaml Outdated
namespace: {{.Values.certManager.namespace}}
spec:
dnsNames:
- certs.oie.intel.com
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove any references to intel stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@badhrinathpa badhrinathpa force-pushed the cert-manager branch 2 times, most recently from 386b6f9 to 328cbc0 Compare September 28, 2022 22:01
adibrastegarnia
adibrastegarnia requested changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@adibrastegarnia adibrastegarnia left a comment

Choose a reason for hiding this comment

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

bump version of onos-config and onos-umbrella chart

@badhrinathpa badhrinathpa force-pushed the cert-manager branch 2 times, most recently from d5b298d to fa604d9 Compare October 4, 2022 23:17
@badhrinathpa
Copy link
Contributor Author

bump version of onos-config and onos-umbrella chart

updated.

@badhrinathpa badhrinathpa reopened this Nov 7, 2022
SeanCondon
SeanCondon approved these changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@SeanCondon SeanCondon left a comment

Choose a reason for hiding this comment

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

A few comments included, but otherwise looks good.

It can't work with onos-umbrella as there's no issuer here - that can be added at a later date if required

onos-config/values.yaml Outdated
@@ -24,6 +24,22 @@ global:

replicaCount: 1

grpc:
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 should have logging in it somewhere - you're not enabling grpc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

onos-topo/values.yaml Outdated
@@ -19,6 +19,22 @@ global:

replicaCount: 1

grpc:
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment - should include logging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

onos-topo/values.yaml Outdated
certManager:
enabled: false
name: onos-topo-cert
namespace: rocaas
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

onos-topo/templates/secret.yaml Outdated
kind: Certificate
metadata:
name: {{.Values.certManager.name}}
namespace: {{.Values.certManager.namespace}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be .Release.namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

onos-config/templates/secret.yaml Outdated
kind: Certificate
metadata:
name: {{.Values.certManager.name}}
namespace: {{.Values.certManager.namespace}}
Copy link
Contributor

Choose a reason for hiding this comment

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

should be .Release.namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

onos-config/values.yaml Outdated
certManager:
enabled: false
name: onos-config-cert
namespace: rocaas
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

onos-config/values.yaml
enabled: false
name: onos-config-cert
namespace: rocaas
secretName: onos-config-secret
Copy link
Contributor

Choose a reason for hiding this comment

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

the actual secrets are getting a generated name like

onos-config-cert-qkppl                Opaque               1      25m
onos-topo-cert-x7c5k                  Opaque               1      25m

and so don't match up with what's here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would happen if the cert manager is not used to generate certificate.

@badhrinathpa badhrinathpa force-pushed the cert-manager branch 2 times, most recently from 6af754a to 1261539 Compare November 16, 2022 23:37
@badhrinathpa badhrinathpa requested review from adibrastegarnia and removed request for tomikazi and ray-milkey November 16, 2022 23:49
@SeanCondon
Copy link
Contributor

retest this please

@SeanCondon
Update deployment.yaml
328eb2f 
Adding the closing `{{- end }}`
@SeanCondon
Copy link
Contributor

retest this please

@SeanCondon SeanCondon merged commit d72db03 into onosproject:master Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmbaker scottmbaker scottmbaker approved these changes

@adibrastegarnia adibrastegarnia adibrastegarnia approved these changes

@SeanCondon SeanCondon SeanCondon approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@badhrinathpa @onf-bot @SeanCondon @adibrastegarnia @scottmbaker