Add NEG CRD definition #1154
Add NEG CRD definition #1154
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @swetharepakula! |
|
Hi @swetharepakula. 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
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
I assume you tagged me for API review? |
| type NetworkEndpointType string | ||
|
|
||
| const ( | ||
| VmIpPortEndpointType = NetworkEndpointType("GCE_VM_IP_PORT") |
There was a problem hiding this comment.
Decision - should we keep the GCP CONST_STYLE_NAMES or adopt K8s ConstStyleNames (and convert to/from) ?
K8s style would be my preference, but it's a weak preference
There was a problem hiding this comment.
I notice that we use the same constants in the neg types (https://github.com/kubernetes/ingress-gce/blob/master/pkg/neg/types/types.go#L38-L40). That leads me to use GCP style constants for consistency, but I don't have a strong preference either.
There was a problem hiding this comment.
If we already published them, should we just be re-using the same consts?
There was a problem hiding this comment.
Yes I agree. @freehan and I had that discussion. Our plan is to migrate the other package to depend on these constants in a followup PR.
|
My point is that it's the "link" not the "self link" once you remove it
from the self.
…On Fri, Jun 12, 2020 at 4:25 PM Minhan Xia ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/apis/servicenetworkendpointgroup/v1beta1/types.go
<#1154 (comment)>
:
> + // Last time the NEG syncer syncs associated NEGs.
+ // +optional
+ Condition []NegCondition `json:"conditions,omitempty"`
+
+ // Last time the NEG syncer syncs associated NEGs.
+ // +optional
+ LastSyncTime metav1.Time `json:"lastTransitionTime,omitempty"`
+}
+
+// NegObjectReference is the object reference to the NEG resource in GCE
+type NegObjectReference struct {
+ // The unique identifier for the NEG resource in GCE API.
+ Id uint64 `json:"id,omitempty,string"`
+
+ // SelfLink is the GCE Server-defined URL for the resource.
+ SelfLink string `json:"selfLink,omitempty"`
SelfLink is sort of specific to GCE objects. It follows a format and
parser are built around that.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1154 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVCVCGT646STOH447NLRWK2OZANCNFSM4N4SOLQQ>
.
|
|
Also not optionality and things like that. A lot of thought went into that
proposal :)
…On Fri, Jun 12, 2020 at 5:14 PM Swetha Repakula ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/apis/servicenetworkendpointgroup/v1beta1/types.go
<#1154 (comment)>
:
> +
+ // NetworkEndpointType: Type of network endpoints in this network
+ // endpoint group.
+ NetworkEndpointType NetworkEndpointType `json:"networkEndpointType,omitempty"`
+}
+
+type NetworkEndpointType string
+
+const (
+ VmIpPortEndpointType = NetworkEndpointType("GCE_VM_IP_PORT")
+ VmIpEndpointType = NetworkEndpointType("GCE_VM_IP")
+ NonGCPPrivateEndpointType = NetworkEndpointType("NON_GCP_PRIVATE_IP_PORT")
+)
+
+// NegCondition contains details for the current condition of this NEG.
+type NegCondition struct {
Looking at that doc, I am going to remove LastUpdateTime which will be
covered by LastSyncTime in NegObjectReference. I will add Reason and
ObservedGeneration fields.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1154 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVDVZPVNFIWSGAD2C43RWLAGVANCNFSM4N4SOLQQ>
.
|
swetharepakula
force-pushed
the
neg-crd
branch
3 times, most recently
from
44bdd70 to
445e19f
Compare
| type NetworkEndpointType string | ||
|
|
||
| const ( | ||
| VmIpPortEndpointType = NetworkEndpointType("GCE_VM_IP_PORT") |
There was a problem hiding this comment.
If we already published them, should we just be re-using the same consts?
swetharepakula
force-pushed
the
neg-crd
branch
2 times, most recently
from
7db45a3 to
05adde2
Compare
|
/assign |
swetharepakula
force-pushed
the
neg-crd
branch
2 times, most recently
from
7b141cd to
4f1e4d9
Compare
|
I have addressed the comments given so far. I think this PR is ready for another round of reviews. Thanks! |
|
LGTM. Leaving @bowei for final approval |
| limitations under the License. | ||
| */ | ||
|
|
||
| package servicenetworkendpointgroup |
There was a problem hiding this comment.
yikes this is super long -- I'm assuming this is autogenerated.
There was a problem hiding this comment.
I think we can change the package name. However, the CRD will be ServiceNetworkEndpointGroup, so I kept this package name to stay consistent. We could drop the Service prefix.
k8s-ci-robot
added
the
approved
swetharepakula
force-pushed
the
neg-crd
branch
2 times, most recently
from
f02dcdb to
8cee965
Compare
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, freehan, swetharepakula 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 |
NEG CRD is needed to enabled custom named negs. This CRD also exposes more information about the status of the NEG to users.