feat!: fully support project CRD #1388

krancour · 2024-01-11T22:42:20Z

Fixes #558
Fixes #667

Summary

This PR relieves the API server of responsibility (and permissions) for creating specially labeled namespaces that we count as "projects."

Project creation via API server now entails creation of a Project resource. The controller reconciles new Project resources by creating the corresponding namespace. Project resources are set as the owner of the associated namespace, so Project deletion takes the associated namespace with it. The reverse is also true. Namespaces have a finalizer that, upon deletion, is only cleared by the controller deleting the associated Project resource.

Follow-up PRs will add to this to create things like ServiceAccounts, Roles, and RoleBindings in the project namespaces.

Breakdown

About 2/3 of this is generated code, so please don't be intimidated by the size.

Narrowly scoped commits cover each of the following:

CRD changes: Add Project status field with phase and message (for communicating reconciliation errors)
Corresponding protobuf changes: Mirror the CRD changes
Run codegen
Corresponding type conversion changes
Update API server endpoints to operate on Project resources instead of operating directly on namespaces
Refactor "apply" endpoints on API server: Necessary so the API server is smart enough to pick Projects out of manifests and apply them first, just as would normally happen with a namespace, since a Project is a proxy for a namespace
Corresponding CLI changes: Align with the protobuf changes
New webhook: Validate new Project resources by checking the a namespace with the same name doesn't already exist
Controller changes: Reconcile new Project CRs and deleted Namespace CRs
Garbage collector changes: Trivial change -- adapts to a renamed constant in the api/v1alpga1 package
Corresponding chart changes: Set up new webhook + changes to permissions for API server and controller
UI changes: Trivial -- adapt to Project now being a root resource type
Run go mod tidy: Refactor of API server "apply" endpoints removed a dependency
Corresponding devx changes: Enable debug level logging for management controller when running with Tilt

What doesn't change

Anywhere in the code where we validate the existence of a Project, we still do so by validating the existence of the corresponding namespace, since that's usually what we actually care about.

This PR does not address #1389. That will be addressed in a follow-up.

netlify · 2024-01-11T22:42:26Z

✅ Deploy Preview for docs-kargo-akuity-io ready!

Name	Link
🔨 Latest commit	`83e8283`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/65aeaa8cf1743e00080caba9
😎 Deploy Preview	https://deploy-preview-1388.kargo.akuity.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-01-11T22:45:22Z

Codecov Report

Attention: 125 lines in your changes are missing coverage. Please review.

Comparison is base (f0b8ebd) 46.78% compared to head (ff249a3) 46.45%.

❗ Current head ff249a3 differs from pull request most recent head 83e8283. Consider uploading reports for the commit 83e8283 to get more accurate results

Files	Patch %	Lines
internal/api/common.go	0.00%	23 Missing ⚠️
...nternal/controller/management/projects/projects.go	84.44%	19 Missing and 2 partials ⚠️
internal/api/delete_project_v1alpha1.go	0.00%	18 Missing ⚠️
internal/webhook/project/webhook.go	62.50%	15 Missing ⚠️
internal/api/list_projects_v1alpha1.go	0.00%	7 Missing ⚠️
api/v1alpha1/project_types.go	0.00%	6 Missing ⚠️
internal/api/create_or_update_resource_v1alpha1.go	0.00%	6 Missing ⚠️
internal/api/create_resource_v1alpha1.go	0.00%	6 Missing ⚠️
internal/api/delete_resource_v1alpha1.go	0.00%	6 Missing ⚠️
internal/api/update_resource_v1alpha1.go	0.00%	6 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1388      +/-   ##
==========================================
- Coverage   46.78%   46.45%   -0.33%     
==========================================
  Files         136      140       +4     
  Lines       11897    12013     +116     
==========================================
+ Hits         5566     5581      +15     
- Misses       6129     6243     +114     
+ Partials      202      189      -13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

krancour · 2024-01-18T20:50:10Z

api/v1alpha1/labels.go

 	FreightLabelKey = "kargo.akuity.io/freight"
+	ProjectLabelKey = "kargo.akuity.io/project"


Changed this symbol just for consistency with the others.

krancour · 2024-01-18T21:23:09Z

api/service/v1alpha1/service.proto

-message Project {
-  string name = 1;
-  google.protobuf.Timestamp create_time = 2;
-}


This moved to types.proto where we have the protobuf definitions of anything that's also a CRD.

krancour · 2024-01-18T21:24:44Z

charts/kargo/templates/webhooks/webhooks.yaml

+    apiGroups: ["kargo.akuity.io"]
+    apiVersions: ["v1alpha1"]
+    resources: ["projects"]
+    operations: ["CREATE"]


For the moment, only need to validate creates.

krancour · 2024-01-18T21:25:25Z

cmd/controlplane/webhooks.go

+			if err = freight.SetupWebhookWithManager(mgr); err != nil {
+				return errors.Wrap(err, "setup Freight webhook")
+			}
+			if err = project.SetupWebhookWithManager(mgr); err != nil {
+				return errors.Wrap(err, "setup Project webhook")
 			}
 			if err = promotion.SetupWebhookWithManager(mgr); err != nil {
 				return errors.Wrap(err, "setup Promotion webhook")
 			}
 			if err = promotionpolicy.SetupWebhookWithManager(mgr); err != nil {
 				return errors.Wrap(err, "setup PromotionPolicy webhook")
 			}
-			if err = freight.SetupWebhookWithManager(mgr); err != nil {
-				return errors.Wrap(err, "setup Freight webhook")
+			if err = stage.SetupWebhookWithManager(mgr); err != nil {
+				return errors.Wrap(err, "setup Stage webhook")
 			}


Besides adding a webhook, I reordered these to be alphabetical.

krancour · 2024-01-18T21:28:17Z

internal/kubeclient/manifest/manifest.go

-			return nil, nil, errors.Wrap(err, "new resmap from data")
-		}
-		cluster = make([]*unstructured.Unstructured, 0, resMap.Size())
-		for _, r := range resMap.ClusterScoped() {


To make this recognize Project as a cluster-scoped resource required jumping through a lot of hoops that were more easily avoided by refactoring this whole parser into the new splitYAML() function that was adapted from GitOps Engine.

Signed-off-by: Kent Rancourt <[email protected]>

jessesuen · 2024-01-23T23:47:54Z

internal/api/create_resource_v1alpha1.go

+	projects, otherResources, err := splitYAML(req.Msg.GetManifest())
 	if err != nil {
 		return nil, connect.NewError(connect.CodeInvalidArgument, errors.Wrap(err, "parse manifest"))
 	}
-
-	size := len(cluster) + len(namespaced)
-	res := make([]*svcv1alpha1.CreateResourceResult, 0, size)
-	for _, obj := range cluster {
-		res = append(res, s.createResource(ctx, obj))
-	}
-	for _, obj := range namespaced {
-		if err := s.validateProject(ctx, obj.GetNamespace()); err != nil {
-			res = append(res, &svcv1alpha1.CreateResourceResult{
-				Result: &svcv1alpha1.CreateResourceResult_Error{
-					Error: err.Error(),
-				},
-			})
-			continue
-		}
-		res = append(res, s.createResource(ctx, obj))
+	resources := append(projects, otherResources...)
+	res := make([]*svcv1alpha1.CreateResourceResult, 0, len(resources))
+	for _, r := range resources {
+		resource := r // Avoid implicit memory aliasing
+		res = append(res, s.createResource(ctx, &resource))


Now that the Namespace creation is async (it requires a reconciliation by the management controller before the underlying namespace actually gets created), have you noticed if we encounter timing-related failures because of that slight delay introduced by Project -> Namespace creation? (e.g. the Namespace didn't get created in time before the Stage).

It is possible, but so far I've seen it only once out of probably 50+ times I have done this.

I'd been contemplating how to make that more foolproof. I could probably add logic here to wait for project readiness before moving on to non-project resources.

I'm going to open a follow-up issue.

Right here so we don't lose track:

#1415

jessesuen · 2024-01-24T00:01:05Z

internal/webhook/project/webhook.go

+	// Validate that a namespace matching the project name doesn't already exist.
+	namespace := &corev1.Namespace{}
+	if err := w.getNamespaceFn(
+		ctx,
+		client.ObjectKey{Name: project.Name},
+		namespace,
+	); err != nil {
+		if client.IgnoreNotFound(err) == nil {
+			// This is good. The namespace doesn't already exist.
+			return nil, nil
+		}
+		return nil, apierrors.NewInternalError(
+			errors.Wrapf(err, "error getting namespace %q", project.Name),
+		)
+	}


Since you also have logic in the Project controller to fail project initialization if the namespace already exists, does the webhook serve to provide immediate feedback instead of delayed feedback of a pre-existing namespace?

Yes. That's it exactly. Fail synchronously if we can spot the problem sooner.

krancour added kind/enhancement area/webhooks area/controller area/cli area/charts area/api size/large labels Jan 11, 2024

krancour added this to the v0.4.0 milestone Jan 11, 2024

krancour self-assigned this Jan 11, 2024

krancour force-pushed the krancour/project-crd branch 9 times, most recently from b72bb94 to f4adf35 Compare January 18, 2024 16:40

krancour marked this pull request as ready for review January 18, 2024 16:40

krancour requested a review from a team as a code owner January 18, 2024 16:40

krancour commented Jan 18, 2024

View reviewed changes

krancour added 3 commits January 22, 2024 12:48

resource type changes

73adf5d

Signed-off-by: Kent Rancourt <[email protected]>

corresponding protobuf changes

82c4220

Signed-off-by: Kent Rancourt <[email protected]>

run codegen

336b8a8

Signed-off-by: Kent Rancourt <[email protected]>

krancour added 12 commits January 22, 2024 12:48

corresponding type conversion changes

a3cb70e

Signed-off-by: Kent Rancourt <[email protected]>

corresponding api server endpoint changes

d265b8c

Signed-off-by: Kent Rancourt <[email protected]>

make api server apply projects before other resource types

f87dd66

Signed-off-by: Kent Rancourt <[email protected]>

corresponding cli changes

876e935

Signed-off-by: Kent Rancourt <[email protected]>

create project webhook

52cb918

Signed-off-by: Kent Rancourt <[email protected]>

corresponding controller changes

97b960a

Signed-off-by: Kent Rancourt <[email protected]>

corresponding garbage collector changes

0981640

Signed-off-by: Kent Rancourt <[email protected]>

corresponding chart changes

4fc3c02

Signed-off-by: Kent Rancourt <[email protected]>

corresponding ui changes

bbcc79f

Signed-off-by: Kent Rancourt <[email protected]>

go mod tidy

43ac0d5

Signed-off-by: Kent Rancourt <[email protected]>

corresponding tilt changes

18cc23a

Signed-off-by: Kent Rancourt <[email protected]>

additional predicate and comments on namespace reconciler

83e8283

Signed-off-by: Kent Rancourt <[email protected]>

krancour force-pushed the krancour/project-crd branch from e16a4ea to 83e8283 Compare January 22, 2024 17:48

jessesuen approved these changes Jan 24, 2024

View reviewed changes

krancour mentioned this pull request Jan 24, 2024

make kargo apply slightly more foolproof wrt async project reconcilation #1415

Closed

krancour added this pull request to the merge queue Jan 24, 2024

Merged via the queue into akuity:main with commit 8b630df Jan 24, 2024
14 checks passed

krancour deleted the krancour/project-crd branch January 24, 2024 00:43

This was referenced Jan 26, 2024

refactor: create project namespaces synchronously #1431

Merged

refactor: no special treatment for ns in api server apply #1432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: fully support project CRD #1388

feat!: fully support project CRD #1388

krancour commented Jan 11, 2024 •

edited

Loading

netlify bot commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading

krancour Jan 18, 2024

krancour Jan 18, 2024

krancour Jan 18, 2024

krancour Jan 18, 2024

krancour Jan 18, 2024

jessesuen Jan 23, 2024

krancour Jan 24, 2024

krancour Jan 24, 2024

jessesuen Jan 24, 2024

krancour Jan 24, 2024

		FreightLabelKey = "kargo.akuity.io/freight"
		ProjectLabelKey = "kargo.akuity.io/project"

feat!: fully support project CRD #1388

feat!: fully support project CRD #1388

Conversation

krancour commented Jan 11, 2024 • edited Loading

Summary

Breakdown

What doesn't change

netlify bot commented Jan 11, 2024 • edited Loading

✅ Deploy Preview for docs-kargo-akuity-io ready!

codecov bot commented Jan 11, 2024 • edited Loading

Codecov Report

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

krancour commented Jan 11, 2024 •

edited

Loading

netlify bot commented Jan 11, 2024 •

edited

Loading

codecov bot commented Jan 11, 2024 •

edited

Loading