FEATURE: Add `config auth_group` command #921

namsic · 2025-10-22T13:02:36Z

🔗 Related Issue

jam2in/arcus-works#791

⌨️ What I did

config auth_group 명령을 통해 acl group을 조회/변경할 수 있도록 합니다.
변경 대상 acl group에 현재 acl group의 정보가 모두 포함된 경우에만 변경할 수 있습니다.
변경 요청은 스레드 생성 후 END 응답합니다. (실제 변경 작업의 성공/실패 여부와 무관)

jhpark816 · 2025-10-22T23:53:45Z

@ing-eoking
리뷰 진행해 주세요

sasl_auxprop.c

memcached.c

sasl_auxprop.c

ing-eoking · 2025-10-23T01:08:27Z

sasl_auxprop.c

+static const char *zk_root = "/arcus_acl";
+static const char *g_ensemble_list;
+static char g_group_buf1[GROUP_MAXLEN];
+static char g_group_buf2[GROUP_MAXLEN];


g_group_buf가 2개가 필요한가요?

group을 변경하거나 조회할 때 메모리를 할당하거나 해제하는 구현을 단순화하기 위해 두었습니다.

memcached.c

sasl_defs.c

namsic

현재 PR이 메모리 관리나 mutex 구조 등에서 상당히 복잡하고 직관적이지 않은 편이고,
PR 구현을 무시하고 완전히 새로운 구현을 제시해 주어도 됩니다.

memcached.c

namsic · 2025-10-23T02:06:57Z

sasl_auxprop.c

+static const char *zk_root = "/arcus_acl";
+static const char *g_ensemble_list;
+static char g_group_buf1[GROUP_MAXLEN];
+static char g_group_buf2[GROUP_MAXLEN];


group을 변경하거나 조회할 때 메모리를 할당하거나 해제하는 구현을 단순화하기 위해 두었습니다.

sasl_auxprop.c

jhpark816

일부 리뷰

jhpark816 · 2025-10-23T01:49:40Z

sasl_auxprop.h

 void arcus_auxprop_wakeup(void);

+const char *auxprop_get_acl_group(void);
+int auxprop_set_acl_group(const char *group_name);


어떤 용어가 가장 나은가요?

acl group

auth group

user group

jhpark816 · 2025-10-23T02:32:00Z

memcached.c

+            out_string(c, "SERVER_ERROR out of memory");
+        } else {
+            pthread_t tid;
+            int ret = pthread_create(&tid, NULL, sasl_set_auth_group, (void*)config);


sasl_set_auth_group() 호출할 thread main loop를 따로 두세요.

init_sasl_thread() 참고하세요.
thread main loop는 memcached.c에 있어야 합니다.

memcached.c

jhpark816 · 2025-10-23T02:36:47Z

sasl_auxprop.c

        return NULL;
    }

+    snprintf(group_zpath, sizeof(group_zpath), "%s/%s", zk_root, group_name);


group_zpath를 매번 만들지 않고,
group_name이 변경되면 1회만 만들면 됩니다.

ing-eoking · 2025-10-23T04:57:50Z

개인적으로 table을 교체하는 로직은 acl_refresh_thread에서 처리하면 좋을 것 같습니다.

namsic · 2025-10-23T06:11:19Z

구현 변경 제안

by @ing-eoking

config 변경 명령은 group name 변경 후 refresh 요청하는 형태로 구현
- old table과 비교하는 로직도 refresh 스레드가 수행
- group name 변경 후 refresh 요청했는데 이미 진행 중인 refresh 작업이 있으면,
  다음 refresh 주기까지 반영 안 됨

group name 조회 요청 시, strdup으로 메모리 할당하여 반환

memcached.c에서 응답 반환 후 free 해야 함

if (ntokens == 3) {
  char buf[50];
  char *group_name = sasl_get_auth_group();
  sprintf(buf, "auth_group %s\r\nEND", group_name);
  out_string(c, buf);
  free(group_name);

또는, memcached.c 에서 버퍼 전달

if (ntokens == 3) {
  char buf[50];
  int len = sprintf(buf, "auth group ");
  len += sasl_get_auth_group(buf + len, 50 - len - 5);
  sprintf(buf + len, "\r\nEND");
  out_string(c, buf);
}

jhpark816 · 2025-10-27T01:46:17Z

@namsic
어떤 상태인가요?

namsic · 2025-10-27T01:47:38Z

어떤 상태인가요?

리뷰 의견에 따라 구현을 정리해 둔 상태이고, 테스트 및 최종 검토 후 리뷰 재요청하겠습니다.

namsic

아래 변경이 적용되었습니다.

acl table 비교 및 group 변경 작업을 acl refresh thread가 수행
- auth group 변경 명령은 new group name 설정하고 reload 요청
old table을 다시 조회하지 않고, 기존 g_sasltable과 비교
- 락을 잡은 상태로 수행
auth group 조회 시 strdup 하여 반환
- 호출한 측(memcached.c)에서 free 해야 함

어떤 용어가 가장 나은가요?

acl group

auth group

user group

사용자 인터페이스로는 auth group이 가장 나아 보입니다.
내부 구현 측면에서 sasl_defs와 sasl_auxprop의 인터페이스를 구분해야 하는데,
현재 PR은 아래와 같이 하고 있습니다.

sasl_defs에서: sasl_get_auth_group
sasl_auxprop에서: arcus_auxprop_get_group

sasl_auxprop.c

ing-eoking · 2025-10-27T04:59:36Z

memcached.c

+    if (ntokens == 3) {
+        char buf[50];
+        char *group_name = sasl_get_auth_group();
+        if (!group_name) {


아래 로직과 같이 성공 -> 에러 순으로 배치하는 편이 좋을 것 같습니다.

jhpark816

일부 리뷰하였습니다.

반영하지 않을 PR이라도
버그나 수정해야 할 사항이라면 반영해 두는 것이 좋겠습니다.

jhpark816 · 2025-10-27T06:36:29Z

sasl_auxprop.c

+                mc_logger->log(EXTENSION_LOG_WARNING, NULL,
+                    "SECURITY_EVENT ACL group change to %s failed: missing key %s\n",
+                    new_group_name, old_entry->key);
+                pthread_mutex_unlock(&g_sasltable_lock);


제거해야 합니다.

jhpark816 · 2025-10-27T06:46:46Z

sasl_auxprop.c

+    snprintf(g_group_zpath, sizeof(g_group_zpath), "%s", new_group_zpath);
+    pthread_mutex_unlock(&g_sasltable_lock);
+
+    _table_free(old_table);


is_valid 변수를 두는 아래 형태의 코드가 읽기에 나을 것 같습니다.

char new_key[TABLE_KEY_LEN]; struct sasl_entry *new_entry; bool is_valid = true; for (int i = 0; i < SASL_TABLE_SIZE; i++) { struct sasl_entry *old_entry = g_sasltable[i]; while (old_entry) { snprintf(new_key, sizeof(new_key), "%s/%s", new_group_zpath, old_entry->key + origin_group_zpath_len); new_entry = new_table[hash_function(new_key) % SASL_TABLE_SIZE]; while (new_entry) { if (strcmp(new_entry->key, new_key) == 0 && new_entry->value_len == old_entry->value_len && memcmp(new_entry->value, old_entry->value, old_entry->value_len) == 0) { break; } new_entry = new_entry->next; } if (new_entry == NULL) { // new_entry not found is_valid = false; break; } old_entry = old_entry->next; } } if (is_valid) { } else { }

jhpark816 · 2025-10-27T06:50:10Z

sasl_auxprop.c

    while (!acl_thread_stopreq) {
        ts.tv_sec += REFRESH_PERIOD;
        pthread_mutex_lock(&acl_thread_lock);
        if (!acl_thread_stopreq) {


조건 변경
(!acl_thread_stopreq && !change_acl_group)

jhpark816 · 2025-10-27T07:40:55Z

sasl_auxprop.c

+                mc_logger->log(EXTENSION_LOG_INFO, NULL, "SECURITY_EVENT ACL group changed to: %s\n", g_group_name);
+                continue;
+            }
+        }


acl_thread_stopreq 변수와 마찬가지로 volatile 변수로 두고,
한번 true 설정되면 다른 스레드에 의해 false로 변경되지 않으므로 아래와 같이 사용할 수 있을 것 같습니다.

if (change_acl_group) { int ret = validate_new_acl_group(); if (ret != 0) { continue; } }

최신의 acl_table과 compare하여 valid 여부를 판단해야 하므로,
아래 코드를 먼저 수행하여 기존 acl_table을 reload하고 나서,
acl group 변경 여부를 판단하는 것이 맞지 않나 생각합니다.

jhpark816

일부 리뷰

jhpark816 · 2025-10-28T12:06:46Z

sasl_auxprop.c

+                    memcmp(new_entry->value, old_entry->value, old_entry->value_len) == 0) {
+                    mc_logger->log(EXTENSION_LOG_WARNING, NULL,
+                        "SECURITY_EVENT ACL group change to %s failed: missing key %s\n",
+                        new_group_name, old_entry->key);


missing key %s 이런 출력이 초기 PR에 있었나요?

if 문 안에서는 old_entry에 해당하는 new_entry를 찾은 상태이므로, 로그 출력하면 안 됩니다.

missing key %s 이런 출력이 초기 PR에 있었나요?

acl group 변경은 모두 로그로 남긴다는 설계에 따라 초기 PR에서도 실패 시 동일한 로그를 기록하고 있었습니다.

jhpark816 · 2025-10-28T12:10:38Z

sasl_auxprop.c

+        pthread_mutex_unlock(&g_sasltable_lock);
+        _table_free(old_table);
+        return 0;
+    } else {


이 위치에 로그 출력이 오면 좋겠습니다.

mc_logger->log(EXTENSION_LOG_WARNING, NULL, "SECURITY_EVENT ACL group change to %s failed: missing key %s\n", new_group_name, old_entry->key);

jhpark816 · 2025-10-28T12:11:15Z

sasl_auxprop.c

+            }
+            if (new_entry == NULL) {
+              is_valid = false;
+              break;


char *missing_key 변수를 따로 두고, 여기서 설정하면 될 것 같습니다.
아니면, 이 위치에서 로그 출력해야 하고요.

missing key는 따로 기록하지 않고, 단순 실패 사실만 로그로 남기는 것으로 하겠습니다.

jhpark816 · 2025-10-28T12:14:50Z

sasl_auxprop.c

+            if (ret == 0) {
+                mc_logger->log(EXTENSION_LOG_INFO, NULL, "SECURITY_EVENT ACL group changed to: %s\n", g_group_name);
+            }
+        }


아래에 있는 로그 출력이 위로 올라가야 할 것 같습니다.

jhpark816 · 2025-10-28T12:32:39Z

sasl_auxprop.c


    acl_thread_running = true;
    while (!acl_thread_stopreq) {
        ts.tv_sec += REFRESH_PERIOD;


기존 코드에 대한 코멘트입니다.

두 번째 loop 부터는 ts 값을 다시 구해야 하지 않는 지 ?

초기에는 init 시점에 랜덤으로 구한 오프셋을 기준으로 매일 같은 시간에 refresh 시작되는 것을 의도했는데,

reload auth 또는 config auth_group처럼 임의 시점에 refresh하는 로직이 추가되면서 맞지 않게 되었습니다.
매 loop마다 ts 값을 다시 계산하는 형태로 수정하겠습니다.

@namsic
아래에 대한 수정은 별도 PR로 올려주세요.
auth group 변경 기능은 반영하지 않을 예정이고,
아래 수정은 기존 코드에 대한 수정이기 때문입니다.

clock_gettime(CLOCK_REALTIME, &ts);

그리고, loop 들어오지 전에 random 방식의 tv_sec 설정하는 의미도 살려야 되지 않는 지 ?

clock_gettime(CLOCK_REALTIME, &ts); srand(ts.tv_sec); ts.tv_sec += rand() % REFRESH_PERIOD;

그리고, loop 들어오지 전에 random 방식의 tv_sec 설정하는 의미도 살려야 되지 않는 지 ?

기존에는 초기 구동 시 random offset을 주어 시점을 흩어놓고 그 뒤로는 고정 주기(REFRESH_PERIOD)마다 값을 갱신하는 형태였고,
다른 조작이 없으면 하루 한 번 조회 작업을 수행합니다.

매 loop마다 random offset 설정하도록 하면 (reload 작업 여부와 무관하게) REFRESH_PERIOD 24h 값의 의미가 아래와 같이 달라집니다.

갱신 주기는 매 번 랜덤이며, 최대 24H 이내에 갱신 보장

확률에 따라 24H 동안 N번 갱신 작업 수행

매 번 rand 하는 것으로 변경할까요?

기존 동작은 아래와 같았습니다.

첫 번째 wait duration : (rand() % REFRESH_PERIOD) + REFRESH_PERIOD

그 이후 wait duration : REFRESH_PERIOD

어떤 코멘트 없이 아래와 같이 변경되어서
(rand() % REFRESH_PERIOD) 코드가 있는 것을 확인하였는 지를 코멘트로 물은 것입니다.
혹시 실수로 확인하지 못했을까 봐 물은 것입니다.

모든 wait duration : REFRESH_PERIOD

기존 동작대로 해도 되고, 매번 rand 해도 됩니다.
특정 의도로 구현한 부분을 변경한다면, 이렇게 변경했다는 코멘트만 남겨두면 됩니다.

참고로, REFRESH_PERIOD를 12시간으로 줄이고, 아래 방식으로 매번 rand하면 적당해 보입니다.

(rand() % REFRESH_PERIOD) + REFRESH_PERIOD

매번 rand 방식으로 할 경우,
어떤 rand 방식으로 하면 적당한 지도 스스로 결정할 수 있다고 봅니다.

jhpark816 · 2025-10-28T12:42:44Z

sasl_auxprop.c

+            change_acl_group = false;
+            pthread_mutex_unlock(&acl_thread_lock);
+            if (ret == 0) {
+                mc_logger->log(EXTENSION_LOG_INFO, NULL, "SECURITY_EVENT ACL group changed to: %s\n", g_group_name);


로그 출력을 validate_new_acl_group() 안에 넣는 것이 어떤지 ?

jhpark816 · 2025-10-28T12:46:54Z

sasl_auxprop.c

 #define PROPNAME_MAXLEN 32
+// 16: buffer for "/arcus_acl", "/" separator, "\0" terminator, etc.
+#define TABLE_KEY_LEN 16 + GROUP_MAXLEN + USERNAME_MAXLEN + PROPNAME_MAXLEN
 #define VALUE_MAXLEN 8192 /* from Cyrus SASL's sasldb auxprop plugin */


아래 용어가 어떤지?

SASL_KEY_MAXLEN

SASL_VALUE_MAXLEN

jhpark816 requested a review from ing-eoking October 22, 2025 23:53

ing-eoking reviewed Oct 23, 2025

View reviewed changes

namsic commented Oct 23, 2025

View reviewed changes

jhpark816 reviewed Oct 23, 2025

View reviewed changes

namsic force-pushed the config_auth_group branch from b4b1af2 to 57a0af5 Compare October 26, 2025 10:34

namsic force-pushed the config_auth_group branch from 57a0af5 to 3b6d1f5 Compare October 27, 2025 02:42

namsic commented Oct 27, 2025

View reviewed changes

namsic marked this pull request as ready for review October 27, 2025 02:45

namsic requested review from ing-eoking and jhpark816 October 27, 2025 02:45

jhpark816 reviewed Oct 27, 2025

View reviewed changes

sasl_auxprop.c Show resolved Hide resolved

namsic force-pushed the config_auth_group branch from 3b6d1f5 to dfd6eba Compare October 27, 2025 06:01

ing-eoking reviewed Oct 27, 2025

View reviewed changes

jhpark816 reviewed Oct 27, 2025

View reviewed changes

namsic force-pushed the config_auth_group branch from dfd6eba to d4cb819 Compare October 28, 2025 11:53

jhpark816 reviewed Oct 28, 2025

View reviewed changes

namsic force-pushed the config_auth_group branch from d4cb819 to 8a0fbb4 Compare October 29, 2025 02:15

namsic mentioned this pull request Oct 29, 2025

INTERNAL: Randomize ACL refresh schedule and reduce period to 12 hours #925

Merged

FEATURE: Add config auth_group command

36f02b1

namsic force-pushed the config_auth_group branch from 8a0fbb4 to 36f02b1 Compare October 30, 2025 10:17

FEATURE: Add config auth_group command #921

Are you sure you want to change the base?

FEATURE: Add config auth_group command #921

Uh oh!

Conversation

namsic commented Oct 22, 2025

🔗 Related Issue

⌨️ What I did

Uh oh!

jhpark816 commented Oct 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

namsic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ing-eoking commented Oct 23, 2025

Uh oh!

namsic commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

구현 변경 제안

Uh oh!

jhpark816 commented Oct 27, 2025

Uh oh!

namsic commented Oct 27, 2025

Uh oh!

namsic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FEATURE: Add `config auth_group` command #921

FEATURE: Add `config auth_group` command #921

namsic commented Oct 23, 2025 •

edited

Loading