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

Support status reporting across multiple OpenStack regions #994

Merged
merged 6 commits into from Jan 17, 2019

Conversation

Projects
None yet
3 participants
@neiljerram
Copy link
Member

neiljerram commented Dec 20, 2018

Release Note

Support status reporting across multiple OpenStack regions

@neiljerram neiljerram requested a review from fasaxc Jan 7, 2019

@caseydavenport caseydavenport added this to the Calico v3.5.0 milestone Jan 10, 2019

@neiljerram neiljerram force-pushed the neiljerram:multi-region branch from 228837f to 05858a7 Jan 10, 2019

Avoid ambiguous meaning for empty Region field
The new code here was consistently using it to mean "no region
configured", which maps to the string "no-region" in etcd key paths.

However, there is a convention that an empty XyzOptions field means that
the caller doesn't care about the value of that field, and that
conflicts with the "no region" meaning.

Resolve that here by changing the new XyzKey.Region and
XyzOptions.Region fields to XyzKey.RegionString and
XyzOptions.RegionString.  A region _string_ is always either "no-region"
or "region-<name>" and so is never validly empty.
@fasaxc
Copy link
Member

fasaxc left a comment

Just a few minor points.

)

type WorkloadEndpointStatusKey struct {
Hostname string `json:"-"`
OrchestratorID string `json:"-"`
WorkloadID string `json:"-"`
EndpointID string `json:"-"`
RegionString string

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

What's the meaning of the json:"-" on the other fields, is that needed here?

This comment has been minimized.

@neiljerram

neiljerram Jan 16, 2019

Author Member

I don't know. I suspect it was just cargo culting, and thought that any real need should be revealed by tests, and so left it off the new field here.

(The json and validate tags are used for validating some v1 structures - https://github.com/projectcalico/libcalico-go/blob/master/lib/validator/v1/validator.go#L91 - but that never applies to Key and Options structures.)

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

As a special case, if the field tag is "-", the field is always omitted. Note that a field with name "-" can still be generated using the tag "-,".

So, why are we omitting those fields from JSON?

This comment has been minimized.

typeStatusReport = reflect.TypeOf(StatusReport{})
)

type ActiveStatusReportKey struct {
Hostname string `json:"-" validate:"required,hostname"`
Hostname string `json:"-" validate:"required,hostname"`
RegionString string

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

Missing the json/validate tags?

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

We should probably validate that it doesn't have a / in it if nothing else.

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

Just a thought: could you make RegionString into a typedef? I.e. type RegionString string Then the user would have to go through the RegionString function.

This comment has been minimized.

@neiljerram

neiljerram Jan 16, 2019

Author Member

Yes, that's a good idea, and I could make the RegionString function do escaping.

This comment has been minimized.

@neiljerram

neiljerram Jan 16, 2019

Author Member

Actually no, that requires too much change everywhere, and is inconsistent which all the other string-like fields that are just 'string'. Let's just do the validation.

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

OK, can live with that; should there be a validate tag, then?

}

type LastStatusReportKey struct {
Hostname string `json:"-" validate:"required,hostname"`
Hostname string `json:"-" validate:"required,hostname"`
RegionString string

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

Missing tags?

This comment has been minimized.

@neiljerram

neiljerram Jan 16, 2019

Author Member

(As above)

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

I don't follow, shouldn't this have a validate tag even if the json tag isn't relevant?

}

func (options WorkloadEndpointStatusListOptions) defaultPathRoot() string {
k := "/calico/felix/v1/host"
k := "/calico/felix/v2/" + options.RegionString + "/host"

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

I think if the region string is empty, this should return "/calico/felix/v2/"

if key.RegionString == "" {
return "", errors.ErrorInsufficientIdentifiers{Name: "regionString"}
}
if strings.Contains(key.RegionString, "/") {

This comment has been minimized.

@fasaxc

fasaxc Jan 16, 2019

Member

Why implement this validation differently from every other key, which seems to use the validate tag?

@fasaxc

fasaxc approved these changes Jan 17, 2019

@neiljerram neiljerram merged commit 8f41ab3 into projectcalico:master Jan 17, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@neiljerram neiljerram deleted the neiljerram:multi-region branch Jan 17, 2019

@caseydavenport caseydavenport removed this from the Calico v3.5.0 milestone Jan 24, 2019

@caseydavenport caseydavenport added this to the Calico v3.6.0 milestone Feb 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.