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

Move system smtp to secret #280

Merged
merged 6 commits into from
Dec 20, 2019
Merged

Conversation

miguelsorianod
Copy link
Contributor

This PR changes the deployment of the SMTP configuration from a ConfigMap to a Secret.
The reason of this is because some of the configuration contains credentials.

@miguelsorianod
Copy link
Contributor Author

Pending to implement the upgrade procedure but the current PR state should have the needed code in templates and operator to deploy SMTP configuration with a secret instead of a configmap.
@eguzki you can review this part for the moment

@miguelsorianod miguelsorianod force-pushed the move-system-smtp-to-secret branch from d042026 to e979a47 Compare November 18, 2019 14:25
@miguelsorianod
Copy link
Contributor Author

Updated PR to include upgrade process to migrate from the 'smtp' ConfigMap to the 'system-smtp' Secret.
In the future the order/coordination in how the upgrade is performed might have to be changed to have consistent changes

@eguzki
Copy link
Member

eguzki commented Nov 20, 2019

looking good

@miguelsorianod
Copy link
Contributor Author

Currently the upgrade procedure should work because there isn't an upgrade step at this moment that requires "atomicity" of the changes.
How do you want to proceed? We can merge this and work on a solution when the need arises/handle it in a separate PR or specify what we should solve in this PR before merging it.

@miguelsorianod miguelsorianod changed the title [WIP] Move system smtp to secret Move system smtp to secret Dec 12, 2019
@eguzki
Copy link
Member

eguzki commented Dec 12, 2019

missing smtp secret documentation

@miguelsorianod miguelsorianod force-pushed the move-system-smtp-to-secret branch from e979a47 to a39024d Compare December 16, 2019 16:22
@miguelsorianod
Copy link
Contributor Author

Added system-smtp secret documentation.
Ready for re-review @eguzki

@miguelsorianod miguelsorianod force-pushed the move-system-smtp-to-secret branch from a39024d to a260fdd Compare December 16, 2019 16:56
@miguelsorianod
Copy link
Contributor Author

I've noticed the APIManager controller unit test related to the upgrade fails. The reason for that is that the upgrade code expects the "system" ConfigMap to exist (see migrateSystemSMTPData method in upgrade.go) but it does not exist anymore due to it has been removed in this PR being replaced by a secret.

Should we make the UpgradeApiManager implement an interface and use an interface instead from the apimanager_controller being the upgrader a field of the ReconcileAPIManager struct? and mock it in the test so the upgrade method returns an arbitrary result? What do you think?

@eguzki
Copy link
Member

eguzki commented Dec 17, 2019

Why not just create the smtp configmap for the upgrade test? you can even test the config map has been replaced by secret.

@miguelsorianod miguelsorianod force-pushed the move-system-smtp-to-secret branch from 09a7a86 to 1bb71cb Compare December 17, 2019 19:08
@miguelsorianod
Copy link
Contributor Author

miguelsorianod commented Dec 17, 2019

Why not just create the smtp configmap for the upgrade test? you can even test the config map has been replaced by secret.

I thought about that but this means that in the next release the test will break/need to be changed so they would be prone to errors. Also the number of calls to the Reconcile method in the test will vary depending on the number of elements to upgrade.

In any case I've implemented it so you can see how the addition of this new functionality has affected the test in the apimanager_controller_test (in a new commit). Additionally I also needed to mock system-sidekiq DC and add more mock elements into the system-app DC.
I've also modified the upgrade.go implementation when creating the system-smtp secret because it was being created without the controllerreference and now it sets the secret data using Data instead of StringData field due to when mocking the API calls the StringData seems that is not automatically converted to Data when creating the object and this caused tests to fail too. You can re-review that file too.

@eguzki
Copy link
Member

eguzki commented Dec 18, 2019

In the next release, all the upgrade tests do not make sense at all and have to be "removed", as it has to be done with the upgrade code.

doc/apimanager-reference.md Show resolved Hide resolved
@miguelsorianod miguelsorianod force-pushed the move-system-smtp-to-secret branch from 1bb71cb to 3992aca Compare December 18, 2019 16:18
@miguelsorianod miguelsorianod force-pushed the move-system-smtp-to-secret branch from 3992aca to f3e442a Compare December 19, 2019 10:13
@codeclimate
Copy link

codeclimate bot commented Dec 19, 2019

Code Climate has analyzed commit f3e442a and detected 21 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 4
Duplication 6
Style 11

View more on Code Climate.

@miguelsorianod
Copy link
Contributor Author

system-smtp documentation table updated

@miguelsorianod miguelsorianod merged commit 265c674 into master Dec 20, 2019
@miguelsorianod miguelsorianod deleted the move-system-smtp-to-secret branch December 20, 2019 15:05
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.

3 participants