config: change default destination sync type to azure for azure #2306
Conversation
viktor-f
requested review from
Zash,
simonklb,
aarnq,
raviranjanelastisys and
anders-elastisys
|
|
||
| objectStorage: | ||
| sync: | ||
| ## If Harbor or Thanos are using swift then we will automatically use swift for the sync, regardless of the value set for type. |
There was a problem hiding this comment.
I don't understand this comment. I'm sure it is obvious once you know why but to someone that does not it's not clear what is going on here.
Does it mean that the value of objectStorage.sync.destinationType is completely ignored if .harbor.persistence.type=swift or .thanos.objectStorage.type=swift or that the sync of those specific buckets are ignoring the value of objectStorage.sync.destinationType? I assume the latter?
To me the object storage configuration throughout the project feels a bit complicated. It really shows in the complex "set-me-if" cases. I wonder if we could simplify it somehow? Worthy of an issue?
There was a problem hiding this comment.
This is trying to say that the sync of those specific buckets are ignoring this value. You can check the logic here. I did just copy this line from the base config file, but I could perhaps try to rewrite it to be more clear.
I agree that it is complicated, but it is also supporting a lot of different possibilities. I'm not sure this is something we would spend time on soon, unless it was needed to support something else.
There was a problem hiding this comment.
Time spent simplifying is time saved dealing with complexity in the future but I agree it's not high priority.
At least having tests for each of the config cases is something I'd like to se. I would be fine just seeing "config -> rendered result" and some "missing dependent config option / set-me-if-x" unit tests.
| if yq_check sc .objectStorage.sync.enabled true; then | ||
| if yq_null sc .objectStorage.sync.destinationType; then | ||
| log_info "objectStorage sync is enabled, will keep previous default destinationType: s3" | ||
| else | ||
| log_info "sync destinationType already has an override, skipping" | ||
| fi | ||
| else | ||
| log_info "object storage sync is not enabled, will use new default destinationType: azure" | ||
| fi |
There was a problem hiding this comment.
Where is the migration actually taking place? Sorry if I'm just blind. 😄 To me it just looks like this figuring out how it's currently configured and logs it but doesn't modify anything.
There was a problem hiding this comment.
Yeah I seem to have completely missed that 😅, got too stuck in making sure the logic worked. Will fix it.
Warning
This is a public repository, ensure not to disclose:
What kind of PR is this?
Required: Mark one of the following that is applicable:
Optional: Mark one or more of the following that are applicable:
Important
Breaking changes should be marked
kind/admin-changeorkind/dev-changedepending on typeCritical security fixes should be marked with
kind/securityWhat does this PR do / why do we need this PR?
Changed the default
objectStorage.sync.destinationTypeto azure, for azure clusters. Having this default to s3 caused issues with network policies / update-ips for clusters that are only using azure storage.Information to reviewers
Init and migration script tested, but I have not re-tested using destinationType as azure.
Checklist
NetworkPolicy Dashboard