GCP forwarding rule/target proxy description field populated #757
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @KatrinaHoffert. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
Oops. That was dumb. /retest |
| defaultZone = "zone-a" | ||
| clusterName = "uid1" | ||
| ingressName = "test" | ||
| namespace = "namespace1" |
There was a problem hiding this comment.
Any particular reason why the namespace was changed from "default" to "namespace1"?
There was a problem hiding this comment.
I changed it solely so that the tests were obvious in using the specified namespace. I was wary of the possibility of "default" being assumed somewhere (since, well, it's the default when no namespace is specified) and wanted the tests to be able to differentiate from that possibility.
pkg/loadbalancers/l7.go
Outdated
| namespace := l.runtimeInfo.Ingress.ObjectMeta.Namespace | ||
| ingressName := l.runtimeInfo.Ingress.ObjectMeta.Name | ||
|
|
||
| return fmt.Sprintf(`{"kubernetes.io/cluster/%s": "owned", "kubernetes.io/namespace": "%s", "kubernetes.io/ingress-name":"%s"}`, clusterName, namespace, ingressName), nil |
There was a problem hiding this comment.
Couple points:
- I would substitute "kubernetes.io/cluster/%s" : "owned" with "kubernetes.io/cluster" : "[CLUSTER]"
- Let me get back to you ASAP on what all information we want in the description. I'm not sure yet if we want more / less / same.
There was a problem hiding this comment.
Seems reasonable. I was just trying to figure out what kinda metadata others might have. It doesn't seem documented anywhere I could find.
|
@rjnagal, have you seen my updates? Curious if there's anything more needed here. |
pkg/loadbalancers/l7.go
Outdated
| namespace := l.runtimeInfo.Ingress.ObjectMeta.Namespace | ||
| ingressName := l.runtimeInfo.Ingress.ObjectMeta.Name | ||
|
|
||
| return fmt.Sprintf(`{"kubernetes.io/cluster": "%s", "kubernetes.io/namespace": "%s", "kubernetes.io/ingress-name":"%s"}`, clusterName, namespace, ingressName), nil |
There was a problem hiding this comment.
It'[s not clear to me what value cluster name will be ?
There was a problem hiding this comment.
A few thoughts:
Service uses "namespace/name" for service-name -- why be different?
The use of "kubernetes.io" is suspect but historically we did it for Service so I guess this is not worse. The context is not really the domain of Kubernetes, so it's borderline but I won't complain.
Service doesn't have cluster name, but probably should. Hint hint.
%q is generally safer than "%s" in every way. Should be a no-op for this case, but a good habit.
There was a problem hiding this comment.
The cluster name is probably not actually the cluster's name. It depends on if the GLBC was launched with the cluster name actually set (ie, the --cluster-uid flag). Of course, it's usually not and doesn't actually need to match the cluster's name. So that value is actually very likely just a UUID that groups the GLBC resources together. I didn't see a good way to get the actual cluster name. It's surprisingly tricky to get -- the only way I see is to utilize that we know the instance groups and query one of the instances to get the cluster-name custom metadata that we set. On GKE, GLBC could be instantiated with the cluster name passed in.
r.e. labels, I was initially looking at how GCE load balancers use kubernetes.io/service-name. But since ingresses aren't services, I had to do some research on how the heck that label was chosen without much luck and decided to use the same labels as AWS for their ingresses, assuming they got them from somewhere and that it at least follow precedent.
On services (well, load balancers specifically), they seem to have the cluster name available, so could easily be extended, too (that's something for another issue, though -- they're not even in this repo). I should look into if it's exposed somewhere accessible on the cluster. It's ctx.ComponentConfig.KubeCloudShared.ClusterName
There was a problem hiding this comment.
The usage of %q has been resolved. I haven't changed the other parts pending the explanation being read.
KatrinaHoffert
force-pushed
the
issue-661
branch
2 times, most recently
from
aebb8a4
to
08f0859
Compare
|
I think consistency between service and ingress on GCE is more useful than
consistency between GCE and AWS, so I would go with
`ingress-name=namespace/name`.
As for cluster name, we ALMOST have a concept of cluster name and we don't
have it on Service yet, so what if we punted that part until we have proper
cluster names?
…On Fri, May 31, 2019 at 3:18 PM Katrina Mitchell ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/loadbalancers/l7.go
<#757 (comment)>
:
> @@ -308,3 +308,17 @@ func GCEResourceName(ingAnnotations map[string]string, resourceName string) stri
resourceName, _ = ingAnnotations[fmt.Sprintf("%v/%v", annotations.StatusPrefix, resourceName)]
return resourceName
}
+
+// getDescription gets a description for the L7 forwarding rule.
+func (l *L7) getDescription() (string, error) {
+ if l.runtimeInfo.Ingress == nil {
+ return "", fmt.Errorf("Missing ingress object for %s", l.Name)
+ }
+
+ // Note: l.Name is not the k8s ingress name, but the GCE internal name.
+ clusterName := l.namer.UID()
+ namespace := l.runtimeInfo.Ingress.ObjectMeta.Namespace
+ ingressName := l.runtimeInfo.Ingress.ObjectMeta.Name
+
+ return fmt.Sprintf(`{"kubernetes.io/cluster": "%s", "kubernetes.io/namespace": "%s", "kubernetes.io/ingress-name":"%s"}`, clusterName, namespace, ingressName), nil
The usage of %q has been resolved. I haven't changed the other parts
pending the explanation being read.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#757?email_source=notifications&email_token=ABKWAVFGDQKAQC7VEIMOMZ3PYGP3RA5CNFSM4HPLFOTKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2JWPLY#discussion_r289566807>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKWAVGKTCWJ4CE4RZF2O5DPYGP3RANCNFSM4HPLFOTA>
.
|
|
Done @thockin. |
|
@rramkumar1 done |
|
@KatrinaHoffert Sorry one last thing, can you squash commits? There needs to be only one commit for this since its pretty small. |
The description is akin to that used by load balancer services and sets `ingress-name=namespace/name`. The forwarding rules and target proxies are affected by this.
|
Squashed @rramkumar1 |
|
/lgtm Thanks! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KatrinaHoffert, rramkumar1 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 |
k8s-ci-robot
added
the
approved
This is for issue #661.
The description are the same values as AWS uses in their Ingress.
Unit tests were updated. Can be viewed by creating an ingress and in Cloud Console, go to Network services > Load balancing > advanced view (description column). Note that the cluster name field is probably not the actual cluster name, but does identify the cluster (and instance group, etc).