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

feat: (IAC-480) Change default certificate generator to openssl #245

Merged
merged 3 commits into from
Jul 5, 2022

Conversation

jarpat
Copy link
Contributor

@jarpat jarpat commented Jun 21, 2022

Changes

Makes the new default for V4_CFG_TLS_GENERATOR to openssl. Also changes CERT_MANAGER_ENABLED to false so cert-manager does not get installed during the baseline by default.

Tests

See internal IAC-480 ticket

@jarpat jarpat added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 21, 2022
@jarpat jarpat requested a review from thpang June 21, 2022 18:26
@jarpat jarpat self-assigned this Jun 21, 2022
Copy link
Member

@thpang thpang left a comment

Choose a reason for hiding this comment

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

Just a general comment here. There is no parity between what used to be in place for the cert-manager and openssl that was installed during the deployment repo install phase. Would be nice, if possible to have openssl as part of the baseline to keep things aligned.

@jarpat
Copy link
Contributor Author

jarpat commented Jun 27, 2022

Just a general comment here. There is no parity between what used to be in place for the cert-manager and openssl that was installed during the deployment repo install phase. Would be nice, if possible to have openssl as part of the baseline to keep things aligned.

I think this is ready for re-review after the discussion we had last week.

@@ -279,7 +279,7 @@ V4_CFG_POSTGRES_SERVERS:

| Name | Description | Type | Default | Required | Notes | Tasks |
| :--- | ---: | ---: | ---: | ---: | ---: | ---: |
| CERT_MANAGER_ENABLED | Whether to deploy tool | bool | true | false | | baseline |
| CERT_MANAGER_ENABLED | Whether to deploy tool | bool | false | false | | baseline |
Copy link
Member

Choose a reason for hiding this comment

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

Just a doc comment.
Since there are now two certificate handling objects that IAC can impact, I think it might help to say, "Whether to deploy certificate-manager into the cluster" to be more specific than "tool" 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.

@dhoucgitter, you're right that description doesn't seem to be clear. I've updated the description and notes section to:

Name Description Type Default Required Notes Tasks
CERT_MANAGER_ENABLED Whether to deploy cert-manager into the cluster using helm bool false false Required if V4_CFG_TLS_GENERATOR is set to cert-manager and it's not already installed baseline

Let me know if that sounds better

Copy link
Member

@dhoucgitter dhoucgitter left a comment

Choose a reason for hiding this comment

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

I added a minor doc comment, otherwise lgtm

Copy link
Member

@thpang thpang left a comment

Choose a reason for hiding this comment

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

LGTM. We'll have future conversations about OpenSSL and possible cluster enablement vs namespace enablement with Viya.

@jarpat jarpat changed the title (IAC-480) Change default certificate generator to openssl feat: (IAC-480) Change default certificate generator to openssl Jul 5, 2022
@jarpat jarpat merged commit cbc46e0 into staging Jul 5, 2022
@jarpat jarpat deleted the default_openssl branch July 5, 2022 20:26
@jarpat jarpat mentioned this pull request Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants