Store feature names in backend service description by MrHohn · Pull Request #305 · kubernetes/ingress-gce

MrHohn · 2018-06-05T20:56:50Z

/assign @rramkumar1

MrHohn · 2018-06-05T20:57:03Z

cc @bowei

rramkumar1 · 2018-06-05T21:11:19Z

pkg/utils/serviceport.go

description is misspelled

Thanks, done.

MrHohn · 2018-06-05T21:30:24Z

pkg/utils/serviceport_test.go

Will remove security policy related thing from this PR.

MrHohn · 2018-06-08T22:11:11Z

@rramkumar1 @bowei Revised PR to store feature names instead. PTAL thanks!

rramkumar1 · 2018-06-08T22:17:01Z

pkg/backends/features/features.go

Why not just iterate through versionToFeatures?

Humm, could you elaborate? Maybe the func name is confusing, it tries to get what features are used by the given servicePort.

Doesn't versionToFeatures contain all non-ga features? I was wondering why you can't just use that in this function rather than doing those if conditions.

Sorry I didn't mean to say iterate, that was confusing.

Humm, this function isn't really relevant to versionToFeatures. It is not about getting all possible non-GA features, but getting the features that are currently in used by the passed in ServicePort. I renamed it to featuresFromServicePort() which hopefully makes a little more sense..

Sorry, I was misunderstanding what that function was doing, makes sense now.

rramkumar1 · 2018-06-08T22:19:34Z

pkg/backends/backends.go

Nit: Remove the period from the comment.

rramkumar1 · 2018-06-08T22:20:40Z

pkg/backends/features/features.go

I wonder if at some point we should add this to the cloud provider layer as an extension method on meta.Version.

rramkumar1 · 2018-06-08T22:23:55Z

pkg/backends/features/features.go

I wonder if this we should put this in serviceport.go?

I'm having some cyclic import issues. I would like to put description struct along with the Description func, which depends on some functions implemented in package "features". Meanwhile package "features" needs to import ServicePort from package "utils"...

rramkumar1 · 2018-06-08T22:25:26Z

pkg/backends/features/features.go

any reason why this is exported? Doesn't seem like its being used outside of this package.

Nope, changed to not exported.

rramkumar1 · 2018-06-08T22:44:14Z

pkg/backends/features/features.go

Can you call this NonGAFeatures to be consistent with the json tag.

Sure, done.

rramkumar1 · 2018-06-08T22:46:59Z

pkg/backends/features/features.go

Can we add a type alias for the features slice?

Sorry, do you mean making a alias type for feature itself?

Yeah I think we should just have type Feature string

Sounds good, done.

MrHohn · 2018-06-08T23:11:43Z

pkg/backends/features/features.go

Changed from non-ga-features to x-features as suggested in doc.

rramkumar1 · 2018-06-10T16:24:55Z

pkg/backends/backends_test.go

Should we have an additional unit test here to test the description functionality or do the existing unit tests cover it?

Added a TODO for myself to add additional test here.

rramkumar1 · 2018-06-11T17:27:54Z

/assign @nicksardo
/assign @bowei

bowei

move description up and make the serviceport, feature do Set(desc)

bowei · 2018-06-13T00:51:02Z

pkg/backends/features/features.go

create a const map that goes from version to the set {1,2,3} and compare the numbers

Thanks, done.

bowei · 2018-06-13T00:53:08Z

pkg/backends/features/features.go

featuresToStringSet

Function removed.

bowei · 2018-06-13T00:53:43Z

pkg/backends/features/features.go

just have this as a string

bowei · 2018-06-13T00:54:34Z

pkg/backends/features/features.go

bowei · 2018-06-13T00:54:45Z

pkg/backends/features/features.go

bowei · 2018-06-13T00:54:59Z

pkg/backends/features/features.go

MrHohn · 2018-06-13T19:26:58Z

move description up and make the serviceport, feature do Set(desc)

@bowei Moved description out of serviceport and features and rearranged some of the functions. PTAL thanks!

bowei · 2018-06-15T06:20:30Z

pkg/backends/backends.go

 // ensureDescription updates the BackendService Description with the expected value
-func ensureDescription(be *composite.BackendService, description string) (needsUpdate bool) {
-	if be.Description == description {
+func ensureDescription(be *composite.BackendService, sp utils.ServicePort) (needsUpdate bool) {


sp *utils.ServicePort?

bowei · 2018-06-15T06:23:02Z

pkg/backends/features/doc.go

+*/
+
+// This package contains the implementations of backend service
+// features.


you should document what needs to happen when a feature is added (do it in a follow up PR)

Sounds good, added a TODO comment.

bowei · 2018-06-15T06:24:13Z

pkg/backends/features/features.go

+
+// IsLowerVersion reutrns if v1 is a lower version than v2.
+func IsLowerVersion(v1, v2 meta.Version) bool {
+	versionMap := map[meta.Version]int{


you can make this a global to avoid constructing it every time the func is called

bowei · 2018-06-15T06:24:48Z

pkg/utils/description.go

+}
+
+// GetDescriptionFromString gets a Description from string,
+func GetDescriptionFromString(descString string) Description {


no need for Get

DescriptionFromString

bowei · 2018-06-15T06:25:02Z

pkg/utils/description.go

+}
+
+// GetDescriptionFromString gets a Description from string,
+func GetDescriptionFromString(descString string) Description {


return *Description

bowei · 2018-06-15T06:25:22Z

pkg/utils/description.go

+	}
+	var desc Description
+	if err := json.Unmarshal([]byte(descString), &desc); err != nil {
+		glog.Errorf("Failed to parse description: %s, falling back to empty list", descString)


this seems problematic, return an error

The idea is that: in case of the description is messed up on backend service, the controller will fall back to assume the backend service was using GA (because the features field is empty) instead of halting the ingress sync.

On the other hand, if fail to parse description blocks the sync, it will prevent controller from fixing the description on backend service as well.

bowei · 2018-06-15T06:25:59Z

pkg/utils/serviceport.go

-	if sp.Protocol == annotations.ProtocolHTTP2 {
-		return meta.VersionAlpha
+// GetDescription returns a Description for this ServicePort.
+func (sp ServicePort) GetDescription() Description {


no need for get

func Description()

return *Description

The function was renamed to GetDescription() because Description itself is a struct now.

bowei · 2018-06-16T19:12:41Z

pkg/backends/features/features.go

+	}
+)
+
+// IsLowerVersion reutrns if v1 is a lower version than v2.


k8s-ci-robot assigned rramkumar1 Jun 5, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 5, 2018

rramkumar1 reviewed Jun 5, 2018

View reviewed changes

MrHohn commented Jun 5, 2018

View reviewed changes

MrHohn force-pushed the backend-service-description-version branch 5 times, most recently from 08a63c7 to d3ec794 Compare June 7, 2018 18:15

MrHohn mentioned this pull request Jun 7, 2018

Add support for security policy #291

Merged

MrHohn force-pushed the backend-service-description-version branch from d3ec794 to 81f2ecb Compare June 8, 2018 21:48

MrHohn changed the title ~~Bake compute version into backend service description~~ Store feature names in backend service description Jun 8, 2018

MrHohn force-pushed the backend-service-description-version branch from 81f2ecb to 284b61c Compare June 8, 2018 22:09

rramkumar1 reviewed Jun 8, 2018

View reviewed changes

MrHohn force-pushed the backend-service-description-version branch 3 times, most recently from cf9c7d6 to 8c00e0f Compare June 8, 2018 23:10

MrHohn commented Jun 8, 2018

View reviewed changes

pkg/backends/features/features.go Outdated

Copy link
Copy Markdown

Member Author

MrHohn Jun 8, 2018

Choose a reason for hiding this comment

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

Changed from non-ga-features to x-features as suggested in doc.

MrHohn force-pushed the backend-service-description-version branch 3 times, most recently from 04864b1 to bf4c748 Compare June 9, 2018 00:50

rramkumar1 reviewed Jun 10, 2018

View reviewed changes

k8s-ci-robot assigned bowei and nicksardo Jun 11, 2018

bowei reviewed Jun 13, 2018

View reviewed changes

MrHohn added 2 commits June 13, 2018 12:22

Store features name in backend service description

da384fe

Add unit tests for backend service description and feature version

32a45a8

MrHohn force-pushed the backend-service-description-version branch from bf4c748 to 32a45a8 Compare June 13, 2018 19:23

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 13, 2018

bowei reviewed Jun 15, 2018

View reviewed changes

Address review comments

c8a8df8

bowei approved these changes Jun 16, 2018

View reviewed changes

pkg/backends/features/features.go

}

)

// IsLowerVersion reutrns if v1 is a lower version than v2.

Copy link
Copy Markdown

Member

bowei Jun 16, 2018

Choose a reason for hiding this comment

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

spelling

bowei merged commit 87d86be into kubernetes:master Jun 16, 2018

Conversation

MrHohn commented Jun 5, 2018

Uh oh!

MrHohn commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrHohn commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rramkumar1 Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrHohn Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rramkumar1 commented Jun 11, 2018

Uh oh!

bowei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rramkumar1 Jun 10, 2018 •

edited

Loading

MrHohn Jun 11, 2018 •

edited

Loading