Fencing: AWS VPC network resource fencer #609
Conversation
…ption build should now work. sorry Klaus
|
Can one of the admins check and authorise this run please: https://ci.kronosnet.org/job/fence-agents/job/fence-agents-pipeline/job/PR-609/1/input |
changed aws metadata calls to use TLS
|
Can one of the admins check and authorise this run please: https://ci.kronosnet.org/job/fence-agents/job/fence-agents-pipeline/job/PR-609/2/input |
|
Can one of the admins check and authorise this run please: https://ci.kronosnet.org/job/fence-agents/job/fence-agents-pipeline/job/PR-609/3/input |
updated spec file to remediate typo
|
Can one of the admins check and authorise this run please: https://ci.kronosnet.org/job/fence-agents/job/fence-agents-pipeline/job/PR-609/4/input |
There was a problem hiding this comment.
Looks good in general.
I think we should name the agent fence_aws_vpc_net or similar to indicate it is a network fence agent.
I've added some comments for improvements.
Can we make remove_security_groups()/keep_only_security_groups() one function to avoid duplicate code?
| import time | ||
| import requests | ||
| from requests import HTTPError | ||
| from requests.exceptions import HTTPError |
There was a problem hiding this comment.
HTTPError isnt used, and if we're going to use them they should be imported with their full name, e.g. import requests.HTTPError.
|
|
||
| sys.path.append("@FENCEAGENTSLIBDIR@") | ||
| #sys.path.append("/usr/share/fence") | ||
| #sys.path.append("/usr/share/fence") |
There was a problem hiding this comment.
agree, in branch changes-_PR609
| #sys.path.append("/usr/share/fence") | ||
|
|
||
| from fencing import * | ||
| from fencing import ( |
There was a problem hiding this comment.
Only import the ones that arent imported by *:
https://github.com/ClusterLabs/fence-agents/blob/main/lib/fencing.py.py#L17-L18
| raise | ||
|
|
||
| # Check if we are the self-fencing node | ||
|
|
There was a problem hiding this comment.
We can remove the blank line here.
There was a problem hiding this comment.
agree, in branch changes-_PR609
| logger.debug("Captured my (%s) state and it %s - returning OK - Proceeding with fencing",instance_id,state.upper()) | ||
| return "ok" | ||
| else: | ||
| logger.debug("Captured my (%s) state it is %s - returning Alert - Unable to fence other nodes",instance_id,state.upper()) |
There was a problem hiding this comment.
You can use formatted strings for these 2 as well for consistency.
|
|
||
| def main(): | ||
| conn = None | ||
| #print("in main") |
There was a problem hiding this comment.
Remove all commented out code in main().
There was a problem hiding this comment.
agree, in branch changes-_PR609
| "access_key", | ||
| "secret_key", | ||
| "secg", | ||
| "plug", |
There was a problem hiding this comment.
Use "port" as explained above.
There was a problem hiding this comment.
?? @oalbrigt i don't have an issue with changing it, however i'd like to check as my understanding from the docs is was port is deprecated to plug?
There was a problem hiding this comment.
Yeah, but port is the name it's using internally in the library and provides both plug and port (shown as deprecated in metadata) parameters.
| except Exception as e: | ||
| logger.error(f"Failed to process input options: {str(e)}") | ||
| #print(f"Error details: {str(e)}") | ||
| sys.exit(1) |
| docs = { | ||
| "shortdesc": "Fence agent for AWS VPC", | ||
| "longdesc": ( | ||
| "fence_aws_vpc is a Power Fencing agent for AWS VPC that works by " |
There was a problem hiding this comment.
Should be "Network and Power Fencing agent".
There was a problem hiding this comment.
agree, in branch changes-_PR609
| "fence_aws_vpc is a Power Fencing agent for AWS VPC that works by " | ||
| "manipulating security groups. It uses the boto3 library to connect to AWS.\n\n" | ||
| "boto3 can be configured with AWS CLI or by creating ~/.aws/credentials.\n" | ||
| "For instructions see: https://boto3.readthedocs.io/en/latest/guide/quickstart.html#configuration" |
There was a problem hiding this comment.
Maybe add a "NOTE:" that says the agent wont be able to power on the node again if onfence-poweroff is set.
There was a problem hiding this comment.
agree, in branch changes-_PR609
yes. ive put in this branch changes-_PR609 |
A networking resource fencer for AWS VPC that works be manipulating security groups