So I would not do this.. Essentially, we should have no knowledge of the semantics of the plugin inside this code.
Since we are passing the node to all plugins, the should essentially skip nodes if it does not pertain to them.

Agreed, this is not good. Is it just a matter of checking if node is Router or Ingress?

Yep.. thats all. Actually, its just Router.. (ingress is old code).

done

Can you please copy the code here? I know its duplication, but we are removing v1alpha3 enmasse from v1. So I don't want these things to get lost.

i did it but i still don't understand the rationale. if you copy the code over you still have identical v1 code, just now it's harder to tell it's identical. you still have the same dependency it's just now masked.
only if you actually make any changes to the code (however small) does it make sense to actually copy imo.

So with this new interface, we might need to call OnOutboundListener twice, once with nil for listerner to append filters (which caller also need to make sure to append them), and again to modify the listener itself?

(same for other plugin function)

please take another look - per our discussion i've made changes to the way the listener is built. now, everything except filters is created and passed to plugins. a final step is to marshal the filters (needed because http filters are in serialized format that cannot easily be appended to).

this is incorrect. You need the final listenAddress before you step into buildListener

basically move after switch

done - thanks.

Move these into the loop to guard against modifications by a plugin please.

done

log.Warn; log.Error is for irrecoverable errors that force Pilot to stop serving.

done

Do we need these vars? Just declare them inline in the CallbackListenerMutableObjects below.

yes, we do. they are passed by ptr and shared with marshalFilters.

Ah, I missed that; it's fine. Also, small nit: httpFilters, not hTTPFilters

i hate this but https://github.com/golang/go/wiki/CodeReviewComments#initialisms. http is high on the watch list. that necessarily leads to local vars looking strange like this. we can relax this rule for pilot i suppose (it's already a mix) but that's the default for go.

What you posted agrees with what I said:

Words in names that are initialisms or acronyms (e.g. "URL" or "NATO") have a consistent case. For example, "URL" should appear as "URL" or "url" (as in "urlPony", or "URLPony"), never as "Url". As an example: ServeHTTP not ServeHttp. For identifiers with multiple initialized "words", use for example "xmlHTTPRequest" or "XMLHTTPRequest".

It should be httpFilters, not hTTPFilters as it is in the PR. This is because initialisms should have a consistent case and visibility requires that the first letter be lower case, therefore the entire initialism should be lower case.

hmm, i wonder if this changed at some point. i could swear i've had to do these silly caps to make some lint check pass. anyway, done.

here and elsewhere, log.Warn

done

Here and elsewhere, try to avoid pre-declaring variables. E.g. for the filters, we only need them once to create the params; just declare them there.

see above

Please update to return the interface (plugin.Plugin), and get rid of the pointer receivers. The empty struct should be passed by value.

ptr->struct: done, although for the record i disagree with this.
what is the reason for returning the interface here?

btw. i assume you're changing Plugin name so i've left this alone.

The reason is that there's no need to leak implementation details. Code should not depend on authentication.Plugin, code should depend on plugin.Plugin. It's to keep the code that depends on this less brittle, and more clearly shows the semantics of the situation (we're implementing a plugin.Plugin; that's what is important, and the specific implementation is not important). I'd actually go further and make (authentication.)Plugin a private type.

W.r.t. pointer vs value, there's a couple of reasons. The empty struct is a true singleton in golang and the compiler can do more to optimize calls with it than with a pointer (which it can't prove much about). Further, you should always prefer value receivers to pointer receivers because a method with a value receiver can always be invoked on both a value and a pointer of the correct type, while a pointer method can only ever be invoked on a pointer of the correct type. See the relevant section of Effective Go.

Is there some reason you think this needs to be a pointer?

One key reason that values beat pointers is that the compiler knows the a function called on a value type can't mutate that value for the caller, so it can generate better/faster code compared to the same function operating on a pointer.

For the same reason, updating the Plugin interface to accept a value of CallbackListenerInputParams rather than a pointer to it, while keeping CallbackListenerMutableObjects a pointer, more clearly expresses the semantics of the usage of those parameters.

thanks for the detailed response.
the argument for returning a concrete type vs. interface is it allows the caller to decide which they want. for example in unit tests inside the package you may want to test private methods - that's not possible unless you have the concrete type. i think leaking should not be a big concern: that is controlled through public vs. private methods. if the implementer really wants a public method outside the interface spec let it be so (although i have a hard time imagining an argument to support that). it's probably unnecessary to use a heavy hand like returning interface to prevent folks from extending beyond the interface if they insist, it can be pushed back in code review.
the reasons for ptrs vs. struct are: one, although right now the struct is empty, that may not remain so going forward, in fact i think it's quite likely to grow given that we are encouraging plugin authors to move more complexity out to plugins. that means that in the future we may end up passing sizeable structs around everywhere without even realizing when it happened. with ptr, the cost may be bigger than zero but it's very small and will always remain fixed going forward. i think that's the main reason why for any struct type one usually sees interfaces implemented for a ptr.
the second reason is actually the contrary of your point above: i think it's better to leave the door open to let methods mutate the receiver. i'm not sure that the plugin model necessarily implies an immutable receiver - i can think of a couple of cases where this might not be so, collecting stats and locking. when in doubt i'd leave struct types as ptr implementations by default unless there's a good reason to do otherwise.
having said that, i think this is really splitting hairs in this case which is why i'm totally ok to make these not ptrs (and interface vs. concrete if you want). your arguments are totally valid - it's a choice and as you're the guy who's responsible for pilot codebase i'm ok with whatever you decide in the end. realistically we are in full control here and the design doesn't have to be be agonized over - it's not a big deal to change these things going forward.
regarding CallbackListenerInputParams - i agree in principle with your reasoning. i think the reason to keep it as a ptr is that generally we want to pass structs by ptr everywhere by default for efficiency (i know the official word is use non-ptr as a first choice but just try doing a code review where you pass a struct by value with the same set of folks). actually that's the reason i changed the signature to return the slices in the original version of this PR rather than having a "mutable" parameter. because that convention leaves you free to pass by ptr everywhere for efficiency with the implicit assumption that all input params are always read only - all mutations are through return values or by methods on a mutable type. but that version got voted down, so here we are. i'd actually prefer to return to the version where mutations are returned rather than done in place.

to paraphrase Martin's response to your question:

Is there some reason you think this needs to be a pointer?

Because we love C and come from C land ;)

Just return a literal:

func NewPlugins() []plugin.Plugin {
    return []plugin.Plugin{
        authn.NewPlugin(),
        mixer.NewPlugin(),
    }
}

Further, no need to name this NewDefaultPlugins; either DefaultPlugins or NewPlugins, (best, IMO) just Plugins. They're inherently the default plugins, since there's no other set of plugins.

name change and return literal: done (there was previously a NewPlugins that did something else).
regarding NewPlugins->Plugins, it's more conventional to name constructors NewType (i've unsuccessfully argued in that past that MakeType is often more appropriate). it seems to be the de facto naming choice for better or worse.

That's fine; actually I don't like New*, it's a java-ism that was imported into our codebase by people new to go. But, since that is the defacto standard in the codebase, I think consistency is more important than "correctness" (for whatever correctness is in this fairly subjective case).

is this function used? Is it the same as v1.AddStandardNodeAttributes?

it's the same. that was exactly my point earlier in the review where @rshriram requested this to be copied in. imo there's no point copying if the code is identical.