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

Add localstack to reference implementation #248

Conversation

rattboi
Copy link
Contributor

@rattboi rattboi commented May 14, 2024

This adds localstack, which involves the following:

  1. Install localstack as argo application
    This needs patching as the helm chart doesn't expose enough to access
    DNS port. I used kustomize + helmCharts directive in order to do a
    service patch. That required...
  2. --enable-helm added to argocd, via configmap.
  3. Delegation of localstack DNS to the localstack service, via Coredns
    Corefile
  4. Add a new Crossplane ProviderConfig for localstack

This should all be non-breaking changes

One further enhancement I'll be working on is making the providerconfig selectable in the backstage template, so that a user can select between targetting localstack or aws proper when generating their app + bucket.

This adds localstack, which involves the following:

1) Install localstack as argo application
 This needs patching as the helm chart doesn't expose enough to access
 DNS port. I used kustomize + helmCharts directive in order to do a
 service patch. That required...
2) --enable-helm added to argocd, via configmap.
3) Delegation of localstack DNS to the localstack service, via Coredns
   Corefile
4) Add a new Crossplane ProviderConfig for localstack

This should all be non-breaking changes

One further enhancement I'll be working on is making the providerconfig
selectable in the backstage template, so that a user can select between
targetting localstack or aws proper when generating their app + bucket.

Signed-off-by: Bradon Kanyid (rattboi) <[email protected]>
@rattboi rattboi force-pushed the rattboi/add-localstack-to-ref-implementation branch from b30ad0d to 8c13d5b Compare May 14, 2024 00:17
Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

Awesome to see localstack integrations. Left some comments, please take a look. We should add documentation about this too.

Comment on lines +35 to +38
localhost.localstack.cloud:53 {
errors
cache 30
forward . 10.96.100.10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possible to re-write instead of pointing to a static IP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://kubernetes.io/docs/tasks/administer-cluster/dns-custom-nameservers/#configuration-of-stub-domain-and-upstream-nameserver-using-coredns

see:

Note:
CoreDNS does not support FQDNs for stub-domains and nameservers (eg: "ns.foo.com"). During translation, all FQDN nameservers will be omitted from the CoreDNS config.

I'd rather point at the service, but that's apparently not an option, which is why I went the ClusterIP route.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is generated. Changes should be made to https://github.com/cnoe-io/idpbuilder/blob/main/hack/argo-cd/argocd-cm.yaml

Then run make build or make embedded-resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Updated scripts to use more-portable `/usr/bin/env bash`. Was necessary
to run the scripts from nixos (no /bin/bash).

Signed-off-by: Bradon Kanyid (rattboi) <[email protected]>
@rattboi rattboi requested a review from nabuskey May 14, 2024 18:01
@nabuskey
Copy link
Collaborator

Changes seems ok but I'm not too familiar with localstack. @csantanapr do you wanna take a look?

@rattboi
Copy link
Contributor Author

rattboi commented May 14, 2024

FYI, I also just opened a PR with localstack to properly expose their DNS service to the kubernetes service.

If this is merged any time soon, I would be able to just reference the new helm chart + values object to drive it, instead of kustomize + helmCharts directive. That would mean not needing to add the --enable-helm bits for Argo.

@nimakaviani
Copy link
Contributor

thanks @rattboi for the great work.

As discussed during the community call, I would love to see whether and how much of it we can pull out into a separate add-on custom package that can go hand in hand with the reference implementation rather than baking it into the ref-impl. Similar to how we have done it with the terraform-integration.

Let me know if you can find some cycles to experiment with it and I will try to do it on my end too, hopefully before the next community meeting.

@rattboi
Copy link
Contributor Author

rattboi commented May 14, 2024

It would be easy enough to do, but you would lose certain integration potentials.

  1. it seems with the crossplane ProviderConfig setting spec.endpoint.hostnameImmutable: true, the need to delegate Localstack DNS isn't necessary, but my first experiment was with using the AWS ACK Controllers, and they have no such option. Running localstack standalone is easy enough, but I don't know how to write it as a separate add-on custom package while also keeping DNS delegation, as that's a change to the Coredns configmap. The only way I can think to do something like that in a composable fashion would be something like Kyverno in order to allow multiple snippets of various package configmaps to compose into the destination CM for Coredns.

  2. Similar to 1, you lose the potential to leverage existing Backstage templates. I could supply my own, but it'll be almost entirely copypasta of what exists in the ref-impl, with the only change necessary being which ProviderConfig to use. Alternatively, I suppose we could go back to using just a string field, but then the user needs to know what magic name to place in the providerConfigName in order to leverage localstack, vs having the options be presented as a dropdown with every reasonable option there. This does feel like something that could potentially source from the environment dynamically, but I'm not positive what templating tool we could use to do that. Perhaps Kyverno, again...

A lot of this goes back to what @greghaynes was saying about finding good interfaces to make these pieces composable. As it is today, I could make this not a part of ref-impl and instead a separate custom package, but it will mean a less-good solution. Perhaps that will spur us to develop the interfaces needed. I'm not sure.

@nimakaviani
Copy link
Contributor

yeah, thanks for highlighting the parts with a tight integration. Let me think through those a bit and we can discuss how to proceed here.

@rattboi
Copy link
Contributor Author

rattboi commented May 15, 2024

I think the cross-cutting composable integrations aren't likely to be solved in my PR, so I will move the localstack work into a custom package as you requested, and strip it down to really only installing localstack + the crossplane providerconfig. I think if that's the level of integration for now, I can greatly simplify things by turning off the dns server bits, no need for the coredns changes then, and also no need for the --enable-helm kustomize bits.

It does mean no dropdown in the backstage template, but I can just remark about that in a readme in the custom package root.

@nimakaviani
Copy link
Contributor

nimakaviani commented May 15, 2024

I think we can leave --enable-helm in there, believing that it will be a safe choice. Can you split that into its own PR and send it through.

Creating a separate custom package sounds good for now. I think I might have a pathway to be able to pull in the config changes for the dns stuff in as well, but will have to try out a few things and I can add those to your changes as I experiment here.

Modifying backstage templates will remain as an outstanding item for now though.

@nimakaviani
Copy link
Contributor

closing this since #257 got merged in place of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants