aws_platform_service_location: add a controller that sync user specified service endpoints to status by abhinavdahiya · Pull Request #119 · openshift/cluster-config-operator

abhinavdahiya · 2020-03-31T23:01:08Z

The controller only works for AWS platform
The service endpoints are validated before syncing to the the status,

no duplicates should be specified for a service
the URL must be hostname or a valid URI without any request paths
If the service endpoints are not allowed or valid, the status service endpoints are not modified.

deads2k · 2020-04-13T17:26:45Z

+	if pstatus := currentInfra.Status.PlatformStatus; pstatus != nil {
+		platformName = pstatus.Type
+	}
+	if platformName == "" {


kube canonical is len() == 0

deads2k · 2020-04-13T17:28:29Z

+		return nil // nothing to do here
+	}
+
+	services := currentInfra.Spec.PlatformSpec.AWS.ServiceEndpoints


deep copy please

d957d96#diff-51a3ce64d08707b029401c01f6a81ac0R61

already deepcopies.

d957d96#diff-51a3ce64d08707b029401c01f6a81ac0R61

already deepcopies.

Given the assignment to status later and the update usage, I'd prefer not to shallow copy this. I think it's just .DeepCopy, right?

ServiceEndpoints is a slice, so no DeepCopy.

i could DeepCopy the entire currentInfra again..
currentInfra.DeepCopy().Spec.PlatformSpec.AWS.ServiceEndpoints ?? @deads2k

Can you provide a little background on why you think deepcopy here would be necessary?

ServiceEndpoints is a slice, so no DeepCopy.

i could DeepCopy the entire currentInfra again..
currentInfra.DeepCopy().Spec.PlatformSpec.AWS.ServiceEndpoints ?? @deads2k

Can you provide a little background on why you think deepcopy here would be necessary?

mutating the input means that if you later use it or diff it somewhere, it won't be what you expect it to be. Makes it hard for me to reason about it's state during this review.

deads2k · 2020-04-13T17:29:16Z

needs owners file

openshift-ci-robot

@abhinavdahiya: 1 invalid OWNERS file

Details

In response to this:

The controller only works for AWS platform
The service endpoints are validated before syncing to the the status,

no duplicates should be specified for a service

the URL must be hostname or a valid URI without any request paths
If the service endpoints are not allowed or valid, the status service endpoints are not modified.

depends on openshift/api#599

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.

openshift-ci-robot

@abhinavdahiya: 1 invalid OWNERS file

Details

In response to this:

The controller only works for AWS platform
The service endpoints are validated before syncing to the the status,

no duplicates should be specified for a service

the URL must be hostname or a valid URI without any request paths
If the service endpoints are not allowed or valid, the status service endpoints are not modified.

depends on openshift/api#599

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.

abhinavdahiya · 2020-04-14T04:21:36Z

/retest

deads2k · 2020-04-14T18:20:36Z

+
+func validateServiceURL(uri string) error {
+	endpoint := uri
+	if !schemeRE.MatchString(endpoint) {


This should really be a parseable URL or it shouldn't. The sometimes API seems "nicer" but it ultimately makes for complication. Let's do a straight url.Parse and then have a list of schemes we allow. I don't think empty should be allowed and I would require https until we have a reason not to.

deads2k · 2020-04-15T20:35:04Z

/lgtm

…ied service endpoints to status The controller only works for AWS platform THe service endpoints are validated before syncing to the the status, - no duplicates should be specified for a service - the URL must be hostname or a valid URI without any request paths If the service endpoints are not allowed or valid, the status service endpoints are not modified.

fabianofranz · 2020-04-16T15:06:58Z

/lgtm

openshift-ci-robot · 2020-04-16T15:07:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, deads2k, fabianofranz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abhinavdahiya · 2020-04-16T18:08:58Z

/retest

abhinavdahiya · 2020-04-16T19:13:40Z

/retest

abhinavdahiya · 2020-04-16T23:04:19Z

/retest

openshift-ci-robot requested review from deads2k and soltysh March 31, 2020 23:02

abhinavdahiya force-pushed the aws_sevice_endpoints branch from 2e74821 to d957d96 Compare April 1, 2020 00:11

deads2k reviewed Apr 13, 2020

View reviewed changes

Comment thread pkg/operator/aws_platform_service_location/controller.go

deads2k reviewed Apr 13, 2020

View reviewed changes

abhinavdahiya force-pushed the aws_sevice_endpoints branch from d957d96 to 28df379 Compare April 13, 2020 22:53

openshift-ci-robot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Apr 13, 2020

openshift-ci-robot reviewed Apr 13, 2020

View reviewed changes

Comment thread pkg/operator/aws_platform_service_location/OWNERS

abhinavdahiya force-pushed the aws_sevice_endpoints branch from 28df379 to c64de12 Compare April 13, 2020 23:26

openshift-ci-robot reviewed Apr 13, 2020

View reviewed changes

Comment thread pkg/operator/aws_platform_service_location/OWNERS

abhinavdahiya force-pushed the aws_sevice_endpoints branch from c64de12 to 470d7bd Compare April 13, 2020 23:31

openshift-ci-robot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Apr 13, 2020

deads2k reviewed Apr 14, 2020

View reviewed changes

Comment thread pkg/operator/aws_platform_service_location/controller.go

abhinavdahiya force-pushed the aws_sevice_endpoints branch from 470d7bd to e16d20e Compare April 14, 2020 23:19

openshift-ci-robot assigned deads2k Apr 15, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 15, 2020

abhinavdahiya and others added 3 commits April 16, 2020 07:41

pkg/operator/aws_platform_service_location: add OWNERS

6f86245

vendor: bump

e3626af

abhinavdahiya force-pushed the aws_sevice_endpoints branch from e16d20e to e3626af Compare April 16, 2020 14:43

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 16, 2020

openshift-ci-robot assigned fabianofranz Apr 16, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020

openshift-merge-robot merged commit d6f34a3 into openshift:master Apr 17, 2020

Conversation

abhinavdahiya commented Mar 31, 2020

Uh oh!

deads2k Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deads2k Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

deads2k Apr 14, 2020 • 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!

abhinavdahiya Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

deads2k Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

deads2k commented Apr 13, 2020

Uh oh!

openshift-ci-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhinavdahiya commented Apr 14, 2020

Uh oh!

deads2k Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deads2k commented Apr 15, 2020

Uh oh!

fabianofranz commented Apr 16, 2020

Uh oh!

openshift-ci-robot commented Apr 16, 2020

Uh oh!

abhinavdahiya commented Apr 16, 2020

Uh oh!

abhinavdahiya commented Apr 16, 2020

Uh oh!

abhinavdahiya commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

deads2k Apr 14, 2020 •

edited

Loading