-
Notifications
You must be signed in to change notification settings - Fork 53
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
Support internal load balancers #2388
Conversation
✅ Deploy Preview for constellation-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
d9bfa3a
to
82c4c0b
Compare
I discussed with @malt3 a possible testing which should be compatible with most (all?) of our current CI tests. Essentially:
Now we should be able to operate as normal from our local system / the CI. Thanks @malt3 for the discussion! |
82c4c0b
to
773578b
Compare
569d481
to
299cac8
Compare
feb625b
to
fe56b32
Compare
fe56b32
to
94121b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have this as a separate test instead of making it an option for the e2e-test-manual workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the e2e-test-manual has already 10 dispatch event intputs and I was uncomfortable merging even more inputs.
I looked at the Terraform changes which lgtm.
Thanks for the test coverage! Should we run them as part of the weekly (at least for GCP)? |
The commit add in-cluster endpoint to terraform output is necessary to add the in-cluster endpoint to the Cilium helm values. |
24c10f5
to
e0be91f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only looked at Helm pkg. 👍
589c733
to
4882cfd
Compare
4882cfd
to
e924f23
Compare
Coverage report
|
Context
Proposed change(s)
internal_loadbalancer
flag which default can be switched totrue
. Then the constellation infra is set up using internal LB(s) only.I manually verified this on every of the 3 CSPs.
Should we build tests for this before merging?
If yes, how?
One way could be to automatically create a single VM with public IP and SSH access in the right subnet when
var.internal_loadbalancer && var.debug
istrue
. We'd need to write that code in our current terraform tough.How to (manually) test? (old)
internal_loadbalancer
totrue
in thevar.tf
file of the CSP.constellation create
as normalscp -r <path to workdir> user@publicIP:~
cdbg deploy
andconstellation init
as normalNote that on gcp and aws you need more cloud permission inside the VM since you are not logged in with your user.
The simplest ways are:
How to (manually) test?
internalLoadBalancer: true
anddebug: true
in yourconstellation-conf.yaml
. Then deploy and initialize your Constellation as usual. See thejump-host
VM in the CSP.Note that on Azure one needs to disable konnectivity inside the bootstrapper like so: 4a6b99d. Since we will drop konnectivity after the Cilium update, I decided to not build another workaround for it. The underlying problem is the same as described above the
PrepareControlPlane
function in theazure
package. Only with the added problem that the exact same solution as done in the function does not work, since konnectivity is a pod so the routing is handled by Cilium.Building testing debug image: https://github.com/edgelesssys/constellation/actions/runs/6494433304
ref/feat-arch-internal-lb/stream/debug/v2.13.0-pre.0.20231012022300-8d7e3b676dcd
E2E Test GCP: https://github.com/edgelesssys/constellation/actions/runs/6419796210/job/17528606182
Checklist