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

Added decryption of resources and patches. Refactored SOPS test data. #1286

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

vlasov-y
Copy link
Contributor

@vlasov-y vlasov-y commented Nov 12, 2024

Hi ✋
In addition to the previous PR, I've added decryption for the resource and patch files before the build process. I also found the test data for SOPS to be quite messy, so I restructured it entirely and streamlined the controller test code.

Signed-off-by: Yuriy <yuriy@vlasov.pro>
Signed-off-by: Yuriy <yuriy@vlasov.pro>
@vlasov-y
Copy link
Contributor Author

@stefanprodan tagging you because you have reviewed #1283 for me 😅

@stefanprodan stefanprodan added area/sops SOPS related issues and pull requests area/testing Testing related issues and pull requests labels Nov 12, 2024
Comment on lines 474 to 494
// Iterate over all resources in the Kustomization file and attempt to decrypt them if they are encrypted.
for _, res := range kus.Resources {
// Determine the format for the resource, defaulting to YAML if not specified.
format := formatForPath(res)
// Visit the resource reference and attempt to decrypt it.
if err := visitRef(res, format, true); err != nil {
return err
}
}
// Iterate over all patches in the Kustomization file and attempt to decrypt their paths if they are encrypted.
for _, patch := range kus.Patches {
if patch.Path == "" {
continue
}
// Determine the format for the patch, defaulting to YAML if not specified.
format := formatForPath(patch.Path)
// Visit the patch reference and attempt to decrypt it.
if err := visitRef(patch.Path, format, false); err != nil {
return err
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Besides the secret/configMap generators, all the other YAML encrypted resources are decrypted in-memory before apply.

Copy link
Contributor Author

@vlasov-y vlasov-y Nov 12, 2024

Choose a reason for hiding this comment

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

I will give a short example that will be close to the case I have faced.
We have victoria-metrics HelmRelease in bases. It contains many services, also it is partially encrypted, because of basicAuth embedded in externalWrite URL.
In overlays, we have many values blocks for the same HelmRelease defined but in separate merge patch files. One of them is alert-manager.sops.yaml that has Slack token and it is also encrypted.
When flux runs kustomization and merges them before the decryption then it is fails on sops decryption stage, because SOPS metadata fields are mixed/overwriten etc. and hashes do not match anymore.
That is why I needed patches to be decrypted before the kustomization processing.

Copy link
Contributor Author

@vlasov-y vlasov-y Nov 12, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok but for resources this will fail the moment you use remote bases no?

Copy link
Contributor Author

@vlasov-y vlasov-y Nov 13, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We load all resources to memory during the kustomization build anyway, do not we?

Exactly, and this PR doubles the memory usage and the Go GC CPU usage.

Can we decrypt resources basing on filename?

I'm not for changing the API to cope with a SOPS upstream issue, I'm Ok with decrypting the patches but after merge SOPS should handle mixed mode.

Copy link
Contributor Author

@vlasov-y vlasov-y Nov 15, 2024

Choose a reason for hiding this comment

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

I'm not for changing the API to cope with a SOPS upstream issue, I'm Ok with decrypting the patches but after merge SOPS should handle mixed mode.

That is not a SOPS issues. SOPS decrypts the file that it encrypts without any issue, but we alter that file, we may adding keys that match encrypted_regex. Merging this PR with patches decryption only won't resolve mix test case completely, but will allow to easily bypass such limitation moving encrypted values to patch files. I think that will make life much easier already.
And I do not like changing API for this case either, that resourcesRegexMatch feels like a "dirty" solution anyway.

Copy link
Member

Choose a reason for hiding this comment

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

@vlasov-y the next dev meeting is in 2 days, feel free to join https://fluxcd.io/community/#meetings

I understand now how SOPS fails, it's due to the regex. I think the best way forward is to decrypt the patches only and document this workaround, where the secret will have an empty stringData entry will all keys coming from patches.

Copy link
Contributor Author

@vlasov-y vlasov-y Nov 19, 2024

Choose a reason for hiding this comment

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

@vlasov-y the next dev meeting is in 2 days, feel free to join https://fluxcd.io/community/#meetings

Thanks!

I think the best way forward is to decrypt the patches only and document this workaround

Thanks, agree. I have tried to benchmark decryption of resources and to find a better algorithm, tried streaming reading of files, but it is slower. Anyway, making patches being decrypted and document the regex issue case will be more than enough to live with Kustomization + SOPS with ease.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated documentation describing the case.
Removed decryption of resources.
Removed incompatible sops tests.

Signed-off-by: Yuriy <yuriy@vlasov.pro>
Signed-off-by: Yuriy <yuriy@vlasov.pro>
Signed-off-by: Yuriy <yuriy@vlasov.pro>
Signed-off-by: Yuriy <yuriy@vlasov.pro>
Signed-off-by: Yuriy <yuriy@vlasov.pro>
Signed-off-by: Yuriy <yuriy@vlasov.pro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sops SOPS related issues and pull requests area/testing Testing related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants