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

WIP: Rebase 1.32 Pre.1 Diff #161

Open
wants to merge 58 commits into
base: kcp-1.32-baseline
Choose a base branch
from
Open

Conversation

mjudeikis
Copy link

What type of PR is this?

What this PR does / why we need it:

Just a diff for visibility.
branch pre.1 was pushed to fork already.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?


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


sttts and others added 30 commits February 12, 2025 16:14
Signed-off-by: Dr. Stefan Schimanski <[email protected]>

Co-authored-by: Mangirdas Judeikis <[email protected]>
Co-authored-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Mangirdas Judeikis <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Partially reverting e8b1d7d.
kcp's GC controller is event-based for now, and without locks we
may miss events during monitor syncs.

There are known issues that locking causes:
* kubernetes#101078
* kubernetes#127105

On-behalf-of: SAP [email protected]
Signed-off-by: Robert Vasek <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
…_quota_controller_patch.go

Ran:

    go run ./hack/kcp/resource_quota_controller_patch.go > pkg/controller/resourcequota/resource_quota_controller_patch.go

and modified the resulting file so that imports are in place, and changed
the main wait.UntilWithContext loop into a closure, so that when UpdateMonitors
is called, updating is done exactly once.

On-behalf-of: SAP [email protected]
Signed-off-by: Robert Vasek <[email protected]>
…gecollector_patch.go

Ran:

    go run ./hack/kcp/garbage_collector_patch.go > pkg/controller/garbagecollector/garbagecollector_patch.go

and modified the resulting file so that imports are in place, and changed
the main wait.UntilWithContext loop into a closure, so that when ResyncMonitors
is called, syncing is done exactly once.

On-behalf-of: SAP [email protected]
Signed-off-by: Robert Vasek <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
…gin and policy plugin framework

Signed-off-by: Marvin Beckers <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
…card partial metadata requests

Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
mjudeikis and others added 26 commits February 12, 2025 18:22
Co-authored-by: Marvin Beckers <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>
Signed-off-by: Mangirdas Judeikis <[email protected]>
On-behalf-of: SAP [email protected]
…bal service account

Signed-off-by: Dr. Stefan Schimanski <[email protected]>
…bal service account fix

Signed-off-by: Dr. Stefan Schimanski <[email protected]>
…eign service account tests

Signed-off-by: Dr. Stefan Schimanski <[email protected]>
…n and policy plugin framework

On-behalf-of: SAP [email protected]
Signed-off-by: Robert Vasek <[email protected]>
kcp doesn't implement protobuf codec yet, and so we need to disable it in the
client code.

This commit comments out --prefers-protobuf command line flag when invoking
client-gen in update-codegen.sh scripts in various places.

TODO: revert once kcp gains protobuf support.

On-behalf-of: SAP [email protected]
Signed-off-by: Robert Vasek <[email protected]>
Ran:

    hack/pin-dependency.sh github.com/kcp-dev/logicalcluster/v3 v3.0.5
    hack/pin-dependency.sh github.com/kcp-dev/apimachinery/v2 v2.0.1-0.20250207161408-e1833e4a94f2
    hack/pin-dependency.sh github.com/kcp-dev/client-go 5ae6774ab861f24965fc963d61af166c012f1ae0

On-behalf-of: SAP [email protected]
Signed-off-by: Robert Vasek <[email protected]>
Ran:

    hack/update-vendor.sh

On-behalf-of: SAP [email protected]
Signed-off-by: Robert Vasek <[email protected]>
Ran:

    hack/update-codegen.sh

On-behalf-of: SAP [email protected]
Signed-off-by: Robert Vasek <[email protected]>
@@ -170,17 +173,33 @@ func (v *legacyValidator) Validate(ctx context.Context, tokenData string, public
manuallyCreatedTokensTotal.WithContext(ctx).Inc()
}

v.patchSecretWithLastUsedDate(ctx, secret)
now := time.Now().UTC()
Copy link
Author

Choose a reason for hiding this comment

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

This looks like could be replaced by the function it replaces. Strange change.

Copy link

Choose a reason for hiding this comment

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

Comment on lines 434 to 438
if len(txnResp.Responses) == 0 || txnResp.Responses[0].GetResponseDeleteRange() == nil {
return errors.New(fmt.Sprintf("invalid DeleteRange response: %v", txnResp.Responses))
}
deleteResp := txnResp.Responses[0].GetResponseDeleteRange()
if deleteResp.Header == nil {
return errors.New("invalid DeleteRange response - nil header")
}
err = decode(s.codec, s.versioner, origState.data, out, deleteResp.Header.Revision, clusterName)
err = decode(s.codec, s.versioner, origState.data, out, txnResp.Revision, clusterName, shardName)
Copy link
Author

Choose a reason for hiding this comment

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

Do we remember why this is different from upstream?
@sttts maybe you recall?

Copy link

@gman0 gman0 Feb 17, 2025

Choose a reason for hiding this comment

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

Upstream's conditionalDelete was the same in v1.31.0 too: https://github.com/kubernetes/kubernetes/blob/v1.31.0/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go#L349-L385

Now, instead of explicitly doing Txn-If-Then-Else-Commit, there is a dedicated OptimisticDelete, and so deleteResp is not used anymore. Is this what you meant, @mjudeikis ?

clusterAware kcpkubernetesclientset.ClusterInterface
}

func (h *hack) AdmissionregistrationV1alpha1() admissionregistrationv1alpha1.AdmissionregistrationV1alpha1Interface {
Copy link
Author

Choose a reason for hiding this comment

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

im surprised this didn't changed

Copy link

Choose a reason for hiding this comment

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

The Interface interface seems to still have that method in upstream's v1.32: https://github.com/kubernetes/kubernetes/blob/v1.32.0/staging/src/k8s.io/client-go/kubernetes/clientset.go#L27

Unless you meant something else, @mjudeikis ?

Copy link
Author

Choose a reason for hiding this comment

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

I mean no new types were added/removed in 1.32.

Copy link

@gman0 gman0 Feb 17, 2025

Choose a reason for hiding this comment

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

git diff kcp-1.31.0..kcp-1.32-pre.7-review -- staging/src/k8s.io/apiserver/pkg/clientsethack/adapter.go says that:

  • coordinationV1alpha1 was replaced by coordinationV1alpha2, and
  • resourceV1beta1 was added

Should I document this somewhere?

@@ -216,23 +209,6 @@ func BuildGenericConfig(

ctx := wait.ContextForChannel(genericConfig.DrainedNotify())

// Use protobufs for self-communication.
Copy link
Author

Choose a reason for hiding this comment

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

TODO: This needs to be reverted once we add protoc support

Comment on lines +206 to +209
if err != nil {
return nil, fmt.Errorf("failed to create peer endpoint lease controller: %w", err)
}

Copy link
Author

Choose a reason for hiding this comment

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

needs to be dropped. There is no error here anymore.

Copy link

@gman0 gman0 Feb 17, 2025

Choose a reason for hiding this comment

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

Hmm seems to have been added by bf01714 by mistake? There doesn't seem to be anything creating an err there either. Thanks!

initializersChain := admission.PluginInitializers{genericInitializer}
initializersChain = append(initializersChain, pluginInitializers...)

admissionPostStartHook := func(hookContext server.PostStartHookContext) error {
Copy link
Author

Choose a reason for hiding this comment

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

Would be good to add comment this this is removed. I think history is bit lost.

Copy link

@gman0 gman0 Feb 17, 2025

Choose a reason for hiding this comment

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

Are there some docs I could link to? Originally removed in 3211e1f , but without details.

Comment on lines +242 to +244
if completed.Authorization != nil {
completed.Authorization.Complete()
}
Copy link
Author

Choose a reason for hiding this comment

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

Potentially quick upstream change and we can drop this.

Copy link

@gman0 gman0 Feb 17, 2025

Choose a reason for hiding this comment

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

We can actually drop this already, (*BuiltInAuthorizationOptions).Complete() already checks if the receiver is nil.

In v1.31.0, this was indeed not being checked: https://github.com/kubernetes/kubernetes/blob/v1.31.0/pkg/kubeapiserver/options/authorization.go#L87-L91

@@ -67,13 +67,6 @@ func (a statusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.O
// track changed fields in the status update.
managedFields := newCustomResourceObject.GetManagedFields()

// KCP PATCH START
Copy link
Author

Choose a reason for hiding this comment

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

The fact this commit is here means we added in some before, and we need to clean and remove them.
This should not exist anymore.

Copy link

Choose a reason for hiding this comment

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

Seems to have been added in c6b52ef . I'll try to go through it and see if there are any more leftovers from this.

Copy link

Choose a reason for hiding this comment

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


// kcp: needed for setKCPOriginalAPIVersionAnnotation().
// It expects a context with clusterContextKey key set.
ctx context.Context
Copy link
Author

Choose a reason for hiding this comment

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

:'( one more ctx wiring. Maybe we could try to get this upstreamed too as its apiserver code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants