-
Notifications
You must be signed in to change notification settings - Fork 23
#118: Ability to handle parameters in GatewayClass #124
Conversation
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.
We have a valid use case for the parameters in adding in the supported data plane gateway controller to use (istio)
So we could actually use the code here and create a default configmap params with the gateway controller to use. This param is set as a transform on the gateway resource
couple of code comments and a suggested change to make the code do some actual work for us |
23e0fb0
to
b8e52ba
Compare
Updated the PR with the suggestions, added some unit tests and updated the verification steps to verify that the patch annotation is set from the value specified in the parameters |
@sergioifg94 just one question before merge. Prob worth having this params created as part of local set up so it is there and being used? |
@maleck13 I added a commit to create the params in the local-setup and the |
hack/local-setup.sh
Outdated
|
||
echo "Adding ${clusterName} to gatewayclass params" | ||
|
||
${YQ_BIN} -i -P -o=json '.downstreamClasses["'$clusterName'"] = "istio"' config/samples/gatewayclass_params.json |
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.
@sergioifg94 why are we keying it off the cluster name? The idea with the gatewayclass params is it is configuration to be applied to gateways of this class, so I would expect all gateways of this class would have the same downstream class
@@ -164,6 +188,15 @@ func (r *GatewayReconciler) reconcileGateway(ctx context.Context, previous gatew | |||
allHosts := trafficAccessor.GetHosts() | |||
managedHosts := []ManagedHost{} | |||
|
|||
// Set the annotations to sync the class name from the parameters | |||
for cluster, downstreamClass := range params.GetDownstreamClasses() { |
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.
yeah same here, I think for the downstream class, each instance would have the same class
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 don't think the params should be keyed of the clustername, the idea with the params is it is additional config for a given gateway class and so generic
@mikenairn I see your PR depends on this one. We can probably merge this, I would just prefer the params weren't keyed against a cluster. The gatewayclass params, IMO are intended as class level configuration rather than cluster level configuration cc @sergioifg94 |
I was planning on waiting until patching annotations for all clusters is supported, but we can merge this as is for now and make those changes later |
@maleck13 I just updated the PR to follow your suggestion, so the parameter is now |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maleck13, sergioifg94 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 |
Description
Ability to get parameters from a resourced referenced in the
parametersRef
of the GatewayClassVerification steps
As the parameters are currently not used, in order to verify this functionality, ensure that the adequate status condition is reconciled when the
parametersRef
is misconfigured.Start the local development clusters
Run the controller
Create the parameters ConfigMap
Create the GatewayClass
Create the Gateway with
.spec.gatewayClassName = mctc-gw-istio-external-instance-per-cluster
Verify that once reconciled, the annotation is set