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.

Sign up for GitHub

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

Jump to bottom

Update sigs.k8s.io/structured-merge-diff to v4.4.0 #121575

Merged
merged 4 commits into from
Nov 1, 2023
Merged

Update sigs.k8s.io/structured-merge-diff to v4.4.0 #121575

merged 4 commits into from
Nov 1, 2023

Conversation

apelisse
Copy link
Member

@apelisse apelisse commented Oct 27, 2023 •
edited by liggitt

What type of PR is this?

/kind bug
/kind feature

It's a feature that solves a bug ...

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #113482

Special notes for your reviewer:

Does this PR introduce a user-facing change?

We probably need something here, not sure how to phrase it yet.

Fixes bugs in handling of server-side apply, create, and update API requests for objects containing duplicate items in keyed lists.
* A `create` or `update` API request with duplicate items in a keyed list no longer wipes out managedFields. Examples include env var entries with the same name, or port entries with the same containerPort in a pod spec.
* A server-side apply request that makes unrelated changes to an object which has duplicate items in a keyed list no longer fails, and leaves the existing duplicate items as-is.
* A server-side apply request that changes an object which has duplicate items in a keyed list, and modifies the duplicated item removes the duplicates and replaces them with the single item contained in the server-side apply request.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 27, 2023
@k8s-ci-robot k8s-ci-robot added area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kube-proxy area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 27, 2023
@dims
Copy link
Member

dims commented Oct 27, 2023

thanks @apelisse ! we had to jump through hoops in #113482 (comment) glad this is sorted out.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 31, 2023
@apelisse
Copy link
Member Author

Fixed the linting issue.

@seans3
Copy link
Contributor

seans3 commented Oct 31, 2023

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 31, 2023
@liggitt
Copy link
Member

liggitt commented Oct 31, 2023 •
edited

/lgtm
/approve

I ran a modified version of the integration test (which puts the new possibilities for managedFields directly into etcd and then exercises update and apply requests) against master before this PR merges to ensure an old server doing an update of an object containing managedFields like this still works (it does, and still drops the managed fields), and an old server doing an apply of an object containing duplicates and managedFields like this does not fail worse (it doesn't, the apply still fails but only the immediate request fails).

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: c97c9afaa9529b0b6ab52ccee328629e6f76f3fd

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, dims, liggitt

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2023
@apelisse
Copy link
Member Author

Thanks for the dilligence Jordan!

@k8s-ci-robot k8s-ci-robot merged commit 593a17d into kubernetes:master Nov 1, 2023
19 checks passed
@thockin
Copy link
Member

thockin commented Nov 1, 2023

  1. We DEFINTELY need a good release note.

  2. Can someone summarize the change in terms of user-facing semantics? We know several failure modes, are they totally eliminated? Mitigated? Compartmentalized?

@liggitt
Copy link
Member

liggitt commented Nov 1, 2023

The failure modes I know of are eliminated. Updated the release note.

  • A create or update API request with duplicate items in a keyed list no longer wipes out managedFields. Examples include env var entries with the same name, or port entries with the same containerPort in a pod spec.
  • A server-side apply request that makes unrelated changes to an object which has duplicate items in a keyed list no longer fails, and leaves the existing duplicate items as-is.
  • A server-side apply request that changes an object which has duplicate items in a keyed list, and modifies the duplicated item removes the duplicates and replaces them with the single item contained in the server-side apply request.

@k8s-ci-robot k8s-ci-robot added 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 Nov 1, 2023
@apelisse
Copy link
Member Author

apelisse commented Nov 1, 2023

Do we need to explain the actual impact to managed fields?

@liggitt
Copy link
Member

liggitt commented Nov 1, 2023

Do we need to explain the actual impact to managed fields?

I don't think so, that's pretty inside baseball to kube-apiserver

@thockin
Copy link
Member

thockin commented Nov 1, 2023

"[SHOULD NOT HAPPEN] failed to update managedFields" err=<
failed to convert new object (default/ssa-test; apps/v1, Kind=Deployment) to smd typed: errors:
.spec.template.spec.containers[name="c"].env: duplicate entries for key [name="FOO"]
.spec.template.spec.containers[name="c"].ports: duplicate entries for key [containerPort=93,protocol="TCP"]

versionKind="/, Kind=" namespace="default" name="ssa-test"

@thockin
Copy link
Member

thockin commented Nov 1, 2023 

$ alias k=`pwd`/_output/bin/kubectl

$ cat /tmp/d.yaml 
apiVersion: apps/v1
kind: Deployment
metadata:
  name: ssa-test
spec:
  replicas: 1
  selector:
    matchLabels:
      app: ssa-test
  template:
    metadata:
      labels:
        app: ssa-test
    spec:
      containers:
      - name: c
        image: busybox
        ports:
        - containerPort: 93
          hostPort: 9376
        - containerPort: 93
          hostPort: 7693
        env:
        - name: FOO
          value: "foo"
        - name: FOO
          value: "foo-$(FOO)"

$ # With an older apiserver...

$ k delete -f /tmp/d.yaml; k create -f /tmp/d.yaml 
Error from server (NotFound): error when deleting "/tmp/d.yaml": deployments.apps "ssa-test" not found
Warning: spec.template.spec.containers[0].env[1].name: duplicate name "FOO"
deployment.apps/ssa-test created

$ k replace -f /tmp/d.yaml 
Warning: spec.template.spec.containers[0].env[1].name: duplicate name "FOO"
deployment.apps/ssa-test replaced

$ k apply -f /tmp/d.yaml 
Warning: resource deployments/ssa-test is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
Warning: spec.template.spec.containers[0].env[1].name: duplicate name "FOO"
deployment.apps/ssa-test configured

$ k apply --server-side -f /tmp/d.yaml 
Error from server: failed to create manager for existing fields: failed to convert new object (default/ssa-test; apps/v1, Kind=Deployment) to smd typed: errors:
  .spec.template.spec.containers[name="c"].env: duplicate entries for key [name="FOO"]
  .spec.template.spec.containers[name="c"].ports: duplicate entries for key [containerPort=93,protocol="TCP"]

$ # With a brand-new apiserver (built minute ago from HEAD)...

$ k delete -f /tmp/d.yaml; k create -f /tmp/d.yaml 
deployment.apps "ssa-test" deleted
deployment.apps/ssa-test created

$ k replace -f /tmp/d.yaml 
deployment.apps/ssa-test replaced

$ k apply -f /tmp/d.yaml 
Warning: resource deployments/ssa-test is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
deployment.apps/ssa-test configured

$ k apply --server-side -f /tmp/d.yaml 
Error from server: failed to create manager for existing fields: failed to convert new object (default/ssa-test; apps/v1, Kind=Deployment) to smd typed: errors:
  .spec.template.spec.containers[name="c"].env: duplicate entries for key [name="FOO"]
  .spec.template.spec.containers[name="c"].ports: duplicate entries for key [containerPort=93,protocol="TCP"]

On the new apiserver the "create" operation logged:

"[SHOULD NOT HAPPEN] failed to update managedFields" err=<
	failed to convert new object (default/ssa-test; apps/v1, Kind=Deployment) to smd typed: errors:
	  .spec.template.spec.containers[name="c"].env: duplicate entries for key [name="FOO"]
	  .spec.template.spec.containers[name="c"].ports: duplicate entries for key [containerPort=93,protocol="TCP"]
 > versionKind="/, Kind=" namespace="default" name="ssa-test"

On the SSA operation (note there's no change in the file) it logged:

apiserver received an error that is not an metav1.Status: &errors.errorString{s:"failed to create manager for existing fields: failed to convert new object (default/ssa-test; apps/v1, Kind=Deployment) to smd typed: errors:\n  .spec.template.spec.containers[name=\"c\"].env: duplicate entries for key [name=\"FOO\"]\n  .spec.template.spec.containers[name=\"c\"].ports: duplicate entries for key [containerPort=93,protocol=\"TCP\"]"}: failed to create manager for existing fields: failed to convert new object (default/ssa-test; apps/v1, Kind=Deployment) to smd typed: errors:
  .spec.template.spec.containers[name="c"].env: duplicate entries for key [name="FOO"]
  .spec.template.spec.containers[name="c"].ports: duplicate entries for key [containerPort=93,protocol="TCP"]

@apelisse
Copy link
Member Author

apelisse commented Nov 1, 2023 •
edited

You still can't server-side apply duplicates. I was re-reading Jordan's note and it doesn't mention that, maybe worth adding:

  • SSA still doesn't allow duplicates, you can replace with a single value or make unrelated changes but you still can't apply duplicates.

@thockin
Copy link
Member

thockin commented Nov 1, 2023

Apparently I screwed up my rebase and didn't have this change. Let me try again

@thockin
Copy link
Member

thockin commented Nov 1, 2023 

$ k delete -f /tmp/d.yaml; k create -f /tmp/d.yaml 
deployment.apps "ssa-test" deleted
deployment.apps/ssa-test created

$ k replace -f /tmp/d.yaml 
deployment.apps/ssa-test replaced

$ k apply -f /tmp/d.yaml 
Warning: resource deployments/ssa-test is missing the kubectl.kubernetes.io/last-applied-configuration annotation which is required by kubectl apply. kubectl apply should only be used on resources created declaratively by either kubectl create --save-config or kubectl apply. The missing annotation will be patched automatically.
deployment.apps/ssa-test configured

$ k apply --server-side -f /tmp/d.yaml 
Error from server: failed to create typed patch object (default/ssa-test; apps/v1, Kind=Deployment): errors:
  .spec.template.spec.containers[name="c"].env: duplicate entries for key [name="FOO"]
  .spec.template.spec.containers[name="c"].ports: duplicate entries for key [containerPort=93,protocol="TCP"]

$ cat /tmp/d2.yaml 
apiVersion: apps/v1
kind: Deployment
metadata:
  name: ssa-test
  labels:
    foo: bar

$ k apply --server-side -f /tmp/d2.yaml 
deployment.apps/ssa-test serverside-applied

As my teenager would say... "my bad"

@thockin
Copy link
Member

thockin commented Nov 1, 2023

I do still get logs:

E1101 17:51:35.316459 3700183 status.go:71] apiserver received an error that is not an metav1.Status: &errors.errorString{s:"failed to create typed patch object (default/ssa-test; apps/v1, Kind=Deployment): errors:\n  .spec.template.spec.containers[name=\"c\"].env: duplicate entries for key [name=\"FOO\"]\n  .spec.template.spec.containers[name=\"c\"].ports: duplicate entries for key [containerPort=93,protocol=\"TCP\"]"}: failed to create typed patch object (default/ssa-test; apps/v1, Kind=Deployment): errors:
  .spec.template.spec.containers[name="c"].env: duplicate entries for key [name="FOO"]
  .spec.template.spec.containers[name="c"].ports: duplicate entries for key [containerPort=93,protocol="TCP"]

@apelisse
Copy link
Member Author

apelisse commented Nov 1, 2023

Something/someone is sending a patch with duplicates. That's literally trying to parse the server-side applied object.

@liggitt
Copy link
Member

liggitt commented Nov 1, 2023

yeah, you can't SSA duplicate items, but you can SSA to objects which have duplicate items added via create or update calls

@deads2k deads2k mentioned this pull request Feb 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@alexzielenski alexzielenski Awaiting requested review from alexzielenski

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@dims dims Awaiting requested review from dims

Assignees

@dims dims

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kube-proxy area/kubectl area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
SIG CLI
Archived in project
[sig-release] Bug Triage
Archived in project
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.

ContainerPorts SSA key is insufficiently unique - SSA fails with "duplicate" error when the contents are valid
7 participants
@apelisse @dims @k8s-ci-robot @hailkomputer @liggitt @seans3 @thockin