fix: sc should use namespace in k8 mode (#3651)

Co-authored-by: Nick Cardin <[email protected]>
infinyon · Nov 2, 2023 · accd85c · accd85c
1 parent 72afd0b
commit accd85c
Show file tree

Hide file tree

Showing 8 changed files with 71 additions and 60 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -559,6 +559,7 @@ jobs:
       FLUVIO_BIN: ~/bin/fluvio
       TEST_BIN: ~/bin/fluvio-test
       SERVER_LOG: fluvio=info
+      K8_NAMESPACE: fluvio-test
     strategy:
  #     fail-fast: false
       matrix:
@@ -609,18 +610,22 @@ jobs:
         run: |
           curl -s https://raw.githubusercontent.com/rancher/k3d/main/install.sh | TAG=${{ env.K3D_VERSION }} bash
           ./k8-util/cluster/reset-k3d.sh
+      - name: Create k8 namespace
+        if: matrix.run-mode == 'local-k8'
+        run: |
+          kubectl create ns ${K8_NAMESPACE}
       - name: Start fluvio cluster
         timeout-minutes: 10
         if: matrix.test != 'smoke-test-tls'
-        run: fluvio cluster start --${{ matrix.run-mode }} --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }}
+        run: fluvio cluster start --${{ matrix.run-mode }} --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }} --namespace ${K8_NAMESPACE}
       - name: Start fluvio cluster TLS
         if: matrix.test == 'smoke-test-tls'
         timeout-minutes: 10
         # note that Local config doesn't not support auth and scopes in the argument for now
         run: >
           AUTH_POLICY=${{ env.AUTH_FILE }}
           X509_AUTH_SCOPES=${{ env.X509_SCOPE_FILE }}
-          fluvio cluster start --${{ matrix.run-mode }} --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }} ${{ env.TLS_ARGS }}
+          fluvio cluster start --${{ matrix.run-mode }} --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }} --namespace ${K8_NAMESPACE} ${{ env.TLS_ARGS }}
       - name: sleep
         run: sleep 15
       - name: Validate test harness
@@ -734,6 +739,7 @@ jobs:
       TEST_BIN: ~/bin/fluvio-test
       UNINSTALL: noclean
       SERVER_LOG: fluvio=debug
+      K8_NAMESPACE: fluvio-test
     strategy:
  #     fail-fast: true
       matrix:
@@ -811,22 +817,25 @@ jobs:
         run: |
           eval $(minikube -p minikube docker-env)
           docker image load --input /tmp/infinyon-fluvio.tar
+      - name: Create k8 namespace
+        run: |
+          kubectl create ns fluvio-test
       - name: Start fluvio cluster
         if: matrix.test == 'smoke-test-k8' || matrix.test == 'stats-test'
         timeout-minutes: 10
         run: |
-          fluvio cluster start --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }}
+          fluvio cluster start --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }}  --namespace ${K8_NAMESPACE}
       - name: Start fluvio cluster TLS
         timeout-minutes: 10
         if: matrix.test == 'smoke-test-k8-tls'
         run: |
-          fluvio cluster start --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }} ${{ env.TLS_ARGS }}
+          fluvio cluster start --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }} ${{ env.TLS_ARGS }}  --namespace ${K8_NAMESPACE}
       - name: Start fluvio cluster TLS with root
         timeout-minutes: 10
         if: matrix.test == 'smoke-test-k8-tls-root-unclean'
         run: |
           make smoke-test-k8-tls-policy-setup
-          fluvio cluster start --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }} ${{ env.TLS_ARGS }} --authorization-config-map authorization
+          fluvio cluster start --develop --spu ${{ matrix.spu }} --rust-log ${{ env.SERVER_LOG }} ${{ env.TLS_ARGS }}  --namespace ${K8_NAMESPACE} --authorization-config-map authorization
       - name: Run ${{ matrix.test }}
         timeout-minutes: 10
         run: |

diff --git a/crates/fluvio-cluster/src/runtime/local/sc.rs b/crates/fluvio-cluster/src/runtime/local/sc.rs
@@ -48,7 +48,9 @@ impl ScProcess {
             ScMode::ReadOnly(path) => {
                 binary.arg("--read-only").arg(path);
             }
-            ScMode::K8s => {}
+            ScMode::K8s => {
+                binary.arg("--k8");
+            }
         };
 
         if let TlsPolicy::Verified(tls) = &self.tls_policy {

diff --git a/crates/fluvio-sc/src/cli.rs b/crates/fluvio-sc/src/cli.rs
@@ -15,33 +15,27 @@ use std::io::ErrorKind;
 use std::path::PathBuf;
 use std::convert::TryFrom;
 
+use clap::Args;
 use tracing::info;
 use tracing::debug;
 use clap::Parser;
 
-use k8_client::K8Config;
 use fluvio_types::print_cli_err;
 use fluvio_types::defaults::TLS_SERVER_SECRET_NAME;
 use fluvio_future::openssl::TlsAcceptor;
 use fluvio_future::openssl::SslVerifyMode;
 
 use crate::services::auth::basic::BasicRbacPolicy;
-use crate::error::ScError;
 use crate::config::ScConfig;
 
 type Config = (ScConfig, Option<BasicRbacPolicy>);
 
 /// cli options
-#[derive(Debug, Parser, Default)]
+#[derive(Debug, Parser)]
 #[command(name = "sc-server", about = "Streaming Controller")]
 pub struct ScOpt {
-    /// run in local mode
-    #[arg(long, conflicts_with_all = &["k8", "read_only"], value_name = "metadata path")]
-    local: Option<PathBuf>,
-
-    /// run on k8
-    #[arg(long)]
-    k8: bool,
+    #[command(flatten)]
+    run_mode: ScOptRunMode,
 
     #[arg(long)]
     /// Address for external service
@@ -75,9 +69,21 @@ pub struct ScOpt {
     /// only allow white list of controllers
     #[arg(long)]
     white_list: Vec<String>,
+}
+
+#[derive(Debug, Args)]
+#[group(required = true, multiple = false)]
+pub struct ScOptRunMode {
+    /// run in local mode
+    #[arg(long, value_name = "metadata path")]
+    local: Option<PathBuf>,
+
+    /// run on k8
+    #[arg(long)]
+    k8: bool,
 
     /// run SC in read only mode
-    #[arg(long, hide = true, conflicts_with_all = &["auth_policy"])]
+    #[arg(long, hide = true)]
     read_only: Option<PathBuf>,
 }
 
@@ -90,32 +96,18 @@ pub enum RunMode<'a> {
 
 impl ScOpt {
     pub fn mode(&self) -> RunMode<'_> {
-        match (&self.local, &self.read_only, self.k8) {
-            (Some(metadata), _, _) => RunMode::Local(metadata),
-            (_, Some(path), _) => RunMode::ReadOnly(path),
-            _ => RunMode::K8s,
+        match (
+            &self.run_mode.local,
+            &self.run_mode.read_only,
+            self.run_mode.k8,
+        ) {
+            (Some(metadata), None, false) => RunMode::Local(metadata),
+            (None, Some(path), false) => RunMode::ReadOnly(path),
+            (None, None, true) => RunMode::K8s,
+            _ => panic!("Params do not satisfy defined run modes"),
         }
     }
 
-    #[allow(clippy::type_complexity)]
-    fn get_sc_and_k8_config(
-        mut self,
-    ) -> Result<(Config, K8Config, Option<(String, TlsConfig)>), ScError> {
-        let k8_config = K8Config::load().expect("no k8 config founded");
-        info!(?k8_config, "k8 config");
-
-        // if name space is specified, use one from k8 config
-        if self.namespace.is_none() {
-            let k8_namespace = k8_config.namespace().to_owned();
-            info!("using {} as namespace from kubernetes config", k8_namespace);
-            self.namespace = Some(k8_namespace);
-        }
-
-        let (sc_config, tls_option) = self.as_sc_config()?;
-
-        Ok((sc_config, k8_config, tls_option))
-    }
-
     /// as sc configuration, 2nd part of tls configuration(proxy addr, tls config)
     /// 3rd part is path to read only metadata config
     #[allow(clippy::wrong_self_convention)]
@@ -131,9 +123,13 @@ impl ScOpt {
             config.private_endpoint = private_addr;
         }
 
+        if let Some(namespace) = self.namespace {
+            config.namespace = namespace
+        }
+
         config.x509_auth_scopes = self.x509_auth_scopes;
         config.white_list = self.white_list.into_iter().collect();
-        config.read_only_metadata = self.read_only.is_some();
+        config.read_only_metadata = self.run_mode.read_only.is_some();
 
         // Set Configuration Authorization Policy
 
@@ -169,16 +165,6 @@ impl ScOpt {
         }
     }
 
-    pub fn parse_k8s_cli_or_exit(self) -> (Config, K8Config, Option<(String, TlsConfig)>) {
-        match self.get_sc_and_k8_config() {
-            Err(err) => {
-                print_cli_err!(err);
-                process::exit(-1);
-            }
-            Ok(config) => config,
-        }
-    }
-
     pub fn parse_cli_or_exit(self) -> (Config, Option<(String, TlsConfig)>) {
         match self.as_sc_config() {
             Err(err) => {

diff --git a/crates/fluvio-sc/src/config/mod.rs b/crates/fluvio-sc/src/config/mod.rs
@@ -1,8 +1,8 @@
 mod sc_config;
 
 pub use self::sc_config::ScConfig;
-
 pub use self::sc_config::ScConfigBuilder;
+pub use self::sc_config::DEFAULT_NAMESPACE;
 
 macro_rules! whitelist {
     ($config:expr,$name:expr,$start:expr) => {

diff --git a/crates/fluvio-sc/src/config/sc_config.rs b/crates/fluvio-sc/src/config/sc_config.rs
@@ -9,6 +9,8 @@ use std::{io::Error as IoError, path::PathBuf};
 use fluvio_types::defaults::SC_PUBLIC_PORT;
 use fluvio_types::defaults::SC_PRIVATE_PORT;
 
+pub const DEFAULT_NAMESPACE: &str = "default";
+
 // -----------------------------------
 // Traits
 // -----------------------------------
@@ -24,7 +26,6 @@ pub struct ScConfig {
     pub read_only_metadata: bool,
     pub public_endpoint: String,
     pub private_endpoint: String,
-    pub run_k8_dispatchers: bool,
     pub namespace: String,
     pub x509_auth_scopes: Option<PathBuf>,
     pub white_list: HashSet<String>,
@@ -36,8 +37,7 @@ impl ::std::default::Default for ScConfig {
             read_only_metadata: false,
             public_endpoint: format!("0.0.0.0:{SC_PUBLIC_PORT}"),
             private_endpoint: format!("0.0.0.0:{SC_PRIVATE_PORT}"),
-            run_k8_dispatchers: true,
-            namespace: "default".to_owned(),
+            namespace: DEFAULT_NAMESPACE.to_owned(),
             x509_auth_scopes: None,
             white_list: HashSet::new(),
         }

diff --git a/crates/fluvio-sc/src/start.rs b/crates/fluvio-sc/src/start.rs
@@ -16,6 +16,7 @@ use crate::{
     cli::{ScOpt, TlsConfig, RunMode},
     services::auth::basic::BasicRbacPolicy,
     config::ScConfig,
+    config::DEFAULT_NAMESPACE,
 };
 
 pub fn main_loop(opt: ScOpt) {
@@ -47,7 +48,18 @@ pub fn main_loop(opt: ScOpt) {
         RunMode::K8s => {
             info!("Running with K8");
 
-            let ((sc_config, auth_policy), k8_config, tls_option) = opt.parse_k8s_cli_or_exit();
+            let ((mut sc_config, auth_policy), tls_option) = opt.parse_cli_or_exit();
+
+            let k8_config = K8Config::load().expect("no k8 config founded");
+            info!(?k8_config, "k8 config");
+
+            // if namespace is default, use one from k8 config
+            if sc_config.namespace == DEFAULT_NAMESPACE {
+                let k8_namespace = k8_config.namespace().to_owned();
+                info!("using {} as namespace from kubernetes config", k8_namespace);
+                sc_config.namespace = k8_namespace;
+            }
+
             let client = create_k8_client(k8_config).expect("failed to create k8 client");
             k8_main_loop(sc_config, client, auth_policy, tls_option)
         }

diff --git a/k8-util/helm/fluvio-app/templates/sc-deployment.yaml b/k8-util/helm/fluvio-app/templates/sc-deployment.yaml
@@ -32,8 +32,9 @@ spec:
             {{- toYaml .Values.scPod.extraEnv | nindent 12 }}
             {{ end }}
           command: ["/fluvio-run", "sc"]
-        {{ if .Values.tls }}
           args:
+            - --k8
+        {{ if .Values.tls }}
             - --tls
             - --enable-client-cert
             - --ca-cert

diff --git a/makefiles/test.mk b/makefiles/test.mk
@@ -7,6 +7,7 @@ DEFAULT_ITERATION?=1000
 SPU_DELAY?=5
 SC_AUTH_CONFIG?=./crates/fluvio-sc/test-data/auth_config
 EXTRA_ARG?=
+KUBECTL_ARG_NAMESPACE=$(if $(K8_NAMESPACE),-n ${K8_NAMESPACE},)
 
 # Test env
 TEST_ENV_AUTH_POLICY=
@@ -148,15 +149,15 @@ smoke-test-k8-tls: TEST_ARG_TABLE_FORMAT_CONFIG=--table-format-config ./tests/te
 smoke-test-k8-tls: build_k8_image smoke-test
 
 smoke-test-k8-tls-policy-setup:
-	kubectl delete configmap authorization --ignore-not-found
-	kubectl create configmap authorization --from-file=POLICY=${SC_AUTH_CONFIG}/policy.json --from-file=SCOPES=${SC_AUTH_CONFIG}/scopes.json
+	kubectl ${KUBECTL_ARG_NAMESPACE} delete configmap authorization --ignore-not-found
+	kubectl ${KUBECTL_ARG_NAMESPACE} create configmap authorization --from-file=POLICY=${SC_AUTH_CONFIG}/policy.json --from-file=SCOPES=${SC_AUTH_CONFIG}/scopes.json
 smoke-test-k8-tls-policy: TEST_ENV_FLV_SPU_DELAY=FLV_SPU_DELAY=$(SPU_DELAY)
 smoke-test-k8-tls-policy: TEST_ARG_EXTRA=--tls --authorization-config-map authorization $(EXTRA_ARG)
 smoke-test-k8-tls-policy: TEST_ARG_TABLE_FORMAT_CONFIG=--table-format-config ./tests/test-table-format-config.yaml
 smoke-test-k8-tls-policy: build_k8_image smoke-test
 
 test-permission-k8:	SC_HOST=$(shell kubectl get node -o json | jq '.items[].status.addresses[0].address' | tr -d '"' )
-test-permission-k8:	SC_PORT=$(shell kubectl get svc fluvio-sc-public -o json | jq '.spec.ports[0].nodePort' )
+test-permission-k8:	SC_PORT=$(shell kubectl ${KUBECTL_ARG_NAMESPACE} get svc fluvio-sc-public -o json | jq '.spec.ports[0].nodePort' )
 test-permission-k8:	test-permission-user1
 
 # run auth policy without setup, UNCLEAN must not be set