feat(validation): Add catalog entry validation in controller by varshaprasad96 · Pull Request #5 · kcp-dev/catalog

varshaprasad96 · 2022-08-31T22:56:32Z

Validate export references in entry spec
Aggregate PermissionClaims and API resources info from
referenced APIExport to entry status

Signed-off-by: Vu Dinh vudinh@outlook.com

varshaprasad96 · 2022-08-31T22:57:55Z

This is based on the CE api defined in commit (6f259d6) and is subject to change. Please hold from reviewing as this is not complete yet.
/hold

ncdc · 2022-09-13T12:18:34Z

If you don't mind, please pause working on this until we have solidified the API. Thanks!

1. Validate export references in entry spec 2. Aggregate PermissionClaims and API resources info from referenced APIExport to entry status Signed-off-by: Vu Dinh <vudinh@outlook.com>

Signed-off-by: Vu Dinh <vudinh@outlook.com>

ncdc · 2022-11-30T21:11:12Z

+	exportPermissionClaims := []apisv1alpha1.PermissionClaim{}
+	invalidExports := []string{}
+	for _, exportRef := range catalogEntry.Spec.Exports {
+		// TODO: verify if path contains the entire heirarchy or just the clusterName.


Path should be the full hierachy/absolute path

ncdc · 2022-11-30T21:11:28Z

+		)
+		logger.V(2).Info("reconciling CatalogEntry")
+		export := apisv1alpha1.APIExport{}
+		err := r.Get(logicalcluster.WithCluster(ctx, logicalcluster.New(path)), types.NamespacedName{Name: name, Namespace: req.Namespace}, &export)


Suggested change

err := r.Get(logicalcluster.WithCluster(ctx, logicalcluster.New(path)), types.NamespacedName{Name: name, Namespace: req.Namespace}, &export)

err := r.Get(logicalcluster.WithCluster(ctx, logicalcluster.New(path)), types.NamespacedName{Name: name}, &export)

APIExports are cluster-scoped

ncdc · 2022-11-30T21:12:40Z

+			// Error reading the object - requeue the request.
+			logger.Error(err, "failed to get resource")
+			continue


This doesn't actually requeue like the comment says

ncdc · 2022-11-30T21:55:52Z

+		cond := conditionsapi.Condition{
+			Type:               catalogv1alpha1.APIExportValidType,
+			Status:             corev1.ConditionTrue,
+			Severity:           conditionsapi.ConditionSeverityNone,
+			LastTransitionTime: metav1.Now(),
+		}
+		conditions.Set(catalogEntry, &cond)


You should be able to use conditions.MarkTrue() here

ncdc · 2022-11-30T21:56:11Z

+		message := fmt.Sprintf("invalid export(s): %s", strings.Join(invalidExports, " ,"))
+		invalidCond := conditionsapi.Condition{
+			Type:               catalogv1alpha1.APIExportValidType,
+			Status:             corev1.ConditionFalse,
+			Severity:           conditionsapi.ConditionSeverityError,
+			LastTransitionTime: metav1.Now(),
+			Message:            message,
+		}
+		conditions.Set(catalogEntry, &invalidCond)


You should be able to use conditions.MarkFalse() here

ncdc · 2022-11-30T22:14:19Z

+		return nil, fmt.Errorf("could not create CatalogEntry %q|%q: %v", workspaceCluster, entryName, err)
+	}
+
+	t.Logf("waiting for CatalogEntry %q|%q to be found", workspaceCluster, entryName)


Suggested change

t.Logf("waiting for CatalogEntry %q|%q to be found", workspaceCluster, entryName)

t.Logf("waiting for CatalogEntry %q|%q to have APIExportValid condition", workspaceCluster, entryName)

ncdc · 2022-11-30T22:14:55Z

+		}
+		return true, nil
+	}); err != nil {
+		return nil, fmt.Errorf("CatalogEntry %q|%q not found: %v", workspaceCluster, entryName, err)


Suggested change

return nil, fmt.Errorf("CatalogEntry %q|%q not found: %v", workspaceCluster, entryName, err)

return nil, fmt.Errorf("error waiting for CatalogEntry %q|%q status: %v", workspaceCluster, entryName, err)

ncdc · 2022-11-30T22:16:48Z

+
+	t.Logf("waiting for CatalogEntry %q|%q to be found", workspaceCluster, entryName)
+	var catalogEntry catalogv1alpha1.CatalogEntry
+	if err := wait.PollImmediate(100*time.Millisecond, wait.ForeverTestTimeout, func() (done bool, err error) {


I would move this into the Test() functions themselves

ncdc · 2022-11-30T22:17:22Z

+		if err != nil {
+			t.Fatalf("unable to create CatalogEntry: %v", err)
+		}
+


Insert polling loop to get entry and expect conditions.IsTrue(entry, catalogv1alpha1.APIExportValidType)

ncdc · 2022-11-30T22:17:57Z

+		if err != nil {
+			t.Fatalf("catalogEntry %q failed to be reconciled in cluster %q: %v", entry.GetName(), workspaceCluster, err)
+		}
+


Insert polling loop

openshift-merge-robot · 2022-12-10T15:47:00Z

@varshaprasad96: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

varshaprasad96 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2022

dinhxuanvu force-pushed the hack/cotalog-controller branch from 106c7b4 to 7ee76a1 Compare September 28, 2022 15:07

dinhxuanvu changed the title ~~WIP: Hack/catalog controller~~ [WIP]: feat(validation): Add catalog entry validation in controller Sep 28, 2022

dinhxuanvu changed the title ~~[WIP]: feat(validation): Add catalog entry validation in controller~~ [WIP] feat(validation): Add catalog entry validation in controller Sep 28, 2022

dinhxuanvu force-pushed the hack/cotalog-controller branch from 5215e6b to 0af5f63 Compare October 6, 2022 15:53

varshaprasad96 mentioned this pull request Oct 11, 2022

API Catalog #7

Open

5 tasks

dinhxuanvu force-pushed the hack/cotalog-controller branch 3 times, most recently from b488298 to 0b17ed1 Compare October 18, 2022 20:26

dinhxuanvu marked this pull request as ready for review October 18, 2022 20:27

dinhxuanvu mentioned this pull request Nov 10, 2022

Revert KB default scaffolding #9

Merged

dinhxuanvu changed the title ~~[WIP] feat(validation): Add catalog entry validation in controller~~ feat(validation): Add catalog entry validation in controller Nov 10, 2022

dinhxuanvu removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2022

varshaprasad96 and others added 2 commits November 10, 2022 20:06

Hack: reconciler logic for verifying if APIExport exists

2e1b9ef

Add CatalogEntry validation in controller

114c40d

1. Validate export references in entry spec 2. Aggregate PermissionClaims and API resources info from referenced APIExport to entry status Signed-off-by: Vu Dinh <vudinh@outlook.com>

dinhxuanvu force-pushed the hack/cotalog-controller branch from 0b17ed1 to a953022 Compare November 11, 2022 01:13

Add e2e test cases to test controller validation process

70824d8

Signed-off-by: Vu Dinh <vudinh@outlook.com>

dinhxuanvu force-pushed the hack/cotalog-controller branch from a953022 to 70824d8 Compare November 11, 2022 20:52

varshaprasad96 and others added 2 commits November 11, 2022 12:58

fix syntax errors

2b7e9a8

Fix e2e test cases

6e38f56

Signed-off-by: Vu Dinh <vudinh@outlook.com>

varshaprasad96 force-pushed the hack/cotalog-controller branch from 1f2341d to 6e38f56 Compare November 12, 2022 07:22

varshaprasad96 and others added 7 commits November 18, 2022 02:29

Fix Makefile changes

964495e

Fix Makefiles and few kcp resources files

35deac4

Signed-off-by: Vu Dinh <vudinh@outlook.com>

Fix image-related issues on Makefile and deployment

1e73061

Signed-off-by: Vu Dinh <vudinh@outlook.com>

Add permission model to apiexport

ad144e4

Fix e2e test cases implementation details

8f8c76b

Signed-off-by: Vu Dinh <vudinh@outlook.com>

Add log level for debugging

7e871ba

Signed-off-by: Vu Dinh <vudinh@outlook.com>

Check if rbac is working

f11c245

Fix resource crd issue

ae3b39e

Signed-off-by: Vu Dinh <vudinh@outlook.com>

ncdc suggested changes Nov 30, 2022

View reviewed changes

Fix path cannot be added error

83778de

varshaprasad96 mentioned this pull request Dec 6, 2022

Add CLI command for binding a catalog entry to a workspace #6

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2022

	err := r.Get(logicalcluster.WithCluster(ctx, logicalcluster.New(path)), types.NamespacedName{Name: name, Namespace: req.Namespace}, &export)
	err := r.Get(logicalcluster.WithCluster(ctx, logicalcluster.New(path)), types.NamespacedName{Name: name}, &export)

	t.Logf("waiting for CatalogEntry %q\|%q to be found", workspaceCluster, entryName)
	t.Logf("waiting for CatalogEntry %q\|%q to have APIExportValid condition", workspaceCluster, entryName)

	return nil, fmt.Errorf("CatalogEntry %q\|%q not found: %v", workspaceCluster, entryName, err)
	return nil, fmt.Errorf("error waiting for CatalogEntry %q\|%q status: %v", workspaceCluster, entryName, err)

Conversation

varshaprasad96 commented Aug 31, 2022 • edited by dinhxuanvu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varshaprasad96 commented Aug 31, 2022

Uh oh!

ncdc commented Sep 13, 2022

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!

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!

openshift-merge-robot commented Dec 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

varshaprasad96 commented Aug 31, 2022 •

edited by dinhxuanvu

Loading