Skip to content
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

✨ Add FrontProxy controller #20

Merged
merged 8 commits into from
Jan 24, 2025

Conversation

SimonTheLeg
Copy link
Contributor

@SimonTheLeg SimonTheLeg commented Jan 17, 2025

Summary

Adds Support for FrontProxy

Some bits and bobs about this, due to timeconstraints:

  1. Currently this code will create all certificates, cas, issuers, configmaps and deployments when a frontproxy resource is added. The frontproxy pod is starting up and does not crash. This was the last topic I got it to. I have not yet verified that it actually does the correct connections and so on. Steps to get this working locally so far

    kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.13.0/cert-manager.crds.yaml
    helm upgrade --install --wait --namespace cert-manager --create-namespace --version v1.13.0 cert-manager jetstack/cert-manager
    helm install etcd oci://registry-1.docker.io/bitnamicharts/etcd --set auth.rbac.enabled=false --set auth.rbac.create=false
    
    make install
    k apply -f ./config/samples/cert-manager/issuer.yaml
    k apply -f config/samples/v1alpha1_rootshard.yaml
    k apply -f config/samples/v1alpha1_frontproxy.yaml
    
    make run
    
  2. Since we dropped the -cert suffix from the operator (compared to the helm chart), the secret created from the issuer $rootshard-$frontproxy-kubeconfig would have the same name as the other kubeconfig secret, which is created by the operator. For this reason I have renamed the latter one to $rootshard-$frontproxy-dynamic-kubeconfig. Dynamic because this kubeconfig as far as I understand it is dynamically changed by the front-proxy. I am also open for other names, this was just the first thing that came to mind :D

  3. I don’t know if there are additional mounts required in the rootshard deployment. I did not check this yet.

  4. The whole templating of names is still a bit messy atm. I tried to make this similar to our new structure in resources.go, but did not have the time to migrate all of them properly.

  5. @embik for now I dropped what we chatted about with the "fp" identifier, because after revisiting the $rootshard-$frontproxy scoping in the chart, I noticed that there were no more overlaps

  6. I have started using FrontProxyBasepath variable to denote common paths, but it is not done everywhere. I am not sure whether we want to have this templated everywhere or hardcoded everywhere. ptal

  7. There is an issue with how the secret is reconciled after you stop the operator locally and restart it. I did not have the time to look further into it, but I am sure it's just a minor thing somewhere in the ReconcilerFunc

    DEBUG   Object differs from generated one       {“type”: “*v1.Secret”, “namespace”: “default”, “name”: “frontproxy-sample-front-proxy-kubeconfig”, “diff”: [“TypeMeta.Kind:  != Secret”, “TypeMeta.APIVersion:  != v1”, “ObjectMeta.UID:  != 9f2e6513-3f49-4c85-8ea3-f572e6768912”, “ObjectMeta.ResourceVersion:  != 10173”, “ObjectMeta.CreationTimestamp.Time: 0001-01-01 00:00:00 +0000 UTC != 2025-01-10 15:15:35 +0100 CET”, “ObjectMeta.ManagedFields: <nil slice> != [{main Update v1 2025-01-10 15:15:35 +0100 CET FieldsV1 {\”f:data\”:{\”.\”:{},\”f:kubeconfig\”:{}},\”f:type\”:{}} }]”, “Type:  != Opaque”]}
    2025-01-10T15:29:09+01:00       ERROR   Reconciler error        {“controller”: “frontproxy”, “controllerGroup”: “operator.kcp.io”, “controllerKind”: “FrontProxy”, “FrontProxy”: {“name”:”frontproxy-sample”,”namespace”:”default”}, “namespace”: “default”, “name”: “frontproxy-sample”, “reconcileID”: “510dee4f-7673-4316-82e4-5a7e3c782215”, “error”: “failed to ensure Secret default/frontpro
    
  8. I have created a dirty deployment_test.go thing, which can just quick and dirty render out the deployment yaml. Maybe it is of help also for you :)

    package frontproxy
    
    import (
    	"os"
    	"testing"
    
    	"github.com/kcp-dev/kcp-operator/api/v1alpha1"
    	appsv1 "k8s.io/api/apps/v1"
    	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
    	"k8s.io/apimachinery/pkg/runtime/schema"
    	"sigs.k8s.io/yaml"
    )
    
    func TestDebug(t *testing.T) {
    	dep := &appsv1.Deployment{}
    	fp := &v1alpha1.FrontProxy{
    		ObjectMeta: v1.ObjectMeta{
    			Name: "frontproxy-sample",
    		},
    	}
    	rs := &v1alpha1.RootShard{
    		ObjectMeta: v1.ObjectMeta{
    			Name: "shard-sample",
    		},
    	}
    
    	f := DeploymentReconciler(fp, rs)
    	name, reconciler := f()
    	res, err := reconciler(dep)
    	if err != nil {
    		t.Error(err)
    	}
    
    	res.SetName(name)
    	res.SetGroupVersionKind(schema.GroupVersionKind{
    		Group:   "apps",
    		Version: "v1",
    		Kind:    "Deployment",
    	})
    
    	b, err := yaml.Marshal(res)
    	if err != nil {
    		t.Error(err)
    	}
    
    	os.WriteFile("/tmp/gen_deploy.yaml", b, 0644)
    
    }

Related issue(s)

Fixes #10

Release Notes

TODO

@kcp-ci-bot kcp-ci-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the DCO. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 17, 2025
@kcp-ci-bot
Copy link
Contributor

Hi @SimonTheLeg. Thanks for your PR.

I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@kcp-ci-bot kcp-ci-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 17, 2025
@embik embik force-pushed the reconcilers-simon branch from f8039f8 to db1149d Compare January 17, 2025 17:25
@kcp-ci-bot kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2025
@embik
Copy link
Member

embik commented Jan 17, 2025

/ok-to-test

@kcp-ci-bot kcp-ci-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 17, 2025
@embik embik force-pushed the reconcilers-simon branch from db1149d to d68bbaa Compare January 20, 2025 14:30
@kcp-ci-bot kcp-ci-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 20, 2025
@embik embik marked this pull request as ready for review January 21, 2025 08:57
@kcp-ci-bot kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2025
README.md Outdated Show resolved Hide resolved
@kcp-ci-bot kcp-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2025
SimonTheLeg and others added 8 commits January 24, 2025 09:00
On-behalf-of: SAP simon.bein@sap.com
Signed-off-by: Simon Bein <simontheleg@gmail.com>
Co-authored-by: Marvin Beckers <marvin@kubermatic.com>
On-behalf-of: SAP simon.bein@sap.com
Signed-off-by: Simon Bein <simontheleg@gmail.com>
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
On-behalf-of: SAP <marvin.beckers@sap.com>
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
On-behalf-of: SAP <marvin.beckers@sap.com>
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
On-behalf-of: SAP <marvin.beckers@sap.com>
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
On-behalf-of: SAP <marvin.beckers@sap.com>
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
On-behalf-of: SAP <marvin.beckers@sap.com>
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
On-behalf-of: SAP <marvin.beckers@sap.com>
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
@embik embik force-pushed the reconcilers-simon branch from 6e52f96 to 1e6054b Compare January 24, 2025 08:19
@kcp-ci-bot kcp-ci-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2025
@embik embik changed the title ✨ Add FrontProxy ✨ Add FrontProxy controller Jan 24, 2025
@embik embik requested a review from xrstf January 24, 2025 12:00
Copy link
Contributor

@xrstf xrstf left a comment

Choose a reason for hiding this comment

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

/approve

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2025
@kcp-ci-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d296437090b0f4269c7049c85a8b79dd375a35b1

@kcp-ci-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xrstf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2025
@kcp-ci-bot kcp-ci-bot merged commit c2198e2 into kcp-dev:main Jan 24, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: implement FrontProxy reconciling
4 participants