OCPBUGS-37059: Support deployment on HCP clusters #219
Conversation
alebedev87
commented
Jul 16, 2024
alebedev87
commented
- Add bound service account token to operand deployment
- Update docs with STS chapter
- Relax node placement for operand
openshift-ci-robot
added
jira/severity-moderate
|
@alebedev87: This pull request references Jira Issue OCPBUGS-37059, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@alebedev87: This pull request references Jira Issue OCPBUGS-37059, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
Infoblox licenses need to be updated to proceed with the infoblox e2e test. |
|
/retest |
|
/assign |
| Name: boundSATokenVolumeName, | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Projected: &corev1.ProjectedVolumeSource{ | ||
| //DefaultMode: pointer.Int32(420), |
There was a problem hiding this comment.
Any reason this is commented out?
I see with ALBO we used 292: https://github.com/openshift/aws-load-balancer-operator/pull/82/files#diff-ce0d597a9e5f20a8fe61c3ba5e6185a6b90cfef2c00bb5b5ef3e97b15b79f480R94
And the default appears to be 420 anyways.
There was a problem hiding this comment.
Any reason this is commented out?
I wanted to remove it but forgot. ExternalDNS Operator volumes don't use defaultMode so I didn't want to make a new case out of it.
I see with ALBO we used 292: https://github.com/openshift/aws-load-balancer-operator/pull/82/files#diff-ce0d597a9e5f20a8fe61c3ba5e6185a6b90cfef2c00bb5b5ef3e97b15b79f480R94
Yes for ALBO I followed Miciah's proposal for the completeness sake. The mounting is still readonly by default as mentioned in the issue I posted as response to Miciah, so we don't really risk anything.
We will need to rethink the defaultMode for ExternalDNS Operator. Setting it explicitly on all the volumes seems to be the way to go. But for this we need to get rid of the filtering we do during the comparison.
| @@ -581,6 +592,21 @@ func (b *externalDNSVolumeBuilder) awsVolumes() []corev1.Volume { | |||
| }, | |||
| }, | |||
| }, | |||
| { | |||
| Name: boundSATokenVolumeName, | |||
There was a problem hiding this comment.
What happens on non-STS clusters? This is included right? Will it be just a dummy volume that does nothing? I see we also always included it with ALBO, so I presume it's not a big deal.
There was a problem hiding this comment.
Will it be just a dummy volume that does nothing?
Yeah, I believe it's a common approach to keep harmful things for all the deployments. C-I-O keeps it for any deployment type too.
docs/usage.md
Outdated
|
|
||
| 1. Generate the trusted policy file using your identity provider: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
BTW you can indent these code blocks ```` by adding at least 3 spaces before every line. It will display the code block logically under the number list. Up to you. Looks like we do that in ALBO docs a bit.
| @@ -201,8 +201,7 @@ func desiredExternalDNSDeployment(cfg *deploymentConfig) (*appsv1.Deployment, er | |||
| } | |||
|
|
|||
| nodeSelectorLbl := map[string]string{ | |||
| osLabel: linuxOS, | |||
| masterNodeRoleLabel: "", | |||
There was a problem hiding this comment.
This change seems pretty reasonable. I don't know any reason why we would restrict to just the masters. It's a deployment, with 1 replica hardcoded, and as far as I know, the External DNS pod is a pretty simple pod, just needs to make API calls to the platform API.
There was a problem hiding this comment.
I think I added this label maybe because I saw it on some cluster operators' operands. I don't remember any strong reason for it and we cannot come up with any. But we see problems related to it. RIP then.
| @@ -16,22 +18,22 @@ the namespace where the _external-dns_ deployments are created so that they can | |||
|
|
|||
| # AWS | |||
There was a problem hiding this comment.
Now that we STS, following the instructions isn't very clear that this section isn't for STS clusters.
About about adding a ## Non-STS clusters section? Just like ALBO. I'd also suggest reorganizing, pulling STS up to be right after## Non-STS clusters section.
There was a problem hiding this comment.
I just wanted to keep the same structure as before with 1 chapter named after each provider. AWS one became bloated of course. I think the reorg for the AWS chapter will come in the near future when we will have to do the same "STS enablement" as on ALBO. I propose to postpone it to these times.
docs/usage.md
Outdated
| @@ -86,18 +88,100 @@ spec: | |||
| The operator makes the assumption that `ExternalDNS` instances which target GovCloud DNS also run on the GovCloud. This is needed to detect the AWS region. | |||
| As for the rest: the usage is exactly the same as for `AWS`. | |||
|
|
|||
| ## STS | |||
There was a problem hiding this comment.
| ## STS | |
| ## STS Clusters |
More aligned with ALBO docs? And provide a tiny bit more context that STS is cluster wide.
In order to enable the usage of the STS-enabled credentials secret which contains the path to the web token, the service account token should be mounted into the ExternalDNS container.
alebedev87
force-pushed
the
support-hcp-deployment
branch
from
a5dfddb
to
bc37983
Compare
- Add a new chapter specific to STS clusters - Add IAM policy artifact needed for AWS STS credentials - Update usage doc to align chapters from different providers
Remove the master node label from the node selector to relax scheduling constraints. This change enables proper installation on Hosted Control Plane clusters. The toleration of the NoSchedule taint remains intact to fit the Single Node OpenShift usecase.
alebedev87
force-pushed
the
support-hcp-deployment
branch
from
bc37983
to
285074d
Compare
|
@alebedev87: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
Thanks for the responses. Looks good to me! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278 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
bot
added
the
approved
|
@alebedev87: Jira Issue OCPBUGS-37059: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-37059 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 openshift-eng/jira-lifecycle-plugin repository. |
|
verified using the image created from the external-dns-operator repo Create secret in STS cluster HENCE marking as verified |
