Skip to content
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

add HostSubnet.EgressCIDRs #64

Merged
merged 2 commits into from
Jul 18, 2018
Merged

Conversation

danwinship
Copy link
Contributor

Allow admins to specify CIDR ranges to automatically assign EgressIPs out of.

(We had talked before about implementing this as an annotation on Node, but it simplifies things to have it on HostSubnet instead, and the future solution with OVN will probably involve CRDs anyway, so there's no reason to use Node now.)

@openshift/sig-networking PTAL

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 29, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2018
@deads2k
Copy link
Contributor

deads2k commented Jul 9, 2018

/assign @smarterclayton

@deads2k deads2k added this to the v3.11 milestone Jul 9, 2018
@@ -72,6 +72,11 @@ type HostSubnet struct {
// EgressIPs is the list of automatic egress IP addresses currently hosted by this node
// +optional
EgressIPs []string `json:"egressIPs,omitempty" protobuf:"bytes,5,rep,name=egressIPs"`
// EgressCIDRs is the list of CIDR ranges available for automatically assigning
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this going to be used? Set by the controller or by the node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set by the controller

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why egressIPs would be read only? Is it because the controller will populate egressIPs automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. For backward-compatibility we have to continue letting people manually assign egress IPs (by setting EgressIPs while leaving EgressCIDRs unset). But if they are setting EgressCIDRs, then they can't also set EgressIPs themselves, because the controller will just end up overwriting it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add that to the godoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@danwinship
Copy link
Contributor Author

(The origin PR is ready-ish to merge once this does. It hasn't managed to pass integration or cmd yet, but that's just flakes)

Copy link

@pravisankar pravisankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@knobunc
Copy link
Contributor

knobunc commented Jul 11, 2018

@smarterclayton @deads2k -- Can this go in when openshift/origin#20258 passes tests?

@danwinship danwinship force-pushed the egress-cidrs branch 2 times, most recently from 44d02c2 to 30297c1 Compare July 12, 2018 20:31
@danwinship
Copy link
Contributor Author

@smarterclayton ok, this is green and the origin PR is green except for /approve

@knobunc
Copy link
Contributor

knobunc commented Jul 18, 2018

@smarterclayton @deads2k -- What's the status on this? Can it go in?

@knobunc
Copy link
Contributor

knobunc commented Jul 18, 2018

The corresponding PR has passed all tests and is just waiting for API approval.

@smarterclayton
Copy link
Contributor

/approve

@smarterclayton
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2018
@smarterclayton smarterclayton merged commit da58561 into openshift:master Jul 18, 2018
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/api that referenced this pull request Feb 1, 2021
Update CSV WebhookDescription struct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants