-
Notifications
You must be signed in to change notification settings - Fork 34
Conversation
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.
# github.com/projectcontour/contour-operator/controller/contour
controller/contour/daemonset.go:209:27: undefined: httpPort
controller/contour/daemonset.go:210:27: undefined: httpPort
controller/contour/daemonset.go:215:27: undefined: httpsPort
controller/contour/daemonset.go:216:27: undefined: httpsPort
controller/contour/daemonset.go:258:34: undefined: xdsPort
# github.com/projectcontour/contour-operator/controller/contour
vet: controller/contour/daemonset.go:209:27: undeclared name: httpPort
controller/contour/daemonset.go
Outdated
Ports: []corev1.ContainerPort{ | ||
{ | ||
Name: "http", | ||
ContainerPort: 80, | ||
ContainerPort: int32(httpPort), | ||
HostPort: int32(httpPort), |
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.
Do we need host ports? Seems like #52 (which adds a "LoadBalancer"-type service) obviates need for host ports or host networking.
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.
I don't think host ports are necessary, but they are specified upstream so I prefer to keep the definitions for the time being. I created #70 to address network endpoint publishing. Also see https://kubernetes.slack.com/archives/C8XRH2R4J/p1603393745340600.
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.
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.
@jpeach regarding #64 (review), this PR depends on #65 for port variables. |
@Miciah commit |
a7886ac
to
1443e8d
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.
$ make test
/Users/jpeach/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
/Users/jpeach/go/bin/controller-gen "crd:crdVersions=v1" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
go test ./... -coverprofile cover.out
? github.com/projectcontour/contour-operator/api/v1alpha1 [no test files]
? github.com/projectcontour/contour-operator/cmd [no test files]
ok github.com/projectcontour/contour-operator/controller/contour 8.042s coverage: 67.5% of statements
? github.com/projectcontour/contour-operator/util [no test files]
--- FAIL: TestDaemonSetConfigChanged (0.00s)
--- FAIL: TestDaemonSetConfigChanged/if_probe_values_are_set_to_default_values (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x1c pc=0x1e68d5f]
goroutine 57 [running]:
testing.tRunner.func1.1(0x1f51fc0, 0x2a87130)
/usr/local/Cellar/go/1.15.3/libexec/src/testing/testing.go:1072 +0x30d
testing.tRunner.func1(0xc00049a180)
/usr/local/Cellar/go/1.15.3/libexec/src/testing/testing.go:1075 +0x41a
panic(0x1f51fc0, 0x2a87130)
/usr/local/Cellar/go/1.15.3/libexec/src/runtime/panic.go:969 +0x1b9
github.com/projectcontour/contour-operator/util/equality_test.TestDaemonSetConfigChanged.func8(0xc00049db00)
/Users/jpeach/upstream/contour-operator/util/equality/equality_test.go:115 +0x21f
github.com/projectcontour/contour-operator/util/equality_test.TestDaemonSetConfigChanged.func9(0xc00049a180)
/Users/jpeach/upstream/contour-operator/util/equality/equality_test.go:132 +0xa4
testing.tRunner(0xc00049a180, 0xc0004a25e0)
/usr/local/Cellar/go/1.15.3/libexec/src/testing/testing.go:1123 +0xef
created by testing.(*T).Run
/usr/local/Cellar/go/1.15.3/libexec/src/testing/testing.go:1168 +0x2b3
FAIL github.com/projectcontour/contour-operator/util/equality 1.513s
ok github.com/projectcontour/contour-operator/util/retryableerror 0.402s coverage: 100.0% of statements
? github.com/projectcontour/contour-operator/util/slice [no test files]
FAIL
make: *** [Makefile:18: test] Error 1
@jpeach commit |
Signed-off-by: Daneyon Hansen <[email protected]>
@@ -267,7 +274,7 @@ func DesiredDaemonSet(contour *operatorv1alpha1.Contour, contourImage, envoyImag | |||
{ | |||
Name: envoyCfgVolName, | |||
MountPath: filepath.Join("/", envoyCfgVolMntDir), | |||
ReadOnly: true, | |||
ReadOnly: 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.
Why switch this 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.
The init container needs to be able to write out the config. This logic follows the example here: https://github.com/projectcontour/contour/blob/main/examples/contour/03-envoy.yaml#L111-L113
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.
Ahh that makes sense. =) Thanks for clarifying @Miciah!
/lgtm |
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.
/lgtm
Previously, the
DaemonSet
name was derived fromcontour.Name + "envoy"
. This PR does the following:DaemonSet
name with upstream until multipleContour
instances per namespace are supported.ReadinessProbe
from the shutdown-manager container to the envoy container (where it belongs).readOnly: false
for Envoy's config volume (as it should be).Requires #65 for port variables.
/assign @jpeach @Miciah
/cc @stevesloka @skriss
Signed-off-by: Daneyon Hansen [email protected]