feat(aws): add multi-arch support #407
Conversation
chrisgacsal
added
enhancement
|
This PR relies on #388 to be merged first. |
chrisgacsal
force-pushed
the
aws-multiarch-support
branch
4 times, most recently
from
07089a7
to
77715e9
Compare
chrisgacsal
force-pushed
the
aws-multiarch-support
branch
from
77715e9
to
3d96016
Compare
chrisgacsal
removed
the
work in progress
| VMCLARITY_AWS_IMAGE_OWNERS=${ScannerAMIOwnersFilter} | ||
| # Comma separated list of architecture:instance_type pairs used by the Provider to determine the instance type | ||
| # for the Scanner instance based on the Targets architecture. | ||
| VMCLARITY_AWS_INSTANCE_MAPPING=${ScannerInstanceTypeMapping} |
There was a problem hiding this comment.
nit: can we change this to "AWS_INSTANCE_TYPE_MAPPING"?
Honestly I wonder if using a single environment variable is a good idea, I think it might be nicer to utilise viper's ability to unmarshal into a struct then we could have:
type InstanceTypeMapping struct {
X8664 string `mapstructure:"x86_64"`
Arm64 string `mapstructure:"arm64"`
}
and then do:
_ = BindEnv("instance_type_mapping.x86_64")
_ = BindEnv("instance_type_mapping.arm64")
Which would translate the the environment vars:
VMCLARITY_AWS_INSTANCE_MAPPING.X86_64=...
VMCLARITY_AWS_INSTANCE_MAPPING.ARM64=...
if we add a StringReplace to switch . for _ then it could be even nicer. WDYT?
There was a problem hiding this comment.
I did exactly this in the first iteration and I like the current one better. Having separate env var for each architecture will bloat the configuration when we start support more than 2 architectures. I have no strong preference so I can change this to use separate env vars.
There was a problem hiding this comment.
I can see how this is more flexible and less messy from an environment var perspective. I'm kind of thinking about it from a yaml configuration perspective if we want to switch (or support both eventually):
instance_type_mapping:
x86_64: foo
arm64: bar
I like the idea that if we separate the structure in the Config struct that we can do things like this too:
v.SetDefault("instance_type_mapping.x86_64", "....")
v.SetDefault("instance_type_mapping.ARM64", "....")
That way you don't override the whole instance_type mapping if you only specify the x64 instance type. If we can unmarshal that into the map[string]string{} then continues to give us flexibility, if we need to add another arch we just need to add another BindEnv. (The regex validation means we have to make a code change to add a new arch today anyway, so adding another BindEnv and SetDefault is the same effort)
I think if I'm right @sagikazarmark might be able to correct me. If we go the route above (combined with your code) we can support both:
If you only want to override the default for x86_64:
INSTANCE_TYPE_MAPPING.x86_64=...
If you want to override everything:
INSTANCE_TYPE_MAPPING=x86_64:<instance type>,arm64:<instance type>
| # for the Scanner instance based on the Targets architecture. | ||
| VMCLARITY_AWS_INSTANCE_MAPPING=${ScannerInstanceTypeMapping} | ||
| # Optional. Always use this architecture for Scanner instance. Make sure that there is a valid mapping defined in VMCLARITY_AWS_INSTANCE_MAPPING | ||
| # for this architecture. |
There was a problem hiding this comment.
Maybe worth specifying that if not provided then the Scanner will attempt to match the arch of the target system?
Do we need this option? Could we just not specify a mapping for a specific arch, and the default behaviour if we don't have a mapping can be use the first arch specified in the mapping or something?
It looks like the current behaviour is to fail if we have a target with an arch we don't understand, I'm not sure this is the best option as we should be able to at least try to scan with a scanner of a different arch and if the scanners fail then so be it.
There was a problem hiding this comment.
Do we need this option? Could we just not specify a mapping for a specific arch, and the default behaviour if we don't have a mapping can be use the first arch specified in the mapping or something?
We can do this too. I just wanted to allow the user to explicitly set the arch they want to use instead of the implicit behaviour.
It looks like the current behaviour is to fail if we have a target with an arch we don't understand, I'm not sure this is the best option as we should be able to at least try to scan with a scanner of a different arch and if the scanners fail then so be it.
I am not sure about falling back to a different arch is a good idea either. It is hard to predict the outcome of the scan as the scanner may fail, may report false positives, etc.
Anyways, will change this too to do a fallback.
| } | ||
|
|
||
| return imageID, nil | ||
| } |
There was a problem hiding this comment.
not sure I like getting the random latest release for an AMI, if they publish a bad release it means that randomly scans which were working will start failing with no change to the system's configuration. I think I would much rather pre-determine this ahead of time as part of the configuration and then if we want to move to a newer version it be a stack upgrade or configuration change.
There was a problem hiding this comment.
I do not think this is an issue as the chance for us encountering a bad image is slim. While the burden of keeping the AMI list update is real. Anyways, will make it a static config.
| @@ -1633,6 +1633,8 @@ components: | |||
| launchTime: | |||
| type: string | |||
| format: date-time | |||
| architecture: | |||
| type: string | |||
There was a problem hiding this comment.
Should this be an enum so that it can be standardised among cloud providers? I'm aware of at least 3 different ways things represent x64 it can be: x86_64 or x64 or AMD64.
I would prefer that we pick one and make that the standard for the API and UI this will be especially important when we start mixing multiple cloud providers reporting assets and you want to use an odata filter to find all the x64 VMs for example.
|
We will be addressing this feature in a separate repo. Lots of useful feedback here that we were able to use for the revisited design. Closing due to unification |
Description
Extend AWS provider config to support setting architecure specific AMI and Instance Type used for Scanner Instance creation.
Type of Change
[ ] Bug Fix
[x] New Feature
[ ] Breaking Change
[ ] Refactor
[x] Documentation
[ ] Other (please describe)
Checklist