Bug 1822200: Custom dns name support #318
Conversation
pkg/actuators/machine/reconciler.go
Outdated
| var domainName string | ||
| for _, i := range dhcp.DhcpOptions[0].DhcpConfigurations { | ||
| if *i.Key == "domain-name" { | ||
| klog.Infof("Domain value %v", i.Values) |
There was a problem hiding this comment.
Do we want this to be logged? Is this debug?
There was a problem hiding this comment.
I meant to remove that, but I'm going to leave that in for now to confirm what AWS actually spits out. I'll remove it in a few.
michaelgugino
force-pushed
the
custom-dns-name-support
branch
from
999187b
to
e8d221d
Compare
michaelgugino
changed the title
openshift-ci-robot
added
bugzilla/severity-high
|
@michaelgugino: This pull request references Bugzilla bug 1822200, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
|
Can we please add a description to this PR and squash commits? |
|
/retest |
1 similar comment
|
/retest |
| @@ -18,6 +19,9 @@ const ( | |||
| userDataSecretKey = "userData" | |||
| ) | |||
|
|
|||
| // dhcpDomainKeyName is a variable so we can reference it in unit tests. | |||
| var dhcpDomainKeyName = "domain-name" | |||
There was a problem hiding this comment.
any reason not to declare this as a const?
There was a problem hiding this comment.
@elmiko need to take a reference in unit tests. Can't take a reference to a constant.
| expected: []string{"openshift.com", "openshift.io"}, | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
it seems like there are several conditions where getCustomDomainFromDHCP can return nil, nil, does it make sense to add those as unit tests as well?
There was a problem hiding this comment.
can you elaborate why not? as far as I can see it definitely make sense and has value to cover more code paths, e.g description: "Returns one", description: "Returns empty", error...
There was a problem hiding this comment.
I'm not a huge fan of mocking up fake cloud responses that don't materially drive decision making in our code. If the cloud sends back some empty stuff, cool, we don't have any domains. This case (returning nil,nil) is actually covered by several other tests because we don't mock any return from DescribeVPCS or DescribeDhcpOptions.
The only case where I can see something actually being empty is vpc.Vpcs[0].DhcpOptionsId. It's maybe possible to have a VPC without a DHCP option set assigned (and this is the case that is covered via other unit tests). It's also probably possible to have a DHCP option set assigned without any actual options, and possibly one defined with some options but nothing for domain-name, but does that actually happen ever? Probably not, or at least not in a VPC you expect to work. And even in that very limited case, the absence doesn't affect us. We're not going to fail on missing info.
Since we're only driving this function with a single input (vpcID) which is entirely contrived in testing, adding a bunch of statements to simulate the AWS API seems pointless. Firstly, there's no guarantee we're going to mock all the possible responses, let alone mock them correctly. Secondly, we're validating the output of this function as a valid input to another function (extractNodeAddresses) via other tests, albeit the nil,nil case. We're validating that this function will return a list of strings. We validate in extractNodeAddresses that a list of strings does what we want.
The more important tests would be validating the error conditions, because that will actually break existing users. But again, that's just mocking an established library and API, so there's no value in doing that. If this code breaks, it will 'fail open' unless we segfault, which we should be defended against properly now.
There was a problem hiding this comment.
that makes sense to me. my main concern with the original question was just to make sure we are exercising all the possible return paths in that function. i wouldn't expect a full mock of the upstream api, just enough to cover the various return paths. but i get what you are saying about the varied paths that could lead to those error conditions based on the range of possible inputs.
There was a problem hiding this comment.
If the cloud sends back some empty stuff, cool, we don't have any domains...
This func has 5 return calls and we only certainly know it does what it's meant for one single return path and only in the case we expect "Returns two".
We don't know what the func does if DhcpOptions legitimately has only 1 domain or has no domains. And so we don't know if any future change to the implementation of the func would break that behaviour.
To me seems a lot of value we could get for the price of adding a few lines including 2 more testCases.
Any ways not a blocker to me...
|
lgtm other than #318 (comment) and #318 (comment) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexander-demichev The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
michaelgugino
force-pushed
the
custom-dns-name-support
branch
2 times, most recently
from
7cb546f
to
369c7d1
Compare
|
@michaelgugino: This pull request references Bugzilla bug 1822200, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
This PR adds support for custom domains provided via DHCP options, primarily for the usecase of private VPCs (but possibly other usecases as well). These domains are necessary to be present in the node addresses to approve CSR requests. Support space delimited custom domains add unit tests
michaelgugino
force-pushed
the
custom-dns-name-support
branch
from
369c7d1
to
4b7bd08
Compare
|
@michaelgugino any reason to split in two commits? feel free to completely smash my initial dev commit, we don't need that in history |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
@michaelgugino: All pull requests linked via external trackers have merged: openshift/cluster-api-provider-aws#318, openshift/machine-api-operator#572. Bugzilla bug 1822200 has been moved to the MODIFIED state. In response to this:
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. |
|
/cherrypick release-4.4 |
|
@michaelgugino: #318 failed to apply on top of branch "release-4.4": In response to this:
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. |
This PR adds support for custom domains provided via DHCP options, primarily for the usecase of private VPCs (but possibly other usecases as well). These domains are necessary to be present in the node addresses to approve CSR requests. Support space delimited custom domains add unit tests Backports: openshift#318
This PR adds support for custom domains provided via DHCP options, primarily for the usecase of private VPCs (but possibly other usecases as well). These domains are necessary to be present in the node addresses to approve CSR requests.
Support space delimited custom domains
add unit tests