-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document all the fields in the Cilium spec #8559
Document all the fields in the Cilium spec #8559
Conversation
Hi @olemarkus. 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. |
/ok-to-test |
a169697
to
73fb299
Compare
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.
Thanks for doing this!
pkg/apis/kops/networking.go
Outdated
// Ipam determines which IP address allocation mode to use. | ||
// "eni" will use AWS native networking for pods | ||
Ipam string `json:"ipam,omitempty"` | ||
// IPTablesRulesNoinstall determins if the base iptables rules for cilium to mainly interact with kube-proxy (and masquerading) |
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.
"determins" is a typo and it looks like a verb is missing.
Perhaps something more like "IPTablesRulesNoinstall disables installing the base IPTables rules used for masquerading and kube-proxy."
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.
Thanks
Co-Authored-By: John Gardiner Myers <jgmyers@proofpoint.com>
EnableNodePort bool `json:"enableNodePort"` | ||
Ipam string `json:"ipam,omitempty"` | ||
// Ipam determines the IP address allocation mode to use. | ||
// "eni" will use AWS native networking for pods. Eni requires masquerade to be set to false. |
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.
It looks like the possible values are "crd", "eni", and "azure". These should be documented.
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.
"crd" should work out of the box. Kops wouldn't support azure though.
Co-Authored-By: John Gardiner Myers <jgmyers@proofpoint.com>
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.
With that possible suggestion I believe we are ready to copy into the versioned APIs.
Co-Authored-By: John Gardiner Myers <jgmyers@proofpoint.com>
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus, rifelpet 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 |
Since there is a massive amount of fields to document, I only added the docs to one of the packages so far. I will add to the other two if there is no problem with the docs I added here.