Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Always retain metadata.finalizers and metadata.annotations #1010

Merged
merged 1 commit into from
Jul 15, 2019
Merged

Always retain metadata.finalizers and metadata.annotations #1010

merged 1 commit into from
Jul 15, 2019

Conversation

marun
Copy link
Contributor

@marun marun commented Jun 20, 2019 •
edited
Loading

Now that overriding via jsonpatch is supported, it is possible to restrict modification of the annotations and finalizers fields to overrides. Modifications to these collection fields can be made using the add or remove jsonpatch operations to ensure that a given item exists (or does not exist) with less chance of conflicting with local controllers.

kind: FederatedDeployment
...
spec:
  ...
  overrides:
    - clusterName: cluster1
      clusterOverrides:
        # Ensure the annotation "foo: bar" exists
        - path: "/metadata/annotations"
          op: "add"
          value:
            foo: bar
        # Ensure an annotation with key "foo" does not exist
        - path: "/metadata/annotations/foo"
          op: "remove"

To support this change, it was necessary to move the application of overrides from before retention of local values to after so that overrides would be applied to the result of retention.

This change suggests implementing selector-based overrides to simplify applying an annotation across multiple clusters.

Fixes #982

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 20, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marun

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 20, 2019
@marun marun requested a review from shashidharatd June 20, 2019 18:48
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 20, 2019
@marun marun mentioned this pull request Jun 20, 2019
Closed
@font
Copy link
Contributor

font commented Jun 24, 2019

@marun please rebase.

@marun
Copy link
Contributor Author

marun commented Jun 25, 2019

Rebased

@marun marun mentioned this pull request Jun 25, 2019
Closed
1 task
@marun marun changed the title Always retain metadata.finalizers Always retain metadata.finalizers and metadata.annotations Jun 25, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 25, 2019
@marun marun changed the title Always retain metadata.finalizers and metadata.annotations WIP Always retain metadata.finalizers and metadata.annotations Jun 25, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2019
@marun marun changed the title WIP Always retain metadata.finalizers and metadata.annotations Always retain metadata.finalizers and metadata.annotations Jul 2, 2019
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 2, 2019
@marun
Copy link
Contributor Author

marun commented Jul 2, 2019

Rebased and ready for review

xunpan
xunpan reviewed Jul 5, 2019
View reviewed changes
Copy link
Contributor

@xunpan xunpan left a comment

Choose a reason for hiding this comment

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

Overall, LGTM.

2 comments for message clarify.

docs/userguide.md Outdated
registered with KubeFed, the following operations are executed in sequence:

- A new resource is computed from the template of the federated resource
- If an existing resource is present, field values are retained
Copy link
Contributor

Choose a reason for hiding this comment

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

field values are retained. I think it is not accurate because only partial field values are retained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. PTAL.

pkg/controller/sync/resource.go Outdated
@@ -165,6 +165,15 @@ func (r *federatedResource) ObjectForCluster(clusterName string) (*unstructured.
}
obj := &unstructured.Unstructured{Object: templateBody}

if len(obj.GetAnnotations()) > 0 {
r.RecordError("AnnotationsNotSupported", errors.New("metadata.annotations will not be set to avoid conflicting with local controllers."))
Copy link
Contributor

Choose a reason for hiding this comment

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

This message seems misunderstanding. Actually, metadata.annotations still can be set. But by override instead of by template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. PTAL

font
font reviewed Jul 9, 2019
View reviewed changes
Copy link
Contributor

@font font left a comment

Choose a reason for hiding this comment

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

This LGTM, just a clarifying comment. I see "local" controllers used throughout. What is meant by "local" here? Local to the cluster? If so, this would technically apply to any controller, not just ones local to the cluster.

@marun
Copy link
Contributor Author

marun commented Jul 10, 2019

@font I've replaced references to 'local controller' with 'controllers in member clusters'. Does that make more sense?

font
font reviewed Jul 10, 2019
View reviewed changes
Copy link
Contributor

@font font left a comment

Choose a reason for hiding this comment

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

@font I've replaced references to 'local controller' with 'controllers in member clusters'. Does that make more sense?

Yep, thanks!

@marun
Copy link
Contributor Author

marun commented Jul 14, 2019

Rebased.

@marun
Always retain metadata.finalizers and metadata.annotations
b63ade8 
Since finalizers and annotations will typically be managed by local
controllers, ensure that these fields cannot be set from the template
and are always retained from an existing resource. It will still be
possible to apply overrides to add or remove entries from these
collections.

As part of this change, it was necessary to perform field retention
before overriding to ensure that retained fields could be the subject
of override directives.
@marun
Copy link
Contributor Author

marun commented Jul 15, 2019

Rebased

@shashidharatd
Copy link
Contributor

/lgtm
Thanks @marun

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
@k8s-ci-robot k8s-ci-robot merged commit 0e8770f into kubernetes-retired:master Jul 15, 2019
@marun marun deleted the retain-finalizers branch July 29, 2019 12:23
This was referenced Aug 8, 2019
Closed
Merged
@makkes makkes mentioned this pull request Apr 22, 2020
Closed
@dev-e
Copy link

dev-e commented Aug 27, 2020

Hi! I think annotations still should propagate for non controller-managed resources, e.g. Ingress or Service because in most cases these resources should be consistent across the entire deployment. If you want to override it for some clusters, it still may be done via jsonpatch. Otherwise, you are forced to add the same annotations for all member clusters which is not very convenient.

@mvaal mvaal mentioned this pull request Dec 17, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xunpan xunpan xunpan left review comments

@font font font left review comments

@gyliu513 gyliu513 Awaiting requested review from gyliu513

@shashidharatd shashidharatd Awaiting requested review from shashidharatd

Assignees

@shashidharatd shashidharatd

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Infinite update loop on FederatedDeployment
6 participants
@marun @k8s-ci-robot @font @shashidharatd @dev-e @xunpan