fix(serverless-component): when adding primary domain name to CloudFront distribution aliases, don't replace other aliases

[dpowell]

I missed this in ea180e6, where the domain alias being added to the distribution would overwrite aliases that were added when we create the distribution. Append the domain alias(es) to the list instead of creating a new Aliases.Items to preserve those.

There don't seem to be any tests for this component, so I manually verified running the built component locally. If I overlooked a test suite that should be updated, let me know.

[dphang]

@dpowell actually now that I think about it I think this may have conflicts by appending to existing aliases, if managing using domain inputs, we should let it manage and overwrite all aliases. By using push, will it not keep appending to existing aliases each time distribution needs to be updated. So you can end up with multiple entries e.g ["example.com", "example.com"]? I assume CloudFront will get rid of duplicate entries but it could possibly be causing issues like #722

Otherwise if we don't want to use domain inputs, then just use the aliases input to manage the domains (and set certificate manually).

[dphang]

Planning to revert this change in next couple days to original behavior which should be more correct, @dpowell please do let me know if you have any issues or concerns.

[dpowell]

The push for the domain’s alias could be conditional on !contains, perhaps.

It wasn’t clear to me that domain was optional, so I’ve been using domain for my “primary” (even though not using Route53 or ACM for it) and aliases for additional. Perhaps I just read too much into that and we could clarify in a docs setting about aliases to use only one or the other.

[dpowell]

Things work fine for me if I use only aliases, so if you'd prefer to require one or the other I think it's safe to revert this, add a validation and perhaps some documentation. If you'd prefer to support a mix I can work up a patch that safely adds aliases and domains if not already present in the distribution config.

Should every deploy "true up" the list of aliases in the distribution with the domain+aliases from the serverless.yml, reverting any manual changes done directly to the distribution? Or just ensure that the current domain and aliases records are present, and if you want to change a previously configured-by-serverless alias you'd have to remove it manually?

[dphang]

@dpowell, yeah, I think the component should manage and override all aliases if either domain or aliases are specified, otherwise it could conflict with manual updates. However, if neither are in the inputs, then it should be non-destructive and don't update unspecified inputs (I fixed this one for aliases input). This is how a lot of IaC works (e.g Terraform): if you configure an attribute, it completely manages it, but if unconfigured it does not touch that (e.g to allow people to slowly import their infrastructure into Terraform).

It would be simpler to keep them separate in my opinion. Though I do realize some people may want the additional aliases - in that case, it's probably better to manage the domain outside of this component and then specify aliases with the full set of names (including domain name) to attach to the CF distribution. Otherwise, it may make the logic much more complex here. In fact, I personally do that because my Route53 config is in a separate account from where my web app is deployed to.

In a future release, we are hoping we can simplify much of this custom deployment logic by exploring proper IaC like CDK or CDK for Terraform. As good as Serverless framework is for Serverless functions, it's not as good as managing complex infra configurations as proper IaC tools. Plus, we've already mostly imported many of the packages (aws-cloudfront, domain) and updated custom logic for deploying ourselves.

@danielcondemarin let me know if you have any thoughts on which you prefer.

[dphang]

@dpowell created this to revert to original behavior: #731. Will do some more manual testing myself since there's no existing tests (another issue is open for adding domain tests) and will update docs in another commit. If you can, please pull and build this branch and see if it works well for you still. Thanks!

[danielcondemarin]

@dphang Personally I think we should aim to "update" distribution settings rather than "overriding" whenever possible. This is particularly beneficial to those reusing the same distribution for next.js deployments and other APIs that may require different CNAMES etc. Hence why I raised #477 a while back.

You're right though, it adds complexity and makes it more difficult to maintain. I think for now we should go with what you suggested and review it once we redo how deployments work in future.