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

feat: trigger ssa metadata for direct controller #1745

Closed
wants to merge 1 commit into from
Closed

Conversation

acpana
Copy link
Collaborator

@acpana acpana commented May 9, 2024

Following the DCL/ TF controllers to trigger ssa metadata annotation.

Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from acpana. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@acpana acpana changed the title feat: trigger ssa metadata feat: trigger ssa metadata for direct controller May 9, 2024
@acpana acpana marked this pull request as ready for review May 9, 2024 06:48
@acpana
Copy link
Collaborator Author

acpana commented May 10, 2024

I'm not 100% sure that we want/ need this.

cc @yuwenma / @maqiuyujoyce

@acpana acpana added this to the 1.118 milestone May 10, 2024
@maqiuyujoyce
Copy link
Collaborator

I traced back to the change when TriggerManagedFieldsMetadata was added. It looks like this is used to enable the externally managed fields on clusters between K8s 1.16 and 1.18 because the metadata is not automatically triggered.

I think it'll be safe if we keep it; though @yuwenma do you think it'll be more sustainable if we keep track of versions of clusters where Config Connector is installed so that we can clean up version-specific logic over time?

@yuwenma
Copy link
Collaborator

yuwenma commented May 14, 2024

I would say may be not a necessity. But I'm not very familiar with the KCC cnrm.cloud.google.com/supports-ssa annotation. By reading its doc https://cloud.google.com/config-connector/docs/concepts/managing-fields-externally#behavior_with_server-side_apply, it is not clear to me how it works with the k8s server-side apply. @justinsb would you mind justifying?

@justinsb
Copy link
Collaborator

As far as I can tell we never read this annotation nor the field manager. I concur with @maqiuyujoyce that this is not needed in modern k8s version, where you basically always have managedFields. Also, given that we aren't using managedFields in our direct controllers (at least not yet), I don't think we need this annotation. If it turns out we do need it, we can always add it later.

@justinsb
Copy link
Collaborator

/hold

Because I don't think we want this in 1.118. If people agree, not sure if we move to 1.119 or just close?

@maqiuyujoyce
Copy link
Collaborator

Yeah I'm okay with not having it as we can always add it if there are some unexpected issues.
Regarding cnrm.cloud.google.com/supports-ssa, I never had to interact with this annotation. But we should update the public doc to reflect our change: https://cloud.google.com/config-connector/docs/reference/annotations

@yuwenma yuwenma modified the milestones: 1.118, Future May 16, 2024
@yuwenma
Copy link
Collaborator

yuwenma commented May 24, 2024

Close the PR to clear up the PR review list. We can re-open this PR if the question comes up to the table in the future.

@yuwenma yuwenma closed this May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants