Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Bugfix ignore and copy from obj #37

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

Bugfix ignore and copy from obj #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4eea630
fix ignore condition in reverse patches
ishankhare07 Nov 16, 2022
f707d4f
fix copyFromObject bug for virtual to host
ishankhare07 Nov 16, 2022
989ece9
add plugin example for knative serving
ishankhare07 Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions examples/knative-serving/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#### Install knative serving on host cluster
```
# installing knative serving CRDs
kubectl apply -f https://github.com/knative/serving/releases/download/knative-v1.6.0/serving-crds.yaml
# installing knative serving core
kubectl apply -f https://github.com/knative/serving/releases/download/knative-v1.6.0/serving-core.yaml
# installing kourier networking layer
kubectl apply -f https://github.com/knative/net-kourier/releases/download/knative-v1.6.0/kourier.yaml
# configure serving to use kourier by default
sleep 10
kubectl patch configmap/config-network \
--namespace knative-serving \
--type merge \
--patch '{"data":{"ingress-class":"kourier.ingress.networking.knative.dev"}}'
# configure serving to use sslip.io as default DNS
kubectl apply -f https://github.com/knative/serving/releases/download/knative-v1.6.0/serving-default-domain.yaml

```

#### Deploy vcluster with knative plugin
```
vcluster create vcluster -f https://raw.githubusercontent.com/loft-sh/vcluster-generic-crd-sync-plugin/main/examples/knative-serving/plugin.yaml
```

#### Create a knative service
```
cat << EOF | kubectl apply -f -
apiVersion: serving.knative.dev/v1
kind: Service
metadata:
name: hello
spec:
template:
spec:
containers:
- image: gcr.io/google-samples/hello-app:1.0
EOF
```
139 changes: 139 additions & 0 deletions examples/knative-serving/plugin.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
plugin:
knative-serving:
image: ghcr.io/loft-sh/vcluster-generic-crd-plugin:latest
imagePullPolicy: Always
rbac:
role:
extraRules:
- apiGroups: ["serving.knative.dev"]
resources: ["services", "configurations", "revisions", "routes"]
verbs: ["create", "delete", "patch", "update", "get", "list", "watch"]
clusterRole:
extraRules:
- apiGroups: ["apiextensions.k8s.io"]
resources: ["customresourcedefinitions"]
verbs: ["get", "list", "watch"]
env:
- name: CONFIG
value: |-
version: v1beta1
mappings:
- fromVirtualCluster:
apiVersion: serving.knative.dev/v1
kind: Service
patches: []
reversePatches:
- op: copyFromObject
fromPath: status
path: status
- op: copyFromObject
fromPath: spec.template.spec.containerConcurrency
path: spec.template.spec.containerConcurrency
conditions:
- empty: true
path: spec.template.spec.containerConcurrency
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.containers[0].name
path: spec.template.spec.containers[0].name
conditions:
- empty: true
path: spec.template.spec.containers[0].name
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.containers[0].readinessProbe
path: spec.template.spec.containers[0].readinessProbe
conditions:
- empty: true
path: spec.template.spec.containers[0].readinessProbe
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.timeoutSeconds
path: spec.template.spec.timeoutSeconds
conditions:
- empty: true
path: spec.template.spec.timeoutSeconds
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.responseStartTimeoutSeconds
path: spec.template.spec.responseStartTimeoutSeconds
conditions:
- empty: true
path: spec.template.spec.responseStartTimeoutSeconds
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.idleTimeoutSeconds
path: spec.template.spec.idleTimeoutSeconds
conditions:
- empty: true
path: spec.template.spec.idleTimeoutSeconds
ignore: true
- op: copyFromObject
fromPath: spec.traffic
path: spec.traffic
conditions:
- empty: true
path: spec.traffic
ignore: true
- fromVirtualCluster:
apiVersion: serving.knative.dev/v1
kind: Configuration
reversePatches:
- op: copyFromObject
fromPath: status
path: status
- op: copyFromObject
fromPath: spec.template.spec.containerConcurrency
path: spec.template.spec.containerConcurrency
conditions:
- empty: true
path: spec.template.spec.containerConcurrency
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.containers[0].name
path: spec.template.spec.containers[0].name
conditions:
- empty: true
path: spec.template.spec.containers[0].name
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.containers[0].readinessProbe
path: spec.template.spec.containers[0].readinessProbe
conditions:
- empty: true
path: spec.template.spec.containers[0].readinessProbe
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.timeoutSeconds
path: spec.template.spec.timeoutSeconds
conditions:
- empty: true
path: spec.template.spec.timeoutSeconds
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.responseStartTimeoutSeconds
path: spec.template.spec.responseStartTimeoutSeconds
conditions:
- empty: true
path: spec.template.spec.responseStartTimeoutSeconds
ignore: true
- op: copyFromObject
fromPath: spec.template.spec.idleTimeoutSeconds
path: spec.template.spec.idleTimeoutSeconds
conditions:
- empty: true
path: spec.template.spec.idleTimeoutSeconds
ignore: true
- fromVirtualCluster:
apiVersion: serving.knative.dev/v1
kind: Route
reversePatches:
- op: copyFromObject
fromPath: status
path: status
- op: copyFromObject
fromPath: spec.traffic
path: spec.traffic
conditions:
- empty: true
path: spec.traffic
12 changes: 6 additions & 6 deletions pkg/patches/patch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ type NameResolver interface {
TranslateNamespaceRef(namespace string) (string, error)
}

func ApplyPatches(obj1, obj2 client.Object, patchesConf []*config.Patch, reversePatchesConf []*config.Patch, nameResolver NameResolver) error {
node1, err := NewJSONNode(obj1)
func ApplyPatches(destObj, sourceObj client.Object, patchesConf []*config.Patch, reversePatchesConf []*config.Patch, nameResolver NameResolver) error {
node1, err := NewJSONNode(destObj)
if err != nil {
return errors.Wrap(err, "new json yaml node")
}

var node2 *yaml.Node
if obj2 != nil {
node2, err = NewJSONNode(obj2)
if sourceObj != nil {
node2, err = NewJSONNode(sourceObj)
if err != nil {
return errors.Wrap(err, "new json yaml node")
}
Expand All @@ -46,7 +46,7 @@ func ApplyPatches(obj1, obj2 client.Object, patchesConf []*config.Patch, reverse

// remove ignore paths from patched object
for _, p := range reversePatchesConf {
if p.Path == "" || (p.Ignore != nil && !*p.Ignore) {
Copy link
Member

@FabianKramm FabianKramm Nov 28, 2022

Choose a reason for hiding this comment

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

Not sure if that is correct, the logic currently is that its true by default, this would make it false by default, so only in the case of ignore: false we want to skip applying the delete patch

if p.Path == "" || (p.Ignore != nil && *p.Ignore) {
continue
}

Expand All @@ -64,7 +64,7 @@ func ApplyPatches(obj1, obj2 client.Object, patchesConf []*config.Patch, reverse
return errors.Wrap(err, "marshal yaml")
}

err = jsonyaml.Unmarshal(objYaml, obj1)
err = jsonyaml.Unmarshal(objYaml, destObj)
if err != nil {
return errors.Wrap(err, "convert object")
}
Expand Down
29 changes: 15 additions & 14 deletions pkg/syncer/generic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package syncer
import (
"context"
"fmt"

"github.com/loft-sh/vcluster-generic-crd-plugin/pkg/config"
"github.com/loft-sh/vcluster-generic-crd-plugin/pkg/patches"
"github.com/loft-sh/vcluster-sdk/log"
Expand Down Expand Up @@ -40,8 +41,8 @@ func (s *patcher) ApplyPatches(ctx context.Context, fromObj, toObj client.Object
}
toObjCopied := toObjBase.DeepCopy()

// apply patches on from object
err = patches.ApplyPatches(toObjCopied, toObj, patchesConfig, reversePatchesConfig, nameResolver)
// apply patches on toObjCopied
err = patches.ApplyPatches(toObjCopied, fromObj, patchesConfig, reversePatchesConfig, nameResolver)
Copy link
Member

@FabianKramm FabianKramm Nov 28, 2022

Choose a reason for hiding this comment

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

why do we want to do this?

Copy link
Contributor Author

@ishankhare07 ishankhare07 Nov 29, 2022

Choose a reason for hiding this comment

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

this is needed in cases where we want to populated controller applied default back into the virtual obj. i.e.
vObj.field1 = pObj.field1.

Currently the behavior of copyFromObject is like vObj.field1 = vObj.field2

But consider the scenario where

vobj
=====
spec.template.timeoutSeconds: nil (unset)
|
|
v
-------------------------------------------
pobj
spec.template.timeoutSeconds: 300 ----------> set by controller to default

I would want to (expect that) copyFromObject in reversePatches as defined

                  - op: copyFromObject
                    fromPath: spec.template.spec.timeoutSeconds
                    path: spec.template.spec.timeoutSeconds
                    conditions:
                      - empty: true
                        path: spec.template.spec.timeoutSeconds

would copy across physical <-> virtual boundary, but since both the args to ApplyPatches originated from same source obj, it was not happening

if err != nil {
return nil, fmt.Errorf("error applying patches: %v", err)
}
Expand Down Expand Up @@ -78,15 +79,15 @@ func (s *patcher) ApplyPatches(ctx context.Context, fromObj, toObj client.Object
return outObject, nil
}

func (s *patcher) ApplyReversePatches(ctx context.Context, fromObj, otherObj client.Object, reversePatchConfig []*config.Patch, nameResolver patches.NameResolver) (controllerutil.OperationResult, error) {
originalUnstructured, err := toUnstructured(fromObj)
func (s *patcher) ApplyReversePatches(ctx context.Context, destObj, sourceObj client.Object, reversePatchConfig []*config.Patch, nameResolver patches.NameResolver) (controllerutil.OperationResult, error) {
Copy link
Member

@FabianKramm FabianKramm Nov 28, 2022

Choose a reason for hiding this comment

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

why rename the parameters?

Copy link
Contributor

@matskiv matskiv Nov 29, 2022

Choose a reason for hiding this comment

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

IMHO original name was confusing. So unless I understand the function incorrectly, the new name is much better.

originalUnstructured, err := toUnstructured(destObj)
if err != nil {
return controllerutil.OperationResultNone, err
}
fromCopied := originalUnstructured.DeepCopy()
destCopied := originalUnstructured.DeepCopy()

// apply patches on from object
err = patches.ApplyPatches(fromCopied, otherObj, reversePatchConfig, nil, nameResolver)
// apply patches on destCopied obj
err = patches.ApplyPatches(destCopied, sourceObj, reversePatchConfig, nil, nameResolver)
if err != nil {
return controllerutil.OperationResultNone, fmt.Errorf("error applying reverse patches: %v", err)
}
Expand All @@ -97,15 +98,15 @@ func (s *patcher) ApplyReversePatches(ctx context.Context, fromObj, otherObj cli
if err != nil {
return controllerutil.OperationResultNone, err
}
afterStatus, hasAfterStatus, err := unstructured.NestedFieldCopy(fromCopied.Object, "status")
afterStatus, hasAfterStatus, err := unstructured.NestedFieldCopy(destCopied.Object, "status")
if err != nil {
return controllerutil.OperationResultNone, err
}

// update status
if (hasBeforeStatus || hasAfterStatus) && !equality.Semantic.DeepEqual(beforeStatus, afterStatus) {
s.log.Infof("Update status of %s during reverse patching", fromCopied.GetName())
err = s.fromClient.Status().Update(ctx, fromCopied)
s.log.Infof("Update status of %s during reverse patching", destCopied.GetName())
err = s.fromClient.Status().Update(ctx, destCopied)
if err != nil {
return controllerutil.OperationResultNone, errors.Wrap(err, "update reverse status")
}
Expand All @@ -117,14 +118,14 @@ func (s *patcher) ApplyReversePatches(ctx context.Context, fromObj, otherObj cli
unstructured.RemoveNestedField(originalUnstructured.Object, "status")
}
if hasAfterStatus {
unstructured.RemoveNestedField(fromCopied.Object, "status")
unstructured.RemoveNestedField(destCopied.Object, "status")
}
}

// compare rest of the object
if !equality.Semantic.DeepEqual(originalUnstructured, fromCopied) {
s.log.Infof("Update %s during reverse patching", fromCopied.GetName())
err = s.fromClient.Update(ctx, fromCopied)
if !equality.Semantic.DeepEqual(originalUnstructured, destCopied) {
s.log.Infof("Update %s during reverse patching", destCopied.GetName())
err = s.fromClient.Update(ctx, destCopied)
if err != nil {
return controllerutil.OperationResultNone, errors.Wrap(err, "update reverse")
}
Expand Down