Support AssumeRole in CRR #2622
Conversation
Hello kerkesni,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes
@@ Coverage Diff @@
## development/9.1 #2622 +/- ##
===================================================
+ Coverage 72.96% 73.01% +0.04%
===================================================
Files 201 201
Lines 13374 13415 +41
===================================================
+ Hits 9759 9795 +36
- Misses 3605 3610 +5
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Kerkesni
force-pushed
the
improvement/BB-647
branch
4 times, most recently
from
d409765 to
01ab3d8
Compare
Artesca doesn't expose vault admin APIs. Skipping the call to getCanonicalIdsByAccountIds and delegating the task of replacing the owner of the object to the destination's Cloudserver. Issue: BB-647
Kerkesni
force-pushed
the
improvement/BB-647
branch
from
923e478 to
f7f393c
Compare
| joi.string(), // site name | ||
| joi.object({ | ||
| transport: transportJoi, | ||
| auth: destinationAuthJoi.required(), |
There was a problem hiding this comment.
- since it is required,
destination.authwill never be used as default - does it make sense to specify a destination with the same transport but a dedicated auth?
| }).required(), | ||
| auth: destinationAuthJoi.optional(), | ||
| sites: joi.object().pattern( | ||
| joi.string(), // site name |
There was a problem hiding this comment.
maybe we want to validate this is in the bootstrap list?
| joi.string(), // site name | |
| joi.string().valid(Joi.in('bootstrapList')), // site name |
| transport: transportJoi, | ||
| auth: joi.object({ | ||
| type: joi.alternatives().try('account', 'role', 'service') | ||
| .required(), | ||
| account: joi.string() | ||
| .when('type', { is: 'account', then: joi.required() }), | ||
| vault: joi.object({ | ||
| host: joi.string().optional(), | ||
| port: joi.number().greater(0).optional(), | ||
| adminPort: joi.number().greater(0).optional(), | ||
| adminCredentialsFile: joi.string().optional(), | ||
| }), | ||
| }).required(), | ||
| auth: destinationAuthJoi.optional(), | ||
| sites: joi.object().pattern( |
There was a problem hiding this comment.
-
there is an
authJoitype in configItems.joi.js, which holds both transport and auth: can't we use it (or extend it) instead of introducting a new destinationAuthJoi type? -
once we have a type with transport+auth, maybe
destinationcan extend this type to add bootstrap? (but maybe this creates lots of override, to make things optional/alternative)
| site.auth.vault.adminCredentials = | ||
| _loadAdminCredentialsFromFile(adminCredentialsFile); | ||
| } | ||
| } |
There was a problem hiding this comment.
set the default vault already, so that "config" handling is done only here (and does not trickle in the "business" code) : i.e. from here on, auth is always specified in each destination' site?
| } | |
| } else { | |
| site.auth.vault = destination.auth.vault; | |
| } |
| transport: transportJoi, | ||
| auth: destinationAuthJoi.required(), | ||
| }).required() | ||
| ).when('auth', { |
There was a problem hiding this comment.
should this condition on be on each destination's auth field ?
(i.e. the site's auth must be specified if there is no "top level" default)
| const destConfig = {}; | ||
| siteNames.forEach(site => { | ||
| destConfig[site] = config.getReplicationSiteDestConfig(site); | ||
| destConfig[site].bootstrapList = bootstrapList; |
There was a problem hiding this comment.
not needed, already done in getReplicationSiteDestConfig()
| if (!destConfig[site]) { | ||
| destConfig[site] = config.getReplicationSiteDestConfig(site); | ||
| } | ||
| destConfig[site].bootstrapList = updatedBootstrapList; |
There was a problem hiding this comment.
boostrapList already set in getReplicationSiteDestConfig(), so either it is an else:
| if (!destConfig[site]) { | |
| destConfig[site] = config.getReplicationSiteDestConfig(site); | |
| } | |
| destConfig[site].bootstrapList = updatedBootstrapList; | |
| if (!destConfig[site]) { | |
| destConfig[site] = config.getReplicationSiteDestConfig(site); | |
| } | |
| else { | |
| destConfig[site].bootstrapList = updatedBootstrapList; | |
| } |
or maybe we should always update the site's configuration:
| if (!destConfig[site]) { | |
| destConfig[site] = config.getReplicationSiteDestConfig(site); | |
| } | |
| destConfig[site].bootstrapList = updatedBootstrapList; | |
| destConfig[site] = config.getReplicationSiteDestConfig(site); |
| type: 'service', | ||
| account: 'service-replication-3', | ||
| }, | ||
| bootstrapList: [ |
There was a problem hiding this comment.
it may/could happen that BACKBEAT_CONFIG_FILE is set before the tests begin (and some test may depend on it...) : the really safe way to handle this is to store the previous value of BACKBEAT_CONFIG_FILE before the test (beforeEach), so that eventually we can either delete the env variable if it was not set or reset it to its original value otherwise
| bootstrapList: [ | ||
| { site: 'aws1', type: 'aws_s3' }, | ||
| { site: 'aws2', type: 'aws_s3' }, | ||
| { site: 'aws3', type: 'aws_s3' } |
There was a problem hiding this comment.
no test for the auth.vault handling (_loadAdminCredentialsFromFile)
Issue: BB-647