Initial access config support #115

tomasliumparas · 2024-12-05T01:31:48Z

Initial AccessConfig version.
To do:

Use domain from TunnelBinding
Discuss if we should create AccessApplications before DNS records due to security reasons
Implement AccessPolicies calls

Currently tested with the following CRD:

apiVersion: networking.cfargotunnel.com/v1alpha1
kind: TunnelBinding
metadata:
  name: whoami-cluster-tun
subjects:
  - name: whoami
  - name: whoami-2 # Points to the second service
tunnelRef:
  kind: ClusterTunnel
  name: k3s-cluster-tunnel
accessConfig:
  name: whoami
  domain: whoami.domain.com
  type: "self_hosted"

#67

adyanth

Thanks for the PR! Did you have a chance to test this out in your cluster to make sure it works? Had a couple questions/comments.

adyanth · 2024-12-06T03:27:57Z

api/v1alpha1/tunnelbinding_types.go

+		// GatewayRules:            []cloudflare.AccessApplicationGatewayRule{},
+		AllowedIdps:       c.Settings.Authentication.AllowedIdps,
+		CustomDenyMessage: c.Settings.Authentication.CustomDenyMessage,
+		LogoURL:           c.Settings.Appearance.CustomLogo,
+		// AUD:                     "",
+		Domain:                  hostname,
+		Type:                    cloudflare.AccessApplicationType(c.Type),
+		SessionDuration:         c.Settings.Authentication.SessionDuration,
+		SameSiteCookieAttribute: c.Settings.Cookies.SameSiteAttribute,
+		CustomDenyURL:           c.Settings.Authentication.CustomDenyUrl,
+		Name:                    hostname,
+		// PrivateAddress:          "",
+		// CorsHeaders: &cloudflare.AccessApplicationCorsHeaders{
+		// 	AllowedMethods:   []string{},
+		// 	AllowedOrigins:   []string{},
+		// 	AllowedHeaders:   []string{},
+		// 	AllowAllMethods:  false,
+		// 	AllowAllHeaders:  false,
+		// 	AllowAllOrigins:  false,
+		// 	AllowCredentials: false,
+		// 	MaxAge:           0,
+		// },
+		// CreatedAt:               &time.Time{},
+		// UpdatedAt:               &time.Time{},
+		// SaasApplication:         &cloudflare.SaasApplication{},
+		AutoRedirectToIdentity: &c.Settings.Authentication.InstantAuth,
+		// SkipInterstitial:        new(bool),


Any reason why some are commented out? Could you remove them if they aren't needed, or are they needed in the future?

I was planning to implement CORS support a bit later.
I can for sure remove the comments for now.

Removed in 7bde824

adyanth · 2024-12-06T03:34:22Z

controllers/cloudflare_api.go

+	return ids, nil
+}
+
+func (c *CloudflareAPI) CreateAccessConfig(name string, config networkingv1alpha1.AccessConfig) error {


Could you remove the dependency of networkingv1alpha1 from this file by calling this function with the newApp already created and accepting an AccessApp instead?

The thing is that I need to call CF api, to have it translated into AccessApp - basically to translate Access Group Names into ids.
That means I would need to include CloudflareAPI from cloudflare_api.go into tunnelbinding_types.go

Or I could move (c *AccessConfig) NewAccessApplication() into controllers package as a function.
I did this, since I originally wanted to have it as a method of AccessConfig. That way it is near the definition of the CRD and it makes a bit easier to map the fields.

Which way you would prefer more?

adyanth · 2024-12-06T03:49:34Z

controllers/cloudflare_api.go

+			if len(config.AccessPolicies) == 0 {
+				c.Log.Info("deleting access policy for application", "applicationId", applicationId, "policyId", policy.ID)
+
+				err := c.CloudflareClient.DeleteAccessPolicy(ctx, accountId, applicationId, policy.ID)


Does this delete only the policies on this particular application?

Yes. Since existing policy list is fetched for current application

existingPolicies, _, err := c.CloudflareClient.AccessPolicies(ctx, accountId, applicationId, ...

adyanth · 2024-12-06T03:50:29Z

controllers/cloudflare_api.go

+		c.Log.Error(err, "failed retrieving existing access policies for application", "applicationId", applicationId)
+	}
+
+	if len(existingPolicies) > 0 {


This if condition is not necessary.

adyanth · 2024-12-06T05:31:30Z

controllers/cloudflare_api.go

+			}
+
+			// Check if the policy is still required
+			required := false


This function seems convoluted. Let me phrase what I understand this function should be doing and you can correct me.

Given the set of existing access policies on the application, and the set of access policies defined in the CRD, you are trying to reconcile them. But there are a couple of problems. Does having the same name of the policy guarantee that the policy wasn't changed? Do we also create access groups here or just refer to them by name?

If possible, I'd like to see this split up into smaller functions and sync all the rules regardless of what was present, unless there was a way you could verify what is set online matches the local config.

This one was a bit tricky.
What I am doing here is checking by name if there are any policies in the application, which are not defined in crd.
If yes, we delete them.

Later on we loop trough policies defined in crd and update them.

Cases I tested:

Policy by same name is changed manually, it should be updated to be matching crd configuration - works

Policy manually added to the ui, should be deleted - works

Policy should be deleted once gone from crd - works

does this makes sense?

tomasliumparas · 2024-12-06T10:04:53Z

Thanks for the PR! Did you have a chance to test this out in your cluster to make sure it works? Had a couple questions/comments.

Yes, seem to work fine.
What I managed to test:

Creation/deletion of Access Applications with/without the policies
Changing order of access policies
Manually updating the policies via Cloudflare
Removing the policies via CRD
Renaming the policies via CRD

Initial access config support

859add3

tomasliumparas changed the title ~~#67 Initial access config support~~ Initial access config support Dec 5, 2024

tomasliumparas added 7 commits December 5, 2024 15:52

Add deletion logic

9348830

Reconcile AccessConfig before DNS, add events

1935b12

Use service fqdn

621aec7

Remove uuid logic

5f79d6d

Add access policy handling

0367005

Handle deletion of access policies

ebdc1de

Fix access policy precedence

04423e4

adyanth reviewed Dec 6, 2024

View reviewed changes

Cleanup unused comments

7bde824

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial access config support #115

Initial access config support #115

tomasliumparas commented Dec 5, 2024 •

edited

Loading

adyanth left a comment

adyanth Dec 6, 2024

tomasliumparas Dec 6, 2024

tomasliumparas Dec 6, 2024 •

edited

Loading

adyanth Dec 6, 2024

tomasliumparas Dec 6, 2024 •

edited

Loading

adyanth Dec 6, 2024

tomasliumparas Dec 6, 2024

adyanth Dec 6, 2024

adyanth Dec 6, 2024

tomasliumparas Dec 6, 2024

tomasliumparas Dec 6, 2024

tomasliumparas commented Dec 6, 2024

Initial access config support #115

Are you sure you want to change the base?

Initial access config support #115

Conversation

tomasliumparas commented Dec 5, 2024 • edited Loading

adyanth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasliumparas Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasliumparas Dec 6, 2024 • edited Loading

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

tomasliumparas commented Dec 6, 2024

tomasliumparas commented Dec 5, 2024 •

edited

Loading

tomasliumparas Dec 6, 2024 •

edited

Loading

tomasliumparas Dec 6, 2024 •

edited

Loading