-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Rebase 1.11 #20033
Rebase 1.11 #20033
Conversation
glide.yaml
Outdated
# pod - sjenning | ||
# origin-3.11-cadvisor-v0.30.1 MISSING PATCHES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjenning patches do not apply. This is going to backlevel you. You'll need to update the fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fork has been updated openshift/google-cadvisor#3 @runcom
if err := authHandler.requestContextMapper.Update(req, ctx); err != nil { | ||
return false, fmt.Errorf("failed to attach audit event to context: %v", err) | ||
} | ||
req = req.WithContext(request.WithAuditEvent(ctx, ev)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liggitt did I get this bit right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't see why the requestContextMapper.Update call was required before... we're updating a field of a pointer audit event in place. I don't see why the ev.ResponseStatus.Message = getAuthMethods(req)
call wouldn't be all that is required.
the new request isn't being propagated anywhere useful, so the req = req.WithContext(...)
isn't likely to be effective.
@soltysh, where is this data supposed to end up, and how do we tell if it is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind it is to log the failed authn requests, since we can't pass the failedHandler, like uptream does. This will be then logged in the regular audit log. The way it was previously done was to update the request with the new data in the context. I don't think the current implementation is right, you're just updating the request object, but you're not updating the mapper between request and context with the new state. The other option is that my understanding of requestContextMapper
is wrong, in which case this is not needed at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think req = req.WithContext(request.WithAuditEvent(ctx, ev))
should be dropped
@soltysh, can you describe what we should look for in an audit event to know if this is working correctly with that removed? a failed oauth login attempt should result in an audit event with the auth method included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WithAuditEvent just wraps the context (Golang's linear wrapping chain context design...). So the returned context is a different object than before. You have to attach that to the request again. That's why we had the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same is true for the req = req.WithContext(...)
line now. If you look into WithContext, it does a shallow copy, but does not mutate the old req
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or in other words: a context is not a map which is updated in-place. It is a chain of if key == bla { .... } else ask-the-underlying context
constructs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but we're modifying the existing audit event here, so we shouldn't need to change the context at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, AuditEventFrom is called before. So we change the event in place. Didn't see that from the snippet.
3dd4f26
to
de18cb9
Compare
@jwmatthews I think this is the first time you're in the rebase. There's a commit with JMATTHEWS in the title. You don't need to worry about the rest of the pull, but you should review that commit for accuracy. In this case, it's related to the TSB. |
@@ -112,26 +108,22 @@ func NewPluginInitializer( | |||
svcCache := service.NewServiceResolverCache(kubeInternalClient.Core().Services(metav1.NamespaceDefault).Get) | |||
defaultRegistryFunc, err := svcCache.Defer(defaultRegistry) | |||
if err != nil { | |||
return nil, nil, fmt.Errorf("OPENSHIFT_DEFAULT_REGISTRY variable is invalid %q: %v", defaultRegistry, err) | |||
return nil, fmt.Errorf("OPENSHIFT_DEFAULT_REGISTRY variable is invalid %q: %v", defaultRegistry, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder that we should nuke this environment variable at this point
|
||
"github.com/golang/glog" | ||
"github.com/openshift/origin/pkg/admission/namespaceconditions" | ||
"k8s.io/apimachinery/pkg/api/meta" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: sort the imports based on conventions
@@ -199,7 +206,15 @@ func BuildMasterConfig( | |||
|
|||
kubeAPIServerConfig: kubeAPIServerConfig, | |||
additionalPostStartHooks: map[string]genericapiserver.PostStartHookFunc{ | |||
"openshift.io-AdmissionInit": admissionPostStartHook, | |||
"openshift.io-RESTMapper": func(context genericapiserver.PostStartHookContext) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe post-rebase but can we extract this into separate function?
@@ -9,6 +9,7 @@ import ( | |||
|
|||
restful "github.com/emicklei/go-restful" | |||
"github.com/golang/glog" | |||
"k8s.io/apimachinery/pkg/api/meta" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add empty line...
@@ -4,6 +4,8 @@ import ( | |||
"sync" | |||
|
|||
"github.com/golang/glog" | |||
"k8s.io/apimachinery/pkg/api/meta" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: guess your goimports is broken
if gvk := info.Mapping.GroupVersionKind; kapierror.IsAlreadyExists(err) && | ||
gvk.Kind == roleBindingKind && roleBindingGroups.Has(gvk.Group) && defaultRoleBindingNames.Has(info.Name) { | ||
return false | ||
// TODO, stop doing this crazy thing, but for now it's a very simple way to get the unstructured objects we need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add this into post-rebase issues?
} | ||
return firstPod.Name, nil | ||
} | ||
|
||
// GetFirstPod returns a pod matching the namespace and label selector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k this is the stuff we need to to copy out from kubectl ? can we have follow up issue where we extract this from kubectl into some kind of library in kube?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also if yes, can you add comment/todo that this was copied out? )
if err != nil { | ||
return nil, 0, err | ||
} | ||
pods := []*corev1.Pod{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we initialize this with make and len(podList.Items) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually nvmd. this should be fixed upstream...
SOLTYSH commits lgtm, so far. |
Reminder to self not to squash anything for mfojtik or soltysh before |
de18cb9
to
180b247
Compare
the branch builds (but not on master apparently). Oh the conflicts. @soltysh are the oc changes critical? Can we hold some oc changes out? those may be mine I guess.... |
@soltysh, it compiles. Barring major problems, I don't plan to clean up oc anymore. |
Also, I started switching printers, but it was too much. I may revert the printer commit and keep the patch to get through this rebase if I start having trouble |
@@ -489,7 +489,7 @@ func StartNode(nodeConfig configapi.NodeConfig, components *utilflags.ComponentF | |||
|
|||
// runKubeletInProcess runs the kubelet command using the provide args | |||
func runKubeletInProcess(kubeletArgs []string) error { | |||
cmd := kubeletapp.NewKubeletCommand() | |||
cmd := kubeletapp.NewKubeletCommand(wait.NeverStop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow up to plumb the channel that gets notified of interrupt signals
if len(resource) > 0 { | ||
fmt.Fprintf(out, "%s/%s\n", resource, name) | ||
if len(kindString) > 0 { | ||
fmt.Fprintf(out, "%s/%s\n", kindString, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lowercased, right?
if len(resource) > 0 { | ||
fmt.Fprintf(out, "%s \"%s\" %s%s\n", resource, name, operation, dryRunMsg) | ||
if len(kindString) > 0 { | ||
fmt.Fprintf(out, "%s \"%s\" %s%s\n", kindString, name, operation, dryRunMsg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lowercased, right?
return sysctl.NewMustMatchPatterns(safeWhitelist, allowedUnsafeSysctls, forbiddenSysctls), nil | ||
} | ||
|
||
// TODO promote like kube did |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @ingvagabund
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @php-coder @stlaz
Underlying gonum/graph assigns conflicting IDs to new nodes after node deletion. Thus two different nodes amy be assigned the same ID. This leads to panic when an edge is set between them. Temporarily disabling event handling until the package is fixed. This ensures that no new nodes will be added to the graph after the first node gets removed. Signed-off-by: Michal Minář <miminar@redhat.com>
ed3cecd
to
d206b32
Compare
@@ -138,10 +134,12 @@ func TestGroupReaper(t *testing.T) { | |||
|
|||
actual := []interface{}{} | |||
oreactor := func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) { | |||
t.Logf("oreactor: %#v", action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: my bad, that is not needed here. Post-rebase remove (note to myself).
/test launch-gcp |
@miminar looks like the image pruning fix turned a test red. I need to let this run for GCE but on the next push I'm going to disable that test. |
verify is green when you look at the link, but showing red here. /retest |
@deads2k yeah, it's |
/test unit |
…ses when we got none from the cloudprovider" This reverts commit 7354bbe.
270e67b
to
edbddad
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, mfojtik 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 |
we are back to |
/retest |
@deads2k: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
merging, will start on followups. |
…iags This seems fallout from 1.11 rebase (openshift#20033)
cmd/hypershift
builds, nothing else. I'm only opening it now so that I can point out how much we'd gain by splitting separate components out of the core repo.Failing
Passing
OPENSHIFT_DEFAULT_REGISTRY
env var from @mfojtik (Deprecate OPENSHIFT_DEFAULT_REGISTRY environment variable #20150)TestSNI
unit flakes:
dnsmasq_test.go:180: got an unexpected second call: uk.org.thekelleys.SetDomainServers