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

workerauth: satisfy NodeIdLoader interface #4870

Merged
merged 5 commits into from
Jun 6, 2024
Merged

Conversation

irenarindos
Copy link
Collaborator

Note: tests will fail until the corresponding nodeenrollment changes are merged

Changes to the worker auth repository:

  • Update LodeNodeInformation to correctly populate previous nodeInformation
  • Satisfy the NodeIdLoader interface with LoadByNodeId
  • Update store so that if worker NodeInformation has a previous key set, ensure that the control plane's view of what is current and previous matches
  • Add test for "split brain" scenario

Also added a database trigger to ensure we don't store multiple current or previous records for a worker

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

Really cool, thank you!

@irenarindos irenarindos added this to the 0.17.x milestone Jun 4, 2024
@irenarindos irenarindos marked this pull request as ready for review June 4, 2024 14:05
Copy link

github-actions bot commented Jun 4, 2024

Database schema diff between main and irindos-load-by-nodeid @ c207ad4

To understand how these diffs are generated and some limitations see the
documentation of the script.

Functions

diff --git a/.schema-diff/funcs_ea9f4b717939eb7d2dc352eeec7eb58a7408e1fe/update_worker_auth_authorized.sql b/.schema-diff/funcs_ea9f4b717939eb7d2dc352eeec7eb58a7408e1fe/update_worker_auth_authorized.sql
new file mode 100644
index 000000000..38f17faae
--- /dev/null
+++ b/.schema-diff/funcs_ea9f4b717939eb7d2dc352eeec7eb58a7408e1fe/update_worker_auth_authorized.sql
@@ -0,0 +1,58 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.15
+-- dumped by pg_dump version 14.12 (ubuntu 14.12-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+--
+-- name: update_worker_auth_authorized(); type: function; schema: public; owner: -
+--
+
+create function public.update_worker_auth_authorized() returns trigger
+    language plpgsql
+    as $$
+begin
+    if new.state = 'current' then
+        perform
+        from worker_auth_authorized
+          where state = 'current' and worker_id = new.worker_id and worker_key_identifier != new.worker_key_identifier;
+        if found then
+          raise 'current worker auth already exists; cannot set %s to current', new.worker_key_identifier;
+        end if;
+    end if;
+    if new.state = 'previous' then
+        perform
+        from worker_auth_authorized
+          where state = 'previous' and worker_id = new.worker_id and worker_key_identifier != new.worker_key_identifier;
+        if found then
+          raise 'previous worker auth already exists; cannot set %s to previous', new.worker_key_identifier;
+        end if;
+    end if;
+    return new;
+end;
+$$;
+
+
+--
+-- name: function update_worker_auth_authorized(); type: comment; schema: public; owner: -
+--
+
+comment on function public.update_worker_auth_authorized() is 'update_worker_auth_authorized is a before update trigger function for the worker_auth_authorized table.';
+
+
+--
+-- postgresql database dump complete
+--
+

Tables

Unchanged

Views

Unchanged

Triggers

diff --git a/.schema-diff/triggers_ea9f4b717939eb7d2dc352eeec7eb58a7408e1fe/worker_auth_authorized update_worker_auth_authorized.sql b/.schema-diff/triggers_ea9f4b717939eb7d2dc352eeec7eb58a7408e1fe/worker_auth_authorized update_worker_auth_authorized.sql
new file mode 100644
index 000000000..a955169ef
--- /dev/null
+++ b/.schema-diff/triggers_ea9f4b717939eb7d2dc352eeec7eb58a7408e1fe/worker_auth_authorized update_worker_auth_authorized.sql	
@@ -0,0 +1,29 @@
+--
+-- postgresql database dump
+--
+
+-- dumped from database version 13.15
+-- dumped by pg_dump version 14.12 (ubuntu 14.12-1.pgdg22.04+1)
+
+set statement_timeout = 0;
+set lock_timeout = 0;
+set idle_in_transaction_session_timeout = 0;
+set client_encoding = 'utf8';
+set standard_conforming_strings = on;
+select pg_catalog.set_config('search_path', '', false);
+set check_function_bodies = false;
+set xmloption = content;
+set client_min_messages = warning;
+set row_security = off;
+
+--
+-- name: worker_auth_authorized update_worker_auth_authorized; type: trigger; schema: public; owner: -
+--
+
+create trigger update_worker_auth_authorized before update on public.worker_auth_authorized for each row execute function public.update_worker_auth_authorized();
+
+
+--
+-- postgresql database dump complete
+--
+

Indexes

Unchanged

Constraints

Unchanged

Foreign Key Constraints

Unchanged

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

🤞🏻 would love to have Jeffs review on this as well but LGTM!

@irenarindos
Copy link
Collaborator Author

Adding @tmessi and @mgaffney to get an eye on the DB changes. Thanks!

internal/server/query.go Outdated Show resolved Hide resolved
internal/server/repository_workerauth.go Show resolved Hide resolved
@irenarindos irenarindos requested a review from tmessi June 6, 2024 14:51
@irenarindos irenarindos merged commit 08f391c into main Jun 6, 2024
61 of 62 checks passed
@irenarindos irenarindos deleted the irindos-load-by-nodeid branch June 6, 2024 17:03
@irenarindos irenarindos modified the milestones: 0.17.x, 0.16.x Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants