frontegg-mock: initial commit #23176

maddyblue · 2023-11-14T09:37:53Z

Add a frontegg mock server binary. Needed for testdrive and cloud testing.

Motivation

This PR adds a feature that has not yet been specified.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:
- n/a

maddyblue · 2023-11-14T09:39:23Z

@philip-stoev I've sketched out some parts of a frontegg service. I'm not sure how to handle certificate plumbing because I think the test certs service will need to start first, but I need the arguments when making the other services? There must be a correct way to do this. Are you able to take over the python here?

ParkMyCar

Nice!

See the comment about spawning a separate runtime. I have a local branch which largely refactors our test helpers to no longer spawn their own runtimes, so feel free to merge as-is and I can do the refactoring

ParkMyCar · 2023-11-14T16:37:56Z

src/frontegg-mock/src/lib.rs

+
+pub struct FronteggMockServer {
+    pub url: String,
+    pub refreshes: Arc<Mutex<u64>>,


any reason to use Mutex<u64> instead of an AtomicU64?

Nope. This was copied over from the previous location. If I remember I'll change it.

Sounds good

ParkMyCar · 2023-11-14T16:46:02Z

src/frontegg-mock/src/lib.rs

+            auth_requests: Arc::clone(&auth_requests),
+        };
+
+        let runtime = Arc::new(Runtime::new()?);


I'm surprised this works! In main.rs we're also creating a runtime, e.g. #[tokio::main], and we create a runtime here. I thought this would have panicked with "creating a runtime within a runtime".

If possible I would prefer fn start(...) to not spawn its own runtime and instead return the server future, e.g.:

pub fn start(...) -> Result<(FronteggMockServer, BoxFuture<'static, ()>), _> { ... }

This allows us to be more flexible with how FronteggMockServer is used. For example, in main.rs we can create our own runtime, but for our integration tests we can spawn the returned future on the test runtime.

It might not work, but I recall being able to start the binary and it ran. Yes, doing what you describe would be best. I'd definitely prefer to do that refactor in a followup, as this was copied over from existing code.

Sweet, sounds good to me!

test/balancerd/mzcompose.py

maddyblue · 2023-11-17T07:04:03Z

Finally got this working in mzcompose. That was much more than I expected. @philip-stoev can you re-review? I ran out of time today to add more tests. I'll work on those soon.

maddyblue · 2023-11-17T07:12:26Z

misc/python/materialize/mzcompose/composition.py

+            user=user,
+            password=password,
+            port=port,
+            ssl_context=ssl_context,


I'm worried this will break all other callers that have ssl disabled. Am waiting for tests to see.

This indeed caused problems; changed to plumb down the ssl context.

benesch

Dropped in some comments about calling this frontegg-mock consistently! I think that will reduce confusion.

benesch · 2023-11-20T22:32:44Z

misc/images/frontegg/Dockerfile

+    && groupadd --system --gid=999 materialize \
+    && useradd --system --gid=999 --uid=999 --create-home materialize
+
+COPY mz-frontegg-mock /usr/local/bin/frontegg


Why are we changing the binary name here? If it's mz-frontegg-mock in the repo, just stick with mz-frontegg-mock in the Docker image?

benesch · 2023-11-20T22:32:58Z

misc/python/materialize/mzcompose/services/frontegg.py

+from materialize.ui import UIError
+
+
+class Frontegg(Service):


Suggested change

class Frontegg(Service):

class FronteggMock(Service):

benesch · 2023-11-20T22:34:16Z

misc/python/materialize/mzcompose/services/frontegg.py

+            users,
+            "--roles",
+            roles,
+        ]


I don't think you want to be overriding the entrypoint here? Just build up a command instead, and use the default entrypoint. Otherwise you'll lose tini.

benesch · 2023-11-20T22:34:53Z

misc/python/materialize/mzcompose/services/balancerd.py

+        }
+        config: ServiceConfig = {
+            "mzbuild": mzbuild,
+            "entrypoint": entrypoint,


See below comment about entrypoint vs command.

benesch · 2023-11-20T22:35:50Z

misc/images/frontegg/mzbuild.yml

+# the Business Source License, use of this software will be governed
+# by the Apache License, Version 2.0.
+
+name: frontegg


I think calling this frontegg-mock would be less confusing. Also matches the binary name that way!

Suggested change

name: frontegg

name: frontegg-mock

benesch · 2023-11-20T22:36:02Z

misc/python/materialize/mzcompose/services/frontegg.py

+        elif encoding_key_file:
+            entrypoint += ["--encoding-key-file", encoding_key_file]
+        else:
+            raise UIError("frontegg service must specify encoding-key[-file]")


Suggested change

raise UIError("frontegg service must specify encoding-key[-file]")

raise UIError("FronteggMock service must specify encoding-key[-file]")

benesch · 2023-11-20T22:36:52Z

test/test-certs/create-certs.sh

@@ -135,7 +135,7 @@ create_cert() {
        -passout pass:$SSL_SECRET
 }

-for i in materialized producer postgres certuser
+for i in materialized producer postgres certuser balancerd frontegg


Suggested change

for i in materialized producer postgres certuser balancerd frontegg

for i in materialized producer postgres certuser balancerd frontegg-mock

benesch · 2023-11-20T22:37:16Z

misc/python/materialize/mzcompose/services/frontegg.py

+        name: str = "frontegg",
+        mzbuild: str = "frontegg",


Suggested change

name: str = "frontegg",

mzbuild: str = "frontegg",

name: str = "frontegg-mock",

mzbuild: str = "frontegg-mock",

Add a frontegg mock server.

maddyblue · 2023-11-21T06:29:49Z

Renamed everything to frontegg-mock.

def-

Generally seems good to me, thanks for the additional testing! I want to check coverage too (https://buildkite.com/materialize/coverage/builds/321) but ran into some issues, so don't let that be a blocker.

def- · 2023-11-21T18:03:36Z

misc/images/frontegg-mock/Dockerfile

+    && TZ=UTC DEBIAN_FRONTEND=noninteractive apt-get -qy install \
+        ca-certificates \
+        tini \
+    && groupadd --system --gid=999 materialize \


Why do we have to specify gid and uid here?

No idea, I cargo culted from other dockerfiles.

def- · 2023-11-21T18:04:58Z

misc/python/materialize/mzcompose/services/balancerd.py

+        config: ServiceConfig = {
+            "mzbuild": mzbuild,
+            "command": command,
+            "ports": [6875, 6876, 6877, 6878],


MZ_BALANCER_SQL/HTTP_LISTEN_ADDR ports are 6880/6881 though. Do we not want to expose them?
Edit: seen later, those are exposed in environmentd.

test/balancerd/mzcompose.py

maddyblue requested a review from a team as a code owner November 14, 2023 09:37

maddyblue requested a review from ParkMyCar November 14, 2023 09:37

ParkMyCar approved these changes Nov 14, 2023

View reviewed changes

maddyblue requested a review from a team November 15, 2023 01:59

philip-stoev requested review from philip-stoev and removed request for a team November 15, 2023 08:15

philip-stoev reviewed Nov 15, 2023

View reviewed changes

test/balancerd/mzcompose.py Show resolved Hide resolved

philip-stoev approved these changes Nov 15, 2023

View reviewed changes

maddyblue force-pushed the frontegg-mock branch from 002820b to 26572b9 Compare November 17, 2023 06:58

maddyblue force-pushed the frontegg-mock branch 2 times, most recently from db58c40 to dc7a576 Compare November 17, 2023 07:11

maddyblue commented Nov 17, 2023

View reviewed changes

maddyblue mentioned this pull request Nov 20, 2023

adapter: Refactor environmentd test #23275

Merged

5 tasks

maddyblue force-pushed the frontegg-mock branch 3 times, most recently from 6aea2b0 to 1e425c9 Compare November 20, 2023 20:44

benesch reviewed Nov 20, 2023

View reviewed changes

frontegg-mock: initial commit

f98da38

Add a frontegg mock server.

maddyblue force-pushed the frontegg-mock branch from 1e425c9 to f98da38 Compare November 21, 2023 06:29

def- mentioned this pull request Nov 21, 2023

coverage: Add forgotten balancerd binary #23344

Merged

5 tasks

def- approved these changes Nov 21, 2023

View reviewed changes

maddyblue merged commit 623134a into MaterializeInc:main Nov 21, 2023
75 of 76 checks passed

maddyblue deleted the frontegg-mock branch November 21, 2023 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontegg-mock: initial commit #23176

frontegg-mock: initial commit #23176

maddyblue commented Nov 14, 2023

maddyblue commented Nov 14, 2023

ParkMyCar left a comment

ParkMyCar Nov 14, 2023

maddyblue Nov 15, 2023

ParkMyCar Nov 15, 2023

ParkMyCar Nov 14, 2023

maddyblue Nov 15, 2023

ParkMyCar Nov 15, 2023

maddyblue commented Nov 17, 2023

maddyblue Nov 17, 2023

maddyblue Nov 20, 2023

benesch left a comment

benesch Nov 20, 2023

benesch Nov 20, 2023

benesch Nov 20, 2023

benesch Nov 20, 2023

benesch Nov 20, 2023

benesch Nov 20, 2023

benesch Nov 20, 2023

benesch Nov 20, 2023

maddyblue commented Nov 21, 2023

def- left a comment

def- Nov 21, 2023

maddyblue Nov 21, 2023

def- Nov 21, 2023

	raise UIError("frontegg service must specify encoding-key[-file]")
	raise UIError("FronteggMock service must specify encoding-key[-file]")

	for i in materialized producer postgres certuser balancerd frontegg
	for i in materialized producer postgres certuser balancerd frontegg-mock

frontegg-mock: initial commit #23176

frontegg-mock: initial commit #23176

Conversation

maddyblue commented Nov 14, 2023

Motivation

Checklist

maddyblue commented Nov 14, 2023

ParkMyCar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddyblue commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddyblue commented Nov 21, 2023

def- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment