Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-low
|
@tthvo: This pull request references Jira Issue OCPBUGS-115159, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughAWS machine sets now generate IAM profiles from the cluster and machine role. AWS IAM provisioning now processes all configured compute pools and supports edge-specific policies, profiles, and roles. ChangesAWS IAM role and profile support
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change gives edge compute pools dedicated IAM resources while preserving custom instance-profile behavior, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
Full details: Stable And Deterministic Test NamesExplanation The pull request adds and changes only static table-test names in Full details: Test Structure And QualityExplanation PASS. The pull request changes a standard Go Full details: Microshift Test CompatibilityExplanation PASS — The pull request adds no new Ginkgo e2e tests. The changed test uses Go's Full details: Single Node Openshift (Sno) Test CompatibilityExplanation PASS: The pull request adds no Ginkgo e2e tests. The only test change is a standard Go Full details: Topology-Aware Scheduling CompatibilityExplanation PASS — The pull request changes AWS IAM role/profile selection and default profile names. It does not add or modify deployment manifests, operator scheduling logic, pod affinity, topology spread constraints, PDBs, replica calculations, or control-plane/worker node selectors. The existing MachineSet worker and edge labels, taints, and replica distribution are unchanged in the PR diff. Therefore, no topology-unaware scheduling constraint is introduced. Full details: Ote Binary Stdout ContractExplanation PASS: The PR adds no process-level stdout writes. Added production code uses Full details: Ipv6 And Disconnected Network Test CompatibilityExplanation PASS: The PR adds no new Ginkgo e2e tests. The only changed test file uses Go Full details: No-Weak-CryptoExplanation The pull request does not introduce weak cryptography or custom cryptographic code. The four changed Go files only add role-specific IAM profile and policy handling. Added-line analysis found no MD5, SHA1, RC4, Blowfish, ECB, encryption, or secret/token comparisons. The apparent DES matches are AWS API names such as Full details: Container-PrivilegesExplanation PASS: The pull request changes only Go IAM and machine-set logic plus tests. The two-commit diff adds no Kubernetes/container manifest and no Full details: No-Sensitive-Data-In-LogsExplanation No sensitive-data logging was introduced. The pull request adds no log calls or log arguments; it only changes IAM profile selection and expands existing IAM reconciliation to edge pools. The existing debug message logs an IAM instance-profile name, which is an infrastructure identifier, not a password, token, API key, PII, session ID, hostname, or customer payload. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@tthvo: This pull request references Jira Issue OCPBUGS-115159, which is valid. 3 validation(s) were run on this bug
DetailsIn 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. |
|
/test e2e-aws-ovn e2e-aws-default-config |
|
/test aws-private e2e-aws-default-config |
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-aws-ovn-edge-zones These e2es failed because no preferred instance type was found for the selected zones. However, they previously passed with the same commit for other local/wavelength zones. This is expected as some zones have limited capacity and we also randomize zone selection in CI. |
| // for edge nodes if none is requested. | ||
| roles := []string{master} | ||
| for _, compute := range ic.Config.Compute { | ||
| roles = append(roles, compute.Name) |
There was a problem hiding this comment.
-
Does
compute.Namestill enter as "worker", because the below switch believes it does. Is it possible for a different value to be entered? -
This is a list not a set, are there implications when more than one compute is entered?
There was a problem hiding this comment.
Does compute.Name still enter as "worker", because the below switch believes it does. Is it possible for a different value to be entered?
The compute pool name can only be worker or edge (AWS only) :D
installer/pkg/types/validation/installconfig.go
Lines 934 to 942 in 43ef4b6
This is a list not a set, are there implications when more than one compute is entered?
We should only ever have 2 distinct pools (worker and edge) as duplicate pool names are rejected.
installer/pkg/types/validation/installconfig.go
Lines 944 to 947 in 43ef4b6
| instanceProfile := mpool.IAMProfile | ||
| if len(instanceProfile) == 0 { | ||
| instanceProfile = fmt.Sprintf("%s-worker-profile", in.ClusterID) | ||
| instanceProfile = fmt.Sprintf("%s-%s-profile", in.ClusterID, in.Role) |
There was a problem hiding this comment.
On Desetroy: findUntaggableResources exists precisely because IAM instance profiles aren't returned by the resource-groups tagging API, so installer-created profiles must be enumerated by name. The PR adds a new -edge-profile but doesn't add edge to the list to search and destroy.
There was a problem hiding this comment.
Thanks, this is an interesting finding 👀
The destroy code actually found the instance profile via resourcetagging API to clean up. From the destroy log) of e2e-aws-ovn-edge-zones, we can see:
time="2026-09-01T10:29:03Z" level=info msg=Disassociated id=ci-op-lwg83klg-40b45-l2pc4-edge-profile name=ci-op-lwg83klg-40b45-l2pc4-edge-profile role=ci-op-lwg83klg-40b45-l2pc4-edge-role
time="2026-09-01T10:29:03Z" level=info msg=Deleted InstanceProfileName=ci-op-lwg83klg-40b45-l2pc4-edge-profile arn="arn:aws:iam::460538899914:instance-profile/ci-op-lwg83klg-40b45-l2pc4-edge-profile" id=ci-op-lwg83klg-40b45-l2pc4-edge-profile
According AWS docs, it said the following, which confirms the behaviour:
at this time, you can tag only the following IAM resources using the Resource Groups Tagging API:
instance-profile
mfa
oidc-provider
policy
role
saml-provider
server-certificate
user
IIUC, we no longer needs to search instance profile separately. Maybe that's for another follow-up PR, so I'll add "edge" in the list for completeness (better safe than sorry) 👍
The installer only creates IAM resources for the control-plane and worker pool while the edge (local/wavelength zone) pool has none and re-uses the those of worker nodes. This introduces a few problems: - If an IAM role is specified for edge pool, it is ignored. - If an IAM role is specified for worker pool, it's also applied to edge pool unexpectedly. - If an IAM instance profile is specified for worker pool, the edge pool references a non-existing default worker profile since the installer doesn't create one.
Worker and edge machine sets both derived the default instance profile name from a hard-coded "-worker-profile" suffix. Use the pool role so edge machine sets reference the edge instance profile instead of pointing to the worker profile.
03f84e7 to
65963fa
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test e2e-aws-ovn-edge-zones |
|
@tthvo: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
3 participants
Description
The installer only creates IAM resources for the control-plane and worker pool while the edge (local/wavelength zone) pool has none and re-uses the those of worker nodes.
This introduces a few problems:
This PR configures the installer to create dedicated IAM resources for edge pool. When a custom instance profile is defined, that profile continues to be utilized as before.
Summary by CodeRabbit