feat: create ClusterConfig CRD #1233

TristanHoladay · 2025-01-27T20:03:10Z

Description

This PR:

adds a ClusterConfig CRD
moves the CRD generation into a manifest component
updates uds-config watcher / logic to use the ClusterConfig

Related Issue

resolves Create ClusterConfig CRD to configure cluster/cloud parameters #1077

Design Doc

https://www.notion.so/ClusterConfig-v1-183e512f24fc80879e7cce7731617039

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Other (security config, docs update, etc)

Steps to Validate

If this PR introduces new functionality to UDS Core or addresses a bug, please document the steps to test the changes.

Checklist before merging

Test, docs, adr added or updated as needed
Contributor Guide followed

…ponent

… back to operator path

…ator path in values

… upgrades

mjnagel

Still need to go through the config functions more and test out a few deploy scenarios but wanted to get this initial round of feedback out. The approach overall looks sound and aligns with the design so no overall concerns - just will want to validate there's no timing or upgrade issues with the new CRD approaches.

mjnagel · 2025-02-10T23:12:09Z

src/pepr/operator/controllers/config/config.ts

+        configLog.error(
+          "Invalid CA Cert provided in uds-operator-config ClusterConfig, falling back to no CA Cert",
        );


This might be a good one to add an event on the cluster config CR since we fallback to something and they might get unexpected behavior if they just made a typo or something.

mjnagel · 2025-02-10T23:16:25Z

src/pepr/operator/controllers/config/config.ts

+    const cfgSecretList = await K8s(kind.Secret).InNamespace("pepr-system").Get();
+
+    const cfgSecret = cfgSecretList.items.find(s => s.metadata?.name === "uds-operator-config");


I might be missing intent here but .Get() supports getting a secret by name, i.e. this could just be one line:

Suggested change

const cfgSecretList = await K8s(kind.Secret).InNamespace("pepr-system").Get();

const cfgSecret = cfgSecretList.items.find(s => s.metadata?.name === "uds-operator-config");

const cfgSecret = await K8s(kind.Secret).InNamespace("pepr-system").Get("uds-operator-config");

This also could be used for the cluster config, but I see that you're also using the full get to error if multiple cluster configs are found so maybe that one makes sense as is?

mjnagel · 2025-02-10T23:22:55Z

src/pepr/operator/crd/register.ts

@@ -82,4 +83,39 @@ export async function registerCRDs() {
        process.exit(1);
      });
  }
+
+  // Register the Exemption CRD if we're in "admission" or dev mode (Exemptions are watched by the admission controllers)


Comment needs updating to reflect the right CRD, but also probably should update the conditional to run regardless of PEPR_WATCH_MODE? (although this isn't actually in the execution code anymore)

mjnagel · 2025-02-10T23:24:02Z

src/pepr/operator/crd/sources/cluster-config/v1alpha1.ts

+                    pattern:
+                      "^(([0-9]{1,3}\\.){3}[0-9]{1,3}\\/[0-9]+)|(([a-fA-F0-9:]+:+)+[a-fA-F0-9:]+(/[0-9]+)?)$", // Matches IPv4 and IPv6 CIDR


Assuming this regex works, can we use the same on the kubeapiCIDR?

It might also be nice to have tests against this regex in our code somewhere - I think @UnicornChance briefly looked at this for the remoteCidr stuff and we ended up just leaving the validation to k8s because we couldn't find a succinct and solid approach to validating both ipv4 and ipv6.

mjnagel · 2025-02-10T23:26:01Z

src/pepr/operator/crd/sources/cluster-config/v1alpha1.ts

+                adminDomain: {
+                  type: "string",
+                  description:
+                    "Domain all cluster services on the admin gateawy will be exposed on",


Suggested change

"Domain all cluster services on the admin gateawy will be exposed on",

"Domain all cluster services on the admin gateway will be exposed on",

mjnagel · 2025-02-10T23:36:12Z

src/pepr/uds-operator-config/values.yaml

+cluster:
+  attributes:
+    UDS_CLUSTER_NAME: "###ZARF_VAR_CLUSTER_NAME###"
+    UDS_CLUSTER_TAGS: "###ZARF_VAR_CLUSTER_TAGS###"
+  expose:
+    # Domain configuration (admin defaults to `admin.UDS_DOMAIN`)
+    UDS_DOMAIN: "###ZARF_VAR_DOMAIN###"
+    UDS_ADMIN_DOMAIN: "###ZARF_VAR_ADMIN_DOMAIN###"
+    UDS_CA_CERT: "###ZARF_VAR_CA_CERT###"
+  policy:
+    UDS_ALLOW_ALL_NS_EXEMPTIONS: "###ZARF_VAR_ALLOW_ALL_NS_EXEMPTIONS###"
+  networking:
+    KUBEAPI_CIDR: ""
+    KUBENODE_CIDRS: []


imo all of these can follow better values naming standards. As long as we are maintaining parity on the zarf vars, these are net new keys so we can name them better. For example:

Suggested change

cluster:

attributes:

UDS_CLUSTER_NAME: "###ZARF_VAR_CLUSTER_NAME###"

UDS_CLUSTER_TAGS: "###ZARF_VAR_CLUSTER_TAGS###"

expose:

# Domain configuration (admin defaults to `admin.UDS_DOMAIN`)

UDS_DOMAIN: "###ZARF_VAR_DOMAIN###"

UDS_ADMIN_DOMAIN: "###ZARF_VAR_ADMIN_DOMAIN###"

UDS_CA_CERT: "###ZARF_VAR_CA_CERT###"

policy:

UDS_ALLOW_ALL_NS_EXEMPTIONS: "###ZARF_VAR_ALLOW_ALL_NS_EXEMPTIONS###"

networking:

KUBEAPI_CIDR: ""

KUBENODE_CIDRS: []

cluster:

attributes:

clusterName: "###ZARF_VAR_CLUSTER_NAME###"

clusterTags: "###ZARF_VAR_CLUSTER_TAGS###"

expose:

# Domain configuration (admin defaults to `admin.<domain>`)

domain: "###ZARF_VAR_DOMAIN###"

adminDomain: "###ZARF_VAR_ADMIN_DOMAIN###"

caCert: "###ZARF_VAR_CA_CERT###"

policy:

allowAllNsExemptions: "###ZARF_VAR_ALLOW_ALL_NS_EXEMPTIONS###"

networking:

kubeApiCidrs: ""

kubeNodeCidrs: []

For context the other keys were all caps + _ to match with env standards since they were directly mounted as env by pepr. Since we're no longer doing that it would be good to align with helm norms of camelcase here.

mjnagel · 2025-02-10T23:37:51Z

src/pepr/uds-operator-config/values.yaml

+    UDS_CLUSTER_NAME: "###ZARF_VAR_CLUSTER_NAME###"
+    UDS_CLUSTER_TAGS: "###ZARF_VAR_CLUSTER_TAGS###"


For cluster name and tags imo we don't need to expose a zarf var for these, unless there's a specific UI use case I'm missing here - they could just be set as overrides?

Specific to tags, shouldn't that be a list rather than a string (maybe this is string because of zarf var right now though)? It could also be useful to have a values schema for this chart now that we're adding more complexity to it.

On the tag comment I think that would also help clean up the helm template for tags if switched to a proper list.

mjnagel · 2025-02-10T23:42:02Z

src/pepr/uds-operator-config/templates/clusterconfig.yaml

+{{- range $index, $cidr := .Values.operator.KUBENODE_CIDRS }}
+      - {{ $cidr | quote }}
+{{- end }}


I'll probably have to test this out locally so putting a placeholder comment for now but this templating looks off to me. I'd expect that we need to split on , here and range over that (example of this). By default I think range here will either glob things together or it might range over each character?

mjnagel · 2025-02-10T23:47:45Z

src/pepr/zarf.yaml

+              # Label the specific CRDs
+              ./zarf tools kubectl label $(./zarf tools kubectl get crds | awk '/exemptions.uds.dev|packages.uds.dev/ {print "crd/" $1}') "app.kubernetes.io/managed-by=Helm" --overwrite || true
+
+              # Annotate with release name
+              ./zarf tools kubectl annotate $(./zarf tools kubectl get crds | awk '/exemptions.uds.dev|packages.uds.dev/ {print "crd/" $1}') "meta.helm.sh/release-name=uds-cluster-crds" --overwrite || true
+
+              # Annotate with release namespace
+              ./zarf tools kubectl annotate $(./zarf tools kubectl get crds | awk '/exemptions.uds.dev|packages.uds.dev/ {print "crd/" $1}') "meta.helm.sh/release-namespace=pepr-system" --overwrite || true


This feels overly complex, we should be able to just directly label/annotate two CRDs in a single kubectl call without needing a get/awk in there?

Suggested change

# Label the specific CRDs

./zarf tools kubectl label $(./zarf tools kubectl get crds | awk '/exemptions.uds.dev|packages.uds.dev/ {print "crd/" $1}') "app.kubernetes.io/managed-by=Helm" --overwrite || true

# Annotate with release name

./zarf tools kubectl annotate $(./zarf tools kubectl get crds | awk '/exemptions.uds.dev|packages.uds.dev/ {print "crd/" $1}') "meta.helm.sh/release-name=uds-cluster-crds" --overwrite || true

# Annotate with release namespace

./zarf tools kubectl annotate $(./zarf tools kubectl get crds | awk '/exemptions.uds.dev|packages.uds.dev/ {print "crd/" $1}') "meta.helm.sh/release-namespace=pepr-system" --overwrite || true

# Label the specific CRDs

./zarf tools kubectl label crd exemptions.uds.dev packages.uds.dev "app.kubernetes.io/managed-by=Helm" --overwrite || true

# Annotate with release name

./zarf tools kubectl annotate crd exemptions.uds.dev packages.uds.dev "meta.helm.sh/release-name=uds-cluster-crds" --overwrite || true

# Annotate with release namespace

./zarf tools kubectl annotate crd exemptions.uds.dev packages.uds.dev "meta.helm.sh/release-namespace=pepr-system" --overwrite || true

I think the resulting command ends up being the same unless someone had only one of the CRs on their cluster - but the net result should be the same always due to the || true fallback.

mjnagel · 2025-02-10T23:51:39Z

src/pepr/uds-cluster-crds/Chart.yaml

+# This is the chart version. This version number should be incremented each time you make changes
+# to the chart and its templates, including the app version.
+# Versions are expected to follow Semantic Versioning (https://semver.org/)
+version: 0.1.0


Placeholder note mostly for myself - I've encountered issues in the past when charts are updated without a version bump. I think that zarf will upgrade/apply regardless but we may want to validate to ensure that we have the ability to upgrade CRDs easily here without versioning this chart. If not we may want to automate the version bump or ensure there's clear dev notes on how to.

TristanHoladay added 20 commits January 27, 2025 13:01

feat: create ClusterConfig CRD

3f8e351

remove registerCRDS() from pepr.ts and update zarf yamls with new com…

29dffca

…ponent

fix manifests path

7f70461

add ClusterConfig CRD

7f8204e

schema changes; config values and template

9e25add

fix templating

bce89d4

add schema descriptions; remove log level from cluster config and add…

5b0e3fb

… back to operator path

re-gen crds and manifests and export types

ad4e82e

wip

a9a34a0

add headers

a6a567f

add headers

4ddd0e4

add headers

3970383

wip

96a435c

test updates

e624303

headers

f53d8df

remove .only()s

912f38c

fix admission vs watcher bug

05f1628

remove envFrom; move log level to env on main pepr chart; revert oper…

6e3a7fb

…ator path in values

update handlers; test fixes

c2ccddd

validators

c955f49

TristanHoladay marked this pull request as ready for review February 4, 2025 23:06

TristanHoladay requested a review from a team as a code owner February 4, 2025 23:06

TristanHoladay added 8 commits February 4, 2025 16:11

Merge branch 'main' into cluster-config

50dfe9a

fix arg value

ae531d9

wrap crd manifests in chart to control release name and namespace for…

50c0dea

… upgrades

fix base64 verification and add unit tests

3541b56

unit tests for loadUDSConfig

c6e7a8a

refactors

ffbd8f8

fixed args err

7914c27

adding docs

32828c7

mjnagel reviewed Feb 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create ClusterConfig CRD #1233

feat: create ClusterConfig CRD #1233

TristanHoladay commented Jan 27, 2025 •

edited

Loading

mjnagel left a comment

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

mjnagel Feb 10, 2025

		const cfgSecretList = await K8s(kind.Secret).InNamespace("pepr-system").Get();

		const cfgSecret = cfgSecretList.items.find(s => s.metadata?.name === "uds-operator-config");

		pattern:
		"^(([0-9]{1,3}\\.){3}[0-9]{1,3}\\/[0-9]+)\|(([a-fA-F0-9:]+:+)+[a-fA-F0-9:]+(/[0-9]+)?)$", // Matches IPv4 and IPv6 CIDR

	"Domain all cluster services on the admin gateawy will be exposed on",
	"Domain all cluster services on the admin gateway will be exposed on",

		UDS_CLUSTER_NAME: "###ZARF_VAR_CLUSTER_NAME###"
		UDS_CLUSTER_TAGS: "###ZARF_VAR_CLUSTER_TAGS###"

feat: create ClusterConfig CRD #1233

Are you sure you want to change the base?

feat: create ClusterConfig CRD #1233

Conversation

TristanHoladay commented Jan 27, 2025 • edited Loading

Description

Related Issue

Design Doc

Type of change

Steps to Validate

Checklist before merging

mjnagel 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

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

TristanHoladay commented Jan 27, 2025 •

edited

Loading