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

Implmentation of warning headers #69

Merged
merged 1 commit into from
Dec 30, 2022
Merged

Implmentation of warning headers #69

merged 1 commit into from
Dec 30, 2022

Conversation

crobby
Copy link
Contributor

@crobby crobby commented Dec 19, 2022 •
edited
Loading

Pass through warning headers which k8s returns.

Issue: rancher/rancher#39413

@crobby
Copy link
Contributor Author

crobby commented Dec 19, 2022 •
edited
Loading

requires rancher/apiserver#19 to be merged and released first
This has now been merged

@crobby crobby changed the title Initial implmentation of warning headers WIP: Initial implmentation of warning headers Dec 19, 2022
@crobby
Copy link
Contributor Author

crobby commented Dec 20, 2022

This seems to be working well. Sample output complements of curl...

* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
* We are completely uploaded and fine
< HTTP/2 201 
< content-encoding: gzip
< content-type: application/json
< expires: Wed 24 Feb 1982 18:42:00 GMT
< warning: 299 - would violate PodSecurity "restricted:latest": allowPrivilegeEscalation != false (container "nginx" must set securityContext.allowPrivilegeEscalation=false), unrestricted capabilities (container "nginx" must set securityContext.capabilities.drop=["ALL"]), runAsNonRoot != true (pod or container "nginx" must set securityContext.runAsNonRoot=true), seccompProfile (pod or container "nginx" must set securityContext.seccompProfile.type to "RuntimeDefault" or "Localhost")
< x-api-schemas: https://localhost:9553/v1/schemas
< content-length: 1098
< date: Tue, 20 Dec 2022 19:56:18 GMT

@crobby crobby changed the title WIP: Initial implmentation of warning headers Implmentation of warning headers Dec 20, 2022
@crobby crobby mentioned this pull request Dec 21, 2022
Merged
cmurphy
cmurphy reviewed Dec 21, 2022
View reviewed changes
pkg/client/factory.go Outdated
return newClient(ctx, p.clientCfg, s, namespace, p.impersonate)
func (p *Factory) Client(ctx *types.APIRequest, s *types.APISchema, namespace string, warningHandler rest.WarningHandler) (dynamic.ResourceInterface, error) {
config := *p.clientCfg
config.WarningHandler = warningHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be added to newClient and/or newDynamicClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this just be added to newClient and/or newDynamicClient?

That might be a slightly cleaner way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I switched it up a bit and added it to newDynamicClient. I think I like that a bit better.

@crobby
Copy link
Contributor Author

crobby commented Dec 22, 2022

Looks like a little rebasing is also in order. I'll work on that soon.

@crobby crobby force-pushed the warningheaders branch 3 times, most recently from be168df to 7f3e90d Compare December 22, 2022 14:52
cmurphy
cmurphy reviewed Dec 22, 2022
View reviewed changes
pkg/stores/partition/store.go
@@ -384,7 +385,7 @@ func toAPIEvent(apiOp *types.APIRequest, schema *types.APISchema, event watch.Ev
return apiEvent
}

apiEvent.Object = toAPI(schema, event.Object)
apiEvent.Object = toAPI(schema, event.Object, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the first version of this, the toAPIEvent function accepted warnings, are they not needed any more for events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's needed/useful in our case. I'll keep exploring though. It might be back.

@crobby
Copy link
Contributor Author

crobby commented Dec 29, 2022

Updated to reflect new apiserver version after that change merged

maxsokolovsky
maxsokolovsky reviewed Dec 29, 2022
View reviewed changes
go.mod Outdated
github.com/urfave/cli v1.22.10
github.com/urfave/cli/v2 v2.23.7
golang.org/x/sync v0.1.0
k8s.io/api v0.26.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we ready to use the 0.26 versions of the K8s modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good question. It slipped in with my mod updating. I can pull it back for now if we're not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to only bump rancher/apiserver

@crobby crobby force-pushed the warningheaders branch 4 times, most recently from 9a88cbe to a04cbb6 Compare December 29, 2022 15:41
@crobby crobby requested a review from maxsokolovsky December 29, 2022 16:10
maxsokolovsky
maxsokolovsky suggested changes Dec 30, 2022
View reviewed changes
pkg/stores/proxy/proxy_store.go Outdated
TableAdminClientForWatch(ctx *types.APIRequest, schema *types.APISchema, namespace string, warningHandler rest.WarningHandler) (dynamic.ResourceInterface, error)
}

type WarningBuffer struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this type need to be exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Updated accordingly

pkg/stores/proxy/proxy_store.go Outdated
Warnings []types.Warning
}

func (w *WarningBuffer) HandleWarningHeader(code int, agent string, text string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method called anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah...just not in our code. It's what gets called when the actual client sees a warning header

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, just to be sure, if it's called by someone else (who and how?), then does the type need to be exported? I am not finding any calls of this method anywhere across the org.

Copy link
Contributor Author

@crobby crobby Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When we create a client, we create a &warningBuffer to be used for that client. That warningBuffer has the HandleWarningHeader function that gets called down in client-go at runtime. I've witnessed this miracle in the debugger.
https://github.com/kubernetes/client-go/blob/master/rest/warnings.go#L137

@MbolotSuse @crobby
Initial implmentation of warning headers
956b735 
Attempts to pass through warning headers which k8s returns.
Requires an update to rancher/apiserver.
@crobby crobby requested a review from maxsokolovsky December 30, 2022 17:11
@crobby crobby merged commit ca73754 into rancher:master Dec 30, 2022
@rmweir rmweir mentioned this pull request Feb 9, 2023
Closed
@rmweir rmweir mentioned this pull request Feb 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxsokolovsky maxsokolovsky maxsokolovsky approved these changes

@cmurphy cmurphy cmurphy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@crobby @cmurphy @maxsokolovsky @MbolotSuse