Thank you for the review, I'm currently on sick leave and will get back to you as soon as I am able to.

@ravilr can you advise me on the failed checks please?

1. Earthly +generate seems to detect changes on generated files, when running make reviewable everything seems fine and no changes occur on my machine.

2. The e2e test fails for realtime-compositions while trying to delete a resource, I suspect that this has nothing to do with my changes

changes LGTM. Thanks. Still requires review from the maintainers. @Argannor do you want to remove the Draft: prefix from the PR title ?

Reg. CI job failures, may be maintainers can help re-kick them to run again.

Rebased the branch on top of master to resolve merge conflicts. However I can't get the E2E tests to run on my local machine, also it seems like these tests fail on other pull requests as well, therefore I suspect that it's not related to my changes.

Kindly requesting a review from one of the maintainers

While deploying this change in our test cluster, I had to adjust the status.endpoint field of FunctionRevision resources to contain the correct port, as GRPC only does a DNS lookup but does not discover the configured target port.

With these changes and my PR to provider-kubernetes crossplane-contrib/provider-kubernetes#301 I was able to

• run crossplane, provider-kubernetes and function-patch-and-transform on the host network with custom ports
• the Kubernetes API server was able to reach the conversion webhook of provider-kubernetes
• Crossplane was able to communicate with function-patch-and-transform

while running EKS using Calico as the CNI.

Looking forward to your feedback

@Argannor did you happen to have an example of what a DeploymentRuntimeConfig would look like that "overrides the default ports (metrics, webhook, grpc) in both Deployment and Service." You've probably already created one during your testing. I'm particularly interested in what it looks like after the addition of the serviceTemplate.spec that previously has not existed in the DeploymentRuntimeConfig API.

@jbw976 sure thing here you go:

# K8s provider
apiVersion: pkg.crossplane.io/v1
kind: Provider
metadata:
  name: provider-kubernetes
spec:
  package: "index.docker.io/crossplanecontrib/provider-kubernetes:v0.15.0"
  runtimeConfigRef:
    name: provider-kubernetes
---
apiVersion: pkg.crossplane.io/v1beta1
kind: DeploymentRuntimeConfig
metadata:
  name: provider-kubernetes
spec:
  deploymentTemplate:
    spec:
      selector: { }
      template:
        spec:
          hostNetwork: true
          nodeSelector:
            kubernetes.io/os: linux
          imagePullSecrets:
            - name: crossplane-workaround
          containers:
            - name: package-runtime
              args:
                - --webhook-port=9610
  serviceTemplate:
    spec:
      ports:
        - name: webhook
          port: 9200 # doesn't matter
          targetPort: 9610
          protocol: TCP

And more or less the same for functions as well:

apiVersion: pkg.crossplane.io/v1beta1
kind: Function
metadata:
  name: function-patch-and-transform
spec:
  package: xpkg.upbound.io/crossplane-contrib/function-patch-and-transform:v0.7.0
  runtimeConfigRef:
    name: function-patch-and-transform
---
apiVersion: pkg.crossplane.io/v1beta1
kind: DeploymentRuntimeConfig
metadata:
  name: function-patch-and-transform
spec:
  deploymentTemplate:
    spec:
      selector: { }
      template:
        spec:
          hostNetwork: true
          containers:
            - name: package-runtime
              args:
                - --address=:9612
  serviceTemplate:
    spec:
      ports:
        - name: webhook
          port: 9200 # doesn't matter
          targetPort: 9612
          protocol: TCP

And the values for the crossplane helm chart:

hostNetwork: true
dnsPolicy: "ClusterFirstWithHostNet"
webhooks:
  port: 9600
metrics:
  enabled: true
  port: 9601
readiness:
  port: 9602

Edit: removed irrelevant parameters and added values.yaml

Hi @jbw976, thank you for testing these changes! I only used a "real" cluster to test the changes, as I'm currently unable to run kind on my machine.

Let me try to answer your questions:

provider-kubernetes

• Unfortunately I don't have the changes in a public registry, but in a private one.
• That's also the reason for the imagePullSecrets, I host both the modified kubernetes provider and crossplane in a private registry.
• Honestly never tried to remove the port, depends on whether the manifest will still pass the validation

function-patch-and-transform

• Regarding the port name, I introduced changes to name that port "webhook" as in the global webhookPortName, to be able to reliably address this port by name. I agree that grpc would be a better fit here, since that matches the actual use and the port delcared in the deployment is called this way too. In the main branch the service port does not have a name at all
• Regarding the port in use error, I cannot reproduce it since I'm unable to use kind at the moment. I checked the function code and couldn't find any other ports opened by the function except the grpc port itself. So either I'm missing something, or the port is already in use on your machine? Or maybe the fact that the deployment/pod declares the host port prompts kind to try and open them on the host as well, even though they're not used. You can control those via the DeploymentRuntimeConfig as well (see below)

apiVersion: pkg.crossplane.io/v1beta1
kind: DeploymentRuntimeConfig
metadata:
  name: function-patch-and-transform
spec:
  deploymentTemplate:
    spec:
      selector: { }
      template:
        spec:
          hostNetwork: true
          containers:
            - name: package-runtime
              args:
                - --address=:9612
              ports:
                - containerPort: 9612
                  hostPort: 9612
                  name: grpc
                  protocol: TCP
                - containerPort: 9613
                  hostPort: 9613
                  name: metrics
                  protocol: TCP
  serviceTemplate:
    spec:
      ports:
        - name: webhook
          port: 9200 # doesn't matter
          targetPort: 9612
          protocol: TCP

Unfortunately I don't have the changes in a public registry, but in a private one.

I was able to build your branch of provider-kubernetes and publish it to a publicly accessible repo at xpkg.upbound.io/jaredorg/provider-kubernetes-portconfig:v0.0.2.

Honestly never tried to remove the port, depends on whether the manifest will still pass the validation

I can confirm that the port value is required, even if it isn't used 😇

The DeploymentRuntimeConfig "provider-kubernetes" is invalid: spec.serviceTemplate.spec.ports[0].port: Required value

The provider-kubernetes pod looks healthy and running, but I'm curious about the mapping of Service -> Pod ports.

Service:

  ports:
  - name: webhook
    port: 9443
    protocol: TCP
    targetPort: 9610

Pod:

    ports:
    - containerPort: 8080
      hostPort: 8080
      name: metrics
      protocol: TCP
    - containerPort: 9443
      hostPort: 9443
      name: webhook
      protocol: TCP

Is that an issue that the pod's ports for webhook are still using 9443, but the Service is pointing to 9610? Should the DeploymentRuntimeConfig we are using for our testing also include the ports in the deployment template?

In general, how were you testing to validate that the webhook port is accessible and functional? That could help my testing here too 😁

I also tested again with function-patch-and-transform, with nothing else installed in the cluster. It installed and got to a healthy state, so I have to imagine that the usage of HostNetwork with kind opens up ports on my host laptop and causes conflicts when more than 1 provider or function is installed that all want to open 9443...

Without specifying any ports in the deployment template (only specifying them in the service template), I expected the crossplane composition machinery to not be able to reach the function because the Service would not be pointing at the Pod's ports, i.e. the Service points at 9612 and the Pod declares 9443 and 8080:

❯ kcs get service function-patch-and-transform -ojsonpath='{.spec.ports}'
[{"name":"webhook","port":9443,"protocol":"TCP","targetPort":9612}]

❯ kcs get pod -l pkg.crossplane.io/function=function-patch-and-transform -ojsonpath='{.items[0].spec.containers[0].ports}'
[{"containerPort":8080,"hostPort":8080,"name":"metrics","protocol":"TCP"},{"containerPort":9443,"hostPort":9443,"name":"grpc","protocol":"TCP"}]

However, it still worked 😮 - this must be because we're passing --address=:9612 as an arg to the function pod, but I didn't think that would be enough. Can you help me understand why the connection would still work when the Service doesn't point at any ports declared by the Pod? 🤔

Service to Pod port mapping

For me this was not an issue and we decided to do it that way to keep the complexity of this change lower see this discussion above, as modifying the service port would demand for additional changes.

As to why this works, honestly I don't know. I verified it on our AWS EKS cluster with Calico, and the control plane (running outside of the overlay network provided by Calico) reached the container running in the host network on the pods port. But I don't understand how that works. Probably some kind of kubeproxy/iptables magic going on there.

And the ports are not in conflict with each other as each service gets their own IP address.

Testing done

In our test cluster (again AWS EKS + Calico), I deployed:

• crossplane (built from this branch)
• function-patch-and-transform (latest release)
• provider-kubernetes (built from my branch over there)

All of them running in the hostNetwork and ports manually adjusted (as outlined in my previous examples). Then I wrote a Composition for the function and saw that crossplane and the function could talk to each other and worked as expected.

For provider-kubernetes it was a little bit more complicated, so if you deploy an old version (pre 0.11.0 so v0.10.0) and deploy a simple Object manifest, it will be deployed as the v1alpha1 variant. If you now update to a more recent version with the newer v1alpha2 version the provider refuses to start (see this issue). With the changes from my branch in this PR + crossplane from this branch, we can run it in the host network with a non-default port and the provider starts up correctly, and I observed that the conversion webhook was called by the control plane.

I have no idea how to replicate this with kind, as you would somehow need to create this split network, where workloads and control plane are not in the same network.

Regarding function-patch-and-transform and the port mystery

Kubernetes doesn't really care too much about the ports declared in a container, just like docker doesn't really care about the ports exposed in the Dockerfile:

List of ports to expose from the container. Not specifying a port here DOES NOT prevent that port from being exposed. Any port which is listening on the default "0.0.0.0" address inside a container will be accessible from the network. Modifying this array with strategic merge patch may corrupt the data. For more information See kubernetes/kubernetes#108255. Cannot be updated.

Kubernetes API Reference Container Spec

So it's a nice to have to actually declare them, and I would even say that it depends on the concrete runtime if a port gets opened or not but I'm not sure about it. So now we know that kind creates a port forwarding for all declared ports. I'm not sure if the "normal" kubelet does the same.

Thank you for your thorough review! I agree that documentation is necessary for this scenario. Do you want me to add a PR for that as well? In that case I'd ask you to guide me where (which repository) to do that, and where to put the added docs (as in where in the documentation).

I addressed your comments above and adjusted the code accordingly. Except for the comment about webhook service port vs webhook port, as I don't see the need for that (s. my comment above).

Sorry, forgot to address the check-diff question. No I was wondering about that one too. Git doesn't detect any changes, besides I ran earthly +reviewable before, so I'd assume that everything should be up to date already.

Maybe a line-ending issue? I'm working on Windows + WSL, so that wouldn't be the first time where line endings get messed up.

Without specifying any ports in the deployment template (only specifying them in the service template), I expected the crossplane composition machinery to not be able to reach the function because the Service would not be pointing at the Pod's ports, i.e. the Service points at 9612 and the Pod declares 9443 and 8080

I'm still a bit uncomfortable not knowing how the networking mapping is working still, both in your AWS environment and my kind environment, even when the service target port doesn't match the pod/container port. Your explanation of Kubernetes doesn't really care too much about the ports declared in a container could be sufficient for our usage though

From https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#serviceport-v1-core :
the Service.spec.ports[].targetPort can be specified as a Number or name of the port to access on the pods targeted by the service. Number must be in the range 1 to 65535. Name must be an IANA_SVC_NAME. If this is a string, it will be looked up as a named port in the target Pod's container ports

So, if the targetPort in the Service is specified as the correct listening port number of the target pod container in question, Service->pods routing/forwarding doesn't rely on the target pod's container ports spec. Only when specifying the targetPort in the Service as a named port, they do have to be matching with the target pod's container ports spec for the lookup to be successful.

Considering the confusion this might cause for users wanting to customize ports for their setups, to keep the configuration surface simple, i'm wondering whether exposing spec.serviceTemplateSpec.spec in the DeploymentRuntimeConfig CRD is really needed and can be avoided ?

If the pkg runtime Service builder methods are updated to leverage the Service.spec.ports[].targetPort as named port, then there won't be really any need for customizing the ports in spec.serviceTemplateSpec.spec of DeploymentRuntimeConfig resource. One would have to just specify them in the spec.deploymentTemplate.spec.template.spec.containers[0.name==package-runtime].ports[] of the DeploymentRuntimeConfig resource.

Could you think of any other spec.serviceTemplateSpec.spec fields that folks would want to customize other than the ports ? If not, we could as well, not introduce/add it now, and update to use the targetPort as named port reference to target pod container's port.

For example, to override a provider deployment ports, one would just need to specify:

apiVersion: pkg.crossplane.io/v1beta1
kind: DeploymentRuntimeConfig
metadata:
  name: provider-upjet-aws-rds
spec:
  deploymentTemplate:
    spec:
      selector: {}
      template:
        spec:
          hostNetwork: true
          containers:
            name: package-runtime
            ports:
            - containerPort: 18001
              name: webhook
              protocol: TCP
            - containerPort: 19001
              name: metrics
              protocol: TCP
            env:
            - name: WEBHOOK_PORT
              value: "18001"
            - name: METRICS_BIND_ADDRESS
              value: ":19001"

and the pkg runtime Service builder would be just hardcoding the targetPort to targetPort: webhook in case of providers and to targetPort: grpc in case of functions, in the Service resource it builds/renders.

Also @Argannor, i don't think one have to run Function runtime pods in hostNetwork(hence no ports customization required). Are you running on EKS and finding Crossplane-pod-in-hostNetwork not able to talk to -> function grpc pod running in pod network ?

In setups where the pod network is configured to use custom CNI in overlay mode (could be Calico overlay, Cilium overlay, etc..), Pod-to-Pod communications (and Pod-in-HostNetwork to Pod-in-Overlay-network) themselves work over the overlay network enabled by the respective CNI, so Crossplane pod -> Function pods grpc endpoints works seamlessly(with appropriate Networkpolicy[1], if it is being enforced) . But, the the EKS APIServer don't have direct connectivity(route) to talk to the pods on the overlay network.

In case of kube APIServer(or EKS Managed control plane) -> provider conversion webhook (or crossplane core webhook) communications, running provider(or crossplane core) in hostNetwork, hence ports customization to avoid port conflicts with other providers deployment, are required.

In case of Crossplane pod -> function grpc communication, there is no kube APIServer/kube control-plane involved right ?

[1] NetworkPolicy allowing all ingress and egress on grpc port 9443 in crossplane-system namespace, where the crossplane-core pod and the function pods are deployed to:

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: allow-functions-egress-ingress
  namespace: crossplane-system
spec:
  policyTypes:
  - Ingress
  - Egress
  podSelector: {}
  ingress:
  - ports:
    - port: 9443
      protocol: TCP
  egress:
  - ports:
    - port: 9443
      protocol: TCP

Also @Argannor, i don't think one have to run Function runtime pods in hostNetwork(hence no ports customization required). Are you running on EKS and finding Crossplane-pod-in-hostNetwork not able to talk to -> function grpc pod running in pod network ?

You're perfectly right, I think I got confused by the fact that we needed to adjust the port in the FunctionRevisions status, as Crossplane (or better GRPC) would try to communicate via the default port, as it does not respect the ports of the service directly.

To verify that I created a small gist, using

• nginx (as the function)
• a headless service
• and busybox (as the client/crossplane) running on hostNetwork

Where the client container was able to first perform an nslookup to get the overlay network IP and then reach the nginx.

In case of Crossplane pod -> function grpc communication, there is no kube APIServer/kube control-plane involved right ?

That's a good question...

The API server gets involved in the situation as soon as the function tries to manage multiple versions of their own CRDs and need to provide custom versions. But that wouldn't be happening via GRPC, so we would actually need to change the code again to accommodate for an optional webhook port as well. Or am I mistaken here?

If the pkg runtime Service builder methods are updated to leverage the Service.spec.ports[].targetPort as named port, then there won't be really any need for customizing the ports in spec.serviceTemplateSpec.spec of DeploymentRuntimeConfig resource. One would have to just specify them in the spec.deploymentTemplate.spec.template.spec.containers[0.name==package-runtime].ports[] of the DeploymentRuntimeConfig resource.

Could you think of any other spec.serviceTemplateSpec.spec fields that folks would want to customize other than the ports ? If not, we could as well, not introduce/add it now, and update to use the targetPort as named port reference to target pod container's port.

I don't see any other use case, so using the port names directly could make it a less confusing solution, and reduce the verbosity of the necessary config. I like that suggestion.

Update on using port names:

In order for GRPC to know which port the function is listening on we need to set the port as part of the FunctionRevision.status.endpoint (e.g. dns:///function-patch-and-transform.crossplane-system:9613). And since GRPC is not Kubernetes aware, it needs to be a port number.

This makes things more difficult, as the service needs to be setup in the Pre-Hook and the deployment in the Post-Hook, but with the change to port names we need information from both sources (name and namespace from the service, port from the deployment). To avoid this dilemma we could

• either rely on the fact that the service is named after the package, and set the status during Post
• or set the first part of the status dns:///<service-name>.<namespace-name> during Pre and add the port :<port> during Post

Do you have any better ideas? I don't particularly like the prospects of either of those, but the first option could be the lesser evil.

. But that wouldn't be happening via GRPC, so we would actually need to change the code again to accommodate for an optional webhook port as well. Or am I mistaken here?

i don't think Function runtime pods would ever have webhook listener, only grpc port.

In order for GRPC to know which port the function is listening on we need to set the port as part of the FunctionRevision.status.endpoint

since there is no need to run function runtime pods on hostNetwork, hence no ports customization required. So, the servicePort==targetPod containerPort==9443 should just work, as it right now before this PR change, right ?

i don't think Function runtime pods would ever have webhook listener, only grpc port.

Who manages CRDs of functions? Could you please elaborate your reasoning for me?

since there is no need to run function runtime pods on hostNetwork, hence no ports customization required. So, the servicePort==targetPod containerPort==9443 should just work, as it right now before this PR change, right ?

Sorry I was too focused on making it work. You're right, if we don't allow for customization, and it's never run on the host network (which shouldn't be required) we can again hard code it. But then why is the port configurable in the first place (via the SDK) and should we in this case also prevent customization of the container ports, since that would lead to an invalid configuration?

But that wouldn't be happening via GRPC, so we would actually need to change the code again to accommodate for an optional webhook port as well. Or am I mistaken here?

i don't think Function runtime pods would ever have webhook listener, only grpc port.

Who manages CRDs of functions? Could you please elaborate your reasoning for me?

As of today, Function Input CRDs exists and gets deployed through function packages to crossplane cluster, just for capturing the input schema of the function. They are never used for storing CRs of that CRD kind into the kube-api storage. As such, the conversion webhook for function input CRDs doesn't make sense ?

cc @negz @jbw976 to clarify on whether Function runtime pods would ever have Kube-apiserver->webhook-listener-on-function-pod communication call pattern.

since there is no need to run function runtime pods on hostNetwork, hence no ports customization required. So, the servicePort==targetPod containerPort==9443 should just work, as it right now before this PR change, right ?

Sorry I was too focused on making it work. You're right, if we don't allow for customization, and it's never run on the host network (which shouldn't be required) we can again hard code it. But then why is the port configurable in the first place (via the SDK) and should we in this case also prevent customization of the container ports, since that would lead to an invalid configuration?

Are you referring to --address flag/arg to the function ? i guess it is configurable to allow local development/testing to use custom port. But, i'm not sure if it is being used for other purposes. @negz ?

Regardless of the outcome of this discussion, I

• reverted to hard coded grpc ports for functions
• changed provider ports to name based ports
• removed the configurability for service ports

and verified in our test cluster again, that the communication works between:

• crossplane (host network) -> functions (cluster network)
• API server (outside cluster network) -> providers (host network)

Let me know what you think about this.

Excuse my persistence, but what do you think of the latest changes? Do these adress your concerns?

@Argannor @ravilr great work continuing to push on this effort since the last time I reviewed. I had to focus on a few other things with the v1.18 release and Kubecon, but after Kubecon settles down I'm planning on taking another pass through this. I would love to get this merged early in the v1.19 milestone so it can bake a bit and definitely be included as a popular feature of 1.19! 🙇‍♂️

@jbw976 if you're looking to take the latest changes(without the spec.ServiceTemplate in DeploymentRuntimConfig) to ride, the DeploymentRuntimeConfig in provider-k8s.yaml from jbw976@08eb86f would have to be updated to below:

---
apiVersion: pkg.crossplane.io/v1beta1
kind: DeploymentRuntimeConfig
metadata:
  name: provider-kubernetes
spec:
  deploymentTemplate:
    spec:
      selector: {}
      template:
        spec:
          hostNetwork: true
          nodeSelector:
            kubernetes.io/os: linux
          imagePullSecrets:
            - name: crossplane-workaround
          containers:
            - name: package-runtime
              args:
                - --webhook-port=9610            # customize webhook port to hostNetwork 9610; default is 9443, if unset
                - --metrics-bind-address=":9611"    # customize metrics port to hostNetwork 9611; default is 8080, if unset
              ports:
                - containerPort: 9610  # should be set to same thing set in above arg
                  name: webhook
                  protocol: TCP    
                - containerPort: 9611 # should be set to same thing set in above arg
                  name: metrics
                  protocol: TCP               

As for functions runtimeConfig, there isn't a need for running function pods in hostNetwork, as the crossplane pod->function pod communications are within the cluster network (all CNIs enable direct connectivity from cluster node's hostNetwork -> cluster pod network), hence no need for any port customization. As such the recommendation for folks running custom CNI deployments would be to continue to use the Crossplane's default rendered function runtime spec.

@ravilr, thanks for the manifest provided in #5540 (comment)! To get it working, I only had to remove the quotes around - --metrics-bind-address=":9611". See https://github.com/jbw976/crossplane/blob/configurable-ports-review/provider-k8s.yaml for what I got working 😉

Thank you @jbw976 for the review. Unfortunately I'm currently out with a fever and not sure if I can incorporate your review until the code freeze. Feel free to take over.

@Argannor i'm sorry to hear you are feeling ill! I hope you recover soon 🙏

I've taken all your changes and squashed/rebased them to the latest on main, resolved all merge conflicts, and then made a couple small tweaks for my last review feedback. You'll still be the commit author of your changes. I've pushed that all to https://github.com/jbw976/crossplane/tree/argannor-configurable-ports.

I'm doing some more testing now and I'll look through the code once again, but if all that looks good, I'll force push to your branch and then merge this PR!