Skip to content

Implement SaaS-Based Approval Workflow for ClientIntents #569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 56 commits into
base: main
Choose a base branch
from

Conversation

omris94
Copy link
Contributor

@omris94 omris94 commented Feb 26, 2025

Description

This pull request introduces a SaaS-based approval workflow for ClientIntents within the Otterize intents-operator. By default, there are no changes to the user API unless explicitly configured by the user.

Key Changes

  • ApprovedClientIntents CRD:
    A new Custom Resource Definition (CRD) named ApprovedClientIntents has been added. This CRD represents ClientIntents resources that have been approved, facilitating a controlled and auditable access management process.

  • Refactoring of Intents Reconciliation:
    The existing IntentsReconciler logic has been refactored into ApprovedIntentsReconciler. All operations previously triggered by the reconciliation of intents now utilize the new ApprovedClientIntents CRD, ensuring that only approved intents are enforced.

  • Introduction of Approval Flow Reconciler:
    A new reconciler has been implemented to handle the approval flow. By default, the operator auto-approves any ClientIntents. If configured to integrate with the cloud, it reports the ClientIntents as access requests to the cloud and awaits approval status. Once a request is approved, it triggers the creation of an ApprovedClientIntents resource.

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR and in github.com/otterize/docs

evyatarmeged and others added 30 commits January 26, 2025 11:37
…raphQL mutations and queries for applied intents requests
}

// ApprovedIntentsReconciler reconciles a Intents object
type ApprovedIntentsReconciler struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially what was referred to as the Intents Reconciler

@@ -115,21 +86,39 @@ func NewIntentsReconciler(
//+kubebuilder:rbac:groups="apiextensions.k8s.io",resources=customresourcedefinitions,verbs=get;list;watch;update;create;patch
// +kubebuilder:rbac:groups=iam.cnrm.cloud.google.com,resources=iampartialpolicies,verbs=get;list;watch;create;update;patch;delete

// Reconcile is part of the main kubernetes reconciliation loop which aims to
// move the current state of the cluster closer to the desired state.
// Reconcile - main reconciliation loop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation about how the "clientIntents to ApprovedClientIntents" lifecycle works

@omris94 omris94 requested a review from amitlicht February 26, 2025 15:30
@omris94 omris94 changed the title Support SaaS based approval flow of clientIntents Support SaaS based approval flow for clientIntents Feb 26, 2025
omris94 added 12 commits March 10, 2025 19:42
…r upToDates with their corresponding ClientIntents
…lientIntents for every clientIntents while the operator will wait for the migration to end.
1. When first reconciled - add finalizer
2. When deleted - trigger approvedClientIntent deletion
   a. If does not exist - remove finalizer
   b. If exists - delete approvedClientIntent and return
3. When approvedClientIntents is deleted - remove finalizer from clientIntents before removing its own
@omris94 omris94 changed the title Support SaaS based approval flow for clientIntents Implement SaaS-Based Approval Workflow for ClientIntents Mar 17, 2025
@omris94 omris94 marked this pull request as ready for review March 17, 2025 17:51
@@ -66,6 +66,9 @@ const (
SeparateNetpolsForIngressAndEgress = "separate-netpols-for-ingress-and-egress"
SeparateNetpolsForIngressAndEgressDefault = false
ExternallyManagedPolicyWorkloadsKey = "externallyManagedPolicyWorkloads"

EnableIntentsCloudApproval = "enable-intents-cloud-approval"
EnableIntentsCloudApprovalDefault = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change before merge

//+kubebuilder:object:root=true
//+kubebuilder:subresource:status

// ApprovedClientIntents is the Schema for the intents API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix comment?

ObservedGeneration int64 `json:"observedGeneration"`
// ResolvedIPs stores resolved IPs for a domain name - the network mapper populates it when DNS internetTarget is used
// +optional
ResolvedIPs []ResolvedIPs `json:"resolvedIPs,omitempty" yaml:"resolvedIPs,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to open a PR to update the mapper with this change + release them together

@@ -365,14 +374,21 @@ type AzureKeyVaultPolicy struct {
type IntentsStatus struct {
// upToDate field reflects whether the client intents have successfully been applied
// to the cluster to the state specified
// Deprecated: Use ApprovedClientIntents.Status.UpToDate instead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment true?

@@ -365,14 +374,21 @@ type AzureKeyVaultPolicy struct {
type IntentsStatus struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming to ClientIntentsStatus (the name IntentsStatus is even more confusing, now that we have "ClientIntents" and "ApprovedClientIntents" and "ApprovedClientIntentsStatus"

Comment on lines 82 to 83
//+kubebuilder:rbac:groups=k8s.otterize.com,resources=postgresqlserverconfigs,verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups=k8s.otterize.com,resources=mysqlserverconfigs,verbs=get;list;watch;create;update;patch;delete
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup required RBAC permissions here

Namespace: clientIntents.Namespace,
},
func (r *IntentsReconciler) handleCloudAppliedIntentsRequests(ctx context.Context, intents otterizev2alpha1.ClientIntents) error {
if intents.Status.ReviewStatus == otterizev2alpha1.ReviewStatusApproved || !intents.DeletionTimestamp.IsZero() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this "if"? The caller shouldn't call this function if this is the case (and as far as I can tell it really doesn't)

// Will be handled elsewhere
return nil
}
if intents.Status.ReviewStatus == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this already be handled by the main flow? I'd expect the main flow to ensure the status is set.

return res
})
if err != nil {
func (r *IntentsReconciler) queryAppliedIntentsRequestsStatus(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queryAppliedIntentsRequestsStatus sounds like it's just querying (readonly), but really it also queries the cloud and updates the request status. The name should be more indicative.

if address.TargetRef == nil || address.TargetRef.Kind != "Pod" {
continue
}
func (r *IntentsReconciler) handleRequestStatusChanges(ctx context.Context, request operator_cloud_client.AppliedIntentsRequestStatus, clientIntents otterizev2alpha1.ClientIntents) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe handleRequestStatusResponse? It didn't necessarily change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants